Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(23)

Issue 2815893002: Break circular dependency between InitializeDeviceDisablingManager and DeviceDisabledScreen (Closed)

Created:
1 year, 8 months ago by Ivan Šandrk
Modified:
1 year, 8 months ago
Reviewers:
Lei Zhang, achuithb, emaxx
CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Break circular dependency between InitializeDeviceDisablingManager and DeviceDisabledScreen crrev.com/2714493002 uncovered a circular dependency between BrowserProcessPlatformPart::InitializeDeviceDisablingManager and DeviceDisabledScreen::DeviceDisabledScreen. Previously there was a part in the call stack that forked off into an asynchronous call, DeviceDisablingManager::UpdateFromCrosSettings, which would queue up a callback in case the CrosSettings wasn't ready yet (and it wasn't). The referenced CL fixed a bug by immediately loading the settings, but it unfortunately broke the assumption here. The suggested fix breaks the dependency by moving Init() outside of the constructor (so the object exists at the time further code called from Init() tries to access it). This bug was causing a crash for disabled devices (ui wouldn't start). The circle starts on the "device_disabling_manager_.reset" line. TEST=manual BUG=709518 Review-Url: https://codereview.chromium.org/2815893002 Cr-Commit-Position: refs/heads/master@{#464708} Committed: https://chromium.googlesource.com/chromium/src/+/87c761f625a57653cc241f5691cb013ad9ca3efa

Patch Set 1 #

Patch Set 2 : Added a check to the crash site #

Total comments: 10

Patch Set 3 : Using ThreadTaskRunnerHandle, comment about deferring, nits #

Patch Set 4 : Different approach, moved Init() outside constructor #

Total comments: 12

Patch Set 5 : Nits #

Patch Set 6 : Removed unnecessary DCHECKs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -6 lines) Patch
M chrome/browser/browser_process_platform_part_chromeos.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chromeos/login/screens/device_disabled_screen.cc View 1 2 3 4 5 1 chunk +1 line, -3 lines 0 comments Download
M chrome/browser/chromeos/system/device_disabling_manager.h View 1 2 3 4 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/system/device_disabling_manager.cc View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/chromeos/system/device_disabling_manager_unittest.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 50 (31 generated)
Ivan Šandrk
Hey Achuith, Maksim, ptal! Maksim helped me uncover and fix this, and he's somewhat familiar ...
1 year, 8 months ago (2017-04-12 16:25:24 UTC) #6
Ivan Šandrk
On 2017/04/12 16:25:24, Ivan Šandrk wrote: > Hey Achuith, Maksim, ptal! > > Maksim helped ...
1 year, 8 months ago (2017-04-12 16:35:58 UTC) #10
emaxx
I'm fine with the overall approach, but we should try to provide a test coverage ...
1 year, 8 months ago (2017-04-12 17:05:49 UTC) #13
achuithb
+1 to all of Max's suggestions. https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos/login/screens/device_disabled_screen.cc File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos/login/screens/device_disabled_screen.cc#newcode27 chrome/browser/chromeos/login/screens/device_disabled_screen.cc:27: CHECK(device_disabling_manager_); On 2017/04/12 ...
1 year, 8 months ago (2017-04-12 19:45:14 UTC) #14
Ivan Šandrk
Hey guys, I've actually went with a second approach since some tests were crashing. This ...
1 year, 8 months ago (2017-04-13 13:38:00 UTC) #22
emaxx
https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos/login/screens/device_disabled_screen.cc File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos/login/screens/device_disabled_screen.cc#newcode25 chrome/browser/chromeos/login/screens/device_disabled_screen.cc:25: if (view_) BTW, while you are here: this condition ...
1 year, 8 months ago (2017-04-13 13:57:31 UTC) #23
emaxx
On 2017/04/13 13:38:00, Ivan Šandrk wrote: > Hey guys, I've actually went with a second ...
1 year, 8 months ago (2017-04-13 13:58:08 UTC) #26
Ivan Šandrk
Hey Max, no changes yet, just clarifying some things :) https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos/login/screens/device_disabled_screen.cc File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos/login/screens/device_disabled_screen.cc#newcode27 ...
1 year, 8 months ago (2017-04-13 14:22:25 UTC) #27
emaxx
https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos/system/device_disabling_manager.h File chrome/browser/chromeos/system/device_disabling_manager.h (right): https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos/system/device_disabling_manager.h#newcode91 chrome/browser/chromeos/system/device_disabling_manager.h:91: void Init(); On 2017/04/13 14:22:25, Ivan Šandrk wrote: > ...
1 year, 8 months ago (2017-04-13 14:30:01 UTC) #28
Ivan Šandrk
https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos/login/screens/device_disabled_screen.cc File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos/login/screens/device_disabled_screen.cc#newcode25 chrome/browser/chromeos/login/screens/device_disabled_screen.cc:25: if (view_) On 2017/04/13 13:57:31, emaxx wrote: > BTW, ...
1 year, 8 months ago (2017-04-13 14:44:09 UTC) #31
Ivan Šandrk
Maksim, Achuith, are you fine with me landing this without the regression test, and then ...
1 year, 8 months ago (2017-04-13 15:31:09 UTC) #34
emaxx
LGTM. On 2017/04/13 15:31:09, Ivan Šandrk wrote: > Maksim, Achuith, are you fine with me ...
1 year, 8 months ago (2017-04-13 15:56:46 UTC) #35
emaxx
+thestig@: Lei, please review the change in file chrome/browser/browser_process_platform_part_chromeos.cc > P.S. Can you add someone ...
1 year, 8 months ago (2017-04-13 15:57:01 UTC) #37
achuithb
https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos/login/screens/device_disabled_screen.cc File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos/login/screens/device_disabled_screen.cc#newcode27 chrome/browser/chromeos/login/screens/device_disabled_screen.cc:27: CHECK(device_disabling_manager_); On 2017/04/13 13:37:59, Ivan Šandrk wrote: > On ...
1 year, 8 months ago (2017-04-13 18:40:03 UTC) #38
achuithb
lgtm
1 year, 8 months ago (2017-04-13 18:42:56 UTC) #39
Lei Zhang
I don't know this code well, but based on the consistent Init() usage and others' ...
1 year, 8 months ago (2017-04-13 18:46:11 UTC) #40
Lei Zhang
On 2017/04/13 18:40:03, achuithb wrote: > It's fine if you want to leave in the ...
1 year, 8 months ago (2017-04-13 18:49:42 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2815893002/100001
1 year, 8 months ago (2017-04-14 08:59:53 UTC) #47
commit-bot: I haz the power
1 year, 8 months ago (2017-04-14 09:27:03 UTC) #50
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/87c761f625a57653cc241f5691cb...

Powered by Google App Engine
This is Rietveld 408576698