IE7 AJAX breaks with protocol-relative URLs on commons
Closed, ResolvedPublic

Description

Author: ibaker

Description:
IE7 generates "access is denied" errors when attempting to load a URL of the form "//commons.wikimedia.org/w/api.php?foo" using xhr.open within jQuery. This means two things:

  1. The AJAX call never returns useful information, so the features dependent on it break.
  1. JS-heavy pages break when JS execution stops on the error. Currently, this is preventing UploadWizard from working in IE7 on live commons. It does work IE6 and IE8 (even in IE7 compatibility mode).

You can see the error by visiting http://commons.wikimedia.org/wiki/Main_Page?debug=true with IE7 while not logged in. You can also try visiting UploadWizard while logged in, but some confluence of IE bugs prevents the errors from appearing there when debugging with MS Script Debugger is enabled.

If you need to see the problem in UW specifically, I can provide the convoluted recipe for making the debugger work there too, but most likely fixing the errors on Main_Page will fix it as well.

What seems to happen, AFAICT, is that scripts are included from MediaWiki:Common.js using importScript(). This appends a new <script> tag to the page, but because of how .append() is implemented in jQuery, also triggers loading and executing of the script via AJAX. This is the point where it breaks in IE7, although other browsers don't have a problem.

It's possible that the loading of many JS assets from bits.wikimedia.org is part of the problem, since the AJAX requests then break the same-origin policy. However, it works fine in Chrome and Firefox (and, oddly, IE6).


Version: unspecified
Severity: normal

bzimport added a project: MediaWiki-JavaScript.Via ConduitNov 21 2014, 11:51 PM
bzimport added a subscriber: Unknown Object (MLST).
bzimport set Reference to bz30825.
bzimport created this task.Via LegacySep 9 2011, 12:42 AM
brion added a comment.Via ConduitSep 9 2011, 1:22 AM

Can you clarify what you mean about "because of how .append() is implemented in jQuery, also triggers loading and executing of the script via AJAX"?

It seems to do a low-level DOM appendChild, which shouldn't trigger any XMLHTTPRequest-related stuff at all...

bzimport added a comment.Via ConduitSep 9 2011, 5:05 AM

neilk wrote:

(In reply to comment #1)

Can you clarify what you mean about "because of how .append() is implemented in
jQuery, also triggers loading and executing of the script via AJAX"?

It seems to do a low-level DOM appendChild, which shouldn't trigger any
XMLHTTPRequest-related stuff at all...

Follow the domManip() call, you'll see that ultimately it gets to clean(), which records the tag in a "scripts" variable if it's a script. And then, in domManip(), it calls evalScript on all detected scripts, which loads the source URL via ajax - synchronously!

All this happens before it even gets to the standard DOM appendChild().

This guy speculates on why it does this, in the jQuery forums: http://api.jquery.com/append/#comment-67912032 - he thinks it's something to get around IE oddities when loading scripts.

bzimport added a comment.Via ConduitSep 9 2011, 5:13 AM

neilk wrote:

(In reply to comment #2)

(In reply to comment #1)

To be clear, actually, it seems that you *can't* add a script tag to a page with $.append(), $.prepend(), or anything like that. It does the dance described above to load the script but never actually adds it to the page!

Catrope added a comment.Via ConduitSep 9 2011, 5:32 PM

Hmm, does $.getScript() work with protocol-relative URLs in IE7?

brion added a comment.Via ConduitSep 9 2011, 5:40 PM

Urgh, that's nicely buried eight levels deep. :P jQuery core needs better code comments in the non-minified version. :)

Is there a specific reason that jQuery does that instead of just appending script nodes as normal? If we go straight to DOM appendChild() can we work around it, or does that fail to load the scripts on some browsers?

Adding needs-unittest keyword; we should be able to test this behavior from the qunit tests for mediawiki.load by handing constructed URLs to a local test file.

brion added a comment.Via ConduitSep 9 2011, 6:06 PM

Created attachment 9040
Patch with test case, but I can't get it to fail

This patch adds a qunit test case that tries to load a JS script via mw.loader.load() passing it a protocol-relative URL.

However, in my testing on trunk and rel1.18 it works fine on IE 7; in IE 9's dev tools I can see the script is loaded via a <script> tag, not via an XHR.

So either this doesn't happen on 1.18 and trunk, or I'm not triggering the right conditions.

Attached: bug30825-test.diff

brion added a comment.Via ConduitSep 9 2011, 6:15 PM

Aha -- looks like it works on 1.18 and trunk because they've changed to.... using appendChild(). ;)

Seems to have been done in order to make error handling more consistent in r87986 or so -- though it's unclear why it's done with raw DOM instead of jQuery's .append here (unless someone noticed that jQuery's .append() jumps through other hoops and just didn't mention it in the comments).

bzimport added a comment.Via ConduitSep 9 2011, 6:38 PM

neilk wrote:

Okay, the Word of God (aka John Resig) on this jQuery behaviour:

When a script node is inserted into the document it is also executed (by
jQuery). To avoid re-executing it later (this happens a lot, as it turns
out) the script is simply removed from the document.

So, I think we can safely do what we like to work around this behaviour. It is not a workaround for some other strange browser bug; it's an optimization because developers tend to re-add the same script tags over and over. We have ResourceLoader to make sure that never happens. That said, the scripts in question are being added to the page with the legacy importScript().

Catrope added a comment.Via ConduitSep 9 2011, 6:43 PM

Brion merged r87986 in r96679 and I deployed it just now.

brion added a comment.Via ConduitSep 9 2011, 6:46 PM

r96680 adds the test case as a regression test in the qunit suite on trunk.

r87986 merged to 1.17wmf1 in r96679, local manual testing with a stub equivalent of the MediaWiki:AnonymousI18N.js page confirms that the fix takes it from failing with 'access denied' to working.

We should still confirm that UploadWizard all works -- if it's just loading code modules this should definitely fix it, but if it's breaking the API hits too then additional tweaks may be required.

brion added a comment.Via ConduitSep 9 2011, 7:05 PM

r96680 had an erorr in the merge which caused some modules not to load; unfortunately this went live for a few minutes so some users saw breakage from that.

r96682 should resolve it.

brion added a comment.Via ConduitSep 9 2011, 7:05 PM

I mean r96679 had an error in th emerge.

gaaaarrrr I should not type today. ;)

brion added a comment.Via ConduitSep 9 2011, 7:14 PM

Ok, looks like UW's API hits (eg for the actual upload) still have problems. :(

brion added a comment.Via ConduitSep 9 2011, 10:05 PM

r96699 on 1.17wmf1 is a provisional workaround, monkey-patching jQuery.ajax() to add the protocol in.

This gets UW's API requests working on 1.17wmf1 and IE 7 for me, and doesn't appear to break any other browsers.

1.18 and trunk shouldn't need this, as jQuery 1.6 seems to do some fixins internally and doesn't encounter the problem.

bzimport added a comment.Via ConduitSep 9 2011, 11:29 PM

neilk wrote:

I've tested this in every way I can think of -- r96699 looks like it will fix the problem

bzimport added a comment.Via ConduitSep 9 2011, 11:42 PM

neilk wrote:

So, now what? No Friday deploys, defer till Monday?

MarkAHershberger added a comment.Via ConduitSep 10 2011, 1:10 AM

reopen if new deployment shows problems.

Add Comment