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

RESOLVED FIXED in mozilla1.8.1beta2

Status

()

Toolkit
Startup and Profile System
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: Mark Mentovai, Assigned: Mark Mentovai)

Tracking

({fixed1.8.1})

1.8 Branch
mozilla1.8.1beta2
PowerPC
Mac OS X
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

12 years ago
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.)
(Assignee)

Comment 1

12 years ago
Created attachment 229707 [details] [diff] [review]
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.
Attachment #229707 - Flags: first-review?(benjamin)
(Assignee)

Comment 2

12 years ago
Comment on attachment 229707 [details] [diff] [review]
Patch v1

Will have a new version shortly.
Attachment #229707 - Attachment is obsolete: true
Attachment #229707 - Flags: first-review?(benjamin)
(Assignee)

Comment 3

12 years ago
Created attachment 229745 [details] [diff] [review]
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.
Attachment #229745 - Flags: first-review?(benjamin)
(Assignee)

Updated

12 years ago
Attachment #229745 - Flags: second-review?(joshmoz)
(Assignee)

Comment 4

12 years ago
Created attachment 229771 [details] [diff] [review]
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
Attachment #229745 - Attachment is obsolete: true
Attachment #229771 - Flags: second-review?(joshmoz)
Attachment #229771 - Flags: first-review?(benjamin)
Attachment #229745 - Flags: second-review?(joshmoz)
Attachment #229745 - Flags: first-review?(benjamin)

Comment 5

12 years ago
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).
(Assignee)

Comment 6

12 years ago
(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.
(Assignee)

Comment 7

12 years ago
Created attachment 229872 [details] [diff] [review]
v4, fix MOZ_LAUNCHED_CHILD
Attachment #229771 - Attachment is obsolete: true
Attachment #229872 - Flags: second-review?(joshmoz)
Attachment #229872 - Flags: first-review?(benjamin)
Attachment #229771 - Flags: second-review?(joshmoz)
Attachment #229771 - Flags: first-review?(benjamin)

Updated

12 years ago
Attachment #229872 - Flags: first-review?(benjamin) → first-review+

Updated

12 years ago
Attachment #229872 - Flags: second-review?(joshmoz) → second-review+
(Assignee)

Comment 8

12 years ago
Checked in on the trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Summary: Improve Mac behavior through xpcom fork/exec-restart → Improve Mac launch behavior through xpcom fork/exec restart and autoupdate restart
(Assignee)

Comment 9

12 years ago
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.
Attachment #229872 - Flags: approval1.8.1?
(Assignee)

Updated

12 years ago
Blocks: 251613
(Assignee)

Updated

12 years ago
Blocks: 255873
(Assignee)

Updated

12 years ago
Blocks: 311878
(Assignee)

Updated

12 years ago
Blocks: 273731
(Assignee)

Updated

12 years ago
Blocks: 336310
Comment on attachment 229872 [details] [diff] [review]
v4, fix MOZ_LAUNCHED_CHILD

a=mconnor on behalf of drivers for 1.8.1 checkin
Attachment #229872 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 11

12 years ago
Checked in on MOZILLA_1_8_BRANCH before 1.8.1b2.
Keywords: fixed1.8.1

Updated

10 years ago
Component: XRE Startup → Startup and Profile System
QA Contact: xre.startup → startup

Updated

8 years ago
Depends on: 369147
You need to log in before you can comment on or make changes to this bug.