Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutation handling breaks querySelectorAll initialization #445

Closed
morungos opened this issue Apr 5, 2019 · 4 comments
Closed

Mutation handling breaks querySelectorAll initialization #445

morungos opened this issue Apr 5, 2019 · 4 comments

Comments

@morungos
Copy link

morungos commented Apr 5, 2019

During initialization, mutation event handling can mean that rbuggyQSA is temporarily in an inconsistent state. This breaks Sizzle.matchesSelector(), and therefore JQuery.find(), for example, when used in certain mutation event handler.

The issue is:

  1. rbuggyQSA is initialized to an empty array
  2. The detection logic adds a new element to the document
  3. Depending on how that element responds to querySelectorAll, values are added to rbuggyQSA
  4. On completion, rbuggyQSA is converted to a Regexp

Because a mutation observer might be called at Step 2 (it turns out, this isn't in my code, but a library I am using), and if that library happens to call jQuery.find(), rbuggyQSA will still be an array, and therefore Sizzle.matchesSelector() will error out with a message like:

TypeError: rbuggyQSA.test is not a function

I just came across this awkward issue in the wild. I'm working on a small reproduction, but the stack trace makes it fairly clear where the issue lies. It's also been observed elsewhere, e.g.: StevenDevooght/tinyMCE-mention#64.

I'm not sure what the best resolution of this is. If we could detect the issues without adding elements, that would be one strategy. A second might be to ensure that rbuggyQSA is handled more atomically, with the candidate patterns in a second variable before conversion to a Regexp. (This is because my code is actually using jQuery.parseHTML(), which is setting up a new document temporarily, and, therefore, rbuggyQSA probably doesn't need to be re-computed for each new document (and right now, that does seem to be the case).

@morungos
Copy link
Author

morungos commented Apr 8, 2019

Please hold off on this for now -- I have a feeling it isn't mutations that's the problem, but DOMNodeInserted events, which don't get queued as microtasks in the same way, apparently. Instead, they are being emitted synchronously with the elements being added, leading to this recursion. I'm still looking into it.

The same principle applies, though. During the context of an insertion after a change in context, the handler of a DOMNodeInserted will get a corrupted Sizzle due to an incomplete array version of rbuggyQSA before it is turned into a pattern.

@mgol
Copy link
Member

mgol commented May 15, 2019

Mutation Events like DOMNodeInserted are deprecated so any bugs in them would better be solved by migrating to Mutation Observers instead.

@mgol
Copy link
Member

mgol commented May 15, 2019

I'm going to close the issue now; we can reopen if we have a test case showing the issue with Mutation Observers instead of Mutation Events.

@mgol mgol closed this as completed May 15, 2019
@al1357
Copy link

al1357 commented Jul 22, 2019

I also experience this problem on jQuery 1.12.4

A quick fix that removes the error would be to replace

if ( support.qsa && !compilerCache[ selector + " " ] && (!rbuggyQSA || !rbuggyQSA.test( selector )) ) {

with

if ( support.qsa && !compilerCache[ selector + " " ] && (!rbuggyQSA || typeof rbuggyQSA !== 'function' || !rbuggyQSA.test( selector )) ) {

although it is not entirely clear if there are any negative consequences of this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants