WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Jon Lee
Comment 1
2011-05-23 16:37:28 PDT
<
rdar://problem/9482755
>
Jon Lee
Comment 2
2011-05-23 16:43:28 PDT
Changelist listing:
http://trac.webkit.org/changeset/79114/trunk
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
Created
attachment 94724
[details]
Patch
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
Created
attachment 94831
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug