Not clear if its by design. If its not by design this is a regression in 57.

This definitely a regression and is reproducible on M58.0.3029.51;9334.33.0 as well.

Unable to reproduce on 58.0.3015.0;9289.0.0 (dev-channel falco test).

@isandrk: seems odd to be testing with a TEST HWID device and such an old release? Current beta channel is newer than your build (9334.33.0) and dev channel is at 9413.0.0.

Reproduced the issue with an MP Link device and current beta channel release 58.0.3029.40 (Platform version: 9334.28.0).

Please note that if the device is powered on and not logged on, it may go straight to the disabled lock screen without a reboot. Rebooting the unit at this point should show the issue though.

@jayhlee: I did reproduce it on a Minnie. Do you maybe have crash logs?

Crash logs have been hard since I can't get the device to boot without wiping after I repro the issue. I tried reproducing on a device already in dev mode and with dev_boot_usb=1 set so I could boot off USB after repro but when a device is disabled it's also set to block dev mode so it's taken out of dev mode :-/

Agree though we really need a way to get the logs so we can see what's going on. Can we find someone with a servo board or some other method to extract logs after the crash?

Can you maybe ssh into the device? I'm able to do it with my Minnie, but I'm not sure if I have a special configuration that allows me to do that.

So a bit more context.. the device actually partially boots, it's just unable to get the actual UI up and running (crash happens somewhere in UI, I see the stacktrace, but I don't see the symbols so I don't know which function causes the crash).

So the crash happens because device_disabling_manager_ is a nullptr here:

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/screens/device_disabled_screen.cc?rcl=b10caf8de873181849d28ea10bea9be564875e31&l=27

-- but the actual crash happens a bit deeper in the call stack, apparently C++ has no issues calling into member functions of nullptr objects :D (crashes only when it touches some member variable)


Will continue investigating.

I've uploaded a CL with the potential fix crrev.com/2815893002

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/87c761f625a57653cc241f5691cb013ad9ca3efa

commit 87c761f625a57653cc241f5691cb013ad9ca3efa
Author: isandrk <isandrk@chromium.org>
Date: Fri Apr 14 09:26:07 2017

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}

[modify] https://crrev.com/87c761f625a57653cc241f5691cb013ad9ca3efa/chrome/browser/browser_process_platform_part_chromeos.cc
[modify] https://crrev.com/87c761f625a57653cc241f5691cb013ad9ca3efa/chrome/browser/chromeos/login/screens/device_disabled_screen.cc
[modify] https://crrev.com/87c761f625a57653cc241f5691cb013ad9ca3efa/chrome/browser/chromeos/system/device_disabling_manager.cc
[modify] https://crrev.com/87c761f625a57653cc241f5691cb013ad9ca3efa/chrome/browser/chromeos/system/device_disabling_manager.h
[modify] https://crrev.com/87c761f625a57653cc241f5691cb013ad9ca3efa/chrome/browser/chromeos/system/device_disabling_manager_unittest.cc

I assume we will merge this to 58 and 59.

I don't believe 59 has branched yet so a merge to 59 shouldn't be necessary (it's already in master). marchuk@ is CCed on the bug and should be working to reproduce the issue in 59 canary and then verify it's fixed with this patch once patch reaches canary builds (should be by Mon?).

As soon as fix is confirmed working in 59 we should get it merged down to 58 so 58 stable will have it.

Well 59 was branching as I posted comment #14 :-) This patch is in build 60.0.3072.0.

58 - Stable Release: Apr 25 2017 PT

We should get this ball rolling soon if we want it in M58. Should I add Merge-Request-58 label here?

[Auto-generated comment by a script] We noticed that this issue is targeted for M-57; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-57 label, otherwise remove Merge-TBD label. Thanks.

For some reason it's taking awhile for canary to get 60 browser with this fix. Can we merge into 59 dev so we have an image to test with? We really need to get this down to 58 before it goes stable.

Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

isandrk@ can you land this on 59 now?

Jay: done.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c438eb0c1cf953e341a196d7ce57e28eecec7aa7

commit c438eb0c1cf953e341a196d7ce57e28eecec7aa7
Author: Ivan Sandrk <isandrk@google.com>
Date: Wed Apr 19 22:12:22 2017

[Merge to M59] 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}
(cherry picked from commit 87c761f625a57653cc241f5691cb013ad9ca3efa)

Review-Url: https://codereview.chromium.org/2832673002 .
Cr-Commit-Position: refs/branch-heads/3071@{#68}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/c438eb0c1cf953e341a196d7ce57e28eecec7aa7/chrome/browser/browser_process_platform_part_chromeos.cc
[modify] https://crrev.com/c438eb0c1cf953e341a196d7ce57e28eecec7aa7/chrome/browser/chromeos/login/screens/device_disabled_screen.cc
[modify] https://crrev.com/c438eb0c1cf953e341a196d7ce57e28eecec7aa7/chrome/browser/chromeos/system/device_disabling_manager.cc
[modify] https://crrev.com/c438eb0c1cf953e341a196d7ce57e28eecec7aa7/chrome/browser/chromeos/system/device_disabling_manager.h
[modify] https://crrev.com/c438eb0c1cf953e341a196d7ce57e28eecec7aa7/chrome/browser/chromeos/system/device_disabling_manager_unittest.cc

Thanks Ivan, patch has landed in 9460.5.0 / 59.0.3071.15.

@marchuk: can you upgrade a device to current dev channel, confirm the issue with disabling and then try again with the above version? (ping me for images if needed) Once we confirm it's fixed we can merge down to 58.

Verified this issue in Candy.Could reboot and users can see "enterprise admin disabled device"

M	ChromeOS	Chrome	ARC	Type	Channel
59	9460.5.0	59.0.3071.15	(multiple)	release	dev

This bug requires manual review: Only 3 days from stable, we might already have a stable candidate build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Just confirmed on terra -- on latest dev issues still exists, 

when flashed to canary:
Google Chrome Version	
59.0.3071.15
Platform Version	
9460.5.0 (Official Build) canary-channel terra
Issue gone and there is red 'Locked" screen.

Works with no issues on dev:
Google Chrome Version	
59.0.3071.15
Platform Version	
9460.5.0 (Official Build) dev-channel terra

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29c3639d106664d10b7b42aa4765701dec803b5e

commit 29c3639d106664d10b7b42aa4765701dec803b5e
Author: isandrk <isandrk@chromium.org>
Date: Mon Apr 24 19:43:26 2017

Added a test for crash in DeviceDisabledScreen

Follow up CL to crrev.com/2815893002

TEST=Manual. This test was crashing without the fix in cr/2815893002, now it's working fine.
BUG= 709518 

Review-Url: https://codereview.chromium.org/2843513002
Cr-Commit-Position: refs/heads/master@{#466719}

[modify] https://crrev.com/29c3639d106664d10b7b42aa4765701dec803b5e/chrome/browser/chromeos/system/device_disabling_browsertest.cc

Max/David - do you want this fix merged to M58? If so, I think you need to drive a conversation with the TPMs to make sure we have a release vehicle for this. I would probably be uncomfortable merging this directly into the stable channel, especially since "disabled device crashes on boot instead of displaying an error message" isn't quite a P0 bug.

marchuk@: are you going to mark this as verified or is there further testing to perform?

I agree, Drew.

Did this fix actually get merged with 58 stable? If not, is it currently planned for an incremental patch on the stable channel, or not planned until the 59?

I've been following this since I had actually discovered the bug during a school testing session (certainly wasn't a great day...), and reported my replication steps to GAFE Chrome support on the 24th of April. I was directed to this bug report, after they also replicated it and related it to this open report. I've independently confirmed this issue fixed on dev-channel 59.0.3071.25 platform 9460.11.0 on the Reks 7287.133.81 build, tested it around the 26th of April.

Current plan is for this to get to M59 - it doesn't quite meet the bar for a stable push for M58.

We have this issue on 57 and 58 stable - for us testing problem, when disabling the chromebook if on and online, it reboots spontaneously straight to the Chrome logo and sticks within seconds of disabling on console. As above, means no message for returning the device dispalyed.

atwilson@ I see patch has landed in 60, can we get this merged into 59 now so it reaches stable soon?

Per comment #25 and #29, this is fixed on M59. Marking closed + verified again.

ugh - this is a P0 regression that's affecting customers.  Why was this not marked as a P0?  Totally breaking how disabling works is not a P1.  This absolutely has to get into 59 and we should merge to 58 as well just in case we do another 58 stable push.

Also, I'd like a post-mortem.  Do we not have proper automated and manual tests for this?  How did a regression like this get through?

Thanks.

I'll take the blame for filing as P1. The issue only occurs for disabled devices which is technically still working, we're just failing to display the "please return it" message. Assumption is for stolen / lost devices there needs to be some level of admin touch to restore the device anyways so I did not feel this warranted P0.

I do agree that it would be very good to see this merged into M59.

Sorry, getting my bugs crossed today.

Based on atwilson@ comment #37, yes, this is merged and fixed in 59. No further fix should be necessary for this to reach stable soon.

Disagree that this is a P0 - devices *are* disabled, you just don't get the message as Jay mentioned.

As for a post-mortem - don't think this merits a full post-mortem, but happy to start a thread with you/krishna/bartfab about whether we have the resources to add this to the list of features that are manually tested every release.

Comment on customer impact.  

As domain admin for K-12 district we will observe based on content filtering reports, or have reported to us, some activity which may be a violation of our AUP or a threat to network security.  The ability to disable a device with a "Report to admin" message assisted in managing the issue and taking necessary remedial action.

Prior to the emergence of this bug the device could be readily re-enabled and the browser history and/or locally stored files could be examined before they could be erased by the user. The introduction of this bug effectively prevented such a device inspection and ability to document the users' activity on the device.

This bug was detected when a device was disabled due to a user visiting a site that when accessed with another control device was found to be redirecting to a site presenting downloads of malware.  The user's device was manually disabled based on the suspicious URLs being reported by the content filter.  

When the user presented the device and the Chromebook would not boot past the splash screen there was a concern that malicious code had actually been installed on the device creating the 'bricked' condition, as well as a concern that such code could have been injected into the network or could spread through shared documents.  Needless to say, this was a significant concern.  Only by a second instance of a device being disabled were we able to consider that a bug had been introduced, and opened a trouble ticket on April 13 which ultimately led us here.