Page MenuHomePhabricator

Errors in one module can affect other modules via $.ready
Closed, DeclinedPublic

Description

This is not really a ResourceLoader bug but serious enough that some workaround is needed and that probably has to be done via RL.

Due to jQuery bug #10251 [1] (wontfixed), errors in a jQuery domready handler prevent all subsequently added domready handlers from running. This means that errors in less reliable scripts like gadgets or user scripts can break any production code that is loaded after them and relies on $.ready (which is true for most extensions).

[1] http://bugs.jquery.com/ticket/10251

Details

Reference
bz70772

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:56 AM
bzimport set Reference to bz70772.
bzimport added a subscriber: Unknown Object (MLST).
Tgr created this task.Sep 12 2014, 3:07 PM

An example of where this happened would be useful to assess what kind of code did this, who the affected users were, how and by whom it was addressed, etc.

Initially I'm inclined to agree with jQuery and, here too, consider this an invalid bug. Broken code should not be silenced. There's a 1001 ways for gadgets and user scripts to break unrelated modules. That's the DOM for you.

This problem is in many ways similar to extensions vs MediaWiki core. When it goes wrong, the bug is reported, the broken module is found and either fixed or temporarily disabled until a maintainer can look into it. I think it's acceptable to encourage the community to apply a similar policy to their gadgets.

If a gadget is broken, you can't leave it enabled and expect everything to degrade gracefully. There's too many asynchronous paths for code to take that absorbing it would be impossible (or unpractical). That's a responsibility gadget authors carry and should be familiar with if they've been given such access. And even if we could, trying to absorb it is in my opinion not we should want to achieve.

I'm all for graceful degradation and often try to address hurdles and make user experiences as smooth as we can.

But in cases like this, I'm all for letting it fail loudly. That's what errors are for.

Krinkle lowered the priority of this task from High to Medium.Dec 8 2014, 5:36 PM
Krinkle updated the task description. (Show Details)
Krinkle set Security to None.
Krinkle removed a subscriber: Unknown Object (MLST).
Tgr added a comment.Dec 8 2014, 8:09 PM

See the blocked bug (T72756) for an example.

Broken code should definitely not be silenced; the jQuery bug is not about silencing but limiting the effect of an error to the code which caused it. Which is a good practice in general, but I can see how jQuery has no easy way of following it in this case, without causing massive B/C breaks in promise behavior (although special-casing $.ready might be easier).

This wouldn't be a problem on Wikimedia sites if there was an expectation for gadgets to not break, but my experience on medium-to-small wikis is that a few gadget-skin-browser combinations are broken at any given time, so seeing errors in the console in itself is not surprising, and if a completely unrelated product starts misbehaving, gadget maintainers are not going to attach any significance to it. So I'm not sure "loudly" is a fair description of a gadget throwing an exception.

I don't think the same if true for gadgets messing up the DOM or otherwise affecting other modules; while in theory it's possible for that to happen in such a way that the gadget is not suspected, in practice I haven't ever seen that happening.

One way to handle this (that could be inside or outside of jQuery) is to set up a timeout to check whether all domready handlers were executed, and force execution manually if it was broken by an exception. That would not stop the exception from being thrown and reported, but would limit its effect to the domready handler which caused it.

Krinkle added a comment.EditedJan 8 2015, 10:59 AM
  • This applies to any event handler, not just DOMContentLoaded. E.g. click events: http://jsfiddle.net/mj8yeLmy/. Most instances of domready-handlers should probably use a mw.hook if they want to work as expected in the future of partial documents changing without refresh (as is already happening in LiquidThreads, VisualEditor, Flow and core's LivePreview).
  • Regarding gadgets not breaking, the same policy as extensions applies. Extensions can interfere with core, just as gadgets. If they misbehave with a certain language/wikiproject/skin combination; that should be fixed, or the relevant module excluded from running in that case, or disabled/not-enabled by those users (it's common for gadgets to say in their description "Vector-only"). Can you given an example of a gadget causing errors in a certain skin-browser combination? That's relatively rare in my experience (except for outdated copies of gadgets on small wikis; this is generally addressed by loading the gadget from commons/meta/enwiki/mworg directly). It's generally fixed quickly when reported to gadget maintainers.
  • I've seen gadgets affecting the DOM of MediaWiki core and extension modules plenty. It's not rare.

I'm still open to exploring a workaround on our side (in MediaWiki core). Note however that:

  • The $.ready promise does not expose its internal jQuery.Callbacks instance. Neither does it keep track of which have fired.
  • domready handlers should become rare over time. There are very few use cases left for it in our environment.
  • Beware the inconsistency with other event handlers.
Krinkle closed this task as Declined.Sep 18 2015, 9:15 PM
Krinkle claimed this task.