Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2399463002:
Clear mojo URLLoaderClient in ResourceDispatcher on cancellation (Closed)
DescriptionClear mojo URLLoaderClient in ResourceDispatcher on cancellation
If a URL request is cancelled by a requester, there's a chance on
URLResponseBodyConsumer to run OnReadable() on an invalid
ResourceDispatcher::PendingRequestInfo. That causes a null pointer
access.
This CL clears the URLLoaderClient earlier in the cancellation phase to
avoid the crash.
BUG=603396
Committed: https://crrev.com/bc1ffa6150628c6ddb8ae3cf51b8e9d0e965f1a4
Cr-Commit-Position: refs/heads/master@{#423779}
Patch Set 1 #
Total comments: 4
Patch Set 2 : +comment. clear url_loader_client unconditionally #Messages
Total messages: 25 (13 generated)
Description was changed from ========== Clear mojo URLLoaderClient in ResourceDispatcher on cancellation BUG= ========== to ========== Clear mojo URLLoaderClient in ResourceDispatcher on cancellation BUG= ==========
tzik@chromium.org changed reviewers: + yhirano@chromium.org
Description was changed from ========== Clear mojo URLLoaderClient in ResourceDispatcher on cancellation BUG= ========== to ========== Clear mojo URLLoaderClient in ResourceDispatcher on cancellation If a URL request is cancelled by a requester, there's a chance on URLResponseBodyConsumer to run OnReadable() on an invalid ResourceDispatcher::PendingRequestInfo. That causes a null pointer access. This CL clears the URLLoaderClient earlier in the cancellation phase to avoid the crash. BUG= ==========
PTAL
https://codereview.chromium.org/2399463002/diff/1/content/child/resource_disp... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/2399463002/diff/1/content/child/resource_disp... content/child/resource_dispatcher.cc:457: it->second->url_loader_client = nullptr; Would Calling Cancel be enough?
https://codereview.chromium.org/2399463002/diff/1/content/child/resource_disp... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/2399463002/diff/1/content/child/resource_disp... content/child/resource_dispatcher.cc:457: it->second->url_loader_client = nullptr; On 2016/10/05 09:03:00, yhirano wrote: > Would Calling Cancel be enough? As we chatted offline, URLLoaderClient (and URLLoaderClientImpl) doesn't have Cancel(), and I think it's less cleaner to implement it, since we'll have to care a IPC message arrival after the cancellation.
lgtm https://codereview.chromium.org/2399463002/diff/1/content/child/resource_disp... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/2399463002/diff/1/content/child/resource_disp... content/child/resource_dispatcher.cc:457: it->second->url_loader_client = nullptr; Can you run this statement unconditionally?
Description was changed from ========== Clear mojo URLLoaderClient in ResourceDispatcher on cancellation If a URL request is cancelled by a requester, there's a chance on URLResponseBodyConsumer to run OnReadable() on an invalid ResourceDispatcher::PendingRequestInfo. That causes a null pointer access. This CL clears the URLLoaderClient earlier in the cancellation phase to avoid the crash. BUG= ========== to ========== Clear mojo URLLoaderClient in ResourceDispatcher on cancellation If a URL request is cancelled by a requester, there's a chance on URLResponseBodyConsumer to run OnReadable() on an invalid ResourceDispatcher::PendingRequestInfo. That causes a null pointer access. This CL clears the URLLoaderClient earlier in the cancellation phase to avoid the crash. BUG=603396 ==========
tzik@chromium.org changed reviewers: + jam@chromium.org
The CQ bit was checked by tzik@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...
Adding jam@ as a //content owner. PTAL. https://codereview.chromium.org/2399463002/diff/1/content/child/resource_disp... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/2399463002/diff/1/content/child/resource_disp... content/child/resource_dispatcher.cc:457: it->second->url_loader_client = nullptr; On 2016/10/06 06:53:05, yhirano wrote: > Can you run this statement unconditionally? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm is it possible to write a test for this?
On 2016/10/06 15:39:37, jam wrote: > lgtm > > is it possible to write a test for this? Blink's layout tests at virtual/mojo-loading/http/tests/xmlhttprequest will cover this. They are currently not enabled, but we'll enable it before ship it.
On 2016/10/06 15:51:24, tzik wrote: > On 2016/10/06 15:39:37, jam wrote: > > lgtm > > > > is it possible to write a test for this? > > Blink's layout tests at virtual/mojo-loading/http/tests/xmlhttprequest will > cover this. They are currently not enabled, but we'll enable it before ship it. sgtm
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2399463002/#ps20001 (title: "+comment. clear url_loader_client unconditionally")
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 ========== Clear mojo URLLoaderClient in ResourceDispatcher on cancellation If a URL request is cancelled by a requester, there's a chance on URLResponseBodyConsumer to run OnReadable() on an invalid ResourceDispatcher::PendingRequestInfo. That causes a null pointer access. This CL clears the URLLoaderClient earlier in the cancellation phase to avoid the crash. BUG=603396 ========== to ========== Clear mojo URLLoaderClient in ResourceDispatcher on cancellation If a URL request is cancelled by a requester, there's a chance on URLResponseBodyConsumer to run OnReadable() on an invalid ResourceDispatcher::PendingRequestInfo. That causes a null pointer access. This CL clears the URLLoaderClient earlier in the cancellation phase to avoid the crash. BUG=603396 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Clear mojo URLLoaderClient in ResourceDispatcher on cancellation If a URL request is cancelled by a requester, there's a chance on URLResponseBodyConsumer to run OnReadable() on an invalid ResourceDispatcher::PendingRequestInfo. That causes a null pointer access. This CL clears the URLLoaderClient earlier in the cancellation phase to avoid the crash. BUG=603396 ========== to ========== Clear mojo URLLoaderClient in ResourceDispatcher on cancellation If a URL request is cancelled by a requester, there's a chance on URLResponseBodyConsumer to run OnReadable() on an invalid ResourceDispatcher::PendingRequestInfo. That causes a null pointer access. This CL clears the URLLoaderClient earlier in the cancellation phase to avoid the crash. BUG=603396 Committed: https://crrev.com/bc1ffa6150628c6ddb8ae3cf51b8e9d0e965f1a4 Cr-Commit-Position: refs/heads/master@{#423779} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/bc1ffa6150628c6ddb8ae3cf51b8e9d0e965f1a4 Cr-Commit-Position: refs/heads/master@{#423779}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2396393002/ by finnur@chromium.org. The reason for reverting is: This is a speculative revert... Two tests started failing somewhat reliably on Win7 in the build where this was checked in and all the other CLs in that build look completely unrelated. NavigatingExtensionPopupBrowserTest.DownloadViaPost PageLoadMetricsBrowserTest.IgnoreDownloads I've actually already disabled the NavigatingExtensionPopupBrowserTest in a separate CL, but if the PageLoadMetricsBrowserTest doesn't get fixed with this revert then we can undo the revert.. |
