Looks like a M65 regression. Feel free to correct my priority, label, or component selections.

Oh, and in case it helps, I grabbed a feedback report on my Bob when it occurred:

https://listnr.corp.google.com/product/208/report/84904402149

From a UI perspective we should disable any button/option that isn't going to do something. What is not clear to me is whether the problem is that we should be able to powerwash here but are not, or if the wrong UI option is being presented.

Before I reflashed my Bob to canary, it was running beta-channel:

Chrome: 64.0.3282.41
OS: 10176.22.0

And with that image, this same dialog popped up, and it properly restarted and left me on a login screen with the choice to move forward with the actual power wash or to cancel it there. (I canceled it.)

That sounds to me like you're showing the right UI option, but it's just failing to restart and powerwash. (i.e., the former of your 2 options.)

The subsequent behavior is also non-intuitive, but that's not really the point I suppose.

1) From the feedback report: ENTERPRISE_ENROLLED = Not managed, so powerwash should be available.
2) If the issue is indeed in that CL from the description, 
  a) It likely makes sense for the powerwash dialogue to refuse to proceed if the tpm firmware update was requested but is not actually possible (as opposed to ignoring this option and powerwashing);
  b) but we shouldn't have tpm_firmware_update_requested ever set to true for 2.0 devices; I guess, this checkbox is not even shown for 2.0 in the powerwash dialogue.

Was there any CL recently that set the default for tpm_firmware_update_requested to true (e.g. in finch)? I remember the discussion that started if it may be time for that, but I don't know the outcome.

Hi guys,

here's some updates:
1/ i added some logs and confirmed it indeed because the BrowserLifetimeHandler::HandleFactoryReset got tpm_firmware_update_requested(true)

2/ it looks like we added this arg(requestTpmFirmwareUpdate in powerwash_dialog.js) in:
commit bc43a830e47b74f988fd4413f7f8d3a3cf1e7759
Author: Mattias Nissler <mnissler@chromium.org>
Date:   Wed Sep 27 12:13:56 2017 +0000

    Add TPM firmware update option to about page.

    BUG=chromium:742985
    TEST=New webui test case in CrSettingsAboutPageTest.AboutPage

    Change-Id: Ie165e2af36b48d94e8f2ebfc26d70207aff8f14c
    Reviewed-on: https://chromium-review.googlesource.com/684023

and set it to true in about_page.html:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/about_page/about_page.html?type=cs&l=261



but the user is testing the powerwash in reset_page.html, where would not init the requestTpmFirmwareUpdate property:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/reset_page/reset_page.html?type=cs&l=59

so it would be undefined when pass to that HandleFactoryReset()



