Bug 61321 - REGRESSION (r79114): zipcar.com does not load properly.
Summary: REGRESSION (r79114): zipcar.com does not load properly.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Major
Assignee: James Simonsen
URL: http://zipcar.com
Keywords: InRadar, Regression
Depends on:
Blocks: 65405
  Show dependency treegraph
 
Reported: 2011-05-23 16:37 PDT by Jon Lee
Modified: 2011-07-29 18:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.54 KB, patch)
2011-05-24 18:25 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (2.66 KB, patch)
2011-05-25 12:06 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch for landing (2.68 KB, patch)
2011-05-25 12:14 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch for landing (2.68 KB, patch)
2011-05-25 14:27 PDT, James Simonsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 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.
Comment 1 Jon Lee 2011-05-23 16:37:28 PDT
<rdar://problem/9482755>
Comment 2 Jon Lee 2011-05-23 16:43:28 PDT
Changelist listing: http://trac.webkit.org/changeset/79114/trunk
Comment 3 Adam Barth 2011-05-23 16:44:38 PDT
Frowns.  Assigning to myself so this doesn't get lost.  Feel free to steal.
Comment 4 James Simonsen 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.
Comment 5 James Simonsen 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.
Comment 6 Adele Peterson 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?
Comment 7 James Simonsen 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?
Comment 8 Adele Peterson 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
Comment 9 James Simonsen 2011-05-24 18:25:07 PDT
Created attachment 94724 [details]
Patch
Comment 10 Adam Barth 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.
Comment 11 James Simonsen 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.
Comment 12 Andy Estes 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?
Comment 13 Andy Estes 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.
Comment 14 James Simonsen 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?
Comment 15 Maciej Stachowiak 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.
Comment 16 Darin Adler 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.
Comment 17 Adele Peterson 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.
Comment 18 James Simonsen 2011-05-25 12:06:56 PDT
Created attachment 94831 [details]
Patch
Comment 19 Adam Barth 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
Comment 20 James Simonsen 2011-05-25 12:14:42 PDT
Created attachment 94833 [details]
Patch for landing
Comment 21 Darin Adler 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.
Comment 22 Darin Adler 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.
Comment 23 James Simonsen 2011-05-25 14:27:25 PDT
Created attachment 94859 [details]
Patch for landing
Comment 24 Maciej Stachowiak 2011-05-25 15:04:13 PDT
Thanks, James!
Comment 25 WebKit Commit Bot 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>
Comment 26 WebKit Commit Bot 2011-05-25 23:20:55 PDT
All reviewed patches have been landed.  Closing bug.