Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1243353002:
Skip the Service Worker CORS fallback for same origin requests. [1/2 blink] (Closed)
|
Created:
5 years, 5 months ago by horo Modified:
5 years, 3 months ago CC:
blink-reviews, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org, serviceworker-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionSkip the Service Worker CORS fallback for same origin requests. [1/2 blink]
We are sending CORS fallback response from the browser process to the page's renderer process, when the ServiceWorker doesn't handle the CORS fetch request.
But when the request URL's origin is same as the page's security origin, we can skip the fallback and send the request from the browser process.
This cl only changes ASSERT() in DocumentThreadableLoader.
This cl adds "securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url())" in ASSERT().
It is OK because:
- The document which origin is an unique origin can't be controlled by Service Worker.
- Whenever "provider_host_->document_url().GetOrigin() == request()->url().GetOrigin()" is true at ServiceWorkerURLRequestJob::DidDispatchFetchEvent(), canRequest() must be true here.
1/2 blink: this cl.
2/2 chromium: https://codereview.chromium.org/1242953007/
BUG=512764
Committed: https://crrev.com/5d83ca9b0d1849cc0d2bd0d6bb503ea564924e4f
git-svn-id: svn://svn.chromium.org/blink/trunk@199655 bbb929c8-8fbe-4397-9dbb-9b2b20218538
Patch Set 1 : #
Total comments: 3
Patch Set 2 : incorporated tyoshino's comment #
Total comments: 2
Patch Set 3 : add link to crbug.com/336894 #
Total comments: 5
Messages
Total messages: 22 (5 generated)
Patchset #1 (id:1) has been deleted
horo@chromium.org changed reviewers: + tyoshino@chromium.org
tyoshino@ Could you please review this?
https://codereview.chromium.org/1243353002/diff/20001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1243353002/diff/20001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:455: // |m_fallbackRequestForServiceWorker| may be set only wheh the request is wheh -> when https://codereview.chromium.org/1243353002/diff/20001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:457: // origin requests. |m_fallbackRequestForServiceWorker| being set is not a result of loading. So, this text looks a little strange. It's hard to phrase... How about: Even if the request met the conditions to get handled by a Service Worker in the constructor of this class (and therefore |m_fallbackRequestForServiceWorker| is set), the Service Worker may skip processing the request. Only if the request is same origin, the skipped response may come here (wasFetchedViaServiceWorker() returns false) since such a request doesn't have to go through the CORS algorithm by calling loadFallbackRequestForServiceWorker().
Thank you! https://codereview.chromium.org/1243353002/diff/20001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1243353002/diff/20001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:457: // origin requests. On 2015/07/22 11:25:45, tyoshino wrote: > |m_fallbackRequestForServiceWorker| being set is not a result of loading. So, > this text looks a little strange. > > It's hard to phrase... How about: > > Even if the request met the conditions to get handled by a Service Worker in the > constructor of this class (and therefore |m_fallbackRequestForServiceWorker| is > set), the Service Worker may skip processing the request. Only if the request is > same origin, the skipped response may come here (wasFetchedViaServiceWorker() > returns false) since such a request doesn't have to go through the CORS > algorithm by calling loadFallbackRequestForServiceWorker(). Done.
falken@chromium.org changed reviewers: + falken@chromium.org
https://codereview.chromium.org/1243353002/diff/40001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1243353002/diff/40001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:463: // Suborigins for Service Worker. Can you link to a bug or make it more clear to future readers when the FIXME can be addressed? I'm unfamiliar with Suborigins for Service Worker (and I work on SW).
https://codereview.chromium.org/1243353002/diff/40001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1243353002/diff/40001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:463: // Suborigins for Service Worker. On 2015/07/22 22:05:56, falken wrote: > Can you link to a bug or make it more clear to future readers when the FIXME can > be addressed? I'm unfamiliar with Suborigins for Service Worker (and I work on > SW). Done.
https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:464: ASSERT(!m_fallbackRequestForServiceWorker || securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url())); have you investigated carefully that this is consistent with the condition used in https://codereview.chromium.org/1242953007/diff/40001/content/browser/service... ? For example, m_universalAccess which is set when the --disable-web-security flag is specified.
https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:464: ASSERT(!m_fallbackRequestForServiceWorker || securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url())); On 2015/07/23 05:33:16, tyoshino wrote: > have you investigated carefully that this is consistent with the condition used > in > https://codereview.chromium.org/1242953007/diff/40001/content/browser/service... > ? > > For example, m_universalAccess which is set when the --disable-web-security flag > is specified. I don't think these conditions (in blink and content) must be strictly consistent. Whenever "provider_host_->document_url().GetOrigin() == request()->url().GetOrigin()" is true, "securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url()" must be true.
https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:464: ASSERT(!m_fallbackRequestForServiceWorker || securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url())); On 2015/07/23 07:59:44, horo wrote: > On 2015/07/23 05:33:16, tyoshino wrote: > > have you investigated carefully that this is consistent with the condition > used > > in > > > https://codereview.chromium.org/1242953007/diff/40001/content/browser/service... > > ? > > > > For example, m_universalAccess which is set when the --disable-web-security > flag > > is specified. > > I don't think these conditions (in blink and content) must be strictly > consistent. > Whenever "provider_host_->document_url().GetOrigin() == > request()->url().GetOrigin()" is true, > "securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url()" must be > true. Ah, yeah, actually it's enough that condition holds. How about unique origins?
https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:464: ASSERT(!m_fallbackRequestForServiceWorker || securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url())); On 2015/07/23 08:09:42, tyoshino wrote: > On 2015/07/23 07:59:44, horo wrote: > > On 2015/07/23 05:33:16, tyoshino wrote: > > > have you investigated carefully that this is consistent with the condition > > used > > > in > > > > > > https://codereview.chromium.org/1242953007/diff/40001/content/browser/service... > > > ? > > > > > > For example, m_universalAccess which is set when the --disable-web-security > > flag > > > is specified. > > > > I don't think these conditions (in blink and content) must be strictly > > consistent. > > Whenever "provider_host_->document_url().GetOrigin() == > > request()->url().GetOrigin()" is true, > > "securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url()" must > be > > true. > > Ah, yeah, actually it's enough that condition holds. > > How about unique origins? The document which origin is an unique origin can't be controlled by Service Worker.
lgtm https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:464: ASSERT(!m_fallbackRequestForServiceWorker || securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url())); On 2015/07/23 09:42:00, horo wrote: > On 2015/07/23 08:09:42, tyoshino wrote: > > On 2015/07/23 07:59:44, horo wrote: > > > On 2015/07/23 05:33:16, tyoshino wrote: > > > > have you investigated carefully that this is consistent with the condition > > > used > > > > in > > > > > > > > > > https://codereview.chromium.org/1242953007/diff/40001/content/browser/service... > > > > ? > > > > > > > > For example, m_universalAccess which is set when the > --disable-web-security > > > flag > > > > is specified. > > > > > > I don't think these conditions (in blink and content) must be strictly > > > consistent. > > > Whenever "provider_host_->document_url().GetOrigin() == > > > request()->url().GetOrigin()" is true, > > > "securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url()" must > > be > > > true. > > > > Ah, yeah, actually it's enough that condition holds. > > > > How about unique origins? > > The document which origin is an unique origin can't be controlled by Service > Worker. Thanks. Please write that in the CL description.
On 2015/07/23 09:50:14, tyoshino wrote: > lgtm > > https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... > File Source/core/loader/DocumentThreadableLoader.cpp (right): > > https://codereview.chromium.org/1243353002/diff/60001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.cpp:464: > ASSERT(!m_fallbackRequestForServiceWorker || > securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url())); > On 2015/07/23 09:42:00, horo wrote: > > On 2015/07/23 08:09:42, tyoshino wrote: > > > On 2015/07/23 07:59:44, horo wrote: > > > > On 2015/07/23 05:33:16, tyoshino wrote: > > > > > have you investigated carefully that this is consistent with the > condition > > > > used > > > > > in > > > > > > > > > > > > > > > https://codereview.chromium.org/1242953007/diff/40001/content/browser/service... > > > > > ? > > > > > > > > > > For example, m_universalAccess which is set when the > > --disable-web-security > > > > flag > > > > > is specified. > > > > > > > > I don't think these conditions (in blink and content) must be strictly > > > > consistent. > > > > Whenever "provider_host_->document_url().GetOrigin() == > > > > request()->url().GetOrigin()" is true, > > > > "securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url()" > must > > > be > > > > true. > > > > > > Ah, yeah, actually it's enough that condition holds. > > > > > > How about unique origins? > > > > The document which origin is an unique origin can't be controlled by Service > > Worker. > > Thanks. Please write that in the CL description. done
Thank you. lgtm again
horo@chromium.org changed reviewers: + kinuko@chromium.org
kinuko@ Could you please review this?
lgtm
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1243353002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1243353002/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199655
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5d83ca9b0d1849cc0d2bd0d6bb503ea564924e4f |
