Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686803002:
Make nested classes of DevToolsDeviceDiscovery RefCountedThreadSafe (Closed)
DescriptionMake nested classes of DevToolsDeviceDiscovery RefCountedThreadSafe
DevToolsDeviceDiscovery binds a RemoteBrowser into a callback, and passes
it to AndroidDeviceManager::Device to fetch the device info, where
RemoteBrowser has non-thread-safe ref count. AndroidDeviceManager::Device
do some work on another thread and call the callback on the original thread.
However, there may be a reference to the callback left on the target thread,
and if the last reference to the callback is released on the target thread,
a reference to the bound RemoteBrowser instance is also released on the
target thread. That causes a data race on the ref count of RemoteBrowser.
This CL makes RemoteBrowser and related classes be RefCountedThreadSafe, so
that the ref count manipulation doesn't cause the race.
Review-Url: https://codereview.chromium.org/2686803002
Cr-Commit-Position: refs/heads/master@{#449184}
Committed: https://chromium.googlesource.com/chromium/src/+/3fbaee1204d803fba5c0f28e7502f57879546c62
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : s/RefCounted/RefCountedThreadSafe/ #Messages
Total messages: 32 (23 generated)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Description was changed from ========== Pass callbacks around AndroidDeviceManager by value to avoid destruction race BUG= ========== to ========== Pass callbacks around AndroidDeviceManager by value to avoid a destruction race BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Pass callbacks around AndroidDeviceManager by value to avoid a destruction race BUG= ========== to ========== Pass callbacks around AndroidDeviceManager by value to avoid a destruction race DevToolsDeviceDiscovery binds a RemoteBrowser into a Callback, and passes it to AndroidDeviceManager::Device to fetch the device info, where RemoteBrowser has non-thread-safe ref count. AndroidDeviceManager::Device do some job on another thread and call the callback on the original thread. However, there may be a reference to the callback left on the target thread, and if the last reference to the callback is released on the target thread, a reference to the bound RemoteBrowser instance is also released on the target thread. That causes a data race on the ref count of RemoteBrowser. This CL is a workaround for race by pass all related callback by value and move them to consumers, so that we don't leave the reference to the callback on the target thread. ==========
Description was changed from ========== Pass callbacks around AndroidDeviceManager by value to avoid a destruction race DevToolsDeviceDiscovery binds a RemoteBrowser into a Callback, and passes it to AndroidDeviceManager::Device to fetch the device info, where RemoteBrowser has non-thread-safe ref count. AndroidDeviceManager::Device do some job on another thread and call the callback on the original thread. However, there may be a reference to the callback left on the target thread, and if the last reference to the callback is released on the target thread, a reference to the bound RemoteBrowser instance is also released on the target thread. That causes a data race on the ref count of RemoteBrowser. This CL is a workaround for race by pass all related callback by value and move them to consumers, so that we don't leave the reference to the callback on the target thread. ========== to ========== Pass callbacks around AndroidDeviceManager by value to avoid a destruction race DevToolsDeviceDiscovery binds a RemoteBrowser into a callback, and passes it to AndroidDeviceManager::Device to fetch the device info, where RemoteBrowser has non-thread-safe ref count. AndroidDeviceManager::Device do some work on another thread and call the callback on the original thread. However, there may be a reference to the callback left on the target thread, and if the last reference to the callback is released on the target thread, a reference to the bound RemoteBrowser instance is also released on the target thread. That causes a data race on the ref count of RemoteBrowser. This CL is a workaround for race by pass all related callback by value and move them to consumers, so that we don't leave the reference to the callback on the target thread. ==========
tzik@chromium.org changed reviewers: + pfeldman@chromium.org
PTAL
Could you point to where semantics has changed? All I see is std::move instead of a copy, but there are no retaining references touched - everything seems to be on stack.
Ok, caseq@ explained it to me. It might be safer to make RemoteBrowser thread safe ref-counted - otherwise it is too easy to fall into this trap again.
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...
Description was changed from ========== Pass callbacks around AndroidDeviceManager by value to avoid a destruction race DevToolsDeviceDiscovery binds a RemoteBrowser into a callback, and passes it to AndroidDeviceManager::Device to fetch the device info, where RemoteBrowser has non-thread-safe ref count. AndroidDeviceManager::Device do some work on another thread and call the callback on the original thread. However, there may be a reference to the callback left on the target thread, and if the last reference to the callback is released on the target thread, a reference to the bound RemoteBrowser instance is also released on the target thread. That causes a data race on the ref count of RemoteBrowser. This CL is a workaround for race by pass all related callback by value and move them to consumers, so that we don't leave the reference to the callback on the target thread. ========== to ========== Pass callbacks around AndroidDeviceManager by value to avoid a destruction race DevToolsDeviceDiscovery binds a RemoteBrowser into a callback, and passes it to AndroidDeviceManager::Device to fetch the device info, where RemoteBrowser has non-thread-safe ref count. AndroidDeviceManager::Device do some work on another thread and call the callback on the original thread. However, there may be a reference to the callback left on the target thread, and if the last reference to the callback is released on the target thread, a reference to the bound RemoteBrowser instance is also released on the target thread. That causes a data race on the ref count of RemoteBrowser. This CL is a workaround for race by pass all related callback by value and move them to consumers, so that we don't leave the reference to the callback on the target thread. TODO: Update the CL description ==========
On 2017/02/08 18:07:55, pfeldman wrote: > Ok, caseq@ explained it to me. It might be safer to make RemoteBrowser thread > safe ref-counted - otherwise it is too easy to fall into this trap again. To leave that to the log: On the previous code, the Callback instances on the origin thread are left on the stack or on a class member, and its lifetime is unclear. Since the target thread can be scheduled either before or after the origin thread instance is clobbered, the Callback instance and bound arguments can be destroyed either the origin thread or the target thread. That causes the data race of the ref count. A tighten threading assertion on the ref count detected the race actually happened around here. https://codereview.chromium.org/2666423002/ On the proposed code as PS2, Callback instances are passed by value and moved to subsequent consumers of the instance without copy. So, as far as the producer of the Callback instance doesn't keep a reference to the Callback, no reference will be left on the origin thread.
Description was changed from ========== Pass callbacks around AndroidDeviceManager by value to avoid a destruction race DevToolsDeviceDiscovery binds a RemoteBrowser into a callback, and passes it to AndroidDeviceManager::Device to fetch the device info, where RemoteBrowser has non-thread-safe ref count. AndroidDeviceManager::Device do some work on another thread and call the callback on the original thread. However, there may be a reference to the callback left on the target thread, and if the last reference to the callback is released on the target thread, a reference to the bound RemoteBrowser instance is also released on the target thread. That causes a data race on the ref count of RemoteBrowser. This CL is a workaround for race by pass all related callback by value and move them to consumers, so that we don't leave the reference to the callback on the target thread. TODO: Update the CL description ========== to ========== Make nested classes of DevToolsDeviceDiscovery RefCountedThreadSafe DevToolsDeviceDiscovery binds a RemoteBrowser into a callback, and passes it to AndroidDeviceManager::Device to fetch the device info, where RemoteBrowser has non-thread-safe ref count. AndroidDeviceManager::Device do some work on another thread and call the callback on the original thread. However, there may be a reference to the callback left on the target thread, and if the last reference to the callback is released on the target thread, a reference to the bound RemoteBrowser instance is also released on the target thread. That causes a data race on the ref count of RemoteBrowser. This CL makes RemoteBrowser and related classes be RefCountedThreadSafe, so that the ref count manipulation doesn't cause the race. ==========
On 2017/02/08 18:07:55, pfeldman wrote: > Ok, caseq@ explained it to me. It might be safer to make RemoteBrowser thread > safe ref-counted - otherwise it is too easy to fall into this trap again. OK, let's go that way. Updated the CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks!
The CQ bit was checked by tzik@chromium.org
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 tzik@chromium.org
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486604589230570,
"parent_rev": "e530306fa63d398f6eb9e9a7bb33ac310fd7925b", "commit_rev":
"3fbaee1204d803fba5c0f28e7502f57879546c62"}
Message was sent while issue was closed.
Description was changed from ========== Make nested classes of DevToolsDeviceDiscovery RefCountedThreadSafe DevToolsDeviceDiscovery binds a RemoteBrowser into a callback, and passes it to AndroidDeviceManager::Device to fetch the device info, where RemoteBrowser has non-thread-safe ref count. AndroidDeviceManager::Device do some work on another thread and call the callback on the original thread. However, there may be a reference to the callback left on the target thread, and if the last reference to the callback is released on the target thread, a reference to the bound RemoteBrowser instance is also released on the target thread. That causes a data race on the ref count of RemoteBrowser. This CL makes RemoteBrowser and related classes be RefCountedThreadSafe, so that the ref count manipulation doesn't cause the race. ========== to ========== Make nested classes of DevToolsDeviceDiscovery RefCountedThreadSafe DevToolsDeviceDiscovery binds a RemoteBrowser into a callback, and passes it to AndroidDeviceManager::Device to fetch the device info, where RemoteBrowser has non-thread-safe ref count. AndroidDeviceManager::Device do some work on another thread and call the callback on the original thread. However, there may be a reference to the callback left on the target thread, and if the last reference to the callback is released on the target thread, a reference to the bound RemoteBrowser instance is also released on the target thread. That causes a data race on the ref count of RemoteBrowser. This CL makes RemoteBrowser and related classes be RefCountedThreadSafe, so that the ref count manipulation doesn't cause the race. Review-Url: https://codereview.chromium.org/2686803002 Cr-Commit-Position: refs/heads/master@{#449184} Committed: https://chromium.googlesource.com/chromium/src/+/3fbaee1204d803fba5c0f28e7502... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3fbaee1204d803fba5c0f28e7502... |
