Page MenuHomePhabricator

mw.loader state of module stuck at "loading" if request was aborted
Open, MediumPublic

Description

Steps to reproduce:

  • Load MediaWiki from localhost.
  • Stop the web server. -> All further requests get aborted.
  • Try to make a request with $.get():
var d = $.get( '/' ); // The request gets aborted.
  • Enter
d.state(); // "rejected"
  • Try to make a request with mw.loader.using():
var d = mw.loader.using(
	'mediawiki.special',
	function () { console.log( 'ready' ); },
	function () { console.log( 'error' ); }
);
// The request gets aborted.
  • Enter
d.state(); // "pending"
  • Expected result: "rejected"

=> Upstream: https://github.com/jquery/jquery/issues/2413

Details

Reference
bz66598

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 3:27 AM
bzimport set Reference to bz66598.
bzimport added a subscriber: Unknown Object (MLST).
Fomafix created this task.Jun 13 2014, 7:14 PM

It seems that this can be done by making mw.loader.work(), doRequest() and addScript() work together to update the state of failed modules in the registry.

This comment was removed by Fomafix.
Fomafix updated the task description. (Show Details)Dec 16 2014, 7:26 AM
Fomafix set Security to None.
Fomafix updated the task description. (Show Details)
Fomafix added a comment.EditedDec 16 2014, 9:37 AM

The crossDomain: true prevents a callback on HTTP errors.

Without crossDomain: true:

var d = $.ajax( { url: '/', dataType: 'script' } ); // The request gets aborted.
// Wait until the request is aborted.
d.state() // "rejected"

With crossDomain: true:

var d = $.ajax( { url: '/', dataType: 'script', crossDomain: true } ); // The request gets aborted.
// Wait until the request is aborted.
d.state() // "pending"
Krinkle renamed this task from ResourceLoader: mw.loader.using() promise is indefinitely "pending" if load request is aborted to mw.loader state of module stuck at "loading" if request was aborted.Dec 21 2014, 4:28 AM
Krinkle updated the task description. (Show Details)
Krinkle removed a subscriber: Unknown Object (MLST).
Krinkle added a comment.EditedDec 21 2014, 4:30 AM

The underlying problem is that the state machine itself stays at "loading" internally for the mw.loader client. The promise returned by using() merely reflects this.

As @Fomafix's example shows, this is caused by a bug in jQuery (or possibly a limitation in the underlying browser API).

Without crossDomain: true scripts gets loaded by XMLHttpRequest, when they are on the same origin. When the scripts are not on the same origin they always gets loaded by adding a <script> tag.

XMLHttpRequest has a callback. A <script> tag has no callback.

On the WMF projects the script are not on the same origin. On a local installation the scripts are normally on the same origin.

When we would remove crossDomain: true, the loading of the scripts works but some tests fails.

When we would remove crossDomain: true, we would have different types of loading based on origin of the scripts. Both types of loading have to be tested.

When we would remove crossDomain: true, the $.ajax call would generate a callback but mediawiki.js does not forward the callback to mw.loader.using. A lot of changes are necessary to forward the callbacks.

He7d3r added a subscriber: He7d3r.May 18 2015, 5:18 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 18 2015, 9:15 PM
Krinkle closed this task as Resolved.EditedOct 4 2017, 8:57 PM
Krinkle claimed this task.

This is resolved as part of jQuery 3. See T124742: Upgrade to jQuery 3.

jQuery 1.x
$.fn.jquery
//> "1.11.3"
x = $.ajax( 'https://unknown/', { dataType: 'script' } )
//> https://unknown/ net::ERR_NAME_NOT_RESOLVED
x.state()
//> "pending"
jQuery 3.x
$.fn.jquery
//> "3.2.1"
x = $.ajax( 'https://unknown/', { dataType: 'script' } )
//> https://unknown/ net::ERR_NAME_NOT_RESOLVED
x.state()
//> "rejected"
Fomafix reopened this task as Open.Oct 5 2017, 5:59 AM

Reopened. Even with jQuery 3 d.state() returns pending.

@Esanders mentioned that he ran into this recently but I'm not sure what the context was. Should this be prioritized higher? I could work on it if so.

Krinkle moved this task from External to Confirmed Problem on the MediaWiki-ResourceLoader board.EditedOct 5 2017, 7:19 PM

@Fomafix Can you add steps to reproduce the bug? Its seems to be working now. Note that before 1 hour ago, Wikipedia.org wikis were still on jQuery 1. (T124742)

Here are the steps to reproduce on https://en.wikipedia.org/wiki/Wikipedia:Sandbox

$.fn.jquery
//>"3.2.1"
var d = mw.loader.using(
	'mediawiki.special',
	function () { console.log( 'ready' ); },
	function () { console.log( 'error' ); }
);
//>undefined
//>Laden fehlgeschlagen für das <script> mit der Quelle "https://en.wikipedia.org/w/load.php?debug=false&lang=en&modules=mediawiki.special&skin=monobook&version=0ovw7u3".  Wikipedia:Sandbox:1
d.state()
//>"pending"

