New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug



Sign in to add a comment

Class matching fails after moving an element from a quirks-mode document to a standard-mode document

Reported by a...@scirra.com, Jun 15 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.86 Safari/537.36

Steps to reproduce the problem:
1. Visit this URL: https://editor.construct.net/r38/#open=ghost-shooter-tut
2. Wait for everything to finish loading - it should open a project after a moment
3. Press F6
4. Double-click the "Web (HTML5)" option

What is the expected behavior?
At this point in our PWA, we make a querySelector() call for an element, but querySelector() incorrectly returns null and ends up throwing an exception that makes our PWA show a crash dialog.

In the console I can verify the element definitely exists, but is not found by querySelector: https://dl.dropboxusercontent.com/u/15217362/chrome-queryselector-bug.png
(please excuse the minified code)

What went wrong?
Chrome's querySelector call fails to find an element that matches the selector.

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 59.0.3071.86  Channel: stable
OS Version: 10.0
Flash Version: 

This is an urgent issue for us because Chrome are removing support for HTML imports, and this bug occurs when we switch to our polyfill. This means we have to keep using HTML imports until this is fixed, which then means our PWA will be broken when Chrome removes HTML imports.

I tried to create a minimal repro (I know how important that is), but when I make the same call with the same data, querySelector() succeeds. You can see this happening here: https://www.scirra.com/labs/bugs/querySelectorRepro/
So something about our PWA causes the call to fail, but I am at a loss as to how we can further investigate this. Please let me know if you have any suggestions.

The equivalent code works correctly in Firefox. We don't currently officially support Firefox, but you can follow the same repro steps at this slightly different URL:
https://editor.construct.net/r38/?skip-support-check#open=ghost-shooter-tut
In this case the querySelector() call succeeds in Firefox and our PWA works normally. I tested Nightly 56.0a1. I think this proves it really is a Chrome-specific bug.

Canary 61.0.3131.0 is also affected.

We're really stuck here, I know it's hard to debug full pages but we really need to fix this to get off HTML imports. Any help would be much appreciated!
 

Comment 1 by woxxom@gmail.com, Jun 15 2017

FWIW if you set a conditional breakpoint on the failing querySelector, click the dialog in Elements panel inspector, then run $0.querySelector('.nextButton') it successfully finds the element. Interestingly, this.!h_ != $0, but all properties are equal, including this.!h_.querySelector == $0.querySelector

Comment 2 by kochi@chromium.org, Jun 16 2017

Owner: kochi@chromium.org
Status: Assigned (was: Unconfirmed)

Comment 3 by kochi@chromium.org, Jun 16 2017

Cc: kochi@chromium.org
Owner: tkent@chromium.org

Comment 4 by tkent@chromium.org, Jun 16 2017

This bug happens when moving an element with class attribute between a quirks-mode document and a standard-mode document.  I guess editor.construct.net contains imported documents without <!DOCTYPE html> and |this.!h_| is an element in an imported document.



A smaller repro:

<!-- quirks mode -->
<body>
<dialog>
<button class="nextButton"></button>
<button class="nextButton"></button>
</dialog>
<template><div></div></template>
<pre></pre>
<script>
function log(str) { document.querySelector('pre').textContent += str + '\n'; }
log('Main document\'s mode: ' + document.compatMode);
var dialog = document.querySelector('dialog');
var button = dialog.querySelector('.nextButton');
var tc = document.querySelector('template').content;
log('Template document\'s mode: ' + tc.ownerDocument.compatMode);
tc.ownerDocument.adoptNode(button);
log('Second query result should not be null: ' + dialog.querySelector('.nextButton'));
log('Content of context element: ' + dialog.innerHTML);
</script>
</body>

This shows:

Main document's mode: BackCompat
Template document's mode: CSS1Compat
Second query result should not be null: null
Content of context element: 

<button class="nextButton"></button>


Comment 5 by tkent@chromium.org, Jun 16 2017

