Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2188503003:
Prevent DCHECK on shutdown of EmbeddedApplicationRunner (Closed)
|
Created:
4 years, 4 months ago by kmackay Modified:
4 years, 4 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent DCHECK on shutdown of EmbeddedApplicationRunner
When EmbeddedApplicationRunner is destroyed, if the application
instance still has any bound connections, those connections must
be destroyed on the application task runner. Otherwise there is
a DCHECK failure when the binding is destroyed.
BUG=
Committed: https://crrev.com/901b0992a386ab88d88ae27665102719c833c7a9
Cr-Commit-Position: refs/heads/master@{#408246}
Patch Set 1 #
Total comments: 6
Patch Set 2 : don't post whan already on the correct thread #Messages
Total messages: 18 (6 generated)
kmackay@chromium.org changed reviewers: + alokp@chromium.org, rockot@chromium.org
https://codereview.chromium.org/2188503003/diff/1/content/common/mojo/embedde... File content/common/mojo/embedded_application_runner.cc (right): https://codereview.chromium.org/2188503003/diff/1/content/common/mojo/embedde... content/common/mojo/embedded_application_runner.cc:55: // Any extant ServiceContexts must be destroyed on the correct thread. If application_task_runner_ is null, the app has already been torn down and there's nothing to do. So I would change this to if (!application_task_runner_) return; if (application_task_runner_->BelongsToCurrentThread()) Quit(); else // the PostTask you're adding here https://codereview.chromium.org/2188503003/diff/1/content/common/mojo/embedde... content/common/mojo/embedded_application_runner.cc:60: if (thread_) { And you can just delete this block now https://codereview.chromium.org/2188503003/diff/1/content/common/mojo/embedde... content/common/mojo/embedded_application_runner.cc:110: quit_task_runner_->PostTask( Since you're here, mind also changing this to invoke QuitOnRunnerThread synchronously when quit_task_runner_->BelongsToCurrentThread() is true?
https://codereview.chromium.org/2188503003/diff/1/content/common/mojo/embedde... File content/common/mojo/embedded_application_runner.cc (right): https://codereview.chromium.org/2188503003/diff/1/content/common/mojo/embedde... content/common/mojo/embedded_application_runner.cc:55: // Any extant ServiceContexts must be destroyed on the correct thread. On 2016/07/27 18:23:05, Ken Rockot wrote: > If application_task_runner_ is null, the app has already been torn down and > there's nothing to do. > So I would change this to > > > if (!application_task_runner_) > return; > if (application_task_runner_->BelongsToCurrentThread()) > Quit(); > else > // the PostTask you're adding here Done. https://codereview.chromium.org/2188503003/diff/1/content/common/mojo/embedde... content/common/mojo/embedded_application_runner.cc:60: if (thread_) { On 2016/07/27 18:23:05, Ken Rockot wrote: > And you can just delete this block now I wasn't sure if we wanted to make sure that the application thread was dead before completing the destruction of EmbeddedApplicationRunner. I guess it must be safe though since it needs to work when use_own_thread_ is false. https://codereview.chromium.org/2188503003/diff/1/content/common/mojo/embedde... content/common/mojo/embedded_application_runner.cc:110: quit_task_runner_->PostTask( On 2016/07/27 18:23:05, Ken Rockot wrote: > Since you're here, mind also changing this to invoke QuitOnRunnerThread > synchronously when quit_task_runner_->BelongsToCurrentThread() is true? Done.
lgtm
The CQ bit was checked by kmackay@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 commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Oh sorry, I'm not a content OWNER - jam@ or ben@ are owners familiar with mojo + content
kmackay@chromium.org changed reviewers: + ben@chromium.org, jam@chromium.org
lgtm
The CQ bit was checked by kmackay@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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Prevent DCHECK on shutdown of EmbeddedApplicationRunner When EmbeddedApplicationRunner is destroyed, if the application instance still has any bound connections, those connections must be destroyed on the application task runner. Otherwise there is a DCHECK failure when the binding is destroyed. BUG= ========== to ========== Prevent DCHECK on shutdown of EmbeddedApplicationRunner When EmbeddedApplicationRunner is destroyed, if the application instance still has any bound connections, those connections must be destroyed on the application task runner. Otherwise there is a DCHECK failure when the binding is destroyed. BUG= Committed: https://crrev.com/901b0992a386ab88d88ae27665102719c833c7a9 Cr-Commit-Position: refs/heads/master@{#408246} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/901b0992a386ab88d88ae27665102719c833c7a9 Cr-Commit-Position: refs/heads/master@{#408246} |
