JS: addOnloadHook() calls after runOnloadHook() fail
Closed, ResolvedPublic

Description

Author: conrad.irwin

Description:
It is possible for addOnloadHook() to be called after runOnloadhooks(). This can happen, for example, when a new script tag is added to the document head using javascript in IE7. This results in the hook not being run, which can cause problems.

Splarka on IRC proposed the following backwards-compatible solution.

function addOnloadHook(hookFunct, strict) {
// Allows add-on scripts to add onload functions
if(!doneOnloadHook) {

		onloadFuncts[onloadFuncts.length] = hookFunct;

} else if(!strict) {

		hookFunct();  // bug in MSIE script loading

}
}


Version: unspecified
Severity: minor

bzimport added a project: MediaWiki-Interface.Via ConduitNov 21 2014, 10:03 PM
bzimport added a subscriber: wikibugs-l.
bzimport set Reference to bz12773.
bzimport created this task.Via LegacyJan 24 2008, 12:56 PM
bzimport added a comment.Via ConduitJan 27 2008, 5:00 AM

random832 wrote:

This also affects firefox. What is the strict flag needed for; I can't think of a situation as a script author where you would want your function to be dropped on the floor due to a race condition.

bzimport added a comment.Via ConduitApr 7 2008, 12:55 PM

conrad.irwin wrote:

Well, maybe you could educate your users by counting the number of times the script fails to load (with a cookie) and if it fails a few times, alert them to the fact that caching is a good thing (tm). I agree with you, there is not much use for it - however it could (just about plausibly) serve a purpose, and at the expense of 20 bytes may as well stay in there.

bzimport added a comment.Via ConduitApr 7 2008, 3:17 PM

random832 wrote:

This has nothing to do with caching. The problem is that runOnloadHook is run when the main html page is finished loading, regardless of if other scripts are finished executing or not.

In fact, in some situations caching will just make things _worse_ by allowing runOnloadHook to run _earlier_.

bzimport added a comment.Via ConduitApr 7 2008, 3:26 PM

conrad.irwin wrote:

It has everything to do with caching, if the javascript files are cached locally the chances are the load times will be small enough that they are loaded and parsed by the time the page finally comes down from the server - if, on the other hand, the javascript has to be loaded as well - then it takes a much longer time to arrive and is much more likely to arrive after addOnloadHook has run. (That is why doing a soft refresh on pages after a hard refresh is sometimes necessary for very dynamically included files).

If you want an example of this happening visit http://en.wiktionary.org/wiki/WT:PREFS , if you haven't been there before the chances are about 50/50 you will see a blank page (The chances seem to be higher in IE7 than FF2 - though I can't convince myself of a good reason for this). Do a soft refresh, or visit another page and come back to that one, and it will magically populate because the load time of the dynamically included script is much smaller when it has been pre downloaded and the addOnloadHook is thus run.

bzimport added a comment.Via ConduitApr 7 2008, 3:34 PM

random832 wrote:

No - the problem - and the reason caching won't help in some cases - is, the javascript has to _execute_ - not just load and parse - before you get to the end of the rendered page.

But my point is, you can't infer from a failure that the user didn't have the js file cached, so it would be inappropriate to use this as a basis to "educate" a user.

Apropos of nothing, I remember some features (like dismissable sitenotice for everyone) getting canned because of the asinine "no cookies for anons" policy. So why is that WT:PREFS page allowed to exist?

bzimport added a comment.Via ConduitApr 7 2008, 3:53 PM

conrad.irwin wrote:

A patch for wikibits.js with the strict flag

Attached: addOnloadHook_strict.patch

bzimport added a comment.Via ConduitApr 7 2008, 3:53 PM

conrad.irwin wrote:

A patch for wikibits.js without the strict flag

Attached: addOnloadHook_nostrict.patch

bzimport added a comment.Via ConduitApr 7 2008, 3:57 PM

conrad.irwin wrote:

I was only proposing that as a possible reason an Extension might want the script flag ;), I agree it's improbable. I have created two patch files, one with the flag, the other without.

(off topic) I don't believe no cookies for anons is a "policy", as anons get session cookies. WT:PREFS is orders of magnitude lower priority than a sitenotice on 'pedia - both because the user base is smaller and also because it is not advertised at the top of every page. If WT:PREFS were causing problems then I'm sure the devs would let us know, and we could then fix it. (We have discussed WT:PREFS a lot on WT:GP and in Wiktionary - so come and chat there if you want to know more about what we think about it)

bzimport added a comment.Via ConduitApr 7 2008, 7:58 PM

random832 wrote:

The wiki will set a temporary session cookie (PHPSESSID) whenever you
visit the site. If you do not intend to ever log in, you may deny this
cookie, but you cannot log in without it. It will be deleted when you
close your browser session.

More cookies may be set when you log in, to avoid typing in your user
name (or optionally password) on your next visit. These last up to 30
days. You may clear these cookies after use if you are using a public
machine and don't wish to expose your username to future users of the
machine. (If so, clear the browser cache as well.)

It is implied that users who do not log in will not get any cookies _other than_ PHPSESSID, and I wasn't just making up the fact that this sank a proposed implementation of dismissable sitenotice.

bzimport added a comment.Via ConduitApr 7 2008, 8:33 PM

conrad.irwin wrote:

(off topic) Please discuss the irrelevant WT:PREFS at http://en.wiktionary.org/wiki/WT:GP or irc://irc.freednode.net#wiktionary - linking to it was intended (like the discussion about caching) to illustrate my point not to sidetrack the fixing of this bug.

The question of having a "strict" flag or not has not been properly resolved, but as it is trivial to include this as a feature we may as well, just in case.

brion added a comment.Via ConduitMay 19 2008, 10:52 PM

Applied in r35064 (as part of bug 13232)

Add Comment