Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2928603002:
Create and remove |UiDevToolsServer::server_| in the same thread. (Closed)
DescriptionCreate and remove |UiDevToolsServer::server_| in the same thread.
|UiDevToolsServer::server_| is created in an IO thread and gets
cleaned up in a UI thread. This causes the crash when shutting down
DevTools.
BUG=698276
Review-Url: https://codereview.chromium.org/2928603002
Cr-Commit-Position: refs/heads/master@{#483016}
Committed: https://chromium.googlesource.com/chromium/src/+/a73ef52cdb9f89e2fbb4c2076dfc788bb502fb2d
Patch Set 1 : . #
Total comments: 4
Patch Set 2 : . #
Total comments: 2
Patch Set 3 : . #
Depends on Patchset: Messages
Total messages: 71 (63 generated)
Description was changed from ========== Remove |UiDevToolsServer::server_| in the same thread as the created thread. WIP: |UiDevToolsServer::server_| is created in an IO thread and gets cleaned up in a UI thread. This causes the crash when shutting down DevTools. BUG=698276 ========== to ========== Create and remove |UiDevToolsServer::server_| in the same thread. WIP: |UiDevToolsServer::server_| is created in an IO thread and gets cleaned up in a UI thread. This causes the crash when shutting down DevTools. BUG=698276 ==========
The CQ bit was checked by thanhph@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
The CQ bit was checked by thanhph@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by thanhph@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by thanhph@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 #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thanhph@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by thanhph@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by thanhph@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 #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Description was changed from ========== Create and remove |UiDevToolsServer::server_| in the same thread. WIP: |UiDevToolsServer::server_| is created in an IO thread and gets cleaned up in a UI thread. This causes the crash when shutting down DevTools. BUG=698276 ========== to ========== Create and remove |UiDevToolsServer::server_| in the same thread. |UiDevToolsServer::server_| is created in an IO thread and gets cleaned up in a UI thread. This causes the crash when shutting down DevTools. BUG=698276 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanhph@chromium.org changed reviewers: + sadrul@chromium.org
Hi Sadrul, Please review my new patch. Thanks, Thanh.
Patchset #1 (id:240001) has been deleted
The CQ bit was checked by thanhph@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2928603002/diff/260001/components/ui_devtools... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2928603002/diff/260001/components/ui_devtools... components/ui_devtools/devtools_server.cc:72: if (thread_->IsRunning()) This will not work when |io_thread_task_runner_| has been injected from outside during creation. For that case, you would either need to use a WaitableEvent (see examples of that in various places), or do something like this: io_thread_task_runner_->DeleteSoon(FROM_HERE, server_.release());
The CQ bit was checked by thanhph@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:280001) has been deleted
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by thanhph@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...
Thanks Sadrul. I have the new patch with DeleteSoon and have a question below as well. Thanh. https://codereview.chromium.org/2928603002/diff/260001/components/ui_devtools... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2928603002/diff/260001/components/ui_devtools... components/ui_devtools/devtools_server.cc:72: if (thread_->IsRunning()) On 2017/06/26 22:43:30, sadrul wrote: > This will not work when |io_thread_task_runner_| has been injected from outside > during creation. For that case, you would either need to use a WaitableEvent > (see examples of that in various places), or do something like this: > > io_thread_task_runner_->DeleteSoon(FROM_HERE, server_.release()); To elaborate more, with current patch 1, does this mean we are afraid |io_thread_task_runner_| gets cleaned up or gets blocked before |io_thread_task_runner_| is used here? PostTask() uses PostDelayedTask() which has default timedelta = 0, thus invoking StopSever() in future time with no delay (but I guess still could get blocked with current running tasks) and DeleteSoon() uses PostNonNestableTask() which provides this task will not run nested within an already-running task.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2928603002/diff/260001/components/ui_devtools... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2928603002/diff/260001/components/ui_devtools... components/ui_devtools/devtools_server.cc:72: if (thread_->IsRunning()) On 2017/06/27 15:02:29, thanhph wrote: > On 2017/06/26 22:43:30, sadrul wrote: > > This will not work when |io_thread_task_runner_| has been injected from > outside > > during creation. For that case, you would either need to use a WaitableEvent > > (see examples of that in various places), or do something like this: > > > > io_thread_task_runner_->DeleteSoon(FROM_HERE, server_.release()); > > To elaborate more, with current patch 1, does this mean we are afraid > |io_thread_task_runner_| gets cleaned up or gets blocked before > |io_thread_task_runner_| is used here? PostTask() uses PostDelayedTask() which > has default timedelta = 0, thus invoking StopSever() in future time with no > delay (but I guess still could get blocked with current running tasks) and > DeleteSoon() uses PostNonNestableTask() which provides this task will not run > nested within an already-running task. No. The problem with this change was, that if UiDevToolsServer() received a custom io_thread_task_runner during creation, then it does not create |thread_|. Which means this UiDevToolsServer object can still get destroyed before the task you have just posted runs. This means (1) |server_| gets immediately destroyed, still in the wrong thread, and on top of that (2) the posted task does run on the io thread, but |this| has already been destroyed, leading to a use-after-free. The most important part of the approach with DeleteSoon() is that you give up ownership of |server_|. Which means |this| can get destroyed, without destroying the server. And the server will be correctly destroyed on the IO thread. If the IO thread has already been torn down, that means |server_| will just never be destroyed, and will leak instead. But I don't think there's a clean way of fixing that. https://codereview.chromium.org/2928603002/diff/300001/components/ui_devtools... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2928603002/diff/300001/components/ui_devtools... components/ui_devtools/devtools_server.cc:70: if (thread_->IsRunning()) You should check if |thread|_ is non-null first.
The CQ bit was checked by thanhph@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...
Thanks for more details Sadrul. I added a new patch. Thanh. https://codereview.chromium.org/2928603002/diff/260001/components/ui_devtools... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2928603002/diff/260001/components/ui_devtools... components/ui_devtools/devtools_server.cc:72: if (thread_->IsRunning()) On 2017/06/28 04:49:10, sadrul wrote: > On 2017/06/27 15:02:29, thanhph wrote: > > On 2017/06/26 22:43:30, sadrul wrote: > > > This will not work when |io_thread_task_runner_| has been injected from > > outside > > > during creation. For that case, you would either need to use a WaitableEvent > > > (see examples of that in various places), or do something like this: > > > > > > io_thread_task_runner_->DeleteSoon(FROM_HERE, server_.release()); > > > > To elaborate more, with current patch 1, does this mean we are afraid > > |io_thread_task_runner_| gets cleaned up or gets blocked before > > |io_thread_task_runner_| is used here? PostTask() uses PostDelayedTask() which > > has default timedelta = 0, thus invoking StopSever() in future time with no > > delay (but I guess still could get blocked with current running tasks) and > > DeleteSoon() uses PostNonNestableTask() which provides this task will not run > > nested within an already-running task. > > No. The problem with this change was, that if UiDevToolsServer() received a > custom io_thread_task_runner during creation, then it does not create |thread_|. > Which means this UiDevToolsServer object can still get destroyed before the task > you have just posted runs. This means (1) |server_| gets immediately destroyed, > still in the wrong thread, and on top of that (2) the posted task does run on > the io thread, but |this| has already been destroyed, leading to a > use-after-free. > > The most important part of the approach with DeleteSoon() is that you give up > ownership of |server_|. Which means |this| can get destroyed, without destroying > the server. And the server will be correctly destroyed on the IO thread. > > If the IO thread has already been torn down, that means |server_| will just > never be destroyed, and will leak instead. But I don't think there's a clean way > of fixing that. Acknowledged! Thanks for the explanation. https://codereview.chromium.org/2928603002/diff/300001/components/ui_devtools... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2928603002/diff/300001/components/ui_devtools... components/ui_devtools/devtools_server.cc:70: if (thread_->IsRunning()) On 2017/06/28 04:49:10, sadrul wrote: > You should check if |thread|_ is non-null first. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by thanhph@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": 320001, "attempt_start_ts": 1498668141442400,
"parent_rev": "d74f1fcf603fd78c3cd9e0a3123db98e4a1767e3", "commit_rev":
"a73ef52cdb9f89e2fbb4c2076dfc788bb502fb2d"}
Message was sent while issue was closed.
Description was changed from ========== Create and remove |UiDevToolsServer::server_| in the same thread. |UiDevToolsServer::server_| is created in an IO thread and gets cleaned up in a UI thread. This causes the crash when shutting down DevTools. BUG=698276 ========== to ========== Create and remove |UiDevToolsServer::server_| in the same thread. |UiDevToolsServer::server_| is created in an IO thread and gets cleaned up in a UI thread. This causes the crash when shutting down DevTools. BUG=698276 Review-Url: https://codereview.chromium.org/2928603002 Cr-Commit-Position: refs/heads/master@{#483016} Committed: https://chromium.googlesource.com/chromium/src/+/a73ef52cdb9f89e2fbb4c2076dfc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:320001) as https://chromium.googlesource.com/chromium/src/+/a73ef52cdb9f89e2fbb4c2076dfc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
