jQuery errors due to mutation observers and nested jQuery.find()

[morungos]

I've noticed I'm getting errors from deep inside the library. The underlying bug is in Sizzle (part of jQuery) and because that's not going to be easy to update, I'm logging it here too. The issue on the Sizzle library is reported here: jquery/sizzle#445.

In Gmail.js, the issue seems to have come in b3b6256, because that adds a call to jQuery.find() in a function called by a mutation observer. Due to the Sizzle issue, that's broken.

The error likely shows only after adding a listener for view_thread, or view_email (which is what I was listening for), because these are the only cases which use sub_selector values, and it is these which trigger the calls to jQuery.find(). In my case, the main error seemed to show after sending a message, when the message is displayed after sending.

The error manifests as follows:

extension.js:7136 Uncaught TypeError: rbuggyQSA.test is not a function
    at Function.Sizzle [as find] (extension.js:7136)
    at jQuery.fn.init.find (extension.js:9043)
    at String.<anonymous> (extension.js:4244)
    at Function.each (extension.js:6766)
    at Object.Gmail.api.tools.insertion_observer (extension.js:4224)
    at HTMLDocument.<anonymous> (extension.js:4157)
    at HTMLDocument.dispatch (extension.js:11005)
    at HTMLDocument.elemData.handle (extension.js:10825)
    at (anonymous) (extension.js:7572)
    at assert (extension.js:7224)
    --- other frames added ---
    at Sizzle.setDocument (extension.js:7566)
    at Sizzle (extension.js:7091)
    at find (extension.js:9043)
    at (anonymous) (extension.js:4244)
    at each (extension.js:6766)
    at Gmail.api.tools.insertion_observer (extension.js:4224)
    at (anonymous) (extension.js:4157)
    at dispatch (extension.js:11005)
    at elemData.handle (extension.js:10825)

The backtrace is hard to read as the line numbers are broken, but the problem is relatively simple. jQuery injects an element to test querySelectorAll() functionality. That element triggers the Gmail.js mutation observer. Because this happens to build a pattern for querySelectorAll(), the current pattern is temporarily broken. This means a bunch of stuff in jQuery is temporarily broken, and notably find() is vulnerable. I wouldn't expect Sizzle to change. But the important point is that the Gmail.js mutation observer is being called apparently on changes that are internal to jQuery, and are not part of Gmail itself.

I'll try to build a more complete test scenario: in my code it is happening in response to a send, and not much else: I think the document context is changing part way through, which isn't helping.

Does Gmail.js work with other libraries? Or is it only jQuery? I don't need all this Sizzle stuff anyway, so I'd be happy to see the back of it, to be honest.

[morungos]

The following custom event handles the equivalent logic okay, so this is a good workaround. It uses a vanilla querySelector, but that's good enough in modern browsers.

  gmail.observe.register('custom_view_email', {
    class: ["Bu", "nH", ""],
    handler: (match, callback) => {
      const mailElement = match[0].querySelector("div.adn");
      if (mailElement) {
        const message = new gmail.dom.email(mailElement);
        callback(message);
      }
    }
  });

[josteink]

Thanks for the detailed error-report and analysis.

I'm not sure your analysis is 100% correct w.r.t. when this was introduced though. According in commit you linked to there are -2- instances of .find(). One removed, one readded, i.e. a change in how an existing .find() is applied.

My guess is that this bug has been there for a long time, but the rewrite of these parts may have made things worse.

