Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2271133002:
WebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete (Closed)
|
Created:
4 years, 4 months ago by nhiroki Modified:
4 years, 4 months ago CC:
chromium-reviews, blink-reviews, tyoshino+watch_chromium.org, yhirano+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete
Bridge::disconnect() on a worker thread waited until Peer::disconnect() is
complete on the main thread to make sure that Peer never dereferences a pointer
to Bridge after disconnection. However, in the current implementation, the
pointer is CrossThreadWeakPersistent and its existence is appropriately checked
before access[1], so Bridge::disconnect() no longer have to wait until it's
nullified on Peer::disconnect().
[1] https://codereview.chromium.org/2123703002
BUG=639153, 640088
Committed: https://crrev.com/70a476ad4428cd7ca9bfa0fc69fda3deff7268fb
Cr-Commit-Position: refs/heads/master@{#414040}
Patch Set 1 #
Total comments: 2
Patch Set 2 : address review comments #Patch Set 3 : fix #
Messages
Total messages: 19 (12 generated)
The CQ bit was checked by nhiroki@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 ========== WebSocket: Remove an inter-thread synchronous function call BUG=639153, 640088 ========== to ========== WebSocket: Remove an inter-thread synchronous function call for cleanup This CL stops Bridge::disconnect() from waiting until Peer::disconnect() is complete. This was necessary to makes sure that Peer never dereferences Bridge after disconnection, but recent refactorings made it no longer necessary. BUG=639153, 640088 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== WebSocket: Remove an inter-thread synchronous function call for cleanup This CL stops Bridge::disconnect() from waiting until Peer::disconnect() is complete. This was necessary to makes sure that Peer never dereferences Bridge after disconnection, but recent refactorings made it no longer necessary. BUG=639153, 640088 ========== to ========== WebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete This was necessary to makes sure that Peer never dereferences Bridge after disconnection, but recent refactorings made it no longer necessary. BUG=639153, 640088 ==========
Description was changed from ========== WebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete This was necessary to makes sure that Peer never dereferences Bridge after disconnection, but recent refactorings made it no longer necessary. BUG=639153, 640088 ========== to ========== WebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete Bridge::disconnect() on a worker thread waited until Peer::disconnect() is complete on the main thread to make sure that Peer never dereferences a pointer to Bridge after disconnection. However, in the current implementation, the pointer is CrossThreadWeakPersistent and its existence is appropriately checked before access[1], so Bridge::disconnect() no longer have to wait until the pointer is nullified on Peer::disconnect(). [1] https://codereview.chromium.org/2123703002 BUG=639153, 640088 ==========
Description was changed from ========== WebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete Bridge::disconnect() on a worker thread waited until Peer::disconnect() is complete on the main thread to make sure that Peer never dereferences a pointer to Bridge after disconnection. However, in the current implementation, the pointer is CrossThreadWeakPersistent and its existence is appropriately checked before access[1], so Bridge::disconnect() no longer have to wait until the pointer is nullified on Peer::disconnect(). [1] https://codereview.chromium.org/2123703002 BUG=639153, 640088 ========== to ========== WebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete Bridge::disconnect() on a worker thread waited until Peer::disconnect() is complete on the main thread to make sure that Peer never dereferences a pointer to Bridge after disconnection. However, in the current implementation, the pointer is CrossThreadWeakPersistent and its existence is appropriately checked before access[1], so Bridge::disconnect() no longer have to wait until it's nullified on Peer::disconnect(). [1] https://codereview.chromium.org/2123703002 BUG=639153, 640088 ==========
nhiroki@chromium.org changed reviewers: + haraken@chromium.org, yhirano@chromium.org
Hi, can you review this? After this CL, other my CLs[1,2] are no longer necessary. Sorry for wasting your time... [1] https://codereview.chromium.org/2263173002/ [2] https://codereview.chromium.org/2273483003/
lgtm, thank you. https://codereview.chromium.org/2271133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp (right): https://codereview.chromium.org/2271133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:443: m_loaderProxy->postTaskToLoader(BLINK_FROM_HERE, createCrossThreadTask(&Peer::disconnect, wrapCrossThreadPersistent(m_peer.get()))); |m_peer| is a CrossThreadPersistent, so wrapping is not needed.
This looks much simpler. LGTM.
Thank you! https://codereview.chromium.org/2271133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp (right): https://codereview.chromium.org/2271133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:443: m_loaderProxy->postTaskToLoader(BLINK_FROM_HERE, createCrossThreadTask(&Peer::disconnect, wrapCrossThreadPersistent(m_peer.get()))); On 2016/08/24 08:01:42, yhirano wrote: > |m_peer| is a CrossThreadPersistent, so wrapping is not needed. Done.
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2271133002/#ps60001 (title: "fix")
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: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete Bridge::disconnect() on a worker thread waited until Peer::disconnect() is complete on the main thread to make sure that Peer never dereferences a pointer to Bridge after disconnection. However, in the current implementation, the pointer is CrossThreadWeakPersistent and its existence is appropriately checked before access[1], so Bridge::disconnect() no longer have to wait until it's nullified on Peer::disconnect(). [1] https://codereview.chromium.org/2123703002 BUG=639153, 640088 ========== to ========== WebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete Bridge::disconnect() on a worker thread waited until Peer::disconnect() is complete on the main thread to make sure that Peer never dereferences a pointer to Bridge after disconnection. However, in the current implementation, the pointer is CrossThreadWeakPersistent and its existence is appropriately checked before access[1], so Bridge::disconnect() no longer have to wait until it's nullified on Peer::disconnect(). [1] https://codereview.chromium.org/2123703002 BUG=639153, 640088 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== WebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete Bridge::disconnect() on a worker thread waited until Peer::disconnect() is complete on the main thread to make sure that Peer never dereferences a pointer to Bridge after disconnection. However, in the current implementation, the pointer is CrossThreadWeakPersistent and its existence is appropriately checked before access[1], so Bridge::disconnect() no longer have to wait until it's nullified on Peer::disconnect(). [1] https://codereview.chromium.org/2123703002 BUG=639153, 640088 ========== to ========== WebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete Bridge::disconnect() on a worker thread waited until Peer::disconnect() is complete on the main thread to make sure that Peer never dereferences a pointer to Bridge after disconnection. However, in the current implementation, the pointer is CrossThreadWeakPersistent and its existence is appropriately checked before access[1], so Bridge::disconnect() no longer have to wait until it's nullified on Peer::disconnect(). [1] https://codereview.chromium.org/2123703002 BUG=639153, 640088 Committed: https://crrev.com/70a476ad4428cd7ca9bfa0fc69fda3deff7268fb Cr-Commit-Position: refs/heads/master@{#414040} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/70a476ad4428cd7ca9bfa0fc69fda3deff7268fb Cr-Commit-Position: refs/heads/master@{#414040} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
