From 5d7ae11c57bcd4491fd61ad2f4d60a5fddb9573d Mon Sep 17 00:00:00 2001 From: lupo Date: Wed, 9 May 2012 11:01:19 +0200 Subject: [PATCH] (bug 35240) Fix mw.loader state machine. Main changes: * handlePending() correctly handles "missing" and "error" states and propagates error states up the dependency tree. * handlePending() is called whenever a module enters one of the states "ready", "error", or "missing" (in execute() and mw.loader.state()). * load() filters out not only undefined modules, but -- by the logic of the comment there -- also modules in state "error" or "missing". Minor changes: * recurse() renamed to sortDependencies(), also uses a hash for unresolved now instead of an array. * execute() was never called with the second parameter "callback", hence I've removed it. * simplified the "are all dependencies 'ready'?" test and moved it to its own function. The change comes with additional QUnit tests for mw.loader. If I run these tests against the current mediawiki.js, several of them fail. In particular test #86 ("mw.loader real missing dependency") is instructive: if the server returns "missing" for a module, dependent modules never progress beyond "loaded", and if there are jobs (from mw.loader.using()) depending on such a missing module (directly or indirectly), neither ready() nor error() is ever called. Running the tests against the changed mediawiki.js in this change, they all succeed for me. Patchset 2: whitespace changes, $_GET instead of $_REQUEST in testloader. Patchset 3: XML::encodeJsVar() in testloader, deepEqual() in Qunit tests. Patchset 4: rebase Patchset 5: Amend commit message only (typo) Change-Id: Ia67edfc07fc9237def04ed13bb2cee16e519d7af --- resources/mediawiki/mediawiki.js | 236 +++++++++++------- tests/qunit/data/testloader.php | 100 ++++++++ .../resources/mediawiki/mediawiki.test.js | 123 +++++++++ 3 files changed, 372 insertions(+), 87 deletions(-) create mode 100644 tests/qunit/data/testloader.php diff --git a/resources/mediawiki/mediawiki.js b/resources/mediawiki/mediawiki.js index 4f345ab7ea..a9fa1fd84d 100644 --- a/resources/mediawiki/mediawiki.js +++ b/resources/mediawiki/mediawiki.js @@ -481,9 +481,19 @@ var mw = ( function ( $, undefined ) { } /** - * Recursively resolves dependencies and detects circular references + * Resolves dependencies and detects circular references. + * + * @param module String Name of the top-level module whose dependencies shall be + * resolved and sorted. + * @param resolved Array Returns a topological sort of the given module and its + * dependencies, such that later modules depend on earlier modules. The array + * contains the module names. If the array contains already some module names, + * this function appends its result to the pre-existing array. + * @param unresolved Object [optional] Hash used to track the current dependency + * chain; used to report loops in the dependency graph. + * @throws Error if any unregistered module or a dependency loop is encountered */ - function recurse( module, resolved, unresolved ) { + function sortDependencies( module, resolved, unresolved ) { var n, deps, len; if ( registry[module] === undefined ) { @@ -497,12 +507,20 @@ var mw = ( function ( $, undefined ) { registry[module].dependencies = [registry[module].dependencies]; } } + if ( $.inArray( module, resolved ) !== -1 ) { + // Module already resolved; nothing to do. + return; + } + // unresolved is optional, supply it if not passed in + if ( !unresolved ) { + unresolved = {}; + } // Tracks down dependencies deps = registry[module].dependencies; len = deps.length; for ( n = 0; n < len; n += 1 ) { if ( $.inArray( deps[n], resolved ) === -1 ) { - if ( $.inArray( deps[n], unresolved ) !== -1 ) { + if ( unresolved[deps[n]] ) { throw new Error( 'Circular reference detected: ' + module + ' -> ' + deps[n] @@ -510,40 +528,37 @@ var mw = ( function ( $, undefined ) { } // Add to unresolved - unresolved[unresolved.length] = module; - recurse( deps[n], resolved, unresolved ); - // module is at the end of unresolved - unresolved.pop(); + unresolved[module] = true; + sortDependencies( deps[n], resolved, unresolved ); + delete unresolved[module]; } } resolved[resolved.length] = module; } /** - * Gets a list of module names that a module depends on in their proper dependency order + * Gets a list of module names that a module depends on in their proper dependency + * order. * * @param module string module name or array of string module names * @return list of dependencies, including 'module'. * @throws Error if circular reference is detected */ function resolve( module ) { - var modules, m, deps, n, resolved; + var m, resolved; // Allow calling with an array of module names if ( $.isArray( module ) ) { - modules = []; + resolved = []; for ( m = 0; m < module.length; m += 1 ) { - deps = resolve( module[m] ); - for ( n = 0; n < deps.length; n += 1 ) { - modules[modules.length] = deps[n]; - } + sortDependencies( module[m], resolved ); } - return modules; + return resolved; } if ( typeof module === 'string' ) { resolved = []; - recurse( module, resolved, [] ); + sortDependencies( module, resolved ); return resolved; } @@ -598,57 +613,107 @@ var mw = ( function ( $, undefined ) { } /** - * Automatically executes jobs and modules which are pending with satistifed dependencies. + * Determine whether all dependencies are in state 'ready', which means we may + * execute the module or job now. + * + * @param dependencies Array dependencies (module names) to be checked. * - * This is used when dependencies are satisfied, such as when a module is executed. + * @return Boolean true if all dependencies are in state 'ready', false otherwise */ - function handlePending( module ) { - var j, r; + function allReady( dependencies ) { + return filter( 'ready', dependencies ).length === dependencies.length; + } - try { - // Run jobs whose dependencies have just been met - for ( j = 0; j < jobs.length; j += 1 ) { - if ( compare( - filter( 'ready', jobs[j].dependencies ), - jobs[j].dependencies ) ) - { - var callback = jobs[j].ready; - jobs.splice( j, 1 ); - j -= 1; - if ( $.isFunction( callback ) ) { - callback(); + /** + * Log a message to window.console, if possible. Useful to force logging of some + * errors that are otherwise hard to detect, even if mw.log is not available. (I.e., + * this logs also if not in debug mode.) + * + * @param msg String text for the log entry + * @param e Error [optional] to also log. + */ + function log( msg, e ) { + if ( window.console && typeof window.console.log === 'function' ) { + console.log( msg ); + if ( e ) { + console.log( e ); + } + } + } + + /** + * A module has entered state 'ready', 'error', or 'missing'. Automatically update pending jobs + * and modules that depend upon this module. if the given module failed, propagate the 'error' + * state up the dependency tree; otherwise, execute all jobs/modules that now have all their + * dependencies satisfied. On jobs depending on a failed module, run the error callback, if any. + * + * @param module String name of module that entered one of the states 'ready', 'error', or 'missing'. + */ + function handlePending( module ) { + var j, job, hasErrors, m, stateChange; + + // Modules. + if ( $.inArray( registry[module].state, ['error', 'missing'] ) !== -1 ) { + // If the current module failed, mark all dependent modules also as failed. + // Iterate until steady-state to propagate the error state upwards in the + // dependency tree. + do { + stateChange = false; + for ( m in registry ) { + if ( $.inArray( registry[m].state, ['error', 'missing'] ) === -1 ) { + if ( filter( ['error', 'missing'], registry[m].dependencies ).length > 0 ) { + registry[m].state = 'error'; + stateChange = true; + } } } - } - // Execute modules whose dependencies have just been met - for ( r in registry ) { - if ( registry[r].state === 'loaded' ) { - if ( compare( - filter( ['ready'], registry[r].dependencies ), - registry[r].dependencies ) ) - { - execute( r ); + } while ( stateChange ); + } + + // Execute all jobs whose dependencies are either all satisfied or contain at least one failed module. + for ( j = 0; j < jobs.length; j += 1 ) { + hasErrors = filter( ['error', 'missing'], jobs[j].dependencies ).length > 0; + if ( hasErrors || allReady( jobs[j].dependencies ) ) { + // All dependencies satisfied, or some have errors + job = jobs[j]; + jobs.splice( j, 1 ); + j -= 1; + try { + if ( hasErrors ) { + throw new Error ("Module " + module + " failed."); + } else { + if ( $.isFunction( job.ready ) ) { + job.ready(); + } + } + } catch ( e ) { + if ( $.isFunction( job.error ) ) { + try { + job.error( e, [module] ); + } catch ( ex ) { + // A user-defined operation raised an exception. Swallow to protect + // our state machine! + log( 'mw.loader::handlePending> Exception thrown by job.error()', ex ); + } } } } - } catch ( e ) { - // Run error callbacks of jobs affected by this condition - for ( j = 0; j < jobs.length; j += 1 ) { - if ( $.inArray( module, jobs[j].dependencies ) !== -1 ) { - if ( $.isFunction( jobs[j].error ) ) { - jobs[j].error( e, module ); - } - jobs.splice( j, 1 ); - j -= 1; + } + + if ( registry[module].state === 'ready' ) { + // The current module became 'ready'. Recursively execute all dependent modules that are loaded + // and now have all dependencies satisfied. + for ( m in registry ) { + if ( registry[m].state === 'loaded' && allReady( registry[m].dependencies ) ) { + execute( m ); } } - throw e; } } /** * Adds a script tag to the DOM, either using document.write or low-level DOM manipulation, - * depending on whether document-ready has occured yet and whether we are in async mode. + * depending on whether document-ready has occurred yet and whether we are in async mode. * * @param src String: URL to script, will be used as the src attribute in the script tag * @param callback Function: Optional callback which will be run when the script is done @@ -725,7 +790,7 @@ var mw = ( function ( $, undefined ) { * * @param module string module name to execute */ - function execute( module, callback ) { + function execute( module ) { var style, media, i, script, markModuleReady, nestedAddScript; if ( registry[module] === undefined ) { @@ -766,9 +831,6 @@ var mw = ( function ( $, undefined ) { markModuleReady = function() { registry[module].state = 'ready'; handlePending( module ); - if ( $.isFunction( callback ) ) { - callback(); - } }; nestedAddScript = function ( arr, callback, async, i ) { // Recursively call addScript() in its own callback @@ -794,11 +856,9 @@ var mw = ( function ( $, undefined ) { } 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 - if ( window.console && typeof window.console.log === 'function' ) { - console.log( 'mw.loader::execute> Exception thrown by ' + module + ': ' + e.message ); - console.log( e ); - } + log('mw.loader::execute> Exception thrown by ' + module + ': ' + e.message, e); registry[module].state = 'error'; + handlePending( module ); } } @@ -1157,18 +1217,16 @@ var mw = ( function ( $, undefined ) { if ( registry[module] !== undefined && registry[module].script !== undefined ) { throw new Error( 'module already implemented: ' + module ); } - // Mark module as loaded - registry[module].state = 'loaded'; // Attach components registry[module].script = script; registry[module].style = style; registry[module].messages = msgs; - // Execute or queue callback - if ( compare( - filter( ['ready'], registry[module].dependencies ), - registry[module].dependencies ) ) - { - execute( module ); + // The module may already have been marked as erroneous + if ( $.inArray( registry[module].state, ['error', 'missing'] ) === -1 ) { + registry[module].state = 'loaded'; + if ( allReady( registry[module].dependencies ) ) { + execute( module ); + } } }, @@ -1192,21 +1250,19 @@ var mw = ( function ( $, undefined ) { } // Resolve entire dependency map dependencies = resolve( dependencies ); - // If all dependencies are met, execute ready immediately - if ( compare( filter( ['ready'], dependencies ), dependencies ) ) { + if ( allReady( dependencies ) ) { + // Run ready immediately if ( $.isFunction( ready ) ) { ready(); } - } - // If any dependencies have errors execute error immediately - else if ( filter( ['error'], dependencies ).length ) { + } else if ( filter( ['error', 'missing'], dependencies ).length ) { + // Execute error immediately if any dependencies have errors if ( $.isFunction( error ) ) { - error( new Error( 'one or more dependencies have state "error"' ), + error( new Error( 'one or more dependencies have state "error" or "missing"' ), dependencies ); } - } - // Since some dependencies are not yet ready, queue up a request - else { + } else { + // Not all dependencies are ready: queue up a request request( dependencies, ready, error ); } }, @@ -1225,7 +1281,7 @@ var mw = ( function ( $, undefined ) { * be assumed if loading a URL, and false will be assumed otherwise. */ load: function ( modules, type, async ) { - var filtered, m; + var filtered, m, module; // Validate input if ( typeof modules !== 'object' && typeof modules !== 'string' ) { @@ -1264,24 +1320,29 @@ var mw = ( function ( $, undefined ) { // an array of unrelated modules, whereas the modules passed to // using() are related and must all be loaded. for ( filtered = [], m = 0; m < modules.length; m += 1 ) { - if ( registry[modules[m]] !== undefined ) { - filtered[filtered.length] = modules[m]; + module = registry[modules[m]]; + if ( module !== undefined ) { + if ( $.inArray( module.state, ['error', 'missing'] ) === -1 ) { + filtered[filtered.length] = modules[m]; + } } } + if (filtered.length === 0) { + return; + } // Resolve entire dependency map filtered = resolve( filtered ); - // If all modules are ready, nothing dependency be done - if ( compare( filter( ['ready'], filtered ), filtered ) ) { + // If all modules are ready, nothing to be done + if ( allReady( filtered ) ) { return; } - // If any modules have errors - if ( filter( ['error'], filtered ).length ) { + // If any modules have errors: also quit. + if ( filter( ['error', 'missing'], filtered ).length ) { return; } - // Since some modules are not yet ready, queue up a request + // Since some modules are not yet ready, queue up a request. request( filtered, null, null, async ); - return; }, /** @@ -1302,7 +1363,8 @@ var mw = ( function ( $, undefined ) { if ( registry[module] === undefined ) { mw.loader.register( module ); } - if ( state === 'ready' && registry[module].state !== state) { + if ( $.inArray(state, ['ready', 'error', 'missing']) !== -1 + && registry[module].state !== state ) { // Make sure pending modules depending on this one get executed if their // dependencies are now fulfilled! registry[module].state = state; diff --git a/tests/qunit/data/testloader.php b/tests/qunit/data/testloader.php new file mode 100644 index 0000000000..7f4c48dfa0 --- /dev/null +++ b/tests/qunit/data/testloader.php @@ -0,0 +1,100 @@ + array ( + 'src' => 'mw.loader.implement("test.use_missing", function () {start(); ok(false, "Module test.use_missing should not run.");}, {}, {});', + 'deps' => array ( 'test.missing' ) + ), + 'test.use_missing2' => array ( + 'src' => 'mw.loader.implement("test.use_missing2", function () {start(); ok(false, "Module test.use_missing2 should not run.");}, {}, {});', + 'deps' => array ( 'test.missing2' ) + ) +); + +// Copied from ResourceLoaderContext +function expandModuleNames( $modules ) { + $retval = array(); + // For backwards compatibility with an earlier hack, replace ! with . + $modules = str_replace( '!', '.', $modules ); + $exploded = explode( '|', $modules ); + foreach ( $exploded as $group ) { + if ( strpos( $group, ',' ) === false ) { + // This is not a set of modules in foo.bar,baz notation + // but a single module + $retval[] = $group; + } else { + // This is a set of modules in foo.bar,baz notation + $pos = strrpos( $group, '.' ); + if ( $pos === false ) { + // Prefixless modules, i.e. without dots + $retval = explode( ',', $group ); + } else { + // We have a prefix and a bunch of suffixes + $prefix = substr( $group, 0, $pos ); // 'foo' + $suffixes = explode( ',', substr( $group, $pos + 1 ) ); // array( 'bar', 'baz' ) + foreach ( $suffixes as $suffix ) { + $retval[] = "$prefix.$suffix"; + } + } + } + } + return $retval; +} + +function addModule ( $moduleName, &$gotten ) { + global $modules; + + $result = ""; + if ( isset( $gotten[$moduleName] ) ) { + return $result; + } + $gotten[$moduleName] = true; + if ( isset( $modules[$moduleName] ) ) { + $deps = $modules[$moduleName]['deps']; + foreach ( $deps as $depName ) { + $result .= addModule( $depName, $gotten ) . "\n"; + } + $result .= $modules[$moduleName]['src'] . "\n"; + } else { + $result .= 'mw.loader.state( ' . Xml::encodeJsVar( $moduleName ) . ', "missing" );' . "\n"; + } + return $result . "\n"; +} + +$result = ""; + +if ( isset( $_GET['modules'] ) ) { + $toGet = expandModuleNames( $_GET['modules'] ); + $gotten = array(); + foreach ( $toGet as $moduleName ) { + $result .= addModule( $moduleName, $gotten ); + } +} + +echo $result; diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js index fd4710fc69..2f115215af 100644 --- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js +++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js @@ -261,6 +261,129 @@ test( 'mw.loader.implement', function () { }); +test( 'mw.loader erroneous indirect dependency', function() { + expect( 3 ); + mw.loader.register( [ + ['test.module1', '0'], + ['test.module2', '0', ['test.module1']], + ['test.module3', '0', ['test.module2']] + ] ); + mw.loader.implement( 'test.module1', function() { throw new Error( 'expected' ); }, {}, {} ); + strictEqual( mw.loader.getState( 'test.module1' ), 'error', 'Expected "error" state for test.module1' ); + strictEqual( mw.loader.getState( 'test.module2' ), 'error', 'Expected "error" state for test.module2' ); + strictEqual( mw.loader.getState( 'test.module3' ), 'error', 'Expected "error" state for test.module3' ); +} ); + +test( 'mw.loader out-of-order implementation', function() { + expect( 9 ); + mw.loader.register( [ + ['test.module4', '0'], + ['test.module5', '0', ['test.module4']], + ['test.module6', '0', ['test.module5']] + ] ); + mw.loader.implement( 'test.module4', function() {}, {}, {} ); + strictEqual( mw.loader.getState( 'test.module4' ), 'ready', 'Expected "ready" state for test.module4' ); + strictEqual( mw.loader.getState( 'test.module5' ), 'registered', 'Expected "registered" state for test.module5' ); + strictEqual( mw.loader.getState( 'test.module6' ), 'registered', 'Expected "registered" state for test.module6' ); + mw.loader.implement( 'test.module6', function() {}, {}, {} ); + strictEqual( mw.loader.getState( 'test.module4' ), 'ready', 'Expected "ready" state for test.module4' ); + strictEqual( mw.loader.getState( 'test.module5' ), 'registered', 'Expected "registered" state for test.module5' ); + strictEqual( mw.loader.getState( 'test.module6' ), 'loaded', 'Expected "loaded" state for test.module6' ); + mw.loader.implement( 'test.module5', function() {}, {}, {} ); + strictEqual( mw.loader.getState( 'test.module4' ), 'ready', 'Expected "ready" state for test.module4' ); + strictEqual( mw.loader.getState( 'test.module5' ), 'ready', 'Expected "ready" state for test.module5' ); + strictEqual( mw.loader.getState( 'test.module6' ), 'ready', 'Expected "ready" state for test.module6' ); +} ); + +test( 'mw.loader missing dependency', function() { + expect( 13 ); + mw.loader.register( [ + ['test.module7', '0'], + ['test.module8', '0', ['test.module7']], + ['test.module9', '0', ['test.module8']] + ] ); + mw.loader.implement( 'test.module8', function() {}, {}, {} ); + strictEqual( mw.loader.getState( 'test.module7' ), 'registered', 'Expected "registered" state for test.module7' ); + strictEqual( mw.loader.getState( 'test.module8' ), 'loaded', 'Expected "loaded" state for test.module8' ); + strictEqual( mw.loader.getState( 'test.module9' ), 'registered', 'Expected "registered" state for test.module9' ); + mw.loader.state( 'test.module7', 'missing' ); + strictEqual( mw.loader.getState( 'test.module7' ), 'missing', 'Expected "missing" state for test.module7' ); + strictEqual( mw.loader.getState( 'test.module8' ), 'error', 'Expected "error" state for test.module8' ); + strictEqual( mw.loader.getState( 'test.module9' ), 'error', 'Expected "error" state for test.module9' ); + mw.loader.implement( 'test.module9', function() {}, {}, {} ); + strictEqual( mw.loader.getState( 'test.module7' ), 'missing', 'Expected "missing" state for test.module7' ); + strictEqual( mw.loader.getState( 'test.module8' ), 'error', 'Expected "error" state for test.module8' ); + strictEqual( mw.loader.getState( 'test.module9' ), 'error', 'Expected "error" state for test.module9' ); + mw.loader.using( + ['test.module7'], + function() { + ok( false, "Success fired despite missing dependency" ); + ok( true , "QUnit expected() count dummy" ); + }, + function( e, dependencies ) { + strictEqual( $.isArray( dependencies ), true, 'Expected array of dependencies' ); + deepEqual( dependencies, ['test.module7'], 'Error callback called with module test.module7' ); + } + ); + mw.loader.using( + ['test.module9'], + function() { + ok( false, "Success fired despite missing dependency" ); + ok( true , "QUnit expected() count dummy" ); + }, + function( e, dependencies ) { + strictEqual( $.isArray( dependencies ), true, 'Expected array of dependencies' ); + dependencies.sort(); + deepEqual( + dependencies, + ['test.module7', 'test.module8', 'test.module9'], + 'Error callback called with all three modules as dependencies' + ); + } + ); +} ); + +test( 'mw.loader real missing dependency', function() { + expect( 6 ); + + mw.loader.addSource( + 'test', + {'loadScript' : QUnit.fixurl( mw.config.get( 'wgScriptPath' ) + '/tests/qunit/data/testloader.php' )} + ); + mw.loader.register( [ + ['test.missing', '0', [], null, 'test'], ['test.missing2', '0', [], null, 'test'], + ['test.use_missing', '0', ['test.missing'], null, 'test'], + ['test.use_missing2', '0', ['test.missing2'], null, 'test'] + ] ); + + stop(); + // Asynch ahead + + mw.loader.load( ['test.use_missing'] ); + + function verifyModuleStates() { + strictEqual( mw.loader.getState( 'test.missing' ), 'missing', 'Module "test.missing" must have state "missing"' ); + strictEqual( mw.loader.getState( 'test.missing2' ), 'missing', 'Module "test.missing2" must have state "missing"' ); + strictEqual( mw.loader.getState( 'test.use_missing' ), 'error', 'Module "test.use_missing" must have state "error"' ); + strictEqual( mw.loader.getState( 'test.use_missing2' ), 'error', 'Module "test.use_missing2" must have state "error"' ); + } + + mw.loader.using( ['test.use_missing2'], + function() { + start(); + ok( false, "Success called wrongly." ); + ok( true , "QUnit expected() count dummy" ); + verifyModuleStates(); + }, + function( e, dependencies ) { + start(); + ok( true, "Error handler called correctly." ); + deepEqual( dependencies, ['test.missing2'], "Dependencies correct." ); + verifyModuleStates(); + } + ); +} ); + test( 'mw.loader bug29107' , function () { expect(2); -- 2.20.1