mash: Fix screen lock preload crash after login

mash: Fix screen lock preload crash after login

Disable screen lock preload in mash to avoid the crash for now.

BUG=698889
Review-Url: https://codereview.chromium.org/2736813002
Cr-Commit-Position: refs/heads/master@{#455108}
Committed: https://chromium.googlesource.com/chromium/src/+/3bdf8977c45b8073999d40f2f7ec0f56dce7aec4

[xiyuan, 2017-03-06 23:29:43]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 23:30:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-03-06 23:34:23]

xiyuan@chromium.org changed reviewers:
+ jdufault@chromium.org

[jdufault, 2017-03-06 23:45:45]

lgtm

[Daniel Erat, 2017-03-06 23:48:57]

derat@chromium.org changed reviewers:
+ derat@chromium.org

[Daniel Erat, 2017-03-06 23:48:58]

https://codereview.chromium.org/2736813002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/lock/webui_screen_locker.cc (right):

https://codereview.chromium.org/2736813002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:88: if
(ash_util::IsRunningInMash())
the connection between this code and IdleDetector isn't obvious from looking at
this file. from the stack trace in crbug.com/698889, i'm guessing that
PreloadedWebView tries to defer loading the page until the user is idle or
something like that, right? would it be better to make PreloadedWebView just
load the page immediately when running under mash?

[xiyuan, 2017-03-06 23:54:50]

https://codereview.chromium.org/2736813002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/lock/webui_screen_locker.cc (right):

https://codereview.chromium.org/2736813002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:88: if
(ash_util::IsRunningInMash())
On 2017/03/06 23:48:58, Daniel Erat wrote:
> the connection between this code and IdleDetector isn't obvious from looking
at
> this file. from the stack trace in crbug.com/698889, i'm guessing that
> PreloadedWebView tries to defer loading the page until the user is idle or
> something like that, right? would it be better to make PreloadedWebView just
> load the page immediately when running under mash?

Your reading is correct. This is an optimization to make the lock screen show up
faster. Screen lock works without the preloading.

I don't want to preload it for mash because preloading makes it unnecessarily
slow and may make it unnecessarily complicated for mash work. Let's keep it
plain and simple for now. After we fix IdleDetector/UserActivityDetector for
chrome_on_mash, we can bring back the feature easily.

[commit-bot: I haz the power, 2017-03-06 23:58:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 23:58:55]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2017-03-07 01:27:16]

lgtm

https://codereview.chromium.org/2736813002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/lock/webui_screen_locker.cc (right):

https://codereview.chromium.org/2736813002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:88: if
(ash_util::IsRunningInMash())
On 2017/03/06 23:54:50, xiyuan wrote:
> On 2017/03/06 23:48:58, Daniel Erat wrote:
> > the connection between this code and IdleDetector isn't obvious from looking
> at
> > this file. from the stack trace in crbug.com/698889, i'm guessing that
> > PreloadedWebView tries to defer loading the page until the user is idle or
> > something like that, right? would it be better to make PreloadedWebView just
> > load the page immediately when running under mash?
> 
> Your reading is correct. This is an optimization to make the lock screen show
up
> faster. Screen lock works without the preloading.
> 
> I don't want to preload it for mash because preloading makes it unnecessarily
> slow and may make it unnecessarily complicated for mash work. Let's keep it
> plain and simple for now. After we fix IdleDetector/UserActivityDetector for
> chrome_on_mash, we can bring back the feature easily.

thanks for the explanation! would you mind also filing a mash bug to make sure
that we don't forget to wire this back up? :-) (or are you planning to retitle
crbug.com/698889 and use it?)

[xiyuan, 2017-03-07 16:56:03]

Description was changed from

==========
mash: Fix screen lock preload crash after login

Disable screen lock preload in mash to avoid the crash for now.

BUG=698889,626899
==========

to

==========
mash: Fix screen lock preload crash after login

Disable screen lock preload in mash to avoid the crash for now.

BUG=698889
==========

[xiyuan, 2017-03-07 17:00:07]

https://codereview.chromium.org/2736813002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/lock/webui_screen_locker.cc (right):

https://codereview.chromium.org/2736813002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:88: if
(ash_util::IsRunningInMash())
On 2017/03/07 01:27:16, Daniel Erat wrote:
> On 2017/03/06 23:54:50, xiyuan wrote:
> > On 2017/03/06 23:48:58, Daniel Erat wrote:
> > > the connection between this code and IdleDetector isn't obvious from
looking
> > at
> > > this file. from the stack trace in crbug.com/698889, i'm guessing that
> > > PreloadedWebView tries to defer loading the page until the user is idle or
> > > something like that, right? would it be better to make PreloadedWebView
just
> > > load the page immediately when running under mash?
> > 
> > Your reading is correct. This is an optimization to make the lock screen
show
> up
> > faster. Screen lock works without the preloading.
> > 
> > I don't want to preload it for mash because preloading makes it
unnecessarily
> > slow and may make it unnecessarily complicated for mash work. Let's keep it
> > plain and simple for now. After we fix IdleDetector/UserActivityDetector for
> > chrome_on_mash, we can bring back the feature easily.
> 
> thanks for the explanation! would you mind also filing a mash bug to make sure
> that we don't forget to wire this back up? :-) (or are you planning to retitle
> crbug.com/698889 and use it?)

SG. Filed http://crbug.com/699144 to restore the reloading after 698889.

[xiyuan, 2017-03-07 17:00:13]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2017-03-07 17:01:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 17:17:08]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1488906013479830, "parent_rev":
"f693974068bced80bd750931026f248e5b9c6dff", "commit_rev":
"3bdf8977c45b8073999d40f2f7ec0f56dce7aec4"}

[commit-bot: I haz the power, 2017-03-07 17:17:48]

Description was changed from

==========
mash: Fix screen lock preload crash after login

Disable screen lock preload in mash to avoid the crash for now.

BUG=698889
==========

to

==========
mash: Fix screen lock preload crash after login

Disable screen lock preload in mash to avoid the crash for now.

BUG=698889

Review-Url: https://codereview.chromium.org/2736813002
Cr-Commit-Position: refs/heads/master@{#455108}
Committed:
https://chromium.googlesource.com/chromium/src/+/3bdf8977c45b8073999d40f2f7ec...
==========

[commit-bot: I haz the power, 2017-03-07 17:17:50]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/3bdf8977c45b8073999d40f2f7ec...