Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2547163002:
Add thread checker calls to all ArcServiceManager methods (Closed)
|
Created:
4 years ago by Yusuke Sato Modified:
4 years ago Reviewers:
Luis Héctor Chávez CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd thread checker calls to all ArcServiceManager methods
This is a follow-up CL for
https://codereview.chromium.org/2487623002/.
BUG=None
TEST=try
Committed: https://crrev.com/e6b038d7b615c9ee3f65620e9991c0c4ce970f1c
Cr-Commit-Position: refs/heads/master@{#436083}
Patch Set 1 #
Total comments: 5
Patch Set 2 : address comment #Messages
Total messages: 15 (9 generated)
The CQ bit was checked by yusukes@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.
yusukes@chromium.org changed reviewers: + lhchavez@chromium.org
PTAL https://codereview.chromium.org/2547163002/diff/1/components/arc/arc_service_... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2547163002/diff/1/components/arc/arc_service_... components/arc/arc_service_manager.cc:60: bool ArcServiceManager::IsInitialized() { FYI, this method was introduced for https://codereview.chromium.org/2487623002/diff/480001/chrome/browser/chromeo... I was not particularly happy with the introduction, but didn't have a better idea. The EventRouter::Shutdown() function must call arc::ArcServiceManager::Get()->RemoveObserver() when the event router is owned by a non-primary Chrome profile, but it must NOT call the remove function when the profile is primary because for primary profile destruction (i.e. shutdown) ArcServiceManager is destructed first. If you call remove() in that case, Chrome crashes either with DCHECK or null pointer deref. If you have a better idea, I'll create another follow-up CL. https://codereview.chromium.org/2547163002/diff/1/components/arc/arc_service_... components/arc/arc_service_manager.cc:61: DCHECK(!g_arc_service_manager || This DCHECK does not detect wrong threading when g_arc_service_manager is null, but I guess having this DCHECK is still better than nothing.
lgtm https://codereview.chromium.org/2547163002/diff/1/components/arc/arc_service_... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2547163002/diff/1/components/arc/arc_service_... components/arc/arc_service_manager.cc:60: bool ArcServiceManager::IsInitialized() { On 2016/12/02 21:06:54, Yusuke Sato wrote: > FYI, this method was introduced for > https://codereview.chromium.org/2487623002/diff/480001/chrome/browser/chromeo... > > I was not particularly happy with the introduction, but didn't have a better > idea. The EventRouter::Shutdown() function must call > arc::ArcServiceManager::Get()->RemoveObserver() when the event router is owned > by a non-primary Chrome profile, but it must NOT call the remove function when > the profile is primary because for primary profile destruction (i.e. shutdown) > ArcServiceManager is destructed first. If you call remove() in that case, Chrome > crashes either with DCHECK or null pointer deref. > > If you have a better idea, I'll create another follow-up CL. Not at the moment :( https://codereview.chromium.org/2547163002/diff/1/components/arc/arc_service_... components/arc/arc_service_manager.cc:61: DCHECK(!g_arc_service_manager || On 2016/12/02 21:06:54, Yusuke Sato wrote: > This DCHECK does not detect wrong threading when g_arc_service_manager is null, > but I guess having this DCHECK is still better than nothing. This is fine. We have a similar problem in ArcBridgeService: https://cs.chromium.org/chromium/src/components/arc/arc_bridge_service.cc?q=A... you can follow the same pattern if you want since we expect this to be called when |g_arc_service_manager| is null.
https://codereview.chromium.org/2547163002/diff/1/components/arc/arc_service_... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2547163002/diff/1/components/arc/arc_service_... components/arc/arc_service_manager.cc:61: DCHECK(!g_arc_service_manager || On 2016/12/02 22:18:08, Luis Héctor Chávez wrote: > On 2016/12/02 21:06:54, Yusuke Sato wrote: > > This DCHECK does not detect wrong threading when g_arc_service_manager is > null, > > but I guess having this DCHECK is still better than nothing. > > This is fine. We have a similar problem in ArcBridgeService: > https://cs.chromium.org/chromium/src/components/arc/arc_bridge_service.cc?q=A... > > you can follow the same pattern if you want since we expect this to be called > when |g_arc_service_manager| is null. Thanks, done.
The CQ bit was checked by yusukes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2547163002/#ps20001 (title: "address comment")
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": 20001, "attempt_start_ts": 1480717671127140,
"parent_rev": "fb3df34754ffed783e71a1443fd0ce822b4d077d", "commit_rev":
"c5833bc97f63ffe3de0de566562a93d57646b812"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add thread checker calls to all ArcServiceManager methods This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ========== to ========== Add thread checker calls to all ArcServiceManager methods This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try Committed: https://crrev.com/e6b038d7b615c9ee3f65620e9991c0c4ce970f1c Cr-Commit-Position: refs/heads/master@{#436083} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e6b038d7b615c9ee3f65620e9991c0c4ce970f1c Cr-Commit-Position: refs/heads/master@{#436083} |
