Page MenuHomePhabricator

mw.loader state of module stuck at "loading" if request was aborted
Closed, ResolvedPublic

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

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).

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 set Security to None.
Fomafix updated the task description. (Show Details)

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).

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.

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"

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.

@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'.

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 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 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.

Update:

The mw.loader code no longer uses jQuery.ajax so while the bug was resolved there, we actually just use scriptElement.onerror directly now so this is definitely not blocked anymore.

The way it works today is that we make requests to the server, and the server makes one or more callbacks to mw.loader.implement() and or mw.loader.state(). We don't do anything with scriptElement.onload or scriptElement.onerror currently.

What we would need to do is for addScript to allow callers to distinguish between load and error, and then for the mw.loader caller of it to utilize the error callback to implicitly mark all the modules it asked for as being in state "error".

Change 749892 had a related patch set uploaded (by SD0001; author: SD0001):

[mediawiki/core@master] resourceloader: Fix module state stuck at \"loading\" if request was aborted

https://gerrit.wikimedia.org/r/749892

Change 757402 had a related patch set uploaded (by SD0001; author: SD0001):

[mediawiki/core@master] resourceloader: Fix module stuck at \"loading\" if request failed and allow retry

https://gerrit.wikimedia.org/r/757402

Change 757402 abandoned by Krinkle:

[mediawiki/core@master] resourceloader: Fix module stuck at "loading" if request failed and allow retry

Reason:

Closing in favour of https://gerrit.wikimedia.org/r/c/mediawiki/core/+/749892. For now I prefer not to support the notion of retries at this level. It introduces too much complexity and uncertainty long-term. I'd rather encourage higher level mechanism to save state in a more robust way. We can also do things to prevent certain things such as not making a request at all if the browser knows we're offline. In that case the modules would remain in the original 'registered' state for longer.

https://gerrit.wikimedia.org/r/757402

Change 749892 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Set module state "error" if request fails on network

https://gerrit.wikimedia.org/r/749892

matmarex awarded a token.