Bug 49294

Summary: [Web Timing] Zero out some values on cross-origin redirects
Product: WebKit Reporter: James Simonsen <simonjam>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dglazkov, eric, fishd, simonjam, tonyg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.w3.org/TR/navigation-timing/
Bug Depends on:    
Bug Blocks: 30685    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

James Simonsen
Reported 2010-11-09 17:43:43 PST
The spec isn't set finalized here yet, but we'll need to handle the parts of the spec that talk about cross-origin requests.
Attachments
Patch (11.33 KB, patch)
2010-11-22 16:44 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Patch (12.24 KB, patch)
2010-12-10 17:55 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Patch (32.51 KB, patch)
2010-12-15 16:29 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Patch (32.54 KB, patch)
2011-01-05 14:26 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
James Simonsen
Comment 1 2010-11-22 16:44:17 PST
Created attachment 74612 [details] Patch
Adam Barth
Comment 2 2010-12-10 13:13:08 PST
Comment on attachment 74612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74612&action=review Looks fine from a SecurityOrigin point-of-view. I'll let someone who knows WebPeformance give the real r+. > WebCore/loader/MainResourceLoader.cpp:175 > + RefPtr<SecurityOrigin> securityOrigin(SecurityOrigin::create(newRequest.url())); We usually use the = form of the constructor. > WebCore/loader/MainResourceLoader.cpp:176 > + if (!securityOrigin->canRequest(redirectResponse.url())) Interesting. At first I thought this was backwards, but then I realized that it was correct. Maybe add a comment explaining the ordering so we're not confused in the future?
James Simonsen
Comment 3 2010-12-10 17:55:01 PST
Created attachment 76284 [details] Patch
James Simonsen
Comment 4 2010-12-10 17:55:40 PST
(In reply to comment #2) > (From update of attachment 74612 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74612&action=review > > Looks fine from a SecurityOrigin point-of-view. I'll let someone who knows WebPeformance give the real r+. > > > WebCore/loader/MainResourceLoader.cpp:175 > > + RefPtr<SecurityOrigin> securityOrigin(SecurityOrigin::create(newRequest.url())); > > We usually use the = form of the constructor. > > > WebCore/loader/MainResourceLoader.cpp:176 > > + if (!securityOrigin->canRequest(redirectResponse.url())) > > Interesting. At first I thought this was backwards, but then I realized that it was correct. Maybe add a comment explaining the ordering so we're not confused in the future? Addressed both of these. Tony, would you mind taking a look?
Tony Gentilcore
Comment 5 2010-12-13 13:09:12 PST
Comment on attachment 76284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76284&action=review > WebCore/page/PerformanceTiming.cpp:115 > + return 0; The spec doesn't explicitly say to check for different origins in the redirect chain for unloadEvent{Start,End}. Instead they should check whether the page that fired the events is same origin as the current page. Is there a reason to guard them by the redirect chain check as well? Everything else looks good to me.
James Simonsen
Comment 6 2010-12-15 16:29:00 PST
(In reply to comment #5) > (From update of attachment 76284 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76284&action=review > > > WebCore/page/PerformanceTiming.cpp:115 > > + return 0; > > The spec doesn't explicitly say to check for different origins in the redirect chain for unloadEvent{Start,End}. Instead they should check whether the page that fired the events is same origin as the current page. Is there a reason to guard them by the redirect chain check as well? Good catch! Fixed. We've got some good test cases now too.
James Simonsen
Comment 7 2010-12-15 16:29:53 PST
Created attachment 76708 [details] Patch
James Simonsen
Comment 8 2011-01-05 14:26:20 PST
Created attachment 78044 [details] Patch
James Simonsen
Comment 9 2011-01-05 14:27:30 PST
Fishing for an R+. This has already been LGTM'd.
WebKit Commit Bot
Comment 10 2011-01-05 18:41:50 PST
Comment on attachment 78044 [details] Patch Clearing flags on attachment: 78044 Committed r75129: <http://trac.webkit.org/changeset/75129>
WebKit Commit Bot
Comment 11 2011-01-05 18:41:55 PST
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.