Closed Bug 682554 Opened 13 years ago Closed 13 years ago

Yandex maps doesn't load (onreadystatechange should be on the document only)

Categories

(Core :: DOM: Events, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla9
Tracking Status
firefox9 + ---

People

(Reporter: unghost, Assigned: bzbarsky)

References

()

Details

(Keywords: dev-doc-complete, regression, top100)

Attachments

(3 files, 2 obsolete files)

STR:
1) Open http://maps.yandex.ru/

Expected results:
Map loads

Actual results:
Map doesn't load

Last good revision:
20110824030813 - http://hg.mozilla.org/mozilla-central/rev/198c7de0699d

First bad revision:
20110825030823 - http://hg.mozilla.org/mozilla-central/rev/e58e98a89827

Regression window:
hg.mozilla.org/mozilla-central/pushloghtml?fromchange=198c7de0699d&tochange=e58e98a89827
Summary: Yandex maps doesn't load since Aug 25 → Yandex maps doesn't load
Regression range for SeaMonkey builds:

Last good revision:
20110824003041 - http://hg.mozilla.org/mozilla-central/rev/5d9989c3bff6

First bad revision:
20110825003151 - http://hg.mozilla.org/mozilla-central/rev/c5b2cbf6e31b

Regression window:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5d9989c3bff6&tochange=c5b2cbf6e31b
Local track down using Linux x86_64:

The first bad revision is:
changeset:   75838:1f58f9ed470c
user:        Boris Zbarsky <bzbarsky@mit.edu>
date:        Wed Aug 24 15:49:25 2011 -0400
summary:     Bug 659350 part 5.  The guts of the change to move from storing inline event handlers on the JSObject to storing them in the event listener manager directly, so we can easily set/read them via IDL.

http://hg.mozilla.org/mozilla-central/rev/1f58f9ed470c
Component: General → DOM: Events
Product: Firefox → Core
QA Contact: general → events
Blocks: 659350
Setting tracking-firefox9 to ?. FWIW, http://maps.yandex.ru/ is very popular map service in Russia and part of Yandex.ru portal (#21 in Alexa - http://www.alexa.com/siteinfo/yandex.ru)
Looks like Yandex uses a dynamic script loader to load the map scripts.  When prettyprinted and some irrelevant stuff elided, that looks like this:

  (function (c, a, e) {
    var d = a.getElementsByTagName("head")[0],
    b = {};
    c.load = function (g, i, h) {
      var f = a.createElement("script");
      f.src = g;
      if (i) {
        if (f[e] === null) {
          f[e] = function () {
            var j = this.readyState;
            if (j === "loaded" || j === "complete") {
              f[e] = null;
              i(b[h])
            }
          }
        } else {
          f.onload = f.onerror = function () {
            f.onload = f.onerror = null;
            i(b[h])
          }
        }
      }
      d.insertBefore(f, d.lastChild)
    };
  }(require, document, "onreadystatechange"));

where |require| is the loader object, |g| is the script URI, |h| is some sort of closure object consumers can pass in, |i| is the callback function to trigger when the script loads.

In any case, per the current HTML5 spec f[e] would be the .onreadystatechange of the <script> which would be null.  So we end up on the f[e] codepath instead of the f.onload codepath.  But we don't actually fire readystatechange events on <script> (also per spec), so |i| never gets invoked.

Apparently, IE6-8 don't fire onload on scripts, which is why the hoops are being jumped through.

The obvious options here are:

1) Evangelize yandex to detect IE more stringently (which is what RequireJS, say, does). 
   The problem with that is that the above seems like perfectly reasonable capability
   detection to me.
2) Change the spec so that various on* event handlers for events that will never fire on
   an element are not present on that element.  This would make the above sort of
   capability detection actually useful.
3) Implement support for readyState on <script> and fire at least some readystatechange
   events for <script> (at least the one indicating the script is done loading).  This
   would match IE and Opera, at first glance.  This, too, needs a spec change.

