From: Timo Tijhof <krinklemail@gmail.com>
Date: Fri, 11 Sep 2015 05:20:54 +0000 (+0100)
Subject: resourceloader: Jobs created in request() should wait for executing modules
X-Git-Tag: 1.31.0-rc.0~10022^2
X-Git-Url: http://git.cyclocoop.org/%27%20.%20url_absolue%28%24favicon%29%20.%20?a=commitdiff_plain;h=e03165c3cde36f58ed4af883f91b854e88e5df43;p=lhc%2Fweb%2Fwiklou.git

resourceloader: Jobs created in request() should wait for executing modules

Fix a regression from 6e903b94f2. It failed to update this code to account for
the new "executing" state.

This code was sometimes causing the job dependencies to be an empty array
when the module in question is currently being executed. As such the job
wasn't blocked by anything and happily run the next time a random module
invoked handlePending to run any jobs that have been satisfied.

Scenario:

* in-page RLQ: Page loads a module X that contains scripts and styles.
  - state=loading
* Module X is found in storage and implemented.
  - state=executing
* execute() adds styles to css buffer (which is asynchronous).
  It provides a callback from which it would run the module's script, set
  to "ready", and trigger handlePending().
* in-page RLQ: The edit page uses 'mw.loader.using' to require module X
  and has a callback that uses the module.
* using() sees X is not already "ready", so it calls request() for X.
* request() creates a job for "X". It then wrongly filters out X from the list
  of modules to wait for. The filter is intended to remove modules that are
  complete (e.g. "ready", "error", or "missing"). After 6e903b94f2, it also
  removed anything with state "executing". This resulted in a job with
  an empty dependency list (nothing to wait for).
* The callstack is finished. At some point the css buffer will call back.
  Some other module requested before X enters state "ready" and triggers
  handlePending(). It finds an left-over empty job with no dependencies
  and runs it. "It" being the callback of X – eventhough X isn't ready.

The added test fails without the change in mediawiki.js.

Bug: T112232
Change-Id: I3cc0c282e68a37b9b3256b213508362734161655
---

diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js
index c6e41c45b3..eb21c57863 100644
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -1390,9 +1390,10 @@
 				// Add ready and error callbacks if they were given
 				if ( ready !== undefined || error !== undefined ) {
 					jobs.push( {
+						// Narrow down the list to modules that are worth waiting for
 						dependencies: $.grep( dependencies, function ( module ) {
 							var state = mw.loader.getState( module );
-							return state === 'registered' || state === 'loaded' || state === 'loading';
+							return state === 'registered' || state === 'loaded' || state === 'loading' || state === 'executing';
 						} ),
 						ready: ready,
 						error: error
diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
index 86c2eb82ae..b55b1e7cb7 100644
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
@@ -925,6 +925,31 @@
 		mw.loader.load( target );
 	} );
 
+	QUnit.asyncTest( 'mw.loader() executing race (T112232)', 2, function ( assert ) {
+		var done = false;
+
+		// The red herring schedules its CSS buffer first. In T112232, a bug in the
+		// state machine would cause the job for testRaceLoadMe to run with an earlier job.
+		mw.loader.implement(
+			'testRaceRedHerring',
+			function () {},
+			{ css: [ '.mw-testRaceRedHerring {}' ] }
+		);
+		mw.loader.implement(
+			'testRaceLoadMe',
+			function () {
+				done = true;
+			},
+			{ css: [ '.mw-testRaceLoadMe { float: left; }' ] }
+		);
+
+		mw.loader.load( [ 'testRaceRedHerring', 'testRaceLoadMe' ] );
+		mw.loader.using( 'testRaceLoadMe', function () {
+			assert.strictEqual( done, true, 'script ran' );
+			assert.strictEqual( mw.loader.getState( 'testRaceLoadMe' ), 'ready', 'state' );
+		} ).always( QUnit.start );
+	} );
+
 	QUnit.test( 'mw.html', 13, function ( assert ) {
 		assert.throws( function () {
 			mw.html.escape();