Last modified: 2014-09-21 10:17:35 UTC
https://gerrit.wikimedia.org/r/152122 adds an additional if(window.mw){...} around of user scripts. This breaks some scripts in Firefox. Some scripts use forward references of functions like foo(); function foo() { console.log( 'foo' ); } Forward references normally work, but not in an if block. if ( true ) { foo(); function foo() { console.log( 'foo' ); } } generates ReferenceError: foo is not defined A possible workaround is to put this in an additional closure because if ( true ) { ( function () { foo(); function foo() { console.log( 'foo' ); } }() ); } works also in Firefox. Instead of if(window.mw){...} the script has to wrapped with if(window.mw){(function(){...}());}
(In reply to Fomafix from comment #0) > A possible workaround is to put this in an additional closure because > > if ( true ) { > ( function () { > foo(); > function foo() { > console.log( 'foo' ); > } > }() ); > } > > works also in Firefox. This will break all scripts using var foo = 'bar'; to set global variables, as it will turn them into local ones.
> This will break all scripts using var foo = 'bar'; to set global variables, > as it will turn them into local ones. Oh, yes. This would generate other problems.
I had to fix a user's userscript yesterday due to this bug.
(In reply to Fomafix from comment #0) > ... > A possible workaround is to put this in an additional closure ... For that, see bug 63728.
$.globalEval() [1] as suggested in bug 63587 would be a possible solution. Maybe this can added as a general optional parameter for all modules. [1] https://api.jquery.com/jQuery.globalEval/
Why do user scripts have to be enveloped in that if-clause anyway? 1. In most browsers the global mw will be defined before the user script is executed, so there is no need to test for it. 2. If for some reason the loading order is wrong, it would be a bug, that should be reported, and for this should be visible. Testing for the existence of mw will silently hide such a bug. 3. The only case in which mw will not be defined are unsupported browsers (like IE6) which are blacklisted in the startup function. But if a user of such a browser has a user script that doesn't depend on mw and jQuery, why shouldn't it be executed? So there is actually no benefit of wrapping user scripts in that if-clause, but several reasons not to do so.
Michael, if common.js is non-blank, this will lead to site-wide errors in IE6 (and other disabled browsers). That was one of the motivations for Timo's change. But I'll let him comment on strategies to resolve the issue with user scripts.
Another example: https://de.wikipedia.org/w/index.php?title=Wikipedia:Technik/Werkstatt&oldid=133432119#.C3.84nderung_im_Javascript.3F
(In reply to Erik Moeller from comment #7) > Michael, if common.js is non-blank, this will lead to site-wide errors in > IE6 (and other disabled browsers). My comment was limited to user scripts, and did not include site-wide scripts. For MediaWiki:Common.js etc. I agree it is the right thing to wrap it into that if-clause. But I don't think it should be done for User:Foo/common.js etc. While most site-wide scripts should be fixed by now, most user scripts aren't, because it is much more difficult to fix lots of user scripts that to fix a few site-wide scripts. If a user script breaks because it refers to mw without checking for its existence though the user uses a blacklisted browser, it's solely the user's responsibility to fix his own script. But a user script should never break, because the software changes it in a way that changes valid syntax into invalid (as it is in this case, a FunctionDecleration is allowed in global scope, but not in block scope, and the if-clause changes the scope of the script from global to block).
As actually nobody explicitly quoted the relevant standards yet: * ECMAScript defines FunctionDecleration and FunctionExpression. (http://ecma-international.org/ecma-262/5.1/#sec-13) * Firefox (and other browsers) extend the ECMAScript syntax, for Firefox there is also FunctionStatement. (https://developer.mozilla.org/en-US/docs/Web/JavaScript /Reference/Functions_and_function_scope#Conditionally_defining_a_function) * All these look similar, but behave differently. * User scripts affected by this bug use functions defined as FunctionDecleration. * In ECMAScript a FunctionDecleration is not allowed inside a block. * When such a user script is wrapped in an if-clause, Firefox therefor treats the functions as FunctionStatement instead. * This can cause errors, because functions defined as FunctionStatement can't be called before they are defined, while this is possible for functions defined as FunctionDecleration. * Additionally FunctionStatements are not valid in strict mode. This isn't a problem here, because even if the script declares strict mode globally, this won't have any effect after it is wrapped in that if-clause (because the 'use strict'; no longer is in the first line then). Nevertheless one shouldn't rely on syntax extensions by different browsers.
This is not a new problem. Users using older browsers have always had fatal errors because jquery/mediawiki are not loaded in blacklisted browsers not supporting our required version for the javascript engine. They used to fail on a ReferenceError for $ or mw undefined. This error happens because, unlike modern modules that are only loaded in a supported environment, site/user modules are executed in the global scope for legacy reasons and unconditionally referenced in the HTML. As we do, we move with the times and naturally raise our requirements as new technologies come available, become dependant on it, and eventually no longer support older engines for javascript features (basic functionality will remain supported in these browsers however!). Up until recently this applied to MSIE 5, Firefox 2 (and below). This was raised to MSIE 6, Firefox 2 and Opera 11 (and below). This has exposed the ReferenceError wider. Thus, we've wrapped the site/user module in the same "if(window.mw)" conditional as we've wrapped other scripts for years now (mw.config.set, mw.user.options etc.). This means users in older browsers no longer get errors (especially for site scripts. For user scripts it's a minor issue because they are their own scripts, if they frequently use older browsers, they probably don't need that user script as it wouldn't be executed anyway). --- We can't wrap the code in a closure because they need to be executed in the global scope. If we can wrap them in a closure, they wouldn't have to be loaded via hardcoded script tags, wrapped in mw.loader.implement closures and we wouldn't have this problem in the first place. Many of these scripts do indeed have global var statements or function declarations (including relying on function hoisting). To my knowledge, conditional function declarations (or variable declarations for that matter) are a bit silly because it's unconditionally hoisted. e.g. > bar; >> ReferenceError: bar is undefined > if (true) { > var bar = 5; > } > bar; >> (nothing) Because 'var bar;' was hoisted. The same applies to function declarations (executed in V8 under Chrome) > function bar() { return 1; } > bar(); >> 1 > if (true) { > function bar() { return 1; } > } else { > function bar() { return 2; } > } > bar(); >> 2 This is bad practice, but I figured it'd be harmless for an implementation detail like if-window-mw (considering there'd never be any statements before or after the if-statement).
> > function bar() { return 1; } > > bar(); > >> 1 > > > if (true) { > > function bar() { return 1; } > > } else { > > function bar() { return 2; } > > } > > bar(); > >> 2 btw, in Firefox this yields 1.
> quux(); if (true) { function quux() { return 1; } } else { function quux() { return 2; } } V8> 2 SpiderMonkey> quux is undefined
Hm.. so yeah, that's annoying. The condition-wrap is supposed to hide an issue in old browsers, not introduce issues in modern browsers. So far I've only seen two possible solutions: 1) Wrap in $.globalEval, passing the function body as a string. Something like: if (window.mw) { $.globalEval('foo(); function foo() { return 1; }'); } Produced by $content = 'if (window.mw) { $.globalEval(' . FormatJson::encode( $content ) . ' ); }';'; Or: $content = 'if (window.mw) { ' . Xml::encodeJsCall( '$.globalEval', array( $content ) ) . '}';'; Though this is relatively cheap and doesn't bloat the output much in terms of size, it does make it impossible to minify after the fact, so we should probably run the js-filter on it first. 2) Parse the javascript and transform local var statements and function declarations to explicit instead of implicit global properties so that scope doesn't matter. This can be done with something like emscripten, and then use the AST to replace var statements and function declarations with window[key] assignments (and manually hoist function declarations I guess). Then it's safe to use a closure (or even a plain wrap, like now). This requires getting a JS parser in PHP though, or shelling out to nodejs.
Open to other ideas. Of neither #1 or #2 is acceptable, we may have to remove the if-wrap for the time being since modern Firefox triumps priority over grade C browsers.
What about only wrapping the mw.loader.state({"user":"ready"}); at the bottom in an if-clause (or prefixing it with window.mw && ) and leaving the rest of the script alone? I hadn't seen that call to mw.loader.state when I wrote comment 6, but I still think that my general remarks apply: If a user script throws a ReferenceError because it uses mw or $, but the user uses a blacklisted browser, it's the user's fault, and nothing MediaWiki needs to try to work around. And if it doesn't use mw or $, there is no reason not to execute it. This still leaves the question whether site-wide scripts should be wrapped. If admins fix errors with these scripts quickly, it is better to wrap them (as errors in Firefox are noticed much faster than errors in IE6), but if admins don't fix errors, it would be better not to wrap them (as errors in IE6 are less important than errors in Firefox). It seems that the errors in site-wide scripts were fixed quite fast in the last few days, so I think the if-clause is ok for site-wide scripts.
Alternatively, we might be able to do the condition outside the script. Perhaps something like replace this: <script src="./load.php?modules=user&only=scripts&user=Example&version=1"></script> with: <script>if (cond) document.write("\u003Cscript src=\"./load.php?modules=user\u0026only=scripts\u0026user=Example\u0026version=1\"\u003E\u003C/script\u003E");</script> Of course, document.write is horrible, but nothing about this is pretty. document.write can be unreliable, but there are certain scenarios in which it is deterministically and cross-browser safe to use. I wonder if this is such case.
Another solution would be to return early instead of wrap the whole thing. While a return statement is not allowed in the global scope, a throw works fine. if (!cond) throw ''; .. rest of script .. .. mw/$ .. This doesn't cause much disruption since javascript keeps running. The throw only aborts the current script, which is the user/site script we want to unload.
(In reply to Krinkle from comment #19) > Another solution would be to return early instead of wrap the whole thing. > > While a return statement is not allowed in the global scope, a throw works > fine. > > > if (!cond) > throw ''; > .. rest of script .. > .. mw/$ .. > > This doesn't cause much disruption since javascript keeps running. The throw > only aborts the current script, which is the user/site script we want to > unload. Hm.. continuing the monolog; while exceptions are harmless indeed, that's what we started with. Execute the script and let it fail on mw/$ undefined. This wasn't really a problem. Except that old IE tends to actually expose script errors to regular users through an alert, so I guess we can't do this.
I quite like the document.write one, seems it will have the least impact on users with non-broken user JS and should be easy to implement?
Erm, so something was introduced to deal with users of outdated browsers (say, 1 % of our users) and that something broke something else for users of non-outdated browsers (Firefox, about 20 % I guess?) and instead of fixing the problem immediately, what happens is... nothing? Hello???
Krinkle: So is there some conclusion what should be done here, and by who?
I've proposed various solutions. None of which are field-tested, best practice and known to work cross-browser. The inline doc.write solution seems the most straight forward. I'll implement that and test it in as many browsers as I can. While we're already using doc.write in other places, embedding it in the HTML can have side-effects with regards to caching and/or trigger weird bugs (e.g. old IE is known to have incompetent security policies that are undocumented and not compatible with "the web" and can cause semi-random things to fail sometimes).
Change 159086 had a related patch set uploaded by Krinkle: resourceloader: Do condition wrap in HTML instead of JS response https://gerrit.wikimedia.org/r/159086
Change 159086 merged by jenkins-bot: resourceloader: Condition-wrap the HTML tag instead of JS response https://gerrit.wikimedia.org/r/159086
This is marked as "High major", so I assume that the fix will be deployed by someone on Monday?
Bartosz: I'm just seeing this now. No one added it this morning; you should have :)
It wasn't backported in the end :( The fix will roll out with 1.24wmf22.
