PageEdit 1.2.0 would not run under Kubuntu 20.04 and Mint 20

[KevinH]

Please do check it. The only build with Qt 5.12.7 I have access to is the Qt 5.12.7 PageEdit mac release build, and it works properly.

Once I finish rebuilding Qt 5.12.7, I will use it to build PageEdit master just to verify none of our recent PageEdit changes are in any way related to the bug.

Then I will generate a full recursive diff of the qtwebengine modules in 5.12.7 to 5.12.8 to see if anything jumps out as the issue.

[KevinH]

Okay, Qt 5.12.7 build completed and a clean rebuild of PageEdit master shows no bug at all.

So that confirms it is a Qt qtwebengine bug introduced in Qt 5.12.8.

So when I get time over the weekend, I will try to track this down to a specific change in QtWebEngine that is the problem if I can.

[KevinH]

After staring that the blink renderer code for some time, it is clear a bad/partial merge from Chrome was introduced into Qt 5.12.8 and Qt 5.12.9. Somebody reversed a test condition to make it easier to understand but forgot to swap the then and else clauses and as a result completely reversed the logic in places.

I checked the Chrome 80 sources and this bug does not exist (the if test was reversed as were the then and else clauses) so that explains why Qt 5.15 seems to work.

I have now fixed that bug and I am rebuilding Qt once again to see if this fixes our issue or if there is more breakage that needs to be tracked down.

What a mess!

[KevinH]

My build finished and sure enough, the obviously broken code when fixed, actually fixes this problem.

So somebody at Qt did a bad job merging in chrome 79 changes into the QT 5.12.X to create the 5.12.8 tree and introduced a bad break that can cause text to be lost when live editing!

The breakage continued with Qt 5.12.9 since the bug was never reported.


For the record, the bad chrome merge is here:

qtwebengine/src/3rdparty/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_serializer.cc

The broken code (bad chunk) can be easily seen in a diff from the working Qt 5.12.7 to the Qt 5.12.8 version of the file:

@@ -358,10 +362,7 @@
         continue;
       }
 
-      if (!n->GetLayoutObject() &&
-          (!n->IsElementNode() || !ToElement(n)->HasDisplayContentsStyle()) &&
-          !EnclosingElementWithTag(FirstPositionInOrBeforeNode(*n),
-                                   selectTag)) {
+      if (n->GetLayoutObject() || ShouldSerializeUnrenderedElement(*n)) {
         next = Strategy::NextSkippingChildren(*n);
         // Don't skip over pastEnd.
         if (past_end && Strategy::IsDescendantOf(*past_end, *n));

Here they reverse the condition in the "if" to simplify the logic but do not reverse the "then" and "else" clauses.

Here is patch that fixes things:

--- qtwebengine/src/3rdparty/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_serializer.cc.orig	2020-06-19 23:46:16.000000000 -0400
+++ qtwebengine/src/3rdparty/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_serializer.cc	2020-06-19 23:47:33.000000000 -0400
@@ -363,11 +363,6 @@
       }
 
       if (n->GetLayoutObject() || ShouldSerializeUnrenderedElement(*n)) {
-        next = Strategy::NextSkippingChildren(*n);
-        // Don't skip over pastEnd.
-        if (past_end && Strategy::IsDescendantOf(*past_end, *n))
-          next = past_end;
-      } else {
         // Add the node to the markup if we're not skipping the descendants
         AppendStartMarkup(*n);
 
@@ -378,6 +373,11 @@
         }
         AppendEndMarkup(*n);
         last_closed = n;
+      } else {
+        next = Strategy::NextSkippingChildren(*n);
+        // Don't skip over pastEnd.
+        if (past_end && Strategy::IsDescendantOf(*past_end, *n))
+          next = past_end;
       }
     }

Now we just need to get the attention of the person who broke things at Qt and the Linux people to get this stuff fixed!

For the record, I filed the following official bug report:

https://bugreports.qt.io/browse/QTBUG-85160

Now getting someone to actually admit the error and fix things in an LTS 5.12.8 and LTS 5.12.9 may be next to impossible. I am not sure they are even making another 5.12.X release.

So if anyone knows anyone at Ubuntu, and other Linux devs who support Qt, I would be happy to explain the bad merge and provide the fix. The bug is pretty obvious and so hopefully someone will cherry-pick this fix for Linux.

[DiapDealer]

What a mess. The latest Ubuntu LTS is unlkely to ever move beyond the Qt 5.12 series, so unless someone cherry-picks the fix, the issue will linger for a long time. Ubuntu 20.10 moves to Qt 5.14, but that's an upgrade--and people seem to cling to those LTS releases. Plus I have no idea if/where the problem got corrected in Qt 5.13/14.

We've had some interaction in the past with the person who maintains Sigil for Debian/Ubuntu (Mattia Rizzolo[?]), but I don't have any contact details ATM.

[KevinH]

The 5.13 and 5.14 series were closed by the time Qt 5.12.8 was released I think. So those series may not have this bug. It was a late chrome merge to fix cve's in chrome that introduced the breakage for the LTS series that broke things.

[Der_Andi]

Uhhh, what have i done? Such a lot of work you have. I'm really sorry. Thank you very much for testing, Der_Andi

[DiapDealer]

Not your fault.

We'd rather know about bugs than not know!

[KevinH]

Wow that was fast! Qt has acknowledged the mistake and have there own patch already in the system. Instead of swapping the "then" and "else" clauses they reinvert the "if" condition so it matches.

Either fix will work. So all we need now is to get someone from Ubuntu to add the official Qt patch to their Qt 5.12.8 version for Ubuntu 20.04 / Mint 20.04, Kubuntu,etc.

See:

https://codereview.qt-project.org/c/...omium/+/305139

for their official patch awaiting merge.

So if anyone has contacts at Ubuntu, Mint, etc, please point them at that official fix for Qt 5.12.8 and Qt 5.12.9

[DiapDealer]

Great! I added an Ubuntu bug report for the qtwebengine-opensource-src 5.12.8+dfsg-0ubuntu1 source package -- indicating the upstream acknowledgement (affecting 5.12.8/9) and official patch. I asked that they consider cherry-picking the patch so apps that depend the live editing features of QtWebEngine aren't broken for the latest Ubuntu LTS release.

There were several patches they were applying to 5.12.5 that got lifted for 5.12.8, so they're clearly not above patching. Hopefully my report doesn't get the "It's Upstream's Problem" blowoff. Well see.

Debian still seems committed to Qt5.12.5 for its testing and unstable branches. Not sure why they're still hanging their hats on such a bad (bad by Qt's own admission ) version for a future stable release, but I figure there's not much point in adding this to the mix if they are.

[DiapDealer]

Though the bug remains unconfirmed and unassigned, I received a response from someone on Launchpad who thanked me for the heads-up, and indicated they would "fix it" !

https://bugs.launchpad.net/ubuntu/+s...c/+bug/1884550

[KevinH]

Wow! That is good news indeed.

KevinH

[DiapDealer]

Surprised the heck out of me! That should trickle down to Mint, I think. The new Mint that's based on Ubuntu 20.04 LTS is still beta, I believe.

[KevinH]

And in record speed, Qt has merged the patch and closed the bug:

https://github.com/qt/qtwebengine-ch.../tree/69-based

I have never seen a QtBug go from user report to fixed, merged, and closed any faster!

[DiapDealer]

I've had to go through a little bit of dance with Ubuntu. The fact that Qt acknowledges the mistake and has patched it (and now merged the patch) doesn't seem to be enough reason for them to patch their own qt5.12.8 webengine packages. The maintainer asked for more details and possibly a test case. He said the acknowledged upstream mistake would be enough for him, but not for whatever review process Ubuntu has in place.

I've obliged with tons of detail and a small Qt test-case project that demonstrates the issue. Hopefully that will be enough!