Bartosz Dziewoński has submitted this change and it was merged.

Change subject: Correct error handling for exceptions in 'user' module
......................................................................


Correct error handling for exceptions in 'user' module

Rearrange code so that the try...catch which is supposed to catch
exceptions when evalling code actually catches them. Evaluation of
'user' module was wrapped in `mw.loader.using( 'site' ).always( ... )`,
so it could be executed asynchronously, so try...catch never caught
exceptions from it; they bubbled up to all kinds of weird places and
broke things in confusing ways.

I think the same issue could occur for any module when waiting for
legacy modules to load ('wgResourceLoaderLegacyModules').

Bug: T145970
Change-Id: I91e7d0b4e50c786f7302e30a2b7ed43c3cd0da6c
---
M resources/src/mediawiki/mediawiki.js
1 file changed, 45 insertions(+), 42 deletions(-)

Approvals:
  Krinkle: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/resources/src/mediawiki/mediawiki.js 
b/resources/src/mediawiki/mediawiki.js
index 04807f4..89bb83b 100644
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -1278,35 +1278,46 @@
                                registry[ module ].state = 'executing';
 
                                runScript = function () {
-                                       var script, markModuleReady, 
nestedAddScript, legacyWait,
+                                       var script, markModuleReady, 
nestedAddScript, legacyWait, implicitDependencies,
                                                // Expand to include 
dependencies since we have to exclude both legacy modules
                                                // and their dependencies from 
the legacyWait (to prevent a circular dependency).
                                                legacyModules = resolve( 
mw.config.get( 'wgResourceLoaderLegacyModules', [] ) );
-                                       try {
-                                               script = registry[ module 
].script;
-                                               markModuleReady = function () {
-                                                       registry[ module 
].state = 'ready';
-                                                       handlePending( module );
-                                               };
-                                               nestedAddScript = function ( 
arr, callback, i ) {
-                                                       // Recursively call 
queueModuleScript() in its own callback
-                                                       // for each element of 
arr.
-                                                       if ( i >= arr.length ) {
-                                                               // We're at the 
end of the array
-                                                               callback();
-                                                               return;
-                                                       }
 
-                                                       queueModuleScript( arr[ 
i ], module ).always( function () {
-                                                               
nestedAddScript( arr, callback, i + 1 );
-                                                       } );
-                                               };
+                                       script = registry[ module ].script;
+                                       markModuleReady = function () {
+                                               registry[ module ].state = 
'ready';
+                                               handlePending( module );
+                                       };
+                                       nestedAddScript = function ( arr, 
callback, i ) {
+                                               // Recursively call 
queueModuleScript() in its own callback
+                                               // for each element of arr.
+                                               if ( i >= arr.length ) {
+                                                       // We're at the end of 
the array
+                                                       callback();
+                                                       return;
+                                               }
 
-                                               legacyWait = ( $.inArray( 
module, legacyModules ) !== -1 )
-                                                       ? $.Deferred().resolve()
-                                                       : mw.loader.using( 
legacyModules );
+                                               queueModuleScript( arr[ i ], 
module ).always( function () {
+                                                       nestedAddScript( arr, 
callback, i + 1 );
+                                               } );
+                                       };
 
-                                               legacyWait.always( function () {
+                                       implicitDependencies = ( $.inArray( 
module, legacyModules ) !== -1 )
+                                               ? []
+                                               : legacyModules;
+
+                                       if ( module === 'user' ) {
+                                               // Implicit dependency on the 
site module. Not real dependency because
+                                               // it should run after 'site' 
regardless of whether it succeeds or fails.
+                                               implicitDependencies.push( 
'site' );
+                                       }
+
+                                       legacyWait = implicitDependencies.length
+                                               ? mw.loader.using( 
implicitDependencies )
+                                               : $.Deferred().resolve();
+
+                                       legacyWait.always( function () {
+                                               try {
                                                        if ( $.isArray( script 
) ) {
                                                                
nestedAddScript( script, markModuleReady, 0 );
                                                        } else if ( typeof 
script === 'function' ) {
@@ -1319,29 +1330,21 @@
                                                                // Site and 
user modules are legacy scripts that run in the global scope.
                                                                // This is 
transported as a string instead of a function to avoid needing
                                                                // to use 
string manipulation to undo the function wrapper.
-                                                               if ( module === 
'user' ) {
-                                                                       // 
Implicit dependency on the site module. Not real dependency because
-                                                                       // it 
should run after 'site' regardless of whether it succeeds or fails.
-                                                                       
mw.loader.using( 'site' ).always( function () {
-                                                                               
$.globalEval( script );
-                                                                               
markModuleReady();
-                                                                       } );
-                                                               } else {
-                                                                       
$.globalEval( script );
-                                                                       
markModuleReady();
-                                                               }
+                                                               $.globalEval( 
script );
+                                                               
markModuleReady();
+
                                                        } else {
                                                                // Module 
without script
                                                                
markModuleReady();
                                                        }
-                                               } );
-                                       } catch ( e ) {
-                                               // This needs to NOT use mw.log 
because these errors are common in production mode
-                                               // and not in debug mode, such 
as when a symbol that should be global isn't exported
-                                               registry[ module ].state = 
'error';
-                                               mw.track( 
'resourceloader.exception', { exception: e, module: module, source: 
'module-execute' } );
-                                               handlePending( module );
-                                       }
+                                               } catch ( e ) {
+                                                       // This needs to NOT 
use mw.log because these errors are common in production mode
+                                                       // and not in debug 
mode, such as when a symbol that should be global isn't exported
+                                                       registry[ module 
].state = 'error';
+                                                       mw.track( 
'resourceloader.exception', { exception: e, module: module, source: 
'module-execute' } );
+                                                       handlePending( module );
+                                               }
+                                       } );
                                };
 
                                // Add localizations to message system

-- 
To view, visit https://gerrit.wikimedia.org/r/311329
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I91e7d0b4e50c786f7302e30a2b7ed43c3cd0da6c
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Bartosz Dziewoński <matma....@gmail.com>
Gerrit-Reviewer: Bartosz Dziewoński <matma....@gmail.com>
Gerrit-Reviewer: Jack Phoenix <j...@countervandalism.net>
Gerrit-Reviewer: Krinkle <krinklem...@gmail.com>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to