|
|
Created:
3 years, 7 months ago by horo Modified:
3 years, 7 months ago CC:
chromium-reviews, michaeln, mlamouri+watch-content_chromium.org, serviceworker-reviews, creis+watch_chromium.org, tzik, jsbell+serviceworker_chromium.org, nasko+codewatch_chromium.org, jam, kinuko+serviceworker, nhiroki, darin-cc_chromium.org, blink-reviews, horo+watch_chromium.org, kinuko+watch, shimazu+serviceworker_chromium.org, blink-worker-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet the requester context info to the requests for off-main-thread-fetch.
This CL pipes the IsOriginSecure state to WorkerFetchContext and set it to
the RequestExtraData at WorkerFetchContextImpl::WillSendRequest().
And WorkerFetchContext::PrepareRequest() sets the RequestorOrigin of requests.
After this cl, foreign fetch tests will pass.
BUG=443374
R=falken@chromium.org, kinuko@chromium.org
Review-Url: https://codereview.chromium.org/2890723002 .
Cr-Commit-Position: refs/heads/master@{#473086}
Committed: https://chromium.googlesource.com/chromium/src/+/4c4e9325460f277a8db45b8b12d48efaddbdb841
Patch Set 1 #
Total comments: 8
Patch Set 2 : add comments #Patch Set 3 : rebase #
Total comments: 8
Patch Set 4 : rebase and incorporated falken's comment #
Total comments: 2
Patch Set 5 : rebase and s/Set/Sets/ #
Depends on Patchset: Messages
Total messages: 49 (32 generated)
Description was changed from ========== Set the requester context info to the requests for off-main-thread-fetch. This CL pipes the IsOriginSecure state to WorkerFetchContext and set it to the RequestExtraData at WorkerFetchContextImpl::WillSendRequest(). And WorkerFetchContext::PrepareRequest() sets the RequestorOrigin of requests. After this cl, foreign fetch tests will pass. BUG= ========== to ========== Set the requester context info to the requests for off-main-thread-fetch. This CL pipes the IsOriginSecure state to WorkerFetchContext and set it to the RequestExtraData at WorkerFetchContextImpl::WillSendRequest(). And WorkerFetchContext::PrepareRequest() sets the RequestorOrigin of requests. After this cl, foreign fetch tests will pass. BUG=443374 ==========
The CQ bit was checked by horo@chromium.org to run a CQ dry run
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ Please review this.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2890723002/diff/1/content/renderer/service_wo... File content/renderer/service_worker/worker_fetch_context_impl.h (right): https://codereview.chromium.org/2890723002/diff/1/content/renderer/service_wo... content/renderer/service_worker/worker_fetch_context_impl.h:20: just to be sure: this context is going to be associated with a dedicated worker or a shared worker (and not a shared worker), right? Can we document that? https://codereview.chromium.org/2890723002/diff/1/content/renderer/service_wo... content/renderer/service_worker/worker_fetch_context_impl.h:39: // Sets the fetch context status of the parent frame. What's the "parent frame" when this is for a shared worker? There can be multiple frames, right? https://codereview.chromium.org/2890723002/diff/1/content/renderer/service_wo... content/renderer/service_worker/worker_fetch_context_impl.h:42: void set_is_secure_context(bool flag); Can you document what this flag means? https://codereview.chromium.org/2890723002/diff/1/content/renderer/shared_wor... File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right): https://codereview.chromium.org/2890723002/diff/1/content/renderer/shared_wor... content/renderer/shared_worker/embedded_shared_worker_stub.cc:275: worker_fetch_context->set_is_secure_context(IsOriginSecure(url_)); secure context usually means all the ancestor contexts are secure. I guess we don't any kind of ancestor check here because the worker is its own top-level context? Maybe we should comment that assumption just to be explicit.
https://codereview.chromium.org/2890723002/diff/1/content/renderer/service_wo... File content/renderer/service_worker/worker_fetch_context_impl.h (right): https://codereview.chromium.org/2890723002/diff/1/content/renderer/service_wo... content/renderer/service_worker/worker_fetch_context_impl.h:20: On 2017/05/17 08:40:26, falken wrote: > just to be sure: this context is going to be associated with a dedicated worker > or a shared worker (and not a shared worker), right? Can we document that? Done. https://codereview.chromium.org/2890723002/diff/1/content/renderer/service_wo... content/renderer/service_worker/worker_fetch_context_impl.h:39: // Sets the fetch context status of the parent frame. On 2017/05/17 08:40:26, falken wrote: > What's the "parent frame" when this is for a shared worker? There can be > multiple frames, right? Added comment. https://codereview.chromium.org/2890723002/diff/1/content/renderer/service_wo... content/renderer/service_worker/worker_fetch_context_impl.h:42: void set_is_secure_context(bool flag); On 2017/05/17 08:40:26, falken wrote: > Can you document what this flag means? Done. https://codereview.chromium.org/2890723002/diff/1/content/renderer/shared_wor... File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right): https://codereview.chromium.org/2890723002/diff/1/content/renderer/shared_wor... content/renderer/shared_worker/embedded_shared_worker_stub.cc:275: worker_fetch_context->set_is_secure_context(IsOriginSecure(url_)); On 2017/05/17 08:40:26, falken wrote: > secure context usually means all the ancestor contexts are secure. > > I guess we don't any kind of ancestor check here because the worker is its own > top-level context? Maybe we should comment that assumption just to be explicit. Added comment.
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2890723002/diff/40001/content/renderer/servic... File content/renderer/service_worker/worker_fetch_context_impl.h (right): https://codereview.chromium.org/2890723002/diff/40001/content/renderer/servic... content/renderer/service_worker/worker_fetch_context_impl.h:24: // is created on the main thread and passed to the worker thread. Can you add "This class is not used for service workers. For service workers, XXX class is used instead"? Does it make sense for this class to be in content/renderer/service_worker/? Should it be in content/renderer/ instead? https://codereview.chromium.org/2890723002/diff/40001/content/renderer/servic... content/renderer/service_worker/worker_fetch_context_impl.h:49: // https://www.w3.org/TR/secure-contexts/ https://w3c.github.io/webappsec-secure-contexts/ is better, TR links tend to get outdated https://codereview.chromium.org/2890723002/diff/40001/content/renderer/shared... File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right): https://codereview.chromium.org/2890723002/diff/40001/content/renderer/shared... content/renderer/shared_worker/embedded_shared_worker_stub.cc:278: // non-secure. crbug.com/723575 just to confirm, this is not a regression with off-main-thread fetch, the current Chrome already does this right? https://codereview.chromium.org/2890723002/diff/40001/content/renderer/shared... content/renderer/shared_worker/embedded_shared_worker_stub.cc:279: // https://www.w3.org/TR/secure-contexts/#examples-shared-workers link to the newest spec: https://w3c.github.io/webappsec-secure-contexts/#examples-shared-workers
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2890723002/diff/40001/content/renderer/servic... File content/renderer/service_worker/worker_fetch_context_impl.h (right): https://codereview.chromium.org/2890723002/diff/40001/content/renderer/servic... content/renderer/service_worker/worker_fetch_context_impl.h:24: // is created on the main thread and passed to the worker thread. On 2017/05/18 02:14:13, falken wrote: > Can you add "This class is not used for service workers. For service workers, > XXX class is used instead"? Done. > Does it make sense for this class to be in content/renderer/service_worker/? > Should it be in content/renderer/ instead? Humm... This class inherits mojom::ServiceWorkerWorkerClient to receive SetControllerServiceWorker() IPC. So, it is OK to be here... But yes, I think it is also OK to be in content/renderer/. I don't have a strong opinion about it. https://codereview.chromium.org/2890723002/diff/40001/content/renderer/servic... content/renderer/service_worker/worker_fetch_context_impl.h:49: // https://www.w3.org/TR/secure-contexts/ On 2017/05/18 02:14:13, falken wrote: > https://w3c.github.io/webappsec-secure-contexts/ is better, TR links tend to get > outdated Done. https://codereview.chromium.org/2890723002/diff/40001/content/renderer/shared... File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right): https://codereview.chromium.org/2890723002/diff/40001/content/renderer/shared... content/renderer/shared_worker/embedded_shared_worker_stub.cc:278: // non-secure. crbug.com/723575 On 2017/05/18 02:14:13, falken wrote: > just to confirm, this is not a regression with off-main-thread fetch, the > current Chrome already does this right? Yes this is an existing bug even without OffMainThreadFetch. https://codereview.chromium.org/2890723002/diff/40001/content/renderer/shared... content/renderer/shared_worker/embedded_shared_worker_stub.cc:279: // https://www.w3.org/TR/secure-contexts/#examples-shared-workers On 2017/05/18 02:14:13, falken wrote: > link to the newest spec: > https://w3c.github.io/webappsec-secure-contexts/#examples-shared-workers Done.
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
OK lgtm https://codereview.chromium.org/2890723002/diff/60001/content/renderer/servic... File content/renderer/service_worker/worker_fetch_context_impl.h (right): https://codereview.chromium.org/2890723002/diff/60001/content/renderer/servic... content/renderer/service_worker/worker_fetch_context_impl.h:54: // Set whether the worker context is a secure context. nit: Sets
Thank you. https://codereview.chromium.org/2890723002/diff/60001/content/renderer/servic... File content/renderer/service_worker/worker_fetch_context_impl.h (right): https://codereview.chromium.org/2890723002/diff/60001/content/renderer/servic... content/renderer/service_worker/worker_fetch_context_impl.h:54: // Set whether the worker context is a secure context. On 2017/05/18 04:19:52, falken wrote: > nit: Sets Done.
horo@chromium.org changed reviewers: + kinuko@chromium.org
kinuko@ Please review content/renderer/render_frame_impl.cc.
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/2890723002/#ps80001 (title: "rebase and s/Set/Sets/")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Set the requester context info to the requests for off-main-thread-fetch. This CL pipes the IsOriginSecure state to WorkerFetchContext and set it to the RequestExtraData at WorkerFetchContextImpl::WillSendRequest(). And WorkerFetchContext::PrepareRequest() sets the RequestorOrigin of requests. After this cl, foreign fetch tests will pass. BUG=443374 ========== to ========== Set the requester context info to the requests for off-main-thread-fetch. This CL pipes the IsOriginSecure state to WorkerFetchContext and set it to the RequestExtraData at WorkerFetchContextImpl::WillSendRequest(). And WorkerFetchContext::PrepareRequest() sets the RequestorOrigin of requests. After this cl, foreign fetch tests will pass. BUG=443374 R=falken@chromium.org, kinuko@chromium.org Review-Url: https://codereview.chromium.org/2890723002 . Cr-Commit-Position: refs/heads/master@{#473086} Committed: https://chromium.googlesource.com/chromium/src/+/4c4e9325460f277a8db45b8b12d4... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 4c4e9325460f277a8db45b8b12d48efaddbdb841 (presubmit successful). |