I'm going to file a bug on the spec here to decide between 2 & 3, I guess, and in the meantime start working on a patch to do #3.  Doing #2 would be a lot more work from where our code is right now (though doing it for just onreadystatechange would not be too bad; it needs to be on Document and MediaElement but not elsewhere, necessarily).  Doing #1 seems like punishing yandex.  #3 seems like the simplest path forward that doesn't involve the sudden outbreak of sanity of #2.
Assignee: nobody → bzbarsky
Though given the cc list... maybe #1 is an option too.   ;)  mash, what do things look like on your end here?
http://www.w3.org/Bugs/Public/show_bug.cgi?id=13965 filed.

As I pointed out in that bug, doing both #2 and #3 might be the best way forward.
Attachment #556749 - Attachment is obsolete: true
Attachment #556750 - Flags: feedback?(jonas)
Hi, I’m the author of the above code from Yandex. There had been a classical shared callback for both onload on onreadystatechange before a bug in Opera 9.6 was found. The browser fired onreadystatechange with readyState=loaded, but the script’s content wasn’t available at that moment. So we were forced to limit onreadystatechange usage to IE only. Luckily, onreadystatechange equals to null in IE, and to undefied in Opera.

See http://stackoverflow.com/questions/774752/dynamic-script-loading-synchronization

Although we do not support Opera 9.6 anymore, it seems counter-intuitive to return null for onreadystatechange without fully supporting the event. I would consider #2 and #3 options.
> Luckily, onreadystatechange equals to null in IE

The problem is that the HTML5 spec also requires it to be null.  I suggest that you comment in http://www.w3.org/Bugs/Public/show_bug.cgi?id=13965 as needed...
Yes, iirc the spec calls for that and also onreadystatechange on XHR and media elements... but nothing else.
Kyle says in http://www.w3.org/Bugs/Public/show_bug.cgi?id=13965#c5 that the above patch would break LabJS.  So what we probably want instead is the Opera behavior here, not the IE one.  Assuming we go there at all, which I'm less and less convinced of.
I've updated the spec. I believe it's compatible with legacy content, but then I thought the onfoo* changes were too, so...
Ian, could you please link me to the relevant spec section?
I'll write some tests tomorrow too, but in the meantime I figured I'd get the review ball rolling.
Attachment #559034 - Flags: review?(jonas)
Attachment #556750 - Attachment is obsolete: true
Attachment #556750 - Flags: feedback?(jonas)
Priority: -- → P1
Whiteboard: [need review]
Comment on attachment 559034 [details] [diff] [review]
Implementation of the new spec

I won't have time to review this anytime soon. Smaug, would you be able to do the honors?
Attachment #559034 - Flags: review?(jonas) → review?(Olli.Pettay)
Comment on attachment 559034 [details] [diff] [review]
Implementation of the new spec

Tests?

So we don't support all the states, but the spec allows that.


r=me if there will be some tests.
Attachment #559034 - Flags: review?(Olli.Pettay) → review+
> Tests?

See comment 17.  Writing them right now.  ;)
Attached patch The testSplinter Review
Attachment #559309 - Flags: review?(Olli.Pettay)
Comment on attachment 559309 [details] [diff] [review]
The test

Opera says that doing this is not very web-compatible (which will be _great_ fun for IE).  Hixie has reverted the spec changes for this, and we're just going to put onreadystatechange on documents only.
Attachment #559309 - Flags: review?(Olli.Pettay)
Attachment #559658 - Flags: review?(Olli.Pettay) → review+
Blocks: 684671
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dbed30dd67d

Sorry for the lag on landing this...
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/2dbed30dd67d
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Verified fixed on Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110920 Firefox/9.0a1 ID:20110920085517
Thanks a lot, Boris!
Status: RESOLVED → VERIFIED
Summary: Yandex maps doesn't load → Yandex maps doesn't load (onreadystatechange should be on the document only)
Tracking for Firefox Update 9 so if it gets re-opened we will notice.
The DOM event reference now says readystatechange is on document only, and this is mentioned on Firefox 9 for developers.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: