Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1802693002:
Fix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse() (Closed)
|
Created:
4 years, 9 months ago by Nate Chapin Modified:
4 years, 9 months ago Reviewers:
hiroshige CC:
chromium-reviews, blink-reviews, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org, gavinp+loader_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse()
This was regressed in https://codereview.chromium.org/1751203003, which removed an early-clear of ResourceLoader::m_loader in ResourceLoader::cancel().
If m_resource->error() triggers a JS event handler that contains an alert(), we will run a nested event loop. Normally, we call setDefersLoading() on all ResourceLoaders to ensure no resource loading messages are processed. However, before calling m_resource->error(), we call m_fetcher->didFailLoading(), which removes the ResourceLoader from ResourceFetcher::m_loaders, which means ResourceFetcher has no way to call setDefersLoading() on it. When we enter the nested event loop, the response IPC for the ResourceLoader that is being cancelled is processed, and we call didReceiveResponse() in an invalid state.
Add back the early clear of ResourceLoader::m_loader to ensure it can't send messages while completing the cancellation, and add a comment to document why it's necessary.
BUG=594072
Committed: https://crrev.com/c9b6287c3a354def3608e67621eb609612eb7e5d
Cr-Commit-Position: refs/heads/master@{#381086}
Patch Set 1 #
Messages
Total messages: 14 (7 generated)
Description was changed from ========== Fix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse() BUG= ========== to ========== Fix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse() This was regressed in https://codereview.chromium.org/1751203003, which removed an early-clear of ResourceLoader::m_loader in ResourceLoader::cancel(). If m_resource->error() triggers a JS event handler that contains an alert(), we will run a modal dialog. Normally, we call setDefersLoading() on all ResourceLoaders to ensure no resource loading messages are processed. However, before calling m_resource->error(), we call m_fetcher->didFailLoading(), which removes the ResourceLoader from ResourceFetcher::m_loaders, which means ResourceFetcher has no way to call setDefersLoading() on it. When we enter the modal dialog, the response IPC for the ResourceLoader that is being cancelled is processed, and we call didReceiveResponse() in an invalid state. Add back the early clear of ResourceLoader::m_loader to ensure it can't send messages while completing the cancellation, and add a comment to document why it's necessary. BUG=594072 ==========
Description was changed from ========== Fix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse() This was regressed in https://codereview.chromium.org/1751203003, which removed an early-clear of ResourceLoader::m_loader in ResourceLoader::cancel(). If m_resource->error() triggers a JS event handler that contains an alert(), we will run a modal dialog. Normally, we call setDefersLoading() on all ResourceLoaders to ensure no resource loading messages are processed. However, before calling m_resource->error(), we call m_fetcher->didFailLoading(), which removes the ResourceLoader from ResourceFetcher::m_loaders, which means ResourceFetcher has no way to call setDefersLoading() on it. When we enter the modal dialog, the response IPC for the ResourceLoader that is being cancelled is processed, and we call didReceiveResponse() in an invalid state. Add back the early clear of ResourceLoader::m_loader to ensure it can't send messages while completing the cancellation, and add a comment to document why it's necessary. BUG=594072 ========== to ========== Fix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse() This was regressed in https://codereview.chromium.org/1751203003, which removed an early-clear of ResourceLoader::m_loader in ResourceLoader::cancel(). If m_resource->error() triggers a JS event handler that contains an alert(), we will run a nested event loop. Normally, we call setDefersLoading() on all ResourceLoaders to ensure no resource loading messages are processed. However, before calling m_resource->error(), we call m_fetcher->didFailLoading(), which removes the ResourceLoader from ResourceFetcher::m_loaders, which means ResourceFetcher has no way to call setDefersLoading() on it. When we enter the modal dialog, the response IPC for the ResourceLoader that is being cancelled is processed, and we call didReceiveResponse() in an invalid state. Add back the early clear of ResourceLoader::m_loader to ensure it can't send messages while completing the cancellation, and add a comment to document why it's necessary. BUG=594072 ==========
Description was changed from ========== Fix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse() This was regressed in https://codereview.chromium.org/1751203003, which removed an early-clear of ResourceLoader::m_loader in ResourceLoader::cancel(). If m_resource->error() triggers a JS event handler that contains an alert(), we will run a nested event loop. Normally, we call setDefersLoading() on all ResourceLoaders to ensure no resource loading messages are processed. However, before calling m_resource->error(), we call m_fetcher->didFailLoading(), which removes the ResourceLoader from ResourceFetcher::m_loaders, which means ResourceFetcher has no way to call setDefersLoading() on it. When we enter the modal dialog, the response IPC for the ResourceLoader that is being cancelled is processed, and we call didReceiveResponse() in an invalid state. Add back the early clear of ResourceLoader::m_loader to ensure it can't send messages while completing the cancellation, and add a comment to document why it's necessary. BUG=594072 ========== to ========== Fix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse() This was regressed in https://codereview.chromium.org/1751203003, which removed an early-clear of ResourceLoader::m_loader in ResourceLoader::cancel(). If m_resource->error() triggers a JS event handler that contains an alert(), we will run a nested event loop. Normally, we call setDefersLoading() on all ResourceLoaders to ensure no resource loading messages are processed. However, before calling m_resource->error(), we call m_fetcher->didFailLoading(), which removes the ResourceLoader from ResourceFetcher::m_loaders, which means ResourceFetcher has no way to call setDefersLoading() on it. When we enter the nested event loop, the response IPC for the ResourceLoader that is being cancelled is processed, and we call didReceiveResponse() in an invalid state. Add back the early clear of ResourceLoader::m_loader to ensure it can't send messages while completing the cancellation, and add a comment to document why it's necessary. BUG=594072 ==========
japhet@chromium.org changed reviewers: + hiroshige@chromium.org
I can't find a test to reproduce this. Writing a unit test would require add new
accessors to ResourceLoader or making the test a private friend, and probably
writing a complicated nested event loop test. Calling an alert() in a layout
test doesn't actually create a nested event loop, so that won't work.
I've verified that the following snippet reliably crashes in a full build of
chrome, but doesn't with this fix.
<body>
<script>
var x = new XMLHttpRequest();
x.open("GET", "any-url-goes-here.html", true);
x.onabort = function() {
alert("This shouldn't trigger a crash.");
}
x.send();
window.stop();
</script>
</body>
On 2016/03/14 at 20:45:02, japhet wrote:
> I can't find a test to reproduce this. Writing a unit test would require add
new accessors to ResourceLoader or making the test a private friend, and
probably writing a complicated nested event loop test. Calling an alert() in a
layout test doesn't actually create a nested event loop, so that won't work.
>
> I've verified that the following snippet reliably crashes in a full build of
chrome, but doesn't with this fix.
>
> <body>
> <script>
> var x = new XMLHttpRequest();
> x.open("GET", "any-url-goes-here.html", true);
> x.onabort = function() {
> alert("This shouldn't trigger a crash.");
> }
> x.send();
> window.stop();
> </script>
> </body>
Some random thoughts:
1) Can you sync XHR in the onabort handler?
2) Could we abuse web platform things that actually use a sync IPC under the
hood? document.cookie and localStorage come to mind (but I'm guessing they're
mocked out in layout tests)
3) Alternatively, we could add a testRunner helper which purposely enters a
nested message loop…
4) In a unit test, I think we could use
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
to easily intercept didReceiveResponse without adding friends / accessors to
ResourceLoader.
I think (4) might be the best and it shouldn't be too hard: it looks like most
of the plumbing is already there.
On 2016/03/14 21:15:05, dcheng wrote:
> On 2016/03/14 at 20:45:02, japhet wrote:
> > I can't find a test to reproduce this. Writing a unit test would require add
> new accessors to ResourceLoader or making the test a private friend, and
> probably writing a complicated nested event loop test. Calling an alert() in a
> layout test doesn't actually create a nested event loop, so that won't work.
> >
> > I've verified that the following snippet reliably crashes in a full build of
> chrome, but doesn't with this fix.
> >
> > <body>
> > <script>
> > var x = new XMLHttpRequest();
> > x.open("GET", "any-url-goes-here.html", true);
> > x.onabort = function() {
> > alert("This shouldn't trigger a crash.");
> > }
> > x.send();
> > window.stop();
> > </script>
> > </body>
>
> Some random thoughts:
> 1) Can you sync XHR in the onabort handler?
> 2) Could we abuse web platform things that actually use a sync IPC under the
> hood? document.cookie and localStorage come to mind (but I'm guessing they're
> mocked out in layout tests)
> 3) Alternatively, we could add a testRunner helper which purposely enters a
> nested message loop…
> 4) In a unit test, I think we could use
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> to easily intercept didReceiveResponse without adding friends / accessors to
> ResourceLoader.
>
> I think (4) might be the best and it shouldn't be too hard: it looks like most
> of the plumbing is already there.
Oooh, (4) looks promising. I'll look into that, though I may do so in a separate
CL, TPMs want this patch today :/
lgtm.
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1802693002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802693002/1
Message was sent while issue was closed.
Description was changed from ========== Fix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse() This was regressed in https://codereview.chromium.org/1751203003, which removed an early-clear of ResourceLoader::m_loader in ResourceLoader::cancel(). If m_resource->error() triggers a JS event handler that contains an alert(), we will run a nested event loop. Normally, we call setDefersLoading() on all ResourceLoaders to ensure no resource loading messages are processed. However, before calling m_resource->error(), we call m_fetcher->didFailLoading(), which removes the ResourceLoader from ResourceFetcher::m_loaders, which means ResourceFetcher has no way to call setDefersLoading() on it. When we enter the nested event loop, the response IPC for the ResourceLoader that is being cancelled is processed, and we call didReceiveResponse() in an invalid state. Add back the early clear of ResourceLoader::m_loader to ensure it can't send messages while completing the cancellation, and add a comment to document why it's necessary. BUG=594072 ========== to ========== Fix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse() This was regressed in https://codereview.chromium.org/1751203003, which removed an early-clear of ResourceLoader::m_loader in ResourceLoader::cancel(). If m_resource->error() triggers a JS event handler that contains an alert(), we will run a nested event loop. Normally, we call setDefersLoading() on all ResourceLoaders to ensure no resource loading messages are processed. However, before calling m_resource->error(), we call m_fetcher->didFailLoading(), which removes the ResourceLoader from ResourceFetcher::m_loaders, which means ResourceFetcher has no way to call setDefersLoading() on it. When we enter the nested event loop, the response IPC for the ResourceLoader that is being cancelled is processed, and we call didReceiveResponse() in an invalid state. Add back the early clear of ResourceLoader::m_loader to ensure it can't send messages while completing the cancellation, and add a comment to document why it's necessary. BUG=594072 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse() This was regressed in https://codereview.chromium.org/1751203003, which removed an early-clear of ResourceLoader::m_loader in ResourceLoader::cancel(). If m_resource->error() triggers a JS event handler that contains an alert(), we will run a nested event loop. Normally, we call setDefersLoading() on all ResourceLoaders to ensure no resource loading messages are processed. However, before calling m_resource->error(), we call m_fetcher->didFailLoading(), which removes the ResourceLoader from ResourceFetcher::m_loaders, which means ResourceFetcher has no way to call setDefersLoading() on it. When we enter the nested event loop, the response IPC for the ResourceLoader that is being cancelled is processed, and we call didReceiveResponse() in an invalid state. Add back the early clear of ResourceLoader::m_loader to ensure it can't send messages while completing the cancellation, and add a comment to document why it's necessary. BUG=594072 ========== to ========== Fix triggered RELEASE_ASSERT in ResourceLoader::didReceiveResponse() This was regressed in https://codereview.chromium.org/1751203003, which removed an early-clear of ResourceLoader::m_loader in ResourceLoader::cancel(). If m_resource->error() triggers a JS event handler that contains an alert(), we will run a nested event loop. Normally, we call setDefersLoading() on all ResourceLoaders to ensure no resource loading messages are processed. However, before calling m_resource->error(), we call m_fetcher->didFailLoading(), which removes the ResourceLoader from ResourceFetcher::m_loaders, which means ResourceFetcher has no way to call setDefersLoading() on it. When we enter the nested event loop, the response IPC for the ResourceLoader that is being cancelled is processed, and we call didReceiveResponse() in an invalid state. Add back the early clear of ResourceLoader::m_loader to ensure it can't send messages while completing the cancellation, and add a comment to document why it's necessary. BUG=594072 Committed: https://crrev.com/c9b6287c3a354def3608e67621eb609612eb7e5d Cr-Commit-Position: refs/heads/master@{#381086} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/c9b6287c3a354def3608e67621eb609612eb7e5d Cr-Commit-Position: refs/heads/master@{#381086} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
