logs for reference

https://pantheon.corp.google.com/storage/browser/chromiumos-test-logs/bugfiles/misc/652839/

crash link

https://crash.corp.google.com/browse?q=reportid=%2762195b0b00000000%27

Crash link is not useful for an induced crash.

xiyuan@ can you take a look?

trapti@, could you clarify the step of "Click on Apps"? Do you mean the "Apps" menu in the footer bar on the login screen? Or the app launcher button on the shelf after getting into the public session?

We have tried app launcher button on the shelf after getting into the public session

Attaching screenshot for reference

Deleted: IMG_0752.JPG 458 KB

	IMG_0752.JPG 458 KB View Download

The problem is because on crash-restart case, we load the user profile directly. When the ExtensionSystem initializes its external providers, DeviceLocalAccountPolicyBroker is not created. Thus its loader (DeviceLocalAccountExternalPolicyLoader) is not hooked up with the extension system and does not install extensions when policy is loaded.

Does this work before?

Not seen this problem earlier other than M54 but seen various issues in PS in M54.

Note** usually we do not check this by inducing crash.

On the crash-restart case, DeviceLocalAccountPolicyService is created early enough. But UpdateAccountList does not create policy broker for device local accounts until CrosSettings has been verified. And by then, we already passed the Profile/ExtensionService init thus DeviceLocalAccountExternalPolicyLoader is not used.

Question for bartfab@ and atwilson@:
Can we let it go first and bind it with a PrepareTrustedValues to refresh it after trusted values are available?

Proposal: https://codereview.chromium.org/2416473002/

So, I presume we will still register the provider once crossettings are trusted - is the problem just that ExtensionService doesn't apply the policy from the provider outside of init()? Because fixing that (either by tweaking extensions::ManagementPolicy::RegisterProvider() or by having DeviceLocalAccountPolicyService trigger a refresh after registering similar to what we do after a policy update) would probably be a *cleaner* fix.

But I might be missing something since I'm a bit fuzzy these days on how management policy is applied to ExtensionsService.

The force installed extension policy is wired with ExtensionService by registering as a loader of external extension provider [1]. The problem is that 
ExternalProviderImpl::CreateExternalProviders happens before the CrosSettings become trusted in the crash-n-restart case. Therefore, when the loader (DeviceLocalAccountExternalPolicyLoader) is not registered with ExtensionService. When it gets a policy update, its LoadFinished does nothing because it does not have an |owner_| ExternalProviderImpl and not bound with the ExtensionService.

[1]: https://cs.chromium.org/chromium/src/chrome/browser/extensions/external_provider_impl.cc?rcl=1476256712&l=515-518

This still seems fragile as it seems like we'll just fail in the case where CrosSettings are fully untrusted (or, at best, we're relying on some other mechanism to ensure that ExternalLoader::Init() gets called *after* CrosSettings become trusted.

Anyhow, agreed that this fix is better than nothing, and is less invasive than anything else we might do so I'll LGTM the fix.

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

commit a8ad49adf82b43ef6baa1f227e36fa0a6518326d
Author: xiyuan <xiyuan@chromium.org>
Date: Thu Oct 13 14:43:19 2016

cros: Fix crash-n-restart public session losing force-installed apps

Allow temporarily untrusted cros settings to be used in
DeviceLocalAccountPolicyService::UpdateAccountList so that the
DeviceLocalAccountExternalPolicyLoader is properly registered
with ExtensionService on the crash-n-restart case.

BUG= 652839 

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

[modify] https://crrev.com/a8ad49adf82b43ef6baa1f227e36fa0a6518326d/chrome/browser/chromeos/policy/device_local_account_policy_service.cc

Do we need this on M-55 too?

Yes, need this for M55 too.

Your change meets the bar and is auto-approved for M55 (branch: 2883)

[Automated comment] Less than a week to go before stable on M54, we might already have a stable candidate build. Manual review required.

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

commit 1723722da36ffc3bd79017d958bba4e437b8fd70
Author: Xiyuan Xia <xiyuan@google.com>
Date: Fri Oct 14 17:02:13 2016

Merge "cros: Fix crash-n-restart public session losing force-installed apps"

