Bug 61321 – REGRESSION (r79114): zipcar.com does not load properly.

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Jon Lee]

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.

[Jon Lee]

<rdar://problem/9482755>

[Jon Lee]

Changelist listing: http://trac.webkit.org/changeset/79114/trunk

[Adam Barth]

Frowns.  Assigning to myself so this doesn't get lost.  Feel free to steal.

[James Simonsen]

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]

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]

Is it possible to do site-specific hacks for sites that use the old version of the library?

[James Simonsen]

(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]

Here's an example of removal of an obsolete site specific hack:
http://trac.webkit.org/changeset/68683

[James Simonsen]

Created attachment 94724 [details]
Patch

[Adam Barth]

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]

(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]

Is it somehow possible to inject a script into the main world that checks the version of requirejs?

[Andy Estes]

(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]

(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 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 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]

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]

Created attachment 94831 [details]
Patch

[Adam Barth]

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]

Created attachment 94833 [details]
Patch for landing

[Darin Adler]

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 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]

Created attachment 94859 [details]
Patch for landing

[Maciej Stachowiak]

Thanks, James!

[WebKit Commit Bot]

Comment on attachment 94859 [details]
Patch for landing

Clearing flags on attachment: 94859

Committed r87361: <http://trac.webkit.org/changeset/87361>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.