RESOLVED FIXED 61321
REGRESSION (r79114): zipcar.com does not load properly.
https://bugs.webkit.org/show_bug.cgi?id=61321
Summary REGRESSION (r79114): zipcar.com does not load properly.
Jon Lee
Reported 2011-05-23 16:37:10 PDT
There are at least two pages where zipcar.com breaks: 1. Go to zipcar.com. The Flash video on the homepage does not load properly. 2. You will need to have a zipcar membership to go to the second page. Log in, and go to reserve a car (for me, it is http://www.zipcar.com/sf/reservations/search-by-time). A calendar icon should appear next to the date fields, and when you click on the clock icon a drop-down list of times should appear. Neither of these occur. I traced the changelist that breaks this to 79114. The code that makes 2. work doesn't get called because of a bug in the jQuery plugin loading code. On a working browser I see that req.callReady (line 1602) in requireplugins-jquery-1.4.3.js gets called at least twice, in which the second time s.isDone is true. Right now on ToT req.callReady gets called only once, when s.isDone is false.
Attachments
Patch (5.54 KB, patch)
2011-05-24 18:25 PDT, James Simonsen
no flags
Patch (2.66 KB, patch)
2011-05-25 12:06 PDT, James Simonsen
no flags
Patch for landing (2.68 KB, patch)
2011-05-25 12:14 PDT, James Simonsen
no flags
Patch for landing (2.68 KB, patch)
2011-05-25 14:27 PDT, James Simonsen
no flags
Jon Lee
Comment 1 2011-05-23 16:37:28 PDT
Jon Lee
Comment 2 2011-05-23 16:43:28 PDT
Adam Barth
Comment 3 2011-05-23 16:44:38 PDT
Frowns. Assigning to myself so this doesn't get lost. Feel free to steal.
James Simonsen
Comment 4 2011-05-23 18:15:57 PDT
This is an evangelism issue. requirejs is using a non-compliant (and obsolete) technique to force async script download, but in-order execution. Support for this was removed in change 79114. The HTML5-compliant technique was implemented in change 81198 to address bug 50115. The requirejs project needs to be updated to use async=false and zipcar.com needs to use the new version. Alternatively zipcar.com could use LABjs, which is where requirejs copied the old idea from. LABjs already supports the new technique. I can reach out to the requirejs project. I'm not sure what to do about zipcar.
James Simonsen
Comment 5 2011-05-23 18:23:41 PDT
Looking at the code in the requirejs project, it looks like they already support the new technique. We just need zipcar to update to a newer version of requirejs.
Adele Peterson
Comment 6 2011-05-23 18:29:37 PDT
Is it possible to do site-specific hacks for sites that use the old version of the library?
James Simonsen
Comment 7 2011-05-23 19:23:30 PDT
(In reply to comment #6) > Is it possible to do site-specific hacks for sites that use the old version of the library? I'm not sure how the site-specific hacks work. Can you point me to an example?
Adele Peterson
Comment 8 2011-05-24 15:44:51 PDT
Here's an example of removal of an obsolete site specific hack: http://trac.webkit.org/changeset/68683
James Simonsen
Comment 9 2011-05-24 18:25:07 PDT
Adam Barth
Comment 10 2011-05-24 18:45:56 PDT
Comment on attachment 94724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94724&action=review > Source/WebCore/html/HTMLScriptElement.cpp:91 > + if (!settings->needsSiteSpecificQuirks()) > + return false; Do we have a sense for how many sites use an old version of this library? I wonder if we should restrict this behavior to only certain hosts. That's more likely to let us remove the quirk in the future because we can test the individual sites on the quirks list.
James Simonsen
Comment 11 2011-05-24 20:05:08 PDT
(In reply to comment #10) > (From update of attachment 94724 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94724&action=review > > > Source/WebCore/html/HTMLScriptElement.cpp:91 > > + if (!settings->needsSiteSpecificQuirks()) > > + return false; > > Do we have a sense for how many sites use an old version of this library? I wonder if we should restrict this behavior to only certain hosts. That's more likely to let us remove the quirk in the future because we can test the individual sites on the quirks list. I'd prefer that as well. I'm not sure how to get a thorough list of users though. I scanned the Chrome bug reports and the only one that sounded like this issue was another report about Zipcar. I looked at the requirejs history. The now-broken behavior was introduced 6/29/10 in 0.12.0. It was fixed to be HTML5-compliant on 11/13/10 in 0.15.0. Zipcar is running 0.14.5. For the record, they're now on 0.24.0 and there have been 4 releases with the fix in it.
Andy Estes
Comment 12 2011-05-24 20:22:55 PDT
Is it somehow possible to inject a script into the main world that checks the version of requirejs?
Andy Estes
Comment 13 2011-05-24 20:33:42 PDT
(In reply to comment #12) > Is it somehow possible to inject a script into the main world that checks the version of requirejs? This might not be possible to do in a way that doesn't require a script to be injected into every document, come to think of it. Anyway, just a thought.
James Simonsen
Comment 14 2011-05-24 20:36:24 PDT
(In reply to comment #12) > Is it somehow possible to inject a script into the main world that checks the version of requirejs? Evaluating "window.require.version" will give the answer. Is that an acceptable thing to do when we spot a script/cache type?
Maciej Stachowiak
Comment 15 2011-05-24 22:19:54 PDT
Comment on attachment 94724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94724&action=review >>> Source/WebCore/html/HTMLScriptElement.cpp:91 >>> + return false; >> >> Do we have a sense for how many sites use an old version of this library? I wonder if we should restrict this behavior to only certain hosts. That's more likely to let us remove the quirk in the future because we can test the individual sites on the quirks list. > > I'd prefer that as well. I'm not sure how to get a thorough list of users though. I scanned the Chrome bug reports and the only one that sounded like this issue was another report about Zipcar. > > I looked at the requirejs history. The now-broken behavior was introduced 6/29/10 in 0.12.0. It was fixed to be HTML5-compliant on 11/13/10 in 0.15.0. Zipcar is running 0.14.5. For the record, they're now on 0.24.0 and there have been 4 releases with the fix in it. I think it might be less risky to make this specific to just the one site for now. If other popular sites are affected, we will learn soon enough.
Darin Adler
Comment 16 2011-05-25 09:54:04 PDT
Comment on attachment 94724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94724&action=review >>>> Source/WebCore/html/HTMLScriptElement.cpp:91 >>>> + return false; >>> >>> Do we have a sense for how many sites use an old version of this library? I wonder if we should restrict this behavior to only certain hosts. That's more likely to let us remove the quirk in the future because we can test the individual sites on the quirks list. >> >> I'd prefer that as well. I'm not sure how to get a thorough list of users though. I scanned the Chrome bug reports and the only one that sounded like this issue was another report about Zipcar. >> >> I looked at the requirejs history. The now-broken behavior was introduced 6/29/10 in 0.12.0. It was fixed to be HTML5-compliant on 11/13/10 in 0.15.0. Zipcar is running 0.14.5. For the record, they're now on 0.24.0 and there have been 4 releases with the fix in it. > > I think it might be less risky to make this specific to just the one site for now. If other popular sites are affected, we will learn soon enough. I’m not sure it really matters whether it’s site-specific. But we need to either review- or review+ to keep this moving along! > Source/WebCore/html/HTMLScriptElement.cpp:93 > + if (element->getAttribute(typeAttr) != "script/cache") Could use fastGetAttribute here.
Adele Peterson
Comment 17 2011-05-25 10:59:09 PDT
I agree with Maciej - we can just do it for zipcar for now. We can add or remove domains in the future as we learn more, or sites get updated.
James Simonsen
Comment 18 2011-05-25 12:06:56 PDT
Adam Barth
Comment 19 2011-05-25 12:10:21 PDT
Comment on attachment 94831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94831&action=review > Source/WebCore/html/HTMLScriptElement.cpp:90 > + if (url.host() != "www.zipcar.com") equalsIgnoringCase
James Simonsen
Comment 20 2011-05-25 12:14:42 PDT
Created attachment 94833 [details] Patch for landing
Darin Adler
Comment 21 2011-05-25 13:11:45 PDT
Comment on attachment 94833 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=94833&action=review > Source/WebCore/html/HTMLScriptElement.cpp:85 > +static bool needsOldRequirejsQuirk(HTMLScriptElement* element) One other tweak to consider for performance’s sake. We should have the most likely check to fail be the first one in the function. Thus I suggest checking "script/cache" first, and only then checking the host. Unless the host check is already super-fast.
Darin Adler
Comment 22 2011-05-25 13:12:34 PDT
Comment on attachment 94833 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=94833&action=review >> Source/WebCore/html/HTMLScriptElement.cpp:85 >> +static bool needsOldRequirejsQuirk(HTMLScriptElement* element) > > One other tweak to consider for performance’s sake. We should have the most likely check to fail be the first one in the function. Thus I suggest checking "script/cache" first, and only then checking the host. Unless the host check is already super-fast. Sorry, I mean the cheapest check that is will fail on almost every script element, *not* the most likely one to fail.
James Simonsen
Comment 23 2011-05-25 14:27:25 PDT
Created attachment 94859 [details] Patch for landing
Maciej Stachowiak
Comment 24 2011-05-25 15:04:13 PDT
Thanks, James!
WebKit Commit Bot
Comment 25 2011-05-25 23:20:49 PDT
Comment on attachment 94859 [details] Patch for landing Clearing flags on attachment: 94859 Committed r87361: <http://trac.webkit.org/changeset/87361>
WebKit Commit Bot
Comment 26 2011-05-25 23:20:55 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.