Also note: sub-selectors are no longer a thing (that is, you don't need view_thread to observe view_email).

There may be code-remnants of sub-selectors in the code-base, but as a functional concept, they were removed in this commit here: 54f2cf4

If you can find a way to reproduce, and make a clean fix in gmailjs itself, I'll be happy to accept a PR.

[morungos]

Many thanks, @josteink, this is very useful. I've done a little more digging. You're right: I'd trusted blame and I shouldn't have done, the commit added and removed the same code. So this has probably been there a long time.

As far as I can tell the subselectors are still being used internally to make view_email work (I don't use view_thread) and this is why it was an issue. My workaround does what you suggest: register a new event without a subselector, and then implement it using plain DOM calls. Removing them from the API is fine by me, though.

As I said, this is a Sizzle issue, not specifically Gmail.js. I just don't want to wait for that to make it through, if it ever does (jQuery keeps contemplating dropping Sizzle anyway, which would make the issue go away). Re-building jQuery without Sizzle is also another option, but it makes my build process more complex.

I'll see what I can make as a replication. It's not easy, but I might be able to concoct something. Might be hard without jsdom, so I'll try to stay with the spirit of current testing, but I'll need to add dependencies. I'm not a mocha wizard -- tend to use jest for most JS testing these days.

[morungos]

Working on the replication: it now occurs to me that a big factor is the way the view_email event triggers on an empty class. This means that it gets literally all mutations, including the ones generated by jQuery internally. Any filtering there would probably also eliminate this issue, as Sizzle mutations wouldn't trigger Gmail.js mutation handling.

The comment says: "the empty class ("") is for emails opened after thread is rendered." Might it not be better to detect div.adn instead of the empty class, and then skip the jQuery.find() subselection if the passed element is already div.adn? That feels just as correct, and would reduce the amount of work in handling mutation events.

Is this a reasonable option?

[josteink]

I remember going for this particular approach, even though it definitely is suboptimal with regard to efficiency, because the straight up approach of selecting div.adn did not work on new Gmail.

If you can find a better way to implement this DOM-observer I’ll be super happy to merge that as well.

[morungos]

Thanks @josteink -- fortunately the new jsdom includes a nice MutationObserver, so I can start building tests for some of these things, even in mocha. The hard bit is Sizzle, because there is some very weird code in there. I'll see what I can put together over the weekend.

[josteink]

No need to overdo it. I like a good unit-test when it makes sense, but for this particular case I'd be happy with a simple repro-repo which contains a minimal extension to repro the issue, so that we can actually confirm that the issue is resolved before merging the fix.

If implementing this case is possible in a unit-test instead, that's even better, but definitely not a requirement.

[morungos]

Okay, I've done some more testing, and after reading up on microtasks (!) and I am fairly sure that the issue is that inserted elements are not being signalled through MutationObserver, but through DOMNodeInserted events, which are instantly (synchronously) emitted as elements are added to the DOM.

This is why there is recursion. It normally doesn't cause an issue, but it does on one specific case: when using the link to view a sent email message. That seems to be because there is a shift in display title at the same time as an email view. It is very likely that using MutationObserver for inserted elements as well as removed ones would solve the problem entirely, as it would push the jQuery.find() not to instantly emit insertion events.

This is the core bug: jQuery.find(), when in a new context, adds elements that generate DOMNodeInserted events while it is still initializing Sizzle. That should not happen with a MutationObserver as the callbacks would happen from the microtask queue, not synchronously.

I am not sure what the rationale is for using DOMNodeInserted as well as a MutationObserver. Sadly my handy test environment has also dropped DOMNodeInserted so it's not easy to replicate. My jsdom tests suggested that because it uses a microtasks queue for mutation events, it doesn't cause the recursive calls to Sizzle that cause this problem.

I did a quick test and this does remove the error, but it also means I'm getting multiple fires for the view_email event, so I need to look into that.

Thoughts @josteink ?

[josteink]

Good research!

This is the core bug: jQuery.find(), when in a new context, adds elements that generate DOMNodeInserted events while it is still initializing Sizzle. That should not happen with a MutationObserver as the callbacks would happen from the microtask queue, not synchronously.

But now we fire for all mutations, not just insertions. Without any hard data to back that, but simply by the sound of it, that sounds like a much more expensive observer to add.

I did a quick test and this does remove the error, but it also means I'm getting multiple fires for the view_email event, so I need to look into that.

I think may have started off where you are trying to go back, and iirc I came to some of the same conclusions last time I looked into this code (w.r.t new Gmail).

But if you can make the original MutationObserver approach work properly, I'm not opposed to that.

[morungos]

I think it should be less expensive, due to the way multiple mutations and mutations are processed in a single call into the handler. It also allows Text nodes to be skipped, and a bunch of them are coming through into api.tools.insertion_observer in the current implementation. Anyway, I am hoping so.

This isn't a total top priority as I have a workaround, but I'll see if I can throw some evening hours into a working update as a PR.

[josteink]

Any update on this one? 🙂

[josteink]

No updates on this for quite a few months.

@morungos: Do you want me to close this one, or do you plan on submitting a PR?

[morungos]

Sorry, yes, I have been more than a little swamped by a new project. I suggest you close, I'm still aiming to out a PR together, but making a test environment to replicate this has proven more than a little hard. I have a workaround using a custom event, which took the immediate pressure off. I'll add a ticket to our systems, so I don't forget. Many thanks for the assistance when pinning this wee beast down.

…

On Tue, Jul 16, 2019 at 3:40 AM Jostein Kjønigsen ***@***.***> wrote: No updates on this for quite a few months. @morungos <https://github.com/morungos>: Do you want me to close this one, or do you plan on submitting a PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#560?email_source=notifications&email_token=AADL52QOC7M7Q3TQDDNHDN3P7V3PPA5CNFSM4HD7M322YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ77U7A#issuecomment-511703676>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL52U2MYUMKHHZU742C4TP7V3PPANCNFSM4HD7M32Q> .

-- Stuart Watt Chief Technology Officer t: +1 416-522-8287 | e: stuart.watt@turalt.com | w: turalt.com

[josteink]

Thanks for the feedback. Closing as requested then.