345057 - Improve Mac launch behavior through xpcom fork/exec restart and autoupdate restart

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Mark Mentovai]

This surely steps on other bugs (which can be marked dependent or duplicate.)

The current method of implementing xpcom restart with fork/exec has a number of problems unique to the Mac.  Specifically:

 - Platform convention is to take URLs and files to open and print, as
   requested by other applications, from Apple Events instead of the command
   line.  Apple Events live in the initial process' event queue, and are not
   available to the relaunched child.  These Apple Events should be processed
   prior to relaunch and delivered to the child.
 - When the application is launched through normal channels (LaunchServices),
   unless the specifically requested, the application is supposed to
   come to the foreground.  The child is launched using NSTask, which always
   starts its process backgrounded.  The relaunched process should come to
   the foreground automatically if its parent was in the foreground at
   relaunch time.
 - During relaunch, the dock icon does unexpected, weird, and scary things.
   It disappears (or stops bouncing and loses its "running" triangle) when
   the parent process exits.  There is no feedback that the application is
   running until the child makes its first event loop call.  A "running" dock
   icon should be put up for the child as soon as possible after relaunch
   (but not before the parent exits, to avoid duplicate dock icons - this
   should not be a problem in practice, as exit follows relaunch quickly and
   does not take longer to reach than exec overhead takes to complete.)

I can fix all of these bugs in safe ways, and would like to do so for 1.8.1.

(Cc mrbkap and sspitzer because they had asked about this a few weeks ago.)

Comment 1

[Mark Mentovai]

Attachment: Patch v1

This implements everything discussed in comment 0.

This does not handle the case when the updater launches instead of the app, because xpcom has not started at the point the updater is launched, and we can't get a thread to run the event loop.  A possible alternative might be to run a Carbon event loop - this in fact might be better, because there's no guarantee that we'll exhaust the native event pool by calling NS_ProcessPendingEvents.  The old WaitNextEvent implementation got everything confused and caused crashes.

Comment 2

[Mark Mentovai]

Comment on attachment 229707 [details] [diff] [review]
Patch v1

Will have a new version shortly.

Comment 3

[Mark Mentovai]

Attachment: Patch v2, handles startup through update

This handles startup through update by eliminating all requirements that xpcom be running when the command line is parsed.  In order to do this, I've gotten rid of NS_ProcessPendingEvents as discussed in comment 1, replacing it with a quick Carbon loop that only processes waiting Apple events.  (This is safer.)  I also needed to remove a couple of other dependencies on xpcom: code that converts FSSpecs into URL strings and code that builds an nsIURI to check its scheme has been replaced with equivalent Carbon and CoreFoundation calls.

Benjamin, I'd appreciate review from you in most of toolkit/xre.  Josh, I'd like you to look over my changes in nsCommandLineServiceMac and nsAEGetURLSuiteHandler.

Comment 4

[Mark Mentovai]

Attachment: Patch v3

Minor changes since v2:

Added release of the CFURLRef object in AddToCommandLine.

Changed method forcing dock tile to show from WaitNextEvent to ReceiveNextEvent (because WNE can cause the loop to run, which is undesirable)

Got rid of unnecessary public-visible nsCommandLineServiceMac accessors

Comment 5

[Benjamin Smedberg]

Comment on attachment 229771 [details] [diff] [review]
Patch v3

>Index: mozilla/toolkit/xre/nsAppRunner.cpp

>+  PR_SetEnv("MOZ_LAUNCHED_CHILD=1");

You never un-set this. After you're finished needing this envvar you should unset it to avoid polluting the environment, and causing problems with other apps that this process might launch (Firefox launching Thunderbird, or vice-versa).

>+  if (PR_GetEnv("MOZ_LAUNCHED_CHILD")) {
>+    // When the app relaunches, the original process exits.  This causes
>+    // the dock tile to stop bouncing, lose the "running" triangle, and

Is there a reason not to do this in all cases? Then we could skip the envvar entirely, which is really the only thing I find scary about this patch.

>+  // XPCOM shutdown can cause other things to process native events, and
>+  // native event processing can cause the waiting Apple Events to be
>+  // discarded.

hrm, do we really just discard the appleevents? perhaps we should hook appleevents up to the native event queue (for 1.9, not for 1.8).

Comment 6

[Mark Mentovai]

(In reply to comment #5)
> (From update of attachment 229771 [details] [diff] [review] [edit])
> >Index: mozilla/toolkit/xre/nsAppRunner.cpp
> 
> >+  PR_SetEnv("MOZ_LAUNCHED_CHILD=1");
> 
> You never un-set this. After you're finished needing this envvar you should
> unset it to avoid polluting the environment, and causing problems with other
> apps that this process might launch (Firefox launching Thunderbird, or
> vice-versa).

You mean in the child once launched, right?  I initially put PR_SetEnv("MOZ_LAUNCHED_CHILD="); right after the #ifdef XP_MACOSX block that uses that variable.  The reason I took it out was so that we'd get the dock icon back quickly after updater runs.  I'll put it back in for the reasons you state - we can set the variable directly in updater before relaunching the updated app.

> >+  if (PR_GetEnv("MOZ_LAUNCHED_CHILD")) {
> >+    // When the app relaunches, the original process exits.  This causes
> >+    // the dock tile to stop bouncing, lose the "running" triangle, and
> 
> Is there a reason not to do this in all cases? Then we could skip the envvar
> entirely, which is really the only thing I find scary about this patch.

Yes.  The call I make to force the dock tile to show causes the tile to stop bouncing.  I want to let the tile bounce until we hit the event loop for real the first time around.

> >+  // XPCOM shutdown can cause other things to process native events, and
> >+  // native event processing can cause the waiting Apple Events to be
> >+  // discarded.
> 
> hrm, do we really just discard the appleevents? perhaps we should hook
> appleevents up to the native event queue (for 1.9, not for 1.8).

The Apple Events themselves are processed on the native loop, but their handlers are not installed until nsCommandLineServiceMac installs them.  If the call is made too late, the event loop that spins during xpcom shutdown dispatches the Apple Event without the handlers being ready to catch them, so they'll go unprocessed.

Comment 7

[Mark Mentovai]

Attachment: v4, fix MOZ_LAUNCHED_CHILD

Comment 8

[Mark Mentovai]

Checked in on the trunk.

Comment 9

[Mark Mentovai]

Comment on attachment 229872 [details] [diff] [review]
v4, fix MOZ_LAUNCHED_CHILD

I began discussing this with mconnor for 1.8.1 - this fixes a large number of annoying bugs.

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 229872 [details] [diff] [review]
v4, fix MOZ_LAUNCHED_CHILD

a=mconnor on behalf of drivers for 1.8.1 checkin

Comment 11

[Mark Mentovai]

Checked in on MOZILLA_1_8_BRANCH before 1.8.1b2.