> Allow temporarily untrusted cros settings to be used in
> DeviceLocalAccountPolicyService::UpdateAccountList so that the
> DeviceLocalAccountExternalPolicyLoader is properly registered
> with ExtensionService on the crash-n-restart case.
>
> BUG= 652839 
>
> Review-Url: https://codereview.chromium.org/2416473002
> Cr-Commit-Position: refs/heads/master@{#425026}
> (cherry picked from commit a8ad49adf82b43ef6baa1f227e36fa0a6518326d)

Review URL: https://codereview.chromium.org/2420693003 .

Cr-Commit-Position: refs/branch-heads/2883@{#110}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/1723722da36ffc3bd79017d958bba4e437b8fd70/chrome/browser/chromeos/policy/device_local_account_policy_service.cc

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

commit f4c14f7aab240e5b5bd9b6f5b6cad9bb575bcec9
Author: Xiyuan Xia <xiyuan@google.com>
Date: Fri Oct 14 19:57:38 2016

Merge "cros: Fix crash-n-restart public session losing force-installed apps"

> Allow temporarily untrusted cros settings to be used in
> DeviceLocalAccountPolicyService::UpdateAccountList so that the
> DeviceLocalAccountExternalPolicyLoader is properly registered
> with ExtensionService on the crash-n-restart case.
>
> BUG= 652839 
>
> Review-Url: https://codereview.chromium.org/2416473002
> Cr-Commit-Position: refs/heads/master@{#425026}
> (cherry picked from commit a8ad49adf82b43ef6baa1f227e36fa0a6518326d)

Review URL: https://codereview.chromium.org/2408133010 .

Cr-Commit-Position: refs/branch-heads/2840@{#740}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/f4c14f7aab240e5b5bd9b6f5b6cad9bb575bcec9/chrome/browser/chromeos/policy/device_local_account_policy_service.cc

Verified in Peppy.Could see apps after induced crash.


M	ChromeOS	Chrome	ARC	Type	Channel
55	8872.14.0	55.0.2883.17	3356632	release	dev

Verified in Pit.Could see apps after induced crash.

M	ChromeOS	Chrome	ARC	Type	Channel
54	8743.69.0	54.0.2840.68	3364514	release	beta

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

commit 1723722da36ffc3bd79017d958bba4e437b8fd70
Author: Xiyuan Xia <xiyuan@google.com>
Date: Fri Oct 14 17:02:13 2016

Merge "cros: Fix crash-n-restart public session losing force-installed apps"

> Allow temporarily untrusted cros settings to be used in
> DeviceLocalAccountPolicyService::UpdateAccountList so that the
> DeviceLocalAccountExternalPolicyLoader is properly registered
> with ExtensionService on the crash-n-restart case.
>
> BUG= 652839 
>
> Review-Url: https://codereview.chromium.org/2416473002
> Cr-Commit-Position: refs/heads/master@{#425026}
> (cherry picked from commit a8ad49adf82b43ef6baa1f227e36fa0a6518326d)

Review URL: https://codereview.chromium.org/2420693003 .

Cr-Commit-Position: refs/branch-heads/2883@{#110}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/1723722da36ffc3bd79017d958bba4e437b8fd70/chrome/browser/chromeos/policy/device_local_account_policy_service.cc

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

commit f4c14f7aab240e5b5bd9b6f5b6cad9bb575bcec9
Author: Xiyuan Xia <xiyuan@google.com>
Date: Fri Oct 14 19:57:38 2016

Merge "cros: Fix crash-n-restart public session losing force-installed apps"

> Allow temporarily untrusted cros settings to be used in
> DeviceLocalAccountPolicyService::UpdateAccountList so that the
> DeviceLocalAccountExternalPolicyLoader is properly registered
> with ExtensionService on the crash-n-restart case.
>
> BUG= 652839 
>
> Review-Url: https://codereview.chromium.org/2416473002
> Cr-Commit-Position: refs/heads/master@{#425026}
> (cherry picked from commit a8ad49adf82b43ef6baa1f227e36fa0a6518326d)

Review URL: https://codereview.chromium.org/2408133010 .

Cr-Commit-Position: refs/branch-heads/2840@{#740}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/f4c14f7aab240e5b5bd9b6f5b6cad9bb575bcec9/chrome/browser/chromeos/policy/device_local_account_policy_service.cc