Fix QuickLaunch AuraInit Crash

Fix QuickLaunch AuraInit Crash

QuickLaunch will attempt to bring up its own Aura. However it is possible for
QuickLaunch::OnStart to occur after the peer has closed. AuraInit requires a
Directory api to load resources. Currently failing with a CHECK when it is not
available.

This updates AuraInit to report success/failure at initializing. QuickLaunch
uses this to cleanly close the ServiceContext on its side.

This will clean up a series of random crashes occuring in mash_browser_tests

TEST=mash_browser_tests
BUG=678687
Review-Url: https://codereview.chromium.org/2949103002
Cr-Commit-Position: refs/heads/master@{#481883}
Committed: https://chromium.googlesource.com/chromium/src/+/e4175877f5a72d240e89bde552f79fc31e2b0cad

[jonross, 2017-06-22 14:22:40]

jonross@chromium.org changed reviewers:
+ sky@chromium.org

[jonross, 2017-06-22 14:22:40]

Hey sky@,

Could you take a look at this review? Here AuraInit begins to track if
initialization has succeeded, rather that crashing on failure access to
resources.

QuickLauncher uses this as information to shut itself down.

Thanks,
Jon

[sky, 2017-06-22 15:28:59]

https://codereview.chromium.org/2949103002/diff/20001/ui/views/mus/aura_init.cc
File ui/views/mus/aura_init.cc (right):

https://codereview.chromium.org/2949103002/diff/20001/ui/views/mus/aura_init....
ui/views/mus/aura_init.cc:73: if (!InitializeResources(connector))
What do you think of moving everything here into an Init() function that returns
a boolean with WARN_UNUSED_RESULT?

[jonross, 2017-06-22 17:49:30]

https://codereview.chromium.org/2949103002/diff/20001/ui/views/mus/aura_init.cc
File ui/views/mus/aura_init.cc (right):

https://codereview.chromium.org/2949103002/diff/20001/ui/views/mus/aura_init....
ui/views/mus/aura_init.cc:73: if (!InitializeResources(connector))
On 2017/06/22 15:28:58, sky wrote:
> What do you think of moving everything here into an Init() function that
returns
> a boolean with WARN_UNUSED_RESULT?

I had considered that and have no objections. I had just went with the smaller
scoped changed so that I could run the suite with only QuickLauncher changed to
see if other issues were being masked by this.

But can switch over all callsites to an Init() function instead.

I had also been leaving the others around while I digged deeper into the peer
being lost. I've documented it in crbug.com/735051 and it can be detected when
you call into a Sync mojo method call.

I was going to give some thought to addressing this at the InterfacePtr level.
Leaving the other call-sites alone in the interim. But no strong preference.

[sky, 2017-06-22 20:15:32]

If this is only to see how it addresses issues, I'm ok with that. If it
proves useful then we should go with Init(). Add a TODO to that effect.

On Thu, Jun 22, 2017 at 10:49 AM, <jonross@chromium.org> wrote:

>
> https://codereview.chromium.org/2949103002/diff/20001/ui/
> views/mus/aura_init.cc
> File ui/views/mus/aura_init.cc (right):
>
> https://codereview.chromium.org/2949103002/diff/20001/ui/
> views/mus/aura_init.cc#newcode73
> ui/views/mus/aura_init.cc:73: if (!InitializeResources(connector))
> On 2017/06/22 15:28:58, sky wrote:
> > What do you think of moving everything here into an Init() function
> that returns
> > a boolean with WARN_UNUSED_RESULT?
>
> I had considered that and have no objections. I had just went with the
> smaller scoped changed so that I could run the suite with only
> QuickLauncher changed to see if other issues were being masked by this.
>
> But can switch over all callsites to an Init() function instead.
>
> I had also been leaving the others around while I digged deeper into the
> peer being lost. I've documented it in crbug.com/735051 and it can be
> detected when you call into a Sync mojo method call.
>
> I was going to give some thought to addressing this at the InterfacePtr
> level. Leaving the other call-sites alone in the interim. But no strong
> preference.
>
> https://codereview.chromium.org/2949103002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jonross, 2017-06-23 13:05:03]

Todo added

[sky, 2017-06-23 13:28:01]

LGTM

[jonross, 2017-06-23 13:30:37]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2017-06-23 13:30:48]

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-06-23 14:41:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498224637582970,
"parent_rev": "83cad2ae0c47202e124c910ce8f710fda13cf838", "commit_rev":
"e4175877f5a72d240e89bde552f79fc31e2b0cad"}

[commit-bot: I haz the power, 2017-06-23 14:41:51]

Description was changed from

==========
Fix QuickLaunch AuraInit Crash

QuickLaunch will attempt to bring up its own Aura. However it is possible for
QuickLaunch::OnStart to occur after the peer has closed. AuraInit requires a
Directory api to load resources. Currently failing with a CHECK when it is not
available.

This updates AuraInit to report success/failure at initializing. QuickLaunch
uses this to cleanly close the ServiceContext on its side.

This will clean up a series of random crashes occuring in mash_browser_tests

TEST=mash_browser_tests
BUG=678687
==========

to

==========
Fix QuickLaunch AuraInit Crash

QuickLaunch will attempt to bring up its own Aura. However it is possible for
QuickLaunch::OnStart to occur after the peer has closed. AuraInit requires a
Directory api to load resources. Currently failing with a CHECK when it is not
available.

This updates AuraInit to report success/failure at initializing. QuickLaunch
uses this to cleanly close the ServiceContext on its side.

This will clean up a series of random crashes occuring in mash_browser_tests

TEST=mash_browser_tests
BUG=678687

Review-Url: https://codereview.chromium.org/2949103002
Cr-Commit-Position: refs/heads/master@{#481883}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4175877f5a72d240e89bde552f7...
==========

[commit-bot: I haz the power, 2017-06-23 14:41:52]

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