3/ it works if add a default value(false) to it:
+++ b/chrome/browser/resources/settings/reset_page/powerwash_dialog.js
@@ -11,8 +11,10 @@ Polymer({
   is: 'settings-powerwash-dialog',

   properties: {
-    /** @public */
-    requestTpmFirmwareUpdate: Boolean,
+    requestTpmFirmwareUpdate: {
+      type: Boolean,
+      value: false,
+    },
   },





so it must be something wrong when passing undefined boolean through chrome.send to the C++ code

hmmm, think i know what is happening here...


so we pass the undefined boolean to BrowserLifetimeHandler, and it would try to use ListValue::GetBoolean to get it:
void BrowserLifetimeHandler::HandleFactoryReset(
    const base::ListValue* args) {
  bool tpm_firmware_update_requested;
  args->GetBoolean(0, &tpm_firmware_update_requested);
  if (tpm_firmware_update_requested) {


the GetBoolean() would return false if it's undefined without changed the boot_value:
bool DictionaryValue::GetBoolean(StringPiece path, bool* bool_value) const {
  const Value* value;
  if (!Get(path, &value))
    return false;

  return value->GetAsBoolean(bool_value);
}

so the tpm_firmware_update_requested would be left random.



so we can:
1/ add a default value(false) for requestTpmFirmwareUpdate
or
2/ add a default value(false) for tpm_firmware_update_requested:
+++ b/chrome/browser/ui/webui/settings/browser_lifetime_handler.cc
@@ -59,7 +59,7 @@ void BrowserLifetimeHandler::HandleSignOutAndRestart(

 void BrowserLifetimeHandler::HandleFactoryReset(
     const base::ListValue* args) {
-  bool tpm_firmware_update_requested;
+  bool tpm_firmware_update_requested = false;

or
3/ check the result of GetBoolean

My proposal is to

1) fix the UI code to properly pass true or false  (never undefined) to chrome.send.

2) Update the corresponding UI unit test to ensure that. Currently the test is only checking that the correct chrome.send() message is sent, but it does not check the arguments passed to it, see [1].

3) On the C++ side, see [2], instead of silently ignoring the undefined value, add a CHECK as in

CHECK(args->GetBoolean(0, &tpm_firmware_update_requested));

[1] https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/reset_page_test.js?l=112
[2] https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/browser_lifetime_handler.cc?type=cs&sq=package:chromium&l=63

I can take this on. I agree with all of the above suggestions.

Similar case reported by user in  Issue 803446 

CL is up for review:
https://chromium-review.googlesource.com/c/chromium/src/+/887631

 Issue 803446  has been merged into this issue.

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

commit 85a6ad31db1d6ca11cd52be6b0c35edd331aa42e
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Tue Jan 30 19:41:35 2018

BrowserLifetimeHandler: Require true/false update arg

Previously tpm_firmware_update_requested might be undefined.
This makes the argument required and ensures that it is passed.

Bug:  800103 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ia5263d011a58f5f444b64df2e7109083c70b3daf
Reviewed-on: https://chromium-review.googlesource.com/887631
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532997}
[modify] https://crrev.com/85a6ad31db1d6ca11cd52be6b0c35edd331aa42e/chrome/browser/resources/settings/about_page/about_page.js
[modify] https://crrev.com/85a6ad31db1d6ca11cd52be6b0c35edd331aa42e/chrome/browser/resources/settings/lifetime_browser_proxy.js
[modify] https://crrev.com/85a6ad31db1d6ca11cd52be6b0c35edd331aa42e/chrome/browser/resources/settings/reset_page/compiled_resources2.gyp
[modify] https://crrev.com/85a6ad31db1d6ca11cd52be6b0c35edd331aa42e/chrome/browser/resources/settings/reset_page/powerwash_dialog.js
[modify] https://crrev.com/85a6ad31db1d6ca11cd52be6b0c35edd331aa42e/chrome/browser/ui/webui/settings/browser_lifetime_handler.cc
[modify] https://crrev.com/85a6ad31db1d6ca11cd52be6b0c35edd331aa42e/chrome/test/data/webui/settings/about_page_tests.js
[modify] https://crrev.com/85a6ad31db1d6ca11cd52be6b0c35edd331aa42e/chrome/test/data/webui/settings/reset_page_test.js

[Auto-generated comment by a script] We noticed that this issue is targeted for M-65; 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-65 label, otherwise remove Merge-TBD label. Thanks.

Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

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

Steven, thanks for fixing this, and sorry for the breakage. Apologies for not taking action, I only came across this bug just now :-/

Is this getting cherry-picked to 65?

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

commit 0d78dc57a2ecfe804e963a0296add6c69b8a8675
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Mon Feb 05 20:06:51 2018

BrowserLifetimeHandler: Require true/false update arg

Previously tpm_firmware_update_requested might be undefined.
This makes the argument required and ensures that it is passed.

TBR=stevenjb@chromium.org

(cherry picked from commit 85a6ad31db1d6ca11cd52be6b0c35edd331aa42e)

Bug:  800103 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ia5263d011a58f5f444b64df2e7109083c70b3daf
Reviewed-on: https://chromium-review.googlesource.com/887631
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#532997}
Reviewed-on: https://chromium-review.googlesource.com/902430
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#315}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/0d78dc57a2ecfe804e963a0296add6c69b8a8675/chrome/browser/resources/settings/about_page/about_page.js
[modify] https://crrev.com/0d78dc57a2ecfe804e963a0296add6c69b8a8675/chrome/browser/resources/settings/lifetime_browser_proxy.js
[modify] https://crrev.com/0d78dc57a2ecfe804e963a0296add6c69b8a8675/chrome/browser/resources/settings/reset_page/compiled_resources2.gyp
[modify] https://crrev.com/0d78dc57a2ecfe804e963a0296add6c69b8a8675/chrome/browser/resources/settings/reset_page/powerwash_dialog.js
[modify] https://crrev.com/0d78dc57a2ecfe804e963a0296add6c69b8a8675/chrome/browser/ui/webui/settings/browser_lifetime_handler.cc
[modify] https://crrev.com/0d78dc57a2ecfe804e963a0296add6c69b8a8675/chrome/test/data/webui/settings/about_page_tests.js
[modify] https://crrev.com/0d78dc57a2ecfe804e963a0296add6c69b8a8675/chrome/test/data/webui/settings/reset_page_test.js