@Fomafix Thanks, I am able to reproduce the issue now.

$.fn.jquery
//>"3.2.1"

$.ajax('https://unknown/', { dataType: 'script' })
//> https://unknown/?_=1507235997884 net::ERR_INTERNET_DISCONNECTED
$_.state()
//> "rejected"
mw.loader.using(
	'mediawiki.special',
	function () { console.log( 'ready' ); },
	function () { console.log( 'error' ); }
);
//> GET /w/load.php?... net::ERR_INTERNET_DISCONNECTED
$_.state()
"pending"

The code path I was previously considering is mw.loader#execute(), in which the Promise returned from addScript ($.ajax) is used correctly. However, that code is only used in debug mode when a module loads additional urls after the first one. The code path for the initial request is in mw.loader#doRequest() which indeed ignores the return value and never propagates back to mw.loader#work() or mw.loader.using().

If a user tries to load VE with a dropped connection we have no way of knowing if the VE modules fail to load, so the loading bar just hangs. I would think if the ajax request fails the loading promise should reject and the state should move to 'error'.

Krinkle updated the task description. (Show Details)Oct 9 2017, 9:28 PM

If a user tries to load VE with a dropped connection we have no way of knowing if the VE modules fail to load, so the loading bar just hangs. I would think if the ajax request fails the loading promise should reject and the state should move to 'error'.

Yep, that's right. When ResourceLoader was first written, the addScript logic was just that (adding a script tag). Contrary to XHR+eval, there were no error events for script tags. As such, network errors would result in an indefinite "loading" state and there wasn't a whole lot we could do about that.

Eventually the code was simplified to re-use jQuery's add-script logic, via $.ajax. But while it does return a promise, the underlying events aren't there, so it remained "pending".

Browsers have since added support for error events on script elements. jQuery 2.0 started taking a very different approach to cross-domain scripts and part of this new approach meant that error events worked. The jQuery 1.x code base stayed an the older system that enjoyed wider browser support, providing the same $.ajax() abstraction, but (among many overlooked details) lacked error events for cross-domain scripts. I reported this upstream at https://github.com/jquery/jquery/issues/2413. It was acknowledged as a bug, and prioritised for the "compat" branch of jQuery 3.0-alpha (jQuery 3 Compat, was going to become the successor to jQuery 1.x)

However, shortly after, EOL of IE8-10 was announced, at which point it no longer made sense to invest in a jQuery 3 Compat (https://blog.jquery.com/2016/01/14/jquery-3-0-beta-released/), and we focussed on upgrading to jQuery 3.0 instead.

Meanwhile, we've done various refactors internally and for debug mode, and when debug mode creates script tags, we actually track the errors (Promise reject) from addScript. But for the older path for regular load.php requests (which previously didn't have errors anyway), the Promise is still not yet used. We need to start tracking these promises and tie them back to the internal mw.loader#work()'s batch/job logic.

I'll try to prioritise this, but it is non-trivial. For debug mode it was easy because there tracking is limited to a single module and all information is in execute()'s lexical scope. Not so much for the batch request.

Test case on the user interface:
Current behavior:

  • Load a random page.
  • Disconnect connection to webserver.
  • Click on watch.
  • The watching state changes to loading.
  • There are 2 requests: The GET for the loading the module mediawiki.notification and the POST for API request. Both fail.
  • The waching state returns to the previous state.
  • Reconnect connection to webserver.
  • Click on watch.
  • The watching state changes to loading.
  • There is only the POST for the API request.

Expected result:

  • The GET for loading the module mediawiki.notification should also retried.

@Fomafix The promise for mw.loader.using() should indeed be rejected when the connection fails. Thank you for providing a simple way to test this. I've got enough now to start working on it, as sketched in T68598#3670756.

However a module request will not be retried, that would be inconsistent with mw.loader's state guarantees and is outside the scope of this task to reconsider.

Is it possible to add additional states to allow to retry loading the module on the next mw.loader.using after abort the previous loading request?

Krinkle removed Krinkle as the assignee of this task.Jun 25 2018, 5:38 PM
Krinkle added a subscriber: aaron.

@Fomafix Retrying might be worth considering at a later stage, but right now that breaks too many existing assumptions and rules about mw.loader's internal state machine.

I'm moving this out of "Next up" in favour of completing T192623 first. Once that is completed, I think this would be an important bug to fix. Also un-assigning for now, as this might be a good task for @aaron to pick up after gaining more RL familiarity via T192623.

Maybe the online and offline events are also interesting in this context.

Krinkle moved this task from Radar to Doing on the Performance-Team board.Jul 11 2020, 5:53 PM
Krinkle edited projects, added Performance-Team; removed Performance-Team (Radar).
Krinkle moved this task to Radar on the Performance-Team board.
Krinkle edited projects, added Performance-Team (Radar); removed Performance-Team.