Bug 289795 – document.importNode from standards to quirks mode document can result in broken selector matching, because of shared lowercased ElementData::m_classNames

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Brendan Ronan]

Created attachment 474570 [details]
working left, broken right

This issue occurs in safari while running a session with our Glance Cobrowse tool for screensharing.  The tool recreates the DOM for a remote viewer to assist in customer support scenarios.  The tool uses document.importNode to clone elements on the page to prepare them to be sent to the session's viewer.  In order to make sure that native browser functionality has not been overwritten by javascript on the page, importNode is called in a fresh iframe we create on the page

This bug only happens after a refresh of the page during the session. You can see in the first screenshot (where css styles are applied properly) the querySelector for .TopMenuItem returns an item as expected.  In the second screenshot where styles are broken, the querySelector returns null for .TopMenuItem, but returns the element with .topmenuitem.  You can see that the classname is camelCase, but the selector is all lowercase.  I got the idea to try a lowercase selector after seeing this bug from a long time ago: https://bugs.webkit.org/show_bug.cgi?id=159555

I have tried a ton of stuff over the last 2 days to replicate it, including running our clone function (using importNode) on literally every element on the page while the CSS is working correctly, but I could not get the styles to break

The breakage happens late after the page loads and the cobrowse session resumes.  I spent a day in the safari timeline inspector analyzing the exact moment the styles break, and it does not seem tied to any javascript on our side.  I tried blocking various libraries and code on the page I thought might interfere with cobrowse, but no luck.  There is a “styles invalidated“ process that happens right before the break, but this happens all the time on the page whenever the dom/css changes, and I couldn’t figure out what exactly was causing this re-render

 I wanted to find a smoking gun that would reliably reproduce this but have not been able to.  Unfortunately as this happens on one of our customer's pages, I cannot give you access to the page where it is happening

This issue does not happen on every refresh, I would say it happens 6/10 times 

Let me know if there is anything else I can provide

[Alexey Proskuryakov]

Thank you for the report. If the issue resists minification and is flaky on the actual website, it seems extremely unlikely that we'll be able to make progress without access to the website.

[Brendan Ronan]

Would web archives be helpful?

[Alexey Proskuryakov]

If they reproduce the issue, certainly helpful.

[Brendan Ronan]

Created attachment 474595 [details]
webarchive of page taken when rendering problem is present

[Brendan Ronan]

I was not able to reproduce the issue when loading the webarchive file, but maybe something in the structure of the page will help identify the bug

Hi, I work with Brendan and I have a test page where you can reproduce this problem.  Steps:

1. Navigate to https://glancedev1.com/joeb/dev/cyv/chooseyourvisitor.html
2. Open the console and type in document.querySelector(".pageContentClass").  It will return an html section element.
3. Now click the "Submit" button (about half way down the page) and then the "Cobrowse" button in the top left.  You should see a modal with a 4 digit code pop up.  You can ignore this.
4. Now in the console enter document.querySelector(".pageContentClass") again.  This time it returns null.
5. Instead, enter document.querySelector(".pagecontentclass") in the console. It returns the html section element.

[Alexey Proskuryakov]

This reproduces as described, and the class name is unchanged in DOM, yet querySelector() fails:

> document.getElementById("pageContent").className
< "pageContentClass"

> document.querySelector(".pageContentClass")
< null

[Brendan Ronan]

I think the root of the issue here has to do with calling document.importNode on an element from a different window.  Ie: taking an element from the top level page, and calling document.importNode on it in an iframe on that page

[Alexey Proskuryakov]

Yes, this is correct. The lowercasing happens in Element::classAttributeChanged() because the node is imported into a quirks mode document about:blank. This is all good so far - about:blank is quirks mode in Chrome too, and folding the case is appropriate.

    auto shouldFoldCase = document().inQuirksMode() ? SpaceSplitString::ShouldFoldCase::Yes : SpaceSplitString::ShouldFoldCase::No;

Then it's imported back into the main document that's in standards mode (doctype html), but ElementData::m_classNames is copied as lower case.

So the WebKit bug here is that we cannot clone from quirks to standards documents so naively, at least m_classNames needs to be re-parsed, possibly more.

The workaround is to use a standards mode iframe instead of about:blank.

[Alexey Proskuryakov]

Created attachment 474649 [details]
reduced test case

[Radar WebKit Bug Importer]

<rdar://problem/147548958>

[Karl Dubost]

fwiw, with Alexey test case I get null on the 3 browsers with 
Safari Technology Preview  125 / 18.4          20622.1.6
Firefox Nightly            138.0a1       13825.3.11
Google Chrome Canary       136.0.7080.0  7080.0

but the steps to reproduce above for 
https://glancedev1.com/joeb/dev/cyv/chooseyourvisitor.html

After submit and Cobrowse, the console returns:

Safari
> document.querySelector(".pageContentClass")
< null
> document.querySelector(".pagecontentclass")
< <section id="pageContent" class="pageContentClass" data-gid="117">…</section>


Chrome and Firefox
> document.querySelector(".pageContentClass")
< <section id="pageContent" class="pageContentClass" data-gid="117">…</section>
> document.querySelector(".pagecontentclass")
< null

[Karl Dubost]

https://searchfox.org/wubkat/rev/eb2cf5ec637d91ca194ee664da7ccc1127cc6167/Source/WebCore/dom/Element.cpp#2787-2818

there is a comment about casing in
// ElementData::m_classNames or ElementData::m_idForStyleResolution need to be updated with the right case.

[Alexey Proskuryakov]

Comment on attachment 474649 [details]
reduced test case

My bad, the reduction is just wrong (failing to append the element back). There is something else that needs to be done for m_classNames to remain stale in WebKit...

[Alexey Proskuryakov]

I'm failing to make an accurate reduction. Seems like there has to be a code path that bypasses the code in Element.cpp that Karl pointed out.

[Alexey Proskuryakov]

I now think that the issue is different. Looks like ElementData is shared between original and imported elements, so lowercasing in quirks mode document affects the original too. To check this theory, I confirmed that the below experiment fixes the symptom.

@@ -2519,6 +2519,8 @@ void Element::classAttributeChanged(const AtomString& newClassString, AttributeM
     }
 
     auto shouldFoldCase = document().inQuirksMode() ? SpaceSplitString::ShouldFoldCase::Yes : SpaceSplitString::ShouldFoldCase::No;
+    if (shouldFoldCase == SpaceSplitString::ShouldFoldCase::Yes)
+        ensureUniqueElementData();

[Alexey Proskuryakov]

Created attachment 474664 [details]
test case

I think that this is the right test case for this, and it's much simpler.

[Alexey Proskuryakov]

Pull request: https://github.com/WebKit/WebKit/pull/42826

[EWS]

Committed 292542@main (8d714c80eaa3): <https://commits.webkit.org/292542@main>

Reviewed commits have been landed. Closing PR #42826 and removing active labels.

[Brendan Ronan]

(In reply to Alexey Proskuryakov from comment #18)
> Pull request: https://github.com/WebKit/WebKit/pull/42826

Do you know when this will make it into a  Safari release, or where I can go to see that?

[Alexey Proskuryakov]

This fix is available in macOS 26 Tahoe developer beta and in other aligned releases. It's also been available in Safari Technology Preview.