Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2472343002:
Race OnStartLoadingResponseBody and OnReceiveResponse of NavigationPreloadRequest. (Closed)
|
Created:
4 years, 1 month ago by horo Modified:
4 years, 1 month ago Reviewers:
falken CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, mlamouri+watch-content_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, kinuko+serviceworker, horo+watch_chromium.org, darin-cc_chromium.org, kinuko+watch, tzik, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRace OnStartLoadingResponseBody and OnReceiveResponse of NavigationPreloadRequest.
When the content type of the page is not correctly set,
OnStartLoadingResponseBody() of mojom::URLLoaderClient is called befor
OnReceiveResponse(). This behavior is caused by MimeSniffingResourceHandler.
BUG=649558
Committed: https://crrev.com/caeeed9a533b2695fd3b0be36e79862a10101812
Cr-Commit-Position: refs/heads/master@{#429860}
Patch Set 1 #
Total comments: 7
Patch Set 2 : rebase and fix comment #Patch Set 3 : rebase #
Dependent Patchsets: Messages
Total messages: 20 (12 generated)
Description was changed from ========== Race OnStartLoadingResponseBody and OnReceiveResponse of NavigationPreloadRequest. BUG=649558 ========== to ========== Race OnStartLoadingResponseBody and OnReceiveResponse of NavigationPreloadRequest. When the content type of the page is not correctly set, OnStartLoadingResponseBody() of mojom::URLLoaderClient is called befor OnReceiveResponse(). This behavior is caused by MimeSniffingResourceHandler. BUG=649558 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
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...
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ Could you please review this?
https://codereview.chromium.org/2472343002/diff/40001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2472343002/diff/40001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:314: DCHECK(!result_reported_); I'm missing something... how does this DCHECK pass since both OnReceiveRersponse() and OnStartLoadingResposneBody() call MaybeReportToClient()? Does it need to be moved under the if below?
https://codereview.chromium.org/2472343002/diff/40001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2472343002/diff/40001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:314: DCHECK(!result_reported_); On 2016/11/04 09:36:50, falken wrote: > I'm missing something... how does this DCHECK pass since both > OnReceiveRersponse() and OnStartLoadingResposneBody() call > MaybeReportToClient()? Does it need to be moved under the if below? Both OnReceiveRersponse() and OnStartLoadingResposneBody() are called only once. OnReceiveRersponse() sets |response_|. OnStartLoadingResposneBody() sets |body_|. So this check "if (!response_ || !body_.is_valid())" pass only once.
lgtm. Should it be documented in the URLLoaderClient interface that OnStartLoadingResponseBody can be called before OnReceiveResponse? https://codereview.chromium.org/2472343002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2472343002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1928: // navigation preload must be handled correctly. Thanks for docu. s/triggerd/triggered https://codereview.chromium.org/2472343002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1941: // Setting an empty content type to triger MimeSniffingResourceHandler. s/triger/trigger https://codereview.chromium.org/2472343002/diff/40001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2472343002/diff/40001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:314: DCHECK(!result_reported_); On 2016/11/04 09:41:48, horo wrote: > On 2016/11/04 09:36:50, falken wrote: > > I'm missing something... how does this DCHECK pass since both > > OnReceiveRersponse() and OnStartLoadingResposneBody() call > > MaybeReportToClient()? Does it need to be moved under the if below? > > Both OnReceiveRersponse() and OnStartLoadingResposneBody() are called only once. > OnReceiveRersponse() sets |response_|. > OnStartLoadingResposneBody() sets |body_|. > So this check "if (!response_ || !body_.is_valid())" pass only once. Ah yes... I was totally missing something.
> lgtm. Should it be documented in the URLLoaderClient interface that > OnStartLoadingResponseBody can be called before OnReceiveResponse? Ah, yes. We should. https://codereview.chromium.org/2472343002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2472343002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1928: // navigation preload must be handled correctly. On 2016/11/04 09:46:55, falken wrote: > Thanks for docu. s/triggerd/triggered Done. https://codereview.chromium.org/2472343002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1941: // Setting an empty content type to triger MimeSniffingResourceHandler. On 2016/11/04 09:46:55, falken wrote: > s/triger/trigger 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 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/2472343002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Race OnStartLoadingResponseBody and OnReceiveResponse of NavigationPreloadRequest. When the content type of the page is not correctly set, OnStartLoadingResponseBody() of mojom::URLLoaderClient is called befor OnReceiveResponse(). This behavior is caused by MimeSniffingResourceHandler. BUG=649558 ========== to ========== Race OnStartLoadingResponseBody and OnReceiveResponse of NavigationPreloadRequest. When the content type of the page is not correctly set, OnStartLoadingResponseBody() of mojom::URLLoaderClient is called befor OnReceiveResponse(). This behavior is caused by MimeSniffingResourceHandler. BUG=649558 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Race OnStartLoadingResponseBody and OnReceiveResponse of NavigationPreloadRequest. When the content type of the page is not correctly set, OnStartLoadingResponseBody() of mojom::URLLoaderClient is called befor OnReceiveResponse(). This behavior is caused by MimeSniffingResourceHandler. BUG=649558 ========== to ========== Race OnStartLoadingResponseBody and OnReceiveResponse of NavigationPreloadRequest. When the content type of the page is not correctly set, OnStartLoadingResponseBody() of mojom::URLLoaderClient is called befor OnReceiveResponse(). This behavior is caused by MimeSniffingResourceHandler. BUG=649558 Committed: https://crrev.com/caeeed9a533b2695fd3b0be36e79862a10101812 Cr-Commit-Position: refs/heads/master@{#429860} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/caeeed9a533b2695fd3b0be36e79862a10101812 Cr-Commit-Position: refs/heads/master@{#429860} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
