Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2258003003:
[WebSocket] Ignore connection errors in the shutdown sequence (Closed)
|
Created:
4 years, 4 months ago by yhirano Modified:
4 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[WebSocket] Ignore connection errors in the shutdown sequence
WebSocketHandleImpl::OnConnectionError is called in the shutdown sequence
and at that time |*client_| is invalid. In the shutdown sequence it
is not guaranteed that the client destructs the WebSocketHandleImpl
correctly and hence we need to ignore the error.
This is a workaround.
BUG=637949
Committed: https://crrev.com/fce0b57ea91c0bbb263601906caf389ecf41c277
Cr-Commit-Position: refs/heads/master@{#413092}
Patch Set 1 : fix #Patch Set 2 : fix #
Total comments: 1
Messages
Total messages: 31 (21 generated)
The CQ bit was checked by yhirano@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 yhirano@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by yhirano@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...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by yhirano@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...
Description was changed from ========== Fix WebSocket crash on renderer shutdown This is a workaround. BUG=637949 ========== to ========== Fix WebSocket crash on renderer shutdown WebSocketHandleImpl::OnConnectionError is called in the shutdown sequence and at that time |*client_| is invalid. In the shutdown sequence it is not guaranteed that the client destructs the WebSocketHandleImpl correctly and hence we need to ignore the error. This is a workaround. BUG=637949 ==========
yhirano@chromium.org changed reviewers: + haraken@chromium.org
Description was changed from ========== Fix WebSocket crash on renderer shutdown WebSocketHandleImpl::OnConnectionError is called in the shutdown sequence and at that time |*client_| is invalid. In the shutdown sequence it is not guaranteed that the client destructs the WebSocketHandleImpl correctly and hence we need to ignore the error. This is a workaround. BUG=637949 ========== to ========== [WebSocket] Ignore connection errors in the shutdown sequence WebSocketHandleImpl::OnConnectionError is called in the shutdown sequence and at that time |*client_| is invalid. In the shutdown sequence it is not guaranteed that the client destructs the WebSocketHandleImpl correctly and hence we need to ignore the error. This is a workaround. BUG=637949 ==========
Or is it an option to add the 'if(!Platform::current())' check to the callbacks so that they can immediately return? That's what https://codereview.chromium.org/2163683004/ is doing.
On 2016/08/19 07:42:29, haraken wrote: > Or is it an option to add the 'if(!Platform::current())' check to the callbacks > so that they can immediately return? That's what > https://codereview.chromium.org/2163683004/ is doing. That is possible but I'm not sure if it's necessary. We need this "Disconnect() & return" when a callback is called after Oilpan is detached from the main thread and before the message loop is destructed. Is it possible to get a mojo message in the meantime?
On 2016/08/19 07:47:39, yhirano wrote: > On 2016/08/19 07:42:29, haraken wrote: > > Or is it an option to add the 'if(!Platform::current())' check to the > callbacks > > so that they can immediately return? That's what > > https://codereview.chromium.org/2163683004/ is doing. > > That is possible but I'm not sure if it's necessary. > We need this "Disconnect() & return" when a callback is called after Oilpan is > detached from the main thread and before the message loop is destructed. Is it > possible to get a mojo message in the meantime? If it's not possible, how can we hit the crash in the first place? The crash is happening because the mojo callback runs after Oilpan is detached from the main thread. (However, I must admit that I don't understand how it's possible.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/19 07:51:56, haraken wrote: > On 2016/08/19 07:47:39, yhirano wrote: > > On 2016/08/19 07:42:29, haraken wrote: > > > Or is it an option to add the 'if(!Platform::current())' check to the > > callbacks > > > so that they can immediately return? That's what > > > https://codereview.chromium.org/2163683004/ is doing. > > > > That is possible but I'm not sure if it's necessary. > > We need this "Disconnect() & return" when a callback is called after Oilpan is > > detached from the main thread and before the message loop is destructed. Is it > > possible to get a mojo message in the meantime? > > If it's not possible, how can we hit the crash in the first place? The crash is > happening because the mojo callback runs after Oilpan is detached from the main > thread. (However, I must admit that I don't understand how it's possible.) The crash was caused by the channel error notification triggered by the message loop destruction. I don't know other messages can arrive between the Oilpan detach and the message loop destruction.
On 2016/08/19 09:25:13, yhirano wrote: > On 2016/08/19 07:51:56, haraken wrote: > > On 2016/08/19 07:47:39, yhirano wrote: > > > On 2016/08/19 07:42:29, haraken wrote: > > > > Or is it an option to add the 'if(!Platform::current())' check to the > > > callbacks > > > > so that they can immediately return? That's what > > > > https://codereview.chromium.org/2163683004/ is doing. > > > > > > That is possible but I'm not sure if it's necessary. > > > We need this "Disconnect() & return" when a callback is called after Oilpan > is > > > detached from the main thread and before the message loop is destructed. Is > it > > > possible to get a mojo message in the meantime? > > > > If it's not possible, how can we hit the crash in the first place? The crash > is > > happening because the mojo callback runs after Oilpan is detached from the > main > > thread. (However, I must admit that I don't understand how it's possible.) > > The crash was caused by the channel error notification triggered by the message > loop destruction. I don't know other messages can arrive between the Oilpan > detach and the message loop destruction. Makes sense. LGTM.
The CQ bit was checked by yhirano@chromium.org
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 ========== [WebSocket] Ignore connection errors in the shutdown sequence WebSocketHandleImpl::OnConnectionError is called in the shutdown sequence and at that time |*client_| is invalid. In the shutdown sequence it is not guaranteed that the client destructs the WebSocketHandleImpl correctly and hence we need to ignore the error. This is a workaround. BUG=637949 ========== to ========== [WebSocket] Ignore connection errors in the shutdown sequence WebSocketHandleImpl::OnConnectionError is called in the shutdown sequence and at that time |*client_| is invalid. In the shutdown sequence it is not guaranteed that the client destructs the WebSocketHandleImpl correctly and hence we need to ignore the error. This is a workaround. BUG=637949 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [WebSocket] Ignore connection errors in the shutdown sequence WebSocketHandleImpl::OnConnectionError is called in the shutdown sequence and at that time |*client_| is invalid. In the shutdown sequence it is not guaranteed that the client destructs the WebSocketHandleImpl correctly and hence we need to ignore the error. This is a workaround. BUG=637949 ========== to ========== [WebSocket] Ignore connection errors in the shutdown sequence WebSocketHandleImpl::OnConnectionError is called in the shutdown sequence and at that time |*client_| is invalid. In the shutdown sequence it is not guaranteed that the client destructs the WebSocketHandleImpl correctly and hence we need to ignore the error. This is a workaround. BUG=637949 Committed: https://crrev.com/fce0b57ea91c0bbb263601906caf389ecf41c277 Cr-Commit-Position: refs/heads/master@{#413092} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/fce0b57ea91c0bbb263601906caf389ecf41c277 Cr-Commit-Position: refs/heads/master@{#413092}
Message was sent while issue was closed.
darin@chromium.org changed reviewers: + darin@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2258003003/diff/60001/content/renderer/websoc... File content/renderer/websockethandle_impl.cc (right): https://codereview.chromium.org/2258003003/diff/60001/content/renderer/websoc... content/renderer/websockethandle_impl.cc:162: if (!blink::Platform::current()) { This seems like a sensible band-aid. |