Labels: OS-Android OS-Chrome OS-Fuchsia OS-iOS OS-Linux OS-Mac
Summary: Class matching fails after moving an element from a quirks-mode document to a standard-mode document (was: querySelector fails in our PWA)

Comment 6 by tkent@chromium.org, Jun 16 2017

Status: Started (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3d8dd287aeb97d6dbb658da989ac4a4c95853dc7

commit 3d8dd287aeb97d6dbb658da989ac4a4c95853dc7
Author: Kent Tamura <tkent@chromium.org>
Date: Fri Jun 16 09:16:18 2017

Fix class matching after moving an element from a quirks-mode document to a standard-mode document, or vice versa.

We accidentally shared ShareableElementData between documents.
Element::DidMoveToNewDocument() should ensure we don't share.

Bug:  733682 
Change-Id: I6ebb9c033559f6fa42ec92571b529d73d7c74869
Reviewed-on: https://chromium-review.googlesource.com/538514
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479999}
[modify] https://crrev.com/3d8dd287aeb97d6dbb658da989ac4a4c95853dc7/third_party/WebKit/LayoutTests/dom/README.md
[add] https://crrev.com/3d8dd287aeb97d6dbb658da989ac4a4c95853dc7/third_party/WebKit/LayoutTests/dom/element/class_matching_after_adopt.html
[modify] https://crrev.com/3d8dd287aeb97d6dbb658da989ac4a4c95853dc7/third_party/WebKit/Source/core/dom/Element.cpp

Comment 8 by a...@scirra.com, Jun 16 2017

Amazing work, thanks for looking in to this so quickly! I didn't realise any of our documents were in quirks mode - this must be a gotcha with moving from HTML imports to XHR document fetches. Presumably imports default to standards mode but XHR defaults to quirks mode. The workaround is just to put <!DOCTYPE html> in all our imports. After doing that, everything seems to work OK again. So this gives us a way to start using the polyfill before the patch lands in Chrome. Great stuff!

Comment 9 by phistuck@gmail.com, Jun 16 2017

@tkent - is this a good candidate for web-platform-tests?

Comment 10 by tkent@chromium.org, Jun 18 2017

Labels: M-61
Status: Fixed (was: Started)
> @tkent - is this a good candidate for web-platform-tests?

I'm not sure.  This bug strongly depends on a tricky optimization in WebKit/Blink, and I think it's impossible to write this test for one who only knows the DOM specification, and not WebKit/Blink implementation.


Comment 11 by tkent@chromium.org, Jun 18 2017

Filed a WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=173528

Comment 12 by phistuck@gmail.com, Jun 19 2017

#10 - I thought web platform tests accept pretty much any test and since this is easily testable, it could prevent anything like this from happening in the future anywhere.

Comment 13 by kochi@chromium.org, Jun 20 2017

We welcome your contribution ;)

Comment 14 by phistuck@gmail.com, Jun 20 2017

I will use most of the code from #4. @tkent - do you want attribution?

Comment 15 by tkent@chromium.org, Jun 20 2017

> @tkent - do you want attribution?

No. Feel free to use it without the attribution.


Comment 17 by kochi@chromium.org, Jun 26 2017

Thanks for your contribution!

Comment 18 by phistuck@gmail.com, Jun 28 2017

It was accepted! :O
Does that mean you can delete https://crrev.com/3d8dd287aeb97d6dbb658da989ac4a4c95853dc7/third_party/WebKit/LayoutTests/dom/element/class_matching_after_adopt.html or does Chromium not run quirks mode web platform tests at the moment?

Comment 19 by tkent@chromium.org, Jun 28 2017

We import quirks-mode/. So we can delete the test after the next WPT import.

Can you explain how this new behavior matches what specs say?

Quirks mode is a property of documents, not of elements. So if an element is adopted to a document with a different mode, I would expect it to use the rules of that document.

Either revert this or open a spec issue.
zcorpan@,

> Quirks mode is a property of documents, not of elements. So if an element is adopted to a document with a different mode, I would expect it to use the rules of that document.

Right.  The new behavior matches to it.  Do you see any behavior not matching to it?

Sign in to add a comment