Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(78)

Issue 2918653004: Remove dup'ed code for RequestorOrigin and FirstPartyCookie (Closed)

Created:
3 years, 6 months ago by kinuko
Modified:
3 years, 6 months ago
CC:
blink-reviews, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, gavinp+loader_chromium.org, jam, Nate Chapin, kinuko+watch, loading-reviews+fetch_chromium.org, loading-reviews_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, tyoshino+watch_chromium.org, yhirano, Yoav Weiss
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove dup'ed code for RequestorOrigin and FirstPartyCookie - Remove RequestorOrigin and FirstPartyCookie code in RenderFrameImpl (except for PlzNavigate's main frame resource case) - Most of the code now lives in FrameFetchContext - Also fix TODO to initialize ResourceRequest::RequestorOrigin with null Some implementation notes: Before this change, the logic was like: 1. When ResourceFetcher populates an initial resource request FetchContext::SetFirstPartyCookieAndRequestorOrigin was called in PopulateResourceRequest, and this used to do: if (!GetDocument()) return; if (request.FirstPartyForCookies().IsNull()) { request.SetFirstPartyForCookies( GetDocument() ? GetDocument()->FirstPartyForCookies() // [A] : SecurityOrigin::UrlWithUniqueSecurityOrigin()); // [B] } if (request.GetFrameType() == WebURLRequest::kFrameTypeNone && request.RequestorOrigin()->IsUnique()) { request.SetRequestorOrigin(GetDocument()->IsSandboxed(kSandboxOrigin) ? SecurityOrigin::Create(document_->Url()) // [a] : document_->GetSecurityOrigin()); // [b] } 2. After that, FrameFetchContext::WillSendRequest does: if (request.FirstPartyForCookies().IsEmpty()) { if (request.GetFrameType() == blink::WebURLRequest::kFrameTypeTopLevel) request.SetFirstPartyForCookies(request.Url()); // [C] else request.SetFirstPartyForCookies( frame_->GetDocument().FirstPartyForCookies()); // [D] } WebDocument frame_document = frame_->GetDocument(); if (request.RequestorOrigin().IsUnique() && !frame_document.GetSecurityOrigin().IsUnique()) { request.SetRequestorOrigin(frame_document.GetSecurityOrigin()); // [c] } This logic is also called on redirects. Note that: - [B] case is invalid as we return null-document case - [C],[D],[c] happens only if document is null, and for initial top-level frame request case this is always the case - Therefore: [A][C][D] are the valid cases for first-party-cookie, and [a][b][c] are for requestor-origin After this change, FrameFetchContext::WillSendRequest does: if (request.FirstPartyForCookies().IsNull()) { if (request.GetFrameType() == WebURLRequest::kFrameTypeTopLevel) { request.SetFirstPartyForCookies(request.Url()); // == [C] (document is null) } else { Document* document = GetDocument() ? GetDocument() : GetFrame()->GetDocument(); request.SetFirstPartyForCookies(document->FirstPartyForCookies()); // == [A][D] } } if (!request.RequestorOrigin()) { if (request.GetFrameType() == WebURLRequest::kFrameTypeNone) { Document* document = GetDocument(); request.SetRequestorOrigin(document->IsSandboxed(kSandboxOrigin) ? SecurityOrigin::Create(document->Url()) // [a] : document->GetSecurityOrigin()); // [b] } else { request.SetRequestorOrigin( GetFrame()->GetDocument()->GetSecurityOrigin()); // [c] } } BUG=671533, 625969 Review-Url: https://codereview.chromium.org/2918653004 Cr-Commit-Position: refs/heads/master@{#478191} Committed: https://chromium.googlesource.com/chromium/src/+/74a7fb2fa4307c69812945586b39018ae4f231b2

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Patch Set 4 : . #

Patch Set 5 : . #

Patch Set 6 : . #

Patch Set 7 : . #

Patch Set 8 : . #

Total comments: 7

Patch Set 9 : rebase #

Patch Set 10 : Mike comment (link fix) #

Total comments: 6

Patch Set 11 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+54 lines, -72 lines) Patch
M content/renderer/fetchers/resource_fetcher_impl.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +14 lines, -27 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameFetchContext.cpp View 1 2 3 4 5 6 7 8 9 3 chunks +30 lines, -20 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -14 lines 0 comments Download
M third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp View 1 2 3 4 5 6 8 1 chunk +1 line, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/platform/loader/fetch/ResourceRequestTest.cpp View 1 chunk +0 lines, -6 lines 0 comments Download

Messages

Total messages: 72 (57 generated)
kinuko
Current code for RequestorOrigin and FirstPartyCookie is too confusing to me (some code looks duplicated ...
3 years, 6 months ago (2017-06-07 02:00:49 UTC) #38
Mike West
This looks like an improvement to me. Thanks for poking at it. :) LGTM. https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp ...
3 years, 6 months ago (2017-06-07 06:48:44 UTC) #39
kinuko
Thanks! https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp#newcode708 third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:708: // https://tools.ietf.org/html/draft-west-first-party-cookies-04#section-2.1.1 On 2017/06/07 06:48:44, Mike West wrote: ...
3 years, 6 months ago (2017-06-07 06:57:49 UTC) #41
kinuko
Whoa, some tests failing again... will look.
3 years, 6 months ago (2017-06-07 23:49:41 UTC) #45
kinuko
Oh ok, I had uploaded an old patch... will re-up.
3 years, 6 months ago (2017-06-07 23:52:16 UTC) #46
kinuko
Re-uploaded. Nate, could you review as well?
3 years, 6 months ago (2017-06-07 23:57:47 UTC) #50
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/2918653004/diff/220001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/2918653004/diff/220001/content/renderer/render_frame_impl.cc#oldcode4198 content/renderer/render_frame_impl.cc:4198: // `isNull()` check. https://crbug.com/625969 Add 625969 to the BUG ...
3 years, 6 months ago (2017-06-08 04:45:45 UTC) #54
kinuko
Thanks, updated. https://codereview.chromium.org/2918653004/diff/220001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/2918653004/diff/220001/content/renderer/render_frame_impl.cc#oldcode4198 content/renderer/render_frame_impl.cc:4198: // `isNull()` check. https://crbug.com/625969 On 2017/06/08 04:45:44, ...
3 years, 6 months ago (2017-06-08 04:55:31 UTC) #56
tyoshino (SeeGerritForStatus)
lgtm It's great if you could explain briefly how RenderFrameImpl::WillSendRequest() has been merged into SetFirstPartyCookieAndRequestorOrigin() ...
3 years, 6 months ago (2017-06-08 05:30:59 UTC) #59
kinuko
On 2017/06/08 05:30:59, tyoshino wrote: > lgtm > > It's great if you could explain ...
3 years, 6 months ago (2017-06-08 07:29:56 UTC) #62
tyoshino (SeeGerritForStatus)
On 2017/06/08 07:29:56, kinuko wrote: > On 2017/06/08 05:30:59, tyoshino wrote: > > lgtm > ...
3 years, 6 months ago (2017-06-08 08:01:53 UTC) #63
kinuko
On 2017/06/08 08:01:53, tyoshino wrote: > On 2017/06/08 07:29:56, kinuko wrote: > > On 2017/06/08 ...
3 years, 6 months ago (2017-06-08 09:16:37 UTC) #65
kinuko
Nate- I'm thinking about landing this but please feel free to un-CQ or give more ...
3 years, 6 months ago (2017-06-09 03:39:11 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2918653004/240001
3 years, 6 months ago (2017-06-09 03:45:58 UTC) #69
commit-bot: I haz the power
3 years, 6 months ago (2017-06-09 03:50:40 UTC) #72
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/74a7fb2fa4307c69812945586b39...

Powered by Google App Engine
This is Rietveld 408576698