Last modified: 2011-09-10 01:10:45 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T32825, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 30825 - IE7 AJAX breaks with protocol-relative URLs on commons
IE7 AJAX breaks with protocol-relative URLs on commons
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
JavaScript (Other open bugs)
unspecified
All All
: Normal normal (vote)
: ---
Assigned To: Nobody - You can work on this!
: need-unittest
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-09 00:42 UTC by Ian Baker
Modified: 2011-09-10 01:10 UTC (History)
7 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Patch with test case, but I can't get it to fail (1.38 KB, patch)
2011-09-09 18:06 UTC, Brion Vibber
Details

Description Ian Baker 2011-09-09 00:42:12 UTC
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.

2. 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).
Comment 1 Brion Vibber 2011-09-09 01:22:36 UTC
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...
Comment 2 Neil Kandalgaonkar 2011-09-09 05:05:25 UTC
(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.
Comment 3 Neil Kandalgaonkar 2011-09-09 05:13:19 UTC
(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!
Comment 4 Roan Kattouw 2011-09-09 17:32:11 UTC
Hmm, does $.getScript() work with protocol-relative URLs in IE7?
Comment 5 Brion Vibber 2011-09-09 17:40:59 UTC
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.
Comment 6 Brion Vibber 2011-09-09 18:06:59 UTC
Created attachment 9040 [details]
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.
Comment 7 Brion Vibber 2011-09-09 18:15:31 UTC
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).
Comment 8 Neil Kandalgaonkar 2011-09-09 18:38:30 UTC
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. 

-- http://markmail.org/message/gy3tzbnqaxdcnqus


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().
Comment 9 Roan Kattouw 2011-09-09 18:43:38 UTC
Brion merged r87986 in r96679 and I deployed it just now.
Comment 10 Brion Vibber 2011-09-09 18:46:16 UTC
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.
Comment 11 Brion Vibber 2011-09-09 19:05:20 UTC
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.
Comment 12 Brion Vibber 2011-09-09 19:05:56 UTC
I mean r96679 had an error in th emerge.

gaaaarrrr I should not type today. ;)
Comment 13 Brion Vibber 2011-09-09 19:14:25 UTC
Ok, looks like UW's API hits (eg for the actual upload) still have problems. :(
Comment 14 Brion Vibber 2011-09-09 22:05:33 UTC
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.
Comment 15 Neil Kandalgaonkar 2011-09-09 23:29:53 UTC
I've tested this in every way I can think of -- r96699 looks like it will fix the problem
Comment 16 Neil Kandalgaonkar 2011-09-09 23:42:40 UTC
So, now what? No Friday deploys, defer till Monday?
Comment 17 Mark A. Hershberger 2011-09-10 01:10:45 UTC
reopen if new deployment shows problems.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links