736953 - Break XRE_main up into parts that can be called individually

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Jim Mathies [:jimm]]

Win8 metro's app model is a little different from conventional desktop apps - the thread that enters XRE_main is not the thread the UI is rendered on. We need to be able to shunt off the main thread early and drop it into a main metro entry point, then call back using a ui worker thread to finish up threading & xpcom initialization. This patch makes that possible.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

There's no need to prefix the XREMain methods with XRE_, is there?

Comment 3

[Jim Mathies [:jimm]]

(In reply to Ms2ger from comment #2)
> There's no need to prefix the XREMain methods with XRE_, is there?

Not really, but it does make searching for these methods easier. XRE_main was a really big function.

Comment 4

[Jim Mathies [:jimm]]

Attachment: patch v.2

- Catch some missed early exits for command line dump commands.

Comment 5

[Jim Mathies [:jimm]]

Attachment: patch v.3

- fix up NS_TIME_FUNCTION diagnostic code so that it builds.

Comment 6

[Jim Mathies [:jimm]]

Attachment: patch v.4

- linux / mac bustage fixes.

I ran this through try a few times, didn't see any issues.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 607735 [details] [diff] [review]
patch v.4

Dave, since this is a pretty big change in the xre code I figured I'd ask you for r?. Note the patch looks pretty big but really 90% of this is just code reformatting. Comment 1 explains a bit about why these changes were needed. 

Generally what I've done is break up XRE_main into a set of methods in a simple class names XREMain. In win8 metro, we'll need to to call the init parts and then call back so that a different thread is considered the main thread. Here's that code if your curious:

https://hg.mozilla.org/projects/elm/rev/ff3c7db1fce0

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch with -w

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 607735 [details] [diff] [review]
patch v.4

Review of attachment 607735 [details] [diff] [review]:
-----------------------------------------------------------------

I want to look over this a second time to make sure I didn't miss anything but otherwise this is looking pretty good.

::: toolkit/xre/nsAppRunner.cpp
@@ +3097,5 @@
>  
>    // Handle -no-remote and -new-instance command line arguments. Setup
>    // the environment to better accommodate other components and various
>    // restart scenarios.
> +  if (CheckArg("no-remote", true) == ARG_BAD) {

You need to still store this in ar for the else-if below

@@ +3104,5 @@
>    } else if (ar == ARG_FOUND) {
>      SaveToEnv("MOZ_NO_REMOTE=1");
>    }
> +
> +  if (CheckArg("new-instance", true) == ARG_BAD) {

Ditto

@@ +3178,5 @@
> +  bool canRun = false;
> +  rv = mNativeApp->Start(&canRun);
> +  if (NS_FAILED(rv) || !canRun) {
> +    return 1;
> +  }

Why move this stuff earlier?

@@ +3180,5 @@
> +  if (NS_FAILED(rv) || !canRun) {
> +    return 1;
> +  }
> +
> +  rv = mDirProvider.Initialize(mAppData->directory, mAppData->xreDirectory);

This seems to happen twice now

Comment 10

[Jim Mathies [:jimm]]

Attachment: patch v.5 -w

> I want to look over this a second time to make sure I didn't miss anything
> but otherwise this is looking pretty good.

Ok, posting a new rev now.

> 
> ::: toolkit/xre/nsAppRunner.cpp
> @@ +3097,5 @@
> >  
> >    // Handle -no-remote and -new-instance command line arguments. Setup
> >    // the environment to better accommodate other components and various
> >    // restart scenarios.
> > +  if (CheckArg("no-remote", true) == ARG_BAD) {
> 
> You need to still store this in ar for the else-if below
> 
> @@ +3104,5 @@
> >    } else if (ar == ARG_FOUND) {
> >      SaveToEnv("MOZ_NO_REMOTE=1");
> >    }
> > +
> > +  if (CheckArg("new-instance", true) == ARG_BAD) {
> 
> Ditto

fixed.

> 
> @@ +3178,5 @@
> > +  bool canRun = false;
> > +  rv = mNativeApp->Start(&canRun);
> > +  if (NS_FAILED(rv) || !canRun) {
> > +    return 1;
> > +  }
> 
> Why move this stuff earlier?

Debugging mistake on my part, was experimenting with this in mainInit vs. mainStartup and didn't put it back in the right place. :/ Updated.

> 
> @@ +3180,5 @@
> > +  if (NS_FAILED(rv) || !canRun) {
> > +    return 1;
> > +  }
> > +
> > +  rv = mDirProvider.Initialize(mAppData->directory, mAppData->xreDirectory);
> 
> This seems to happen twice now

Removed the incorrectly placed chuck of code.

I'll fire off a new set of try builds as well with these changes.

Comment 11

[Jim Mathies [:jimm]]

Attachment: patch v.5 -w

sorry, missed the -w diff option.

Comment 12

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #11)
> Created attachment 608061 [details] [diff] [review]
> patch v.5 -w
> 
> sorry, missed the -w diff option.

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=e1005c7e270e

These changes passed try.

Comment 13

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0089fd403e76

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0089fd403e76