|
|
Created:
3 years, 8 months ago by Ivan Šandrk Modified:
3 years, 8 months ago CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionBreak 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 #
Messages
Total messages: 50 (31 generated)
Description was changed from ========== 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 posting the UpdateFromCrosSettings function as a task. TEST=manual BUG=709518 ========== to ========== 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 posting the UpdateFromCrosSettings function as a task. TEST=manual BUG=709518 ==========
isandrk@chromium.org changed reviewers: + achuith@chromium.org, emaxx@chromium.org
Description was changed from ========== 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 posting the UpdateFromCrosSettings function as a task. TEST=manual BUG=709518 ========== to ========== 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 posting the UpdateFromCrosSettings function as a task. This bug was causing a crash for disabled devices (ui wouldn't start). TEST=manual BUG=709518 ==========
The CQ bit was checked by isandrk@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...
Hey Achuith, Maksim, ptal! Maksim helped me uncover and fix this, and he's somewhat familiar with this code so I'm also adding him as a reviewer. Ivan
The CQ bit was checked by isandrk@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 ========== 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 posting the UpdateFromCrosSettings function as a task. This bug was causing a crash for disabled devices (ui wouldn't start). TEST=manual BUG=709518 ========== to ========== 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 posting the UpdateFromCrosSettings function as a task. 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 ==========
On 2017/04/12 16:25:24, Ivan Šandrk wrote: > Hey Achuith, Maksim, ptal! > > Maksim helped me uncover and fix this, and he's somewhat familiar with this code > so I'm also adding him as a reviewer. > > > Ivan Sorry if the Description isn't the best, I'm somewhat tired and I wanted to push it out today.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
I'm fine with the overall approach, but we should try to provide a test coverage for this bug. This most likely would be a browser test - maybe you'll find a way to extend an existing browser test for this. We basically need something that crashed before this fix and that works successfully with this fix. https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/screens/device_disabled_screen.cc:27: CHECK(device_disabling_manager_); I think DCHECK is more suitable there. See the Style Guide: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/system/device_disabling_manager.cc (right): https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.cc:80: auto callback = nit: Storing the callback in a variable seems to be unnecessary. https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.cc:83: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback); I think the following way of posting tasks is preferable: base::ThreadTaskRunnerHandle::Get()->PostTask(...) Then you won't need to depend on the "content::" stuff. https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.cc:83: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback); nit: Please add a comment why the deferring is performed here instead of just calling the function.
+1 to all of Max's suggestions. https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/screens/device_disabled_screen.cc:27: CHECK(device_disabling_manager_); On 2017/04/12 17:05:49, emaxx wrote: > I think DCHECK is more suitable there. See the Style Guide: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Personally I don't see the value of DCHECKing on this line when the next line will crash anyway.
The CQ bit was checked by isandrk@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_ozone_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 isandrk@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 ========== 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 posting the UpdateFromCrosSettings function as a task. 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 ========== to ========== 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 ==========
Hey guys, I've actually went with a second approach since some tests were crashing. This one maybe seems better actually. I'm still thinking on how to test this. https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/screens/device_disabled_screen.cc:27: CHECK(device_disabling_manager_); On 2017/04/12 19:45:14, achuithb wrote: > On 2017/04/12 17:05:49, emaxx wrote: > > I think DCHECK is more suitable there. See the Style Guide: > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > Personally I don't see the value of DCHECKing on this line when the next line > will crash anyway. It wasn't really crashing on the next line, it was crashing deeper down the call stack. I got a bit confused by this at first, and it took me some time to figure it out (C++ doesn't crash on calling into nullptr object member functions if they're not virtual, it crashed only when it tried to touch some data). But if that's what you actually meant by your comment, I will remove it (I still don't have a gut feeling for (D)CHECKs). https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/system/device_disabling_manager.cc (right): https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.cc:80: auto callback = On 2017/04/12 17:05:49, emaxx wrote: > nit: Storing the callback in a variable seems to be unnecessary. Done. Should I maybe cleanup the code a bit? Use a temporary callback variable for this call and the previous two? https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.cc:83: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback); On 2017/04/12 17:05:49, emaxx wrote: > I think the following way of posting tasks is preferable: > base::ThreadTaskRunnerHandle::Get()->PostTask(...) > Then you won't need to depend on the "content::" stuff. Done. https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.cc:83: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback); On 2017/04/12 17:05:49, emaxx wrote: > nit: Please add a comment why the deferring is performed here instead of just > calling the function. Done.
https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/screens/device_disabled_screen.cc:25: if (view_) BTW, while you are here: this condition also contradicts our style guide for the DCHECK's. The next line should be done unconditionally (letting it to crash or whatever - we don't care assuming that the DCHECK is correct). https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/screens/device_disabled_screen.cc:27: CHECK(device_disabling_manager_); nit: Use DCHECK here. CHECK's, as explained in the style guide, should be only used in cases like protecting against security vulnerabilities. https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/system/device_disabling_manager.h (right): https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.h:91: void Init(); nit: Move this method upper, right between constructors/destructors and the following members. This would help to notice it. https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.h:91: void Init(); nit: Please add a comment like "Must be called after construction".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/13 13:38:00, Ivan Šandrk wrote: > Hey guys, I've actually went with a second approach since some tests were > crashing. This one maybe seems better actually. I'm fine with this approach either.
Hey Max, no changes yet, just clarifying some things :) https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/screens/device_disabled_screen.cc:27: CHECK(device_disabling_manager_); On 2017/04/13 13:57:31, emaxx wrote: > nit: Use DCHECK here. > CHECK's, as explained in the style guide, should be only used in cases like > protecting against security vulnerabilities. Was waiting on Achuith's response before changing it, but might as well change it immediately :) https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/system/device_disabling_manager.h (right): https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.h:91: void Init(); On 2017/04/13 13:57:31, emaxx wrote: > nit: Please add a comment like "Must be called after construction". Should I maybe create a factory method or something like CreateDeviceDisablingManager, have that one construct the object and call Init() on it, and then make the constructor and Init() private?
https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/system/device_disabling_manager.h (right): https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.h:91: void Init(); On 2017/04/13 14:22:25, Ivan Šandrk wrote: > On 2017/04/13 13:57:31, emaxx wrote: > > nit: Please add a comment like "Must be called after construction". > > Should I maybe create a factory method or something like > CreateDeviceDisablingManager, have that one construct the object and call Init() > on it, and then make the constructor and Init() private? But how will the bug resolved then? If you do all the initialization inside the factory, then something will read nullptr from BrowserProcessPlatformPart during the initialization.
The CQ bit was checked by isandrk@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...
https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/screens/device_disabled_screen.cc:25: if (view_) On 2017/04/13 13:57:31, emaxx wrote: > BTW, while you are here: this condition also contradicts our style guide for the > DCHECK's. The next line should be done unconditionally (letting it to crash or > whatever - we don't care assuming that the DCHECK is correct). Done. https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/screens/device_disabled_screen.cc:27: CHECK(device_disabling_manager_); On 2017/04/13 13:57:31, emaxx wrote: > nit: Use DCHECK here. > CHECK's, as explained in the style guide, should be only used in cases like > protecting against security vulnerabilities. Done. https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/system/device_disabling_manager.h (right): https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.h:91: void Init(); On 2017/04/13 14:30:01, emaxx wrote: > On 2017/04/13 14:22:25, Ivan Šandrk wrote: > > On 2017/04/13 13:57:31, emaxx wrote: > > > nit: Please add a comment like "Must be called after construction". > > > > Should I maybe create a factory method or something like > > CreateDeviceDisablingManager, have that one construct the object and call > Init() > > on it, and then make the constructor and Init() private? > > But how will the bug resolved then? If you do all the initialization inside the > factory, then something will read nullptr from BrowserProcessPlatformPart during > the initialization. Haha good point, forgot about that. Was too focused on one part of code xD https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.h:91: void Init(); On 2017/04/13 13:57:31, emaxx wrote: > nit: Move this method upper, right between constructors/destructors and the > following members. This would help to notice it. Done. https://codereview.chromium.org/2815893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/system/device_disabling_manager.h:91: void Init(); On 2017/04/13 13:57:31, emaxx wrote: > nit: Please add a comment like "Must be called after construction". Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Maksim, Achuith, are you fine with me landing this without the regression test, and then adding the test in a follow-up CL next week? I'm assuming it will take me some time to figure out how to do it, and I think they'd like this to land by this Friday (tomorrow) since M58 goes stable very soon and it should be in there. (I'm ooo tomorrow) P.S. Can you add someone who can lgtm browser_process_platform_part_chromeos? I cannot find anyone in chrome/browser/OWNERS and I'm unsure if I should just use chrome/OWNERS.
LGTM. On 2017/04/13 15:31:09, Ivan Šandrk wrote: > Maksim, Achuith, are you fine with me landing this without the regression test, > and then adding the test in a follow-up CL next week? I'm assuming it will take > me some time to figure out how to do it, and I think they'd like this to land by > this Friday (tomorrow) since M58 goes stable very soon and it should be in > there. (I'm ooo tomorrow) Sure, we may land this with just a manual test and then address the automated testing in a follow-up CL.
emaxx@chromium.org changed reviewers: + thestig@chromium.org
+thestig@: Lei, please review the change in file chrome/browser/browser_process_platform_part_chromeos.cc > P.S. Can you add someone who can lgtm browser_process_platform_part_chromeos? I > cannot find anyone in chrome/browser/OWNERS and I'm unsure if I should just use > chrome/OWNERS. Done. I believe there are no specific OWNERS for this file.
https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/2815893002/diff/20001/chrome/browser/chromeos... 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 2017/04/12 19:45:14, achuithb wrote: > > On 2017/04/12 17:05:49, emaxx wrote: > > > I think DCHECK is more suitable there. See the Style Guide: > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > Personally I don't see the value of DCHECKing on this line when the next line > > will crash anyway. > > It wasn't really crashing on the next line, it was crashing deeper down the call > stack. I got a bit confused by this at first, and it took me some time to figure > it out (C++ doesn't crash on calling into nullptr object member functions if > they're not virtual, it crashed only when it tried to touch some data). > > But if that's what you actually meant by your comment, I will remove it (I still > don't have a gut feeling for (D)CHECKs). It's fine if you want to leave in the DCHECK. In general, it's not hard to figure out there's a null ptr dereference when you get a crash stack, and we don't want to litter our code with DCHECKs before using any pointer.
lgtm
I don't know this code well, but based on the consistent Init() usage and others' approvals, lgtm.
On 2017/04/13 18:40:03, achuithb wrote: > It's fine if you want to leave in the DCHECK. In general, it's not hard to > figure out there's a null ptr dereference when you get a crash stack, and we > don't want to litter our code with DCHECKs before using any pointer. Yep, there's really no need to DCHECK. If you crash it's pretty obvious what happened. Some people like adding DCHECKs for non-NULL pointers at the beginning of functions to document the prerequisite. Others just consistently dereference pointers without checking at all in their code. That's also a strong signal to readers that the pointers are expected to be non-NULL. :)
The CQ bit was checked by isandrk@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 isandrk@chromium.org
The CQ bit was checked by isandrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, achuith@chromium.org, emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/2815893002/#ps100001 (title: "Removed unnecessary DCHECKs")
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": 100001, "attempt_start_ts": 1492160384896100, "parent_rev": "65e89bdb39e1f22f3032cfafdd3f2da1ba64a9b9", "commit_rev": "87c761f625a57653cc241f5691cb013ad9ca3efa"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/87c761f625a57653cc241f5691cb... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/87c761f625a57653cc241f5691cb... |