From: Timo Tijhof <krinklemail@gmail.com>
Date: Fri, 30 Sep 2016 23:59:28 +0000 (+0100)
Subject: mw.loader: Don't unset 'require' after execution in debug mode
X-Git-Tag: 1.31.0-rc.0~5209^2
X-Git-Url: http://git.cyclocoop.org/%7B%24www_url%7Dadmin/compta/operations/recherche.php?a=commitdiff_plain;h=3eef71c701abc899cb842a9bcd00cd1cb18c4e11;p=lhc%2Fweb%2Fwiklou.git

mw.loader: Don't unset 'require' after execution in debug mode

Follows-up bc4e07b6f6.

In production mode, 'module' and 'require' are lexically provided
through closure to the executing script and will continue to be
accessible through the script's life-time.

In debug mode, the script executes without wrapper directly from
disk (and as such, in the global scope) and variables are provided
as global variables. Since a variable can only be set to one value,
we swap the object that 'module' points to after each file. And
to avoid leakage, it is removed in-between files and after the
last one. Aside from it being technically infeasible right now,
async access to 'module' is discouraged regardless.

Late access to 'require', however, is not uncommon and should
work as expected.

Bug: T144879
Change-Id: I6ede10fd42676bb035ea26c693c78bcdc1438a7d
---

diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js
index 3122d42c4f..c7715e5455 100644
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -1217,12 +1217,14 @@
 
 				pendingRequests.push( function () {
 					if ( moduleName && hasOwn.call( registry, moduleName ) ) {
+						// Emulate runScript() part of execute()
 						window.require = mw.loader.require;
 						window.module = registry[ moduleName ].module;
 					}
 					addScript( src ).always( function () {
-						// Clear environment
-						delete window.require;
+						// 'module.exports' should not persist after the file is executed to
+						// avoid leakage to unrelated code. 'require' should be kept, however,
+						// as asynchronous access to 'require' is allowed and expected. (T144879)
 						delete window.module;
 						r.resolve();
 
diff --git a/tests/qunit/data/defineCallMwLoaderTestCallback.js b/tests/qunit/data/defineCallMwLoaderTestCallback.js
index afd886c334..641071a220 100644
--- a/tests/qunit/data/defineCallMwLoaderTestCallback.js
+++ b/tests/qunit/data/defineCallMwLoaderTestCallback.js
@@ -1 +1 @@
-module.exports = 'Define worked.';
+module.exports = 'Defined.';
diff --git a/tests/qunit/data/requireCallMwLoaderTestCallback.js b/tests/qunit/data/requireCallMwLoaderTestCallback.js
index 8bc087b007..815a3b4868 100644
--- a/tests/qunit/data/requireCallMwLoaderTestCallback.js
+++ b/tests/qunit/data/requireCallMwLoaderTestCallback.js
@@ -1,2 +1,6 @@
-var x = require( 'test.require.define' );
-module.exports = 'Require worked.' + x;
+module.exports = {
+	immediate: require( 'test.require.define' ),
+	later: function () {
+		return require( 'test.require.define' );
+	}
+};
diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
index 41d800a0a7..b69c9e83f2 100644
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
@@ -664,7 +664,7 @@
 		} );
 	} );
 
-	QUnit.test( 'require() in debug mode', 1, function ( assert ) {
+	QUnit.test( 'require() in debug mode', function ( assert ) {
 		var path = mw.config.get( 'wgScriptPath' );
 		mw.loader.register( [
 			[ 'test.require.define', '0' ],
@@ -674,9 +674,15 @@
 		mw.loader.implement( 'test.require.define', [ QUnit.fixurl( path + '/tests/qunit/data/defineCallMwLoaderTestCallback.js' ) ] );
 
 		return mw.loader.using( 'test.require.callback' ).then( function ( require ) {
-			var exported = require( 'test.require.callback' );
-			assert.strictEqual( exported, 'Require worked.Define worked.',
-				'module.exports worked in debug mode' );
+			var cb = require( 'test.require.callback' );
+			assert.strictEqual( cb.immediate, 'Defined.', 'module.exports and require work in debug mode' );
+			// Must use try-catch because cb.later() will throw if require is undefined,
+			// which doesn't work well inside Deferred.then() when using jQuery 1.x with QUnit
+			try {
+				assert.strictEqual( cb.later(), 'Defined.', 'require works asynchrously in debug mode' );
+			} catch ( e ) {
+				assert.equal( null, String( e ), 'require works asynchrously in debug mode' );
+			}
 		}, function () {
 			assert.ok( false, 'Error callback fired while loader.using "test.require.callback" module' );
 		} );