469990 - -compose does not work on Mac OS X when TB is already running

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P3)

Description

[hans.mustermann]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4
Build Identifier: Thunderbird 3.0b2pre

dhcp-efra05-32-175:~ micha$ /Applications/Shredder.app/Contents/MacOS/thunderbird-bin -compose [e-mail]
dhcp-efra05-32-175:~ micha$ ps -ef | grep thunder
  501   377   352   0   0:27.08 ttys000    1:06.57 /Applications/Shredder.app/Contents/MacOS/thunderbird-bin
  501   413   352   0   0:00.00 ttys000    0:00.00 grep thunder
dhcp-efra05-32-175:~ micha$ 
dhcp-efra05-32-175:~ micha$ /Applications/Shredder.app/Contents/MacOS/thunderbird-bin -compose 

this exercise results in the error message "A copy of Thunderbird is already open. ..."

However according to documentation a compose window should open.
Thus (I assume) it is also not possible to launch an email directly from StarOffice. This is the problem I started with.

I also tried this with the stable version 2.0.0.18

Reproducible: Always

Steps to Reproduce:
1. start thunderbird on Mac
2. try to start it again with the option -compose
3.
Actual Results:  
error message "A copy of Thunderbird is already open. ..."

Expected Results:  
Start of a compose window

Comment 1

[Wayne Mery (:wsmwk)]

hans, what happens if you use 

/Applications/Shredder.app/Contents/MacOS/thunderbird-bin -compose
to:[e-mail]

note the addition of "to:"

xref Bug 435563 -  compose command line command returns non-zero if thunderbird is running ... where it works in windows bug return code is unexpected

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Using to: doesn't help with trunk builds.  Adding qawanted, as it would be good to test trunk with the current OpenOffice builds and other Mac software that wants to integrate with Thunderbird.  If it turns out to still be a problem, please nominate to block.

Comment 3

[Nomis101]

For exactly the same problem with -remote and -edit on Thunderbird and SeaMonkey, see Bug 472891.

Comment 4

[Wayne Mery (:wsmwk)]

Metzger says "Adding "to:" does not change anything."

Comment 5

[marc]

Has this bug been included in https://bugzilla.mozilla.org/show_bug.cgi?id=287345 or is it still standing alone and unassigned? It would appear that the same behavior is apparent in the Intel architecture and is a major impediment to have TBird be an effective MTA on SnowLeopard

Comment 6

[Robin Wöhler]

This problem still exists on MacOS 10.6.7 using Thunderbird 5.0b2.

Comment 8

[thunderbird-bugs.even]

So is anything happening with this issue?

Comment 9

[Keith White]

Please can this be changed to Intel Architecture and update TB version to latest available as unresolved in latest Thunderbird 10.0.2

Comment 10

[masch82]

This problem still is present - Mac OS X (10.7.4) , Thunderbird 14.0
I suspect something going wrong in the last part of the 
"static nsresult SelectProfile" function in "nsAppRunner.cpp" that doesn't happen on the other platforms.
However I have yet to determine the exact cause ...

Comment 11

[masch82]

If I remove the checks for profile locking in the function
> nsresult nsProfileLock::LockWithFcntl(nsIFile *aLockFile)
in the File "nsProfileLock.cpp" it kind of works - Thunderbird can then handle the -compose commandline argument correctly and opens a new message window with the predefined values (to field, attachments etc.). However this is no real solution because it just opens a second thunderbird process (ignoring the already used profile lock) and then handles the command line args. Meaning that after sending the message the process still stays open on OS X.
This is in contrast to e.g. Windows where only one process gets used. So I suppose the IPC does not happen correctly and TB on OSX can't determine the other running instance correctly or the (XPCOM ?) communication between the processes does not work correctly.
I don't expect to find the cause anytime soon, so if someone who knows the code better could look into this it would be cool.

Comment 12

[Pavel Volkovitskiy]

Thunderbird 24.4.0 on MacOSX 10.9.2 (intel)

Still doesn't work. Could someone update bug's platform and version?

Comment 13

[Tom]

Thunderbird 31.2.0 on OS X 10.9.5. Same issue, and it completely prevents me from being able to reliably pop up a message compose window from my application. I can support almost every other mail client; just not Thunderbird.
How can we update the TB version number in this bug? Would like to clarify that it still exists against current platform/version.

Comment 14

[marc]

Running TBird 31.2.0 on OS 10.9.5
Clicking a mailto link from Firefox, Chrome and Acrobat Pro 11 results in a Tbird message window.
This happens whether Tbird is open or closed.
So, there appear to be complications...

Comment 15

[Wayne Mery (:wsmwk)]

(In reply to Tom from comment #13)
>...
> How can we update the TB version number in this bug? Would like to clarify
> that it still exists against current platform/version.

We typically want the version set to the first version that has the problem. So there is no reason to change it.

Comment 16

[thunderbird-bugs.even]

after 6 years it seems unlikely they care about the platform or version it affects, they are just going to carry on ignoring the bug.

Comment 17

[Magnus Melin [:mkmelin]]

Patches welcome. Likely needs some changes in this file - http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/xre/nsCommandLineServiceMac.cpp

Comment 20

[Javi Rueda (no longer involved in Mozilla projects)]

Bug is still reproducible on x86 hardware.

Comment 21

[jh_github]

Bug is still reproducible in Thunderbird 52.2.1 (64-Bit)

It's a somehow bad that this Bug is already 9 Years old!

We would like to have an option to open an compose window with a pre defined Message and Attachment from our Application. Since this is not possible with Thunderbird we recommend our customers to use Apple Mail or to use Postbox on Mac.

Comment 22

[Yuri Z.]

I am currently working on the patch for this issue and should have it ready in a couple of days.
Basically, what is required is adding an IPC for "mozilla remote" - it was not implemented on Mac, most likely due to Mac users not being used to launching apps from command line - similar to how it is implemented on Windows (that one uses Window messages to notify the already running instance of the passed command line arguments when a new process instance is attempted to be launched).
Any chance anybody could assign me to this bug (or is there another way to submit the patch?)

Comment 23

[Wayne Mery (:wsmwk)]

Thanks for volunteering.  Jorg can advise on submitting a patch.

Comment 24

[Jorg K (CEST = GMT+2)]

I don't know what to say, just attach a patch here that applies to current trunk and we'll take it from there. I never read
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch ;-)

Comment 25

[Yuri Z.]

Attachment: Fix for the issue

Attaching the patch here. Commented it, so those unfamiliar with macOS development could read it easier. I am not completely sure about the correct handling of the -no-remote/MOZ_NO_REMOTE=1, -foreground, -headless and perhaps some other arguments that I missed that should alter the logic described below. 

In short, the current logic is as follows:
The first instance of the process upon startup registers a notification handler. All other instances, having detected this already running instance, broadcast a notification, passing their command line arguments as part of the notification, to this first instance, and then they just quit. The original running instance processes these arguments in its notification callback, and brings itself to front.

This is unless -no-remote or -headless have been specified - in which case this notification mechanism gets bypassed completely, and multiple instances are allowed to run (provided the user also specifies different profile directories - otherwise he will just get a message box "Only one instance of the <productname> can be run at a time").


Tested with Firefox and Thunderbird. 
On Thunderbird, -compose, -mail and other remote arguments should now be working correctly.

To verify the fix, launch Thunderbird, then launch another instance like so:

open -n <Path to Thunderbird.app> --args -compose

or directly by running a binary:

<Path to Thunderbird.app>/Contents/MacOS/thunderbird -compose

Please let me know if the start up logic or anything else should be adjusted or whether I missed any important use cases.

Comment 26

[Jorg K (CEST = GMT+2)]

Thanks for the patch, it's a toolkit patch, so we need a Mozilla core reviewer. They will complain about the coding style:
- no trailing spaces.
- comments like this: // This is a comment.
- lines usually no longer then 80 characters.

Hmm, no Toolkit::XRE component, so let's try Core::Widget Cocoa. Stephen, who could move this forward?

Comment 27

[Yuri Z.]

Attachment: Updated to comply to coding guidelines

Thanks Jorg, I updated the patch, trying to comply to coding guidelines.

Comment 28

[Jorg K (CEST = GMT+2)]

Oh, much better now. Still some trailing spaces, but they don't matter for now. Right now we need to find the right component for the bug and a suitable reviewer. Thanks a lot for your work!

Comment 29

[Yuri Z.]

Attachment: Fix - Cleaner formatting

Comment 30

[Stephen A Pohl [:spohl]]

Comment on attachment 9028264 [details] [diff] [review]
Fix - Cleaner formatting

Adding to my review queue.

Comment 31

[Stephen A Pohl [:spohl]]

Comment on attachment 9028264 [details] [diff] [review]
Fix - Cleaner formatting

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

You need to change your hgrc file to include 8 lines before and after a diff. This should get configured automatically and stored permanently for you when you run `./mach bootstrap` in Terminal.

This looks great at first glance. I will take one more look once my initial comments have been addressed. Please try to apply the corrections to formatting across the entire patch, even if they have only been called out once (such as spacing etc.). Thanks!

::: toolkit/xre/nsNativeAppSupportCocoa.mm
@@ +56,4 @@
>    return NS_OK;
>  }
>  
> +//  Essentially this notification handler implements the

nit: Remove one space before "Essentially"

@@ +63,5 @@
> +// just broadcast this notification and quit (unless -no-remote was specified in
> +// either of these processes), making the original process handle the arguments
> +// passed in this handler.
> +void
> +remoteClientNotificationCallback (CFNotificationCenterRef center,

nit: No space between function name and opening parenthesis.
nit: Function arguments should start with `a`, so `aCenter`, `aObserver` etc. This file is all over the place when it comes to naming arguments, so we might as well just do it right for new functions.

@@ +64,5 @@
> +// either of these processes), making the original process handle the arguments
> +// passed in this handler.
> +void
> +remoteClientNotificationCallback (CFNotificationCenterRef center,
> +                                  void * observer, CFStringRef name,

nit: No space after the type. So: `void* observer`. This is true throughout this file.

@@ +69,5 @@
> +                                  const void * object,
> +                                  CFDictionaryRef userInfo)
> +{
> +  // Autorelease pool to prevent memory leaks, in case there is no outer pool.
> +  @autoreleasepool {

We tend to avoid this notation. You can include MacAutoReleasePool.h and then instantiate a pool like so: `MacAutoreleasePool pool;`. See https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/MacLaunchHelper.mm#24 for an example.

@@ +80,5 @@
> +      // which nsICommandLineRunner understands.
> +      int argc = [args count];
> +      char **argv = NULL;
> +      argv = new char*[argc];
> +      for(int i=0; i<argc; i++) {

nit: Space between `for` and opening parenthesis.
nit: Space between variables and operators, so `i = 0`, `i < argc` etc.

@@ +82,5 @@
> +      char **argv = NULL;
> +      argv = new char*[argc];
> +      for(int i=0; i<argc; i++) {
> +        const char *arg = [[args objectAtIndex:i]
> +                           cStringUsingEncoding:NSUTF8StringEncoding];

nit: This needs to be indented by one more space.

@@ +111,5 @@
> +      // And bring the app's windows to front (do we need to always bring it to front
> +      // when called by a remote process, or do it based on some combination of the
> +      // arguments passed (-foreground?)?).
> +      [[NSRunningApplication currentApplication] activateWithOptions:
> +       (NSApplicationActivateAllWindows | NSApplicationActivateIgnoringOtherApps)];

nit: Indent by one more space.

@@ +158,5 @@
>    *_retval = true;
> +
> +  // What "special" CLI arguments can we expect to be passed - that should alter
> +  // the default "hand args list to remote process and quit" algorithm?
> +  // -headless - was already handled on macOS (allowing running multiple instances

nit: It would be clearer to separate the argument from the description using a colon `:` rather than a dash `-` in this comment, since dashes are preceding each argument name.

@@ +159,5 @@
> +
> +  // What "special" CLI arguments can we expect to be passed - that should alter
> +  // the default "hand args list to remote process and quit" algorithm?
> +  // -headless - was already handled on macOS (allowing running multiple instances
> +  // of the app), meaning this patch shouldn't break it

nit: period at the end of the sentence.

@@ +162,5 @@
> +  // -headless - was already handled on macOS (allowing running multiple instances
> +  // of the app), meaning this patch shouldn't break it
> +  // -no-remote - should always proceed, creating a second instance (which will
> +  // fail on macOS, showing a MessageBox "Only one instance can be run at a time",
> +  // unless a different profile dir path is specified)

nit: period at end of the sentence.

@@ +165,5 @@
> +  // fail on macOS, showing a MessageBox "Only one instance can be run at a time",
> +  // unless a different profile dir path is specified)
> +  // -foreground - make this instance be the active application - looks like it
> +  // actually does nothing on other systems? - on macOS the window of the original
> +  // remote process always becomes foreground

nit: period at the end of the sentence.

@@ +173,5 @@
> +
> +  // Note, that perhaps there are other arguments or environment variables, that
> +  // should alter our "handle args list to remote process and quit" behaviour -
> +  // Mozilla developers can give some insight into that?
> +  @autoreleasepool {

See comment above about autorelease pools.

@@ +185,5 @@
> +    }
> +
> +    // Apart from -no-remote, the user can specify an env variable
> +    // MOZ_NO_REMOTE=1, which makes it behave the same way.
> +    NSDictionary *environmentVariables = [[NSProcessInfo processInfo] environment];

We should only perform this check if shallProceedLikeNoRemote is still == NO.

@@ +230,5 @@
> +      // so we do not pass it on to perhaps another running Thunderbird by another
> +      // logged in user. Distributed notifications is the best candidate
> +      // (while darwin notifications ignore the user context).
> +      NSString *notificationName = [[[NSBundle mainBundle] bundleIdentifier]
> +                                    stringByAppendingString:

nit: fix indent

@@ +247,5 @@
> +      // In case future instances would want to notify us about command line arguments
> +      // passed to them
> +      CFNotificationCenterRef center = CFNotificationCenterGetDistributedCenter();
> +      NSString *notificationName = [[[NSBundle mainBundle] bundleIdentifier]
> +                                    stringByAppendingString:

nit: fix indent

Comment 32

[Yuri Z.]

Attachment: cleaned up formatting

Thanks for looking into this. I tried to fix all the commented formatting flaws. 
Except for this one:

>You need to change your hgrc file to include 8 lines before and after a diff. This should get configured automatically and stored permanently for you when you run `./mach bootstrap` in Terminal.

It looks like ./mach bootstrap doesn't set this on Mac, even though I answer Yes to all hg questions when the script runs. 

Do I understand correctly that you just require 8 empty lines in the beginning and in the end of the .patch file? I was unable to find the .hgrc param that would allow me to set this - could you please advise? Otherwise I think I could just add these lines manually, if my understanding of the request is correct.

Comment 33

[Yuri Z.]

Looks like these are other dupes of this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=834660
https://bugzilla.mozilla.org/show_bug.cgi?id=981665

Comment 34

[Yuri Z.]

Attachment: 7-line unified patch

an update addressing this:
>You need to change your hgrc file to include 8 lines before and after a diff. This should get configured automatically and stored permanently for you when you run `./mach bootstrap` in Terminal.

Comment 35

[Magnus Melin [:mkmelin]]

(In reply to Yuri from comment #32)
> Do I understand correctly that you just require 8 empty lines in the
> beginning and in the end of the .patch file? 

At least 8 line context, not empty lines. In your hgrc, put

[defaults]
diff = -p -U 8

Comment 36

[Yuri Z.]

Magnus, got it, thanks. 
Please see my last attachment.

Comment 37

[Yuri Z.]

Attachment: patch generated with "diff = -p -U 8"

Stephen, can you please re-review the patch? Thank you.

Comment 38

[Yuri Z.]

A kind ping :)

Comment 39

[Stephen A Pohl [:spohl]]

Comment on attachment 9028619 [details] [diff] [review]
patch generated with "diff = -p -U 8"

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

Almost there. Thanks!

::: toolkit/xre/nsNativeAppSupportCocoa.mm
@@ +24,5 @@
>  #include "nsIServiceManager.h"
>  #include "nsIWebNavigation.h"
>  #include "nsIWidget.h"
>  #include "nsIWindowMediator.h"
> +#include "MacAutoReleasePool.h";

Please keep this in alphabetical order.

@@ +83,5 @@
> +    char** argv = NULL;
> +    argv = new char* [argc];
> +    for (int i = 0; i < argc; i++) {
> +      const char* arg = [[args objectAtIndex:i]
> +                          cStringUsingEncoding:NSUTF8StringEncoding];

Use NSString's UTF8String instead.

@@ +85,5 @@
> +    for (int i = 0; i < argc; i++) {
> +      const char* arg = [[args objectAtIndex:i]
> +                          cStringUsingEncoding:NSUTF8StringEncoding];
> +      argv[i] = new char[ strlen(arg) + 1 ];
> +      strcpy(argv[i], arg);

strcpy is unsafe to use. But since you don't seem to be using the C-strings outside of the current memory context, there shouldn't be a need to copy the strings anyway, so this can be removed.

@@ +94,5 @@
> +    nsresult rv = cmdLine->Init([args count], argv, nullptr,
> +                                nsICommandLine::STATE_REMOTE_AUTO);
> +
> +    // Cleaning up C arrays
> +    while ( argc ) {

nit: no space before and after `argc`, but this should go away anyway once we don't copy strings anymore.

@@ +108,5 @@
> +    // Processing the command line, passed from a remote instance
> +    // in the current instance.
> +    cmdLine->Run();
> +
> +    // And bring the app's windows to front (do we need to always bring it to front

It's probably safe to assume that we should always bring the app to the front. If we get reports of a use-case that doesn't fit with this we can adjust at that time.

@@ +112,5 @@
> +    // And bring the app's windows to front (do we need to always bring it to front
> +    // when called by a remote process, or do it based on some combination of the
> +    // arguments passed (-foreground?)?).
> +    [[NSRunningApplication currentApplication] activateWithOptions:
> +      (NSApplicationActivateAllWindows | NSApplicationActivateIgnoringOtherApps)];

Why did you choose to pass these flags?

@@ +156,5 @@
>    }
>  
>    *_retval = true;
> +
> +  // What "special" CLI arguments can we expect to be passed, that should alter

nit: We should rephrase this first sentence and instead of a question, make it a statement about which options can be expected. If there are options that we may not know about, we should say so as well.

@@ +159,5 @@
> +
> +  // What "special" CLI arguments can we expect to be passed, that should alter
> +  // the default "hand args list to remote process and quit" algorithm?
> +  // -headless : was already handled on macOS (allowing running multiple instances
> +  // of the app), meaning this patch shouldn't break it.

nit: Don't refer to "this patch" or "was already handled on macOS" since this will become meaningless as soon as this patch has landed. Rather, refer to it like "we should ensure that this continues to work" or something similar that is self-explanatory when reading nsNativeAppSupportCocoa.mm.

@@ +166,5 @@
> +  // unless a different profile dir path is specified).
> +  // -foreground : make this instance be the active application. Looks like it
> +  // actually does nothing on other systems? On macOS the window of the original
> +  // remote process always becomes foreground.
> +  // The rest of the arguments, to my understanding, should be either passed on to

nit: Try not to refer to yourself in this comment for the same reason as above. It isn't obvious who you are from just reading this file. Something like this could work:
"It is believed that all other arguments should either be passed on to the original running process (exiting the current process), or be processed by the current process (if -no-remote is specified)."

@@ +170,5 @@
> +  // The rest of the arguments, to my understanding, should be either passed on to
> +  // the original running process (exiting the current process), or be processed by
> +  // the current process (if -no-remote is specified).
> +
> +  // Note, that perhaps there are other arguments or environment variables, that

I'm not aware of any such arguments and I'm not sure who could give more insight. We should remove this comment and adjust things if/when we start getting feedback from users.

@@ +214,5 @@
> +
> +  // So, let's check if this is the first instance ever of the process for the
> +  // current user.
> +  if ([[NSRunningApplication runningApplicationsWithBundleIdentifier:
> +       [[NSBundle mainBundle] bundleIdentifier]] count] > 1) {

nit: one more space before `[[NSBundle ...`

@@ +231,5 @@
> +    // We also need to make sure the notifications are "local" to the current user,
> +    // so we do not pass it on to perhaps another running Thunderbird by another
> +    // logged in user. Distributed notifications is the best candidate
> +    // (while darwin notifications ignore the user context).
> +    NSString* notificationName = [[[NSBundle mainBundle] bundleIdentifier]

Create this string once before the `if`/`else` statements to be used both inside the `if` and `else` block.

@@ +233,5 @@
> +    // logged in user. Distributed notifications is the best candidate
> +    // (while darwin notifications ignore the user context).
> +    NSString* notificationName = [[[NSBundle mainBundle] bundleIdentifier]
> +                                   stringByAppendingString:
> +                                  @".distributedNotification.commandLineArgs"];

nit: Add three spaces of indent.

@@ +234,5 @@
> +    // (while darwin notifications ignore the user context).
> +    NSString* notificationName = [[[NSBundle mainBundle] bundleIdentifier]
> +                                   stringByAppendingString:
> +                                  @".distributedNotification.commandLineArgs"];
> +    CFNotificationCenterPostNotification(center,

nit: Simply pass CFNotificationCenterGetDistributedCenter() directly here.

@@ +250,5 @@
> +    // passed to them
> +    CFNotificationCenterRef center = CFNotificationCenterGetDistributedCenter();
> +    NSString* notificationName = [[[NSBundle mainBundle] bundleIdentifier]
> +                                   stringByAppendingString:
> +                                  @".distributedNotification.commandLineArgs"];

nit: Add three spaces of indent.

@@ +251,5 @@
> +    CFNotificationCenterRef center = CFNotificationCenterGetDistributedCenter();
> +    NSString* notificationName = [[[NSBundle mainBundle] bundleIdentifier]
> +                                   stringByAppendingString:
> +                                  @".distributedNotification.commandLineArgs"];
> +    CFNotificationCenterAddObserver(center,

nit: Simply pass CFNotificationCenterGetDistributedCenter() directly here.

Comment 40

[Yuri Z.]

Attachment: Code review Iteration fixes

The arguments passed to activateWithOptions are chosen based on my understanding of the desired user experience: we should activate the other instance of the application even if there is some other app currently active (this is what "ignoring other applications" stands for) and we should bring all opened windows to front, not just the dialog with current keyboard focus (I believe the user would expect the whole application activated).

Comment 41

[Stephen A Pohl [:spohl]]

(In reply to Yuri from comment #40)
> Created attachment 9029503 [details] [diff] [review]
> Code review Iteration fixes
> 
> The arguments passed to activateWithOptions are chosen based on my
> understanding of the desired user experience: we should activate the other
> instance of the application even if there is some other app currently active
> (this is what "ignoring other applications" stands for)

This flag means that if the user has activated another app *after* the current app was deactivated and the notification sent but before the other app became active (for example because there was a delay in sending or processing the notification), then the application still gets activated and essentially steals focus away from the app that the user selected. We shouldn't do this. (See https://developer.apple.com/documentation/appkit/nsapplicationactivationoptions/nsapplicationactivateignoringotherapps?language=objc)

> and we should bring
> all opened windows to front, not just the dialog with current keyboard focus
> (I believe the user would expect the whole application activated).

Without the NSApplicationActivateAllWindows, the main and key window become active. This should be enough, but feel free to provide a screenshot where this is demonstrated to be confusing for a user.

Comment 42

[Yuri Z.]

Attachment: workingMacRemote.patch

Changed the activation policy per your request.

Comment 43

[Stephen A Pohl [:spohl]]

Comment on attachment 9029514 [details] [diff] [review]
workingMacRemote.patch

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

::: toolkit/xre/nsNativeAppSupportCocoa.mm
@@ +8,5 @@
>  #import <CoreServices/CoreServices.h>
>  #import <Cocoa/Cocoa.h>
>  
> +#include "MacAutoReleasePool.h";
> +

nit: no empty line here.

@@ +81,5 @@
> +    // Converting Objective-C array into a C array,
> +    // which nsICommandLineRunner understands.
> +    int argc = [args count];
> +    const char** argv = NULL;
> +    argv = new const char* [argc];

This line can be combined with the previous line. Also:
nit: no space between `char*` and `[argc]`

@@ +88,5 @@
> +      argv[i] = arg;
> +    }
> +
> +    // We can pass aWorkingDir of the calling process as a third argument.
> +    // Is it needed?

Rephrase this comment to say that we're not currently passing the working dir as third argument because it does not appear to be required.

@@ +89,5 @@
> +    }
> +
> +    // We can pass aWorkingDir of the calling process as a third argument.
> +    // Is it needed?
> +    nsresult rv = cmdLine->Init([args count], argv, nullptr,

Pass `argc` instead of `[args count]` here.

@@ +93,5 @@
> +    nsresult rv = cmdLine->Init([args count], argv, nullptr,
> +                                nsICommandLine::STATE_REMOTE_AUTO);
> +
> +    // Cleaning up C array.
> +    delete [] argv;

nit: no space between `delete` and `[]`.

@@ +104,5 @@
> +    // Processing the command line, passed from a remote instance
> +    // in the current instance.
> +    cmdLine->Run();
> +
> +    // And bring the app's windows to front.

nit: the singular `window` may be more accurate here in most instances.

@@ +149,5 @@
>    }
>  
>    *_retval = true;
> +
> +  // Here are the "special" CLI arguments can we expect to be passed, that should alter

nit: "... that we can expect ..."

Comment 44

[Yuri Z.]

Attachment: workingMacRemote.patch

Hopefully this should be it?

Comment 45

[Stephen A Pohl [:spohl]]

Comment on attachment 9029710 [details] [diff] [review]
workingMacRemote.patch

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

Thank you! This is ready to land with this last nit addressed.

Reviewing our coding style guidelines will speed up future reviews:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
https://google.github.io/styleguide/cppguide.html

::: toolkit/xre/nsNativeAppSupportCocoa.mm
@@ +86,5 @@
> +      argv[i] = arg;
> +    }
> +
> +    // We're not currently passing the working dir as third argument because it
> +    // does not appear to be required

nit: Period at end of sentence.

Comment 46

[Yuri Z.]

Attachment: workingMacRemote.patch

Updated

Comment 47

[Stephen A Pohl [:spohl]]

Attachment: Patch

Great. The only thing that was still missing was the commit message. I have added one for you. Please review and make sure that your name and email address appear as you'd expect. I will go ahead and land once I hear back from you.

Comment 48

[Yuri Z.]

Looks good, thanks Stephen

Comment 49

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc31dfbf52cf56c918c51e30fdc22a47c8cec7bd

Comment 50

[Jorg K (CEST = GMT+2)]

Stephen, I don't know how picky we want to be here, but there are still a lot of punctuation and other issues: 

+// Essentially this notification handler implements the
+// "mozilla remote" functionality on Mac, handing command line arguments, passed
+// to a newly launched process, _or?_ to another copy of the process, _lose comma_ that was already running
+// (which had registered the handler for this notification). All other new copies
+// just broadcast this notification and quit (unless -no-remote was specified in
+// either of these processes), making the original process handle the arguments
+// passed in this handler.

+  // Now that we have handled no-remote-like arguments, at this point
+  // 1) Either only the first instance of the process has been launched in any way
+  // (.app double click, "open", "open -n", invoking executable in Terminal, etc
+  // 2) Or the process has been launched with a "macos single instance" mechanism
+  // override (using "open -n" OR directly by invoking the executable in Terminal
+  // instead of clicking the .app bundle's icon, etc).
The stuff after 1) and 2) should be indented. The abbreviation etc. typically has a full stop, as in etc.

+    // This is the first instance ever! Let's register a notification listener here,
+    // In case future instances would want to notify us about command line arguments
+    // passed to them
Uppercase after comma, missing full stop.

There's more, like various times a comma before "that", Firefox is also spelled firefox, etc.

Comment 51

[Stephen A Pohl [:spohl]]

We have to wait for the try builds in comment 49 to come back green, so if Yuri would like to make the additional changes mentioned in comment 50 before we land, that would be great. Thanks!

Comment 52

[Stephen A Pohl [:spohl]]

The build errors will need to be resolved before we can land:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc31dfbf52cf56c918c51e30fdc22a47c8cec7bd

We also need to make sure that none of the test failures are caused by this patch.

Comment 53

[Yuri Z.]

Attachment: workingMacRemote.patch

Hopefully the build failure should be fixed (assuming the header couldn't be found due to a semicolon). 
Also updated the comments per your request.

Comment 54

[Yuri Z.]

if this was not the reason for the other error during build - do you know why "./mach build" completes without errors, while the Try Server is unable to find the MacAutoReleasePool.h header? Are there different include directory paths used when building with these two methods?

Comment 55

[Stephen A Pohl [:spohl]]

(In reply to Yuri from comment #54)
> if this was not the reason for the other error during build - do you know
> why "./mach build" completes without errors, while the Try Server is unable
> to find the MacAutoReleasePool.h header? Are there different include
> directory paths used when building with these two methods?

Besides the semicolon I believe it was because "Autorelease" is spelled with a lower case 'r'. Will push to try again shortly to confirm.

Comment 56

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0c0bba7ded7913c02bb14a7334d62aa90d853fd

Comment 57

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b209b473ad300a00390099718fc822137158b781

Comment 58

[Yuri Z.]

I believe MacAutoReleasePool.h is spelled correctly, unless the filename changed since I checked out the code last time...

Comment 59

[Stephen A Pohl [:spohl]]

(In reply to Yuri from comment #58)
> I believe MacAutoReleasePool.h is spelled correctly, unless the filename
> changed since I checked out the code last time...

It has been spelled with a lower case 'r' since it was checked in back in 2010:

https://hg.mozilla.org/mozilla-central/filelog/c2593a3058afdfeaac5c990e18794ee8257afe99/toolkit/xre/MacAutoreleasePool.h

Things are building properly on try now.

Comment 60

[Yuri Z.]

Attachment: workingMacRemote.patch

I see. My bad. 
In any case, updated it in case you prefer it to be done right.

Comment 61

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd4fa70b72870a1e8a6c7e39b3904202d7bcd40
Bug 469990: Allow command line arguments to be handed off from a new Firefox/Thunderbird process to an existing one when necessary. r=spohl

Comment 62

[Stephen A Pohl [:spohl]]

Attachment: Patch

As landed.

Comment 63

[Yuri Z.]

Thanks Stephen.

I believe the patch should go through other approvals (QA?) now? Do we need to set checkin-needed keyword?
Also if I may ask - how much time does it usually take for the fix to propagate to a release build?

Comment 64

[Stephen A Pohl [:spohl]]

(In reply to Yuri from comment #63)
> Thanks Stephen.
> 
> I believe the patch should go through other approvals (QA?) now? Do we need
> to set checkin-needed keyword?
> Also if I may ask - how much time does it usually take for the fix to
> propagate to a release build?

I have already checked the patch in, so the checkin-needed keyword isn't necessary. You would have set that keyword if you didn't have permissions to commit to mozilla-central and you couldn't find someone to commit the changes for you.

The changes will show up in tomorrow's Firefox Nightly build. Thunderbird will need to merge the patch to make the changes appear there. Then, the changes will ride the Nightly/Beta/Release train as specified here: https://wiki.mozilla.org/Release_Management/Calendar

Can you think of a specific way to test this change in Firefox? If not, we should make sure to test the changes once they've landed in Thunderbird (specifically the "-compose" command line argument).

Comment 65

[Yuri Z.]

Firefox, when launched as a second instance, passes its arguments to the original instance (after applying this change) and activates it - unless -no-remote/-headless are specified. So, as a test one can launch firefox -new-window or something similar to check out if the existing instance opens a new window or not.

As I recall, without this change the second instance of Firefox just shows a message "Only one instance can be run at a time", similarly to Thunderbird.

Comment 66

[Jorg K (CEST = GMT+2)]

(In reply to Stephen A Pohl [:spohl] from comment #64)
> Thunderbird will need to merge the patch to make the changes appear there.
Yes?? This is a toolkit change, so why would we need to do anything? Should also appear in tomorrows Daily build. Sure, to backport to - say - TB 60 we'd need a merge.

Comment 67

[Stephen A Pohl [:spohl]]

Adding qe-verify to get some verification here. To test with Firefox, launch a first instance of Firefox and then launch a second instance of Firefox from Terminal in the following two ways:

$ Firefox.app/Contents/MacOS/firefox --new-window www.mozilla.org
$ Firefox.app/Contents/MacOS/firefox --no-remote --new-window www.mozilla.org

The first call should open a new window with a URL of www.mozilla.org in the currently running instance of Firefox.
The second call should display an error message stating that an instance of Firefox is already running.

Comment 68

[Stephen A Pohl [:spohl]]

(In reply to Jorg K (GMT+1) from comment #66)
> (In reply to Stephen A Pohl [:spohl] from comment #64)
> > Thunderbird will need to merge the patch to make the changes appear there.
> Yes?? This is a toolkit change, so why would we need to do anything? Should
> also appear in tomorrows Daily build. Sure, to backport to - say - TB 60
> we'd need a merge.

I stand corrected. Thanks!

Comment 69

[marc]

Attachment: Error Message

Ran commmand from terminal with preceeding /Application/ with the result shown

Comment 70

[marc]

Attachment: Error Message

Ran other command from terminal with preceding /Applications/ and produced this error message.

Both commands  run on High Sierra with a copy of Firefox 63.0.3 (64 bit) running.

Comment 71

[Stephen A Pohl [:spohl]]

You have to wait for this patch to merge to mozilla-central, and then for the next Nightly build to appear. This would be tomorrow morning, at the earliest.

Comment 72

[Yuri Z.]

I suggest also to verify that Firefox doesn't send notifications to Thunderbird and vice verse:
when Firefox (build containing this change) is launched, updated Thunderbird should launch fine as well, and duplicate instances of the processes should control each its own original process.

Comment 73

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/7bd4fa70b728

Comment 74

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/590bdef6b3123a2c7e9676fb88a4b6b65f344d15
Bug 469990: Backout cset 7bd4fa70b728 for causing a large number of regressions (bug 1513341, bug 1513328, bug 1513335, bug 1513336). r=me, a=ryanvm

Comment 75

[Stephen A Pohl [:spohl]]

Let's get this backed out of beta as well. Thanks!

Comment 76

[Stephen A Pohl [:spohl]]

Yuri, this patch has caused a number of unexpected regressions. You can review bug 1513328, bug 1513335 and bug 1513336 for the various issues that we'll need to resolve before we can land this again.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/978402f68f3c

Comment 78

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/590bdef6b312

Comment 79

[Yuri Z.]

Stephen, I can see that the fix broke autoupdate mechanism, which seems to rely on two instances running simultaneously. Are these three the only regressions occurring or there were others?
I am wondering what other mechanisms may rely on multiple running instances that need to be tested/verified (perhaps, based on some specific command line arguments?)? As you can see from the patch, I am currently only respecting the -no-remote/-headless arguments - otherwise I am not allowing running two instances at the same time.

As an easy (not the best, but reliable) solution to these regressions above for macOS we may consider looking for a new command line argument, say "-force-remote", and allowing remote control only if this argument is found.

Comment 80

[Stephen A Pohl [:spohl]]

(In reply to Yuri from comment #79)

Stephen, I can see that the fix broke autoupdate mechanism, which seems to rely on two instances running simultaneously. Are these three the only regressions occurring or there were others?

Those were the only ones we became aware of. There may be others, so we will need to keep an eye out for new issues once this lands again.

I am wondering what other mechanisms may rely on multiple running instances that need to be tested/verified (perhaps, based on some specific command line arguments?)? As you can see from the patch, I am currently only respecting the -no-remote/-headless arguments - otherwise I am not allowing running two instances at the same time.

As an easy (not the best, but reliable) solution to these regressions above for macOS we may consider looking for a new command line argument, say "-force-remote", and allowing remote control only if this argument is found.

This may or may not be necessary. It would be good to collect the specific arguments that are being used in these instances and see if there is a way to make an exception for these as-is. If not, and additional argument would seem to make sense.

Note that for bug 1513335, we may actually want that behavior to match Windows and Linux, but we should focus the currently running instance of Firefox.

Comment 81

[Yuri Z.]

Attachment: fixv2.patch

Hello Stephen,

The auto-update mechanism should be fixed. I am detecting the update-restart event by checking the MOZ_APP_RESTART=1 environment variable.

Note that for bug 1513335, we may actually want that behavior to match Windows and Linux, but we should focus the currently running instance of Firefox.

Updated the logic so that -no-remote argument (implicitly used by ./mach run) now defines behavior only for duplicate processes, not the original one(s).
I am not activating ignoring other apps though, since you mentioned earlier this would not the desired behaviour.

https://bugzilla.mozilla.org/show_bug.cgi?id=1513336

Please confirm, that this is not happening anymore.
Not sure if this was introduced by our fix.

Comment 82

[Yuri Z.]

Ping

Comment 83

[Stephen A Pohl [:spohl]]

This didn't fall off my radar, but I've been sidetracked with other work. Should be able to look at this shortly.

Comment 84

[Stephen A Pohl [:spohl]]

Attachment: fixv2.patch

This removes some whitespace changes that snuck into your last patch. I will apply any review feedback to this patch to focus on the functional changes.

Comment 85

[Stephen A Pohl [:spohl]]

Attachment: fixv2.patch

And here is a patch that actually applies. Building now to verify the functional changes.

Comment 86

[Stephen A Pohl [:spohl]]

Comment on attachment 9046393 [details] [diff] [review]
fixv2.patch

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

This looks great, thanks! Just one minor comment to address, hence the r-. Once that comment is addressed, we should land this and keep an eye on any other potential fallout.

(In reply to Yuri from comment #81)

> Created attachment 9042355 [details] [diff] [review]
> fixv2.patch
> 
> Hello Stephen,
> 
> The auto-update mechanism should be fixed. I am detecting the update-restart event by checking the MOZ_APP_RESTART=1 environment variable.

I have confirmed that this is now working properly.
 
> >Note that for bug 1513335, we may actually want that behavior to match Windows and Linux, but we should focus the currently running instance of Firefox.
> 
> Updated the logic so that -no-remote argument (implicitly used by ./mach run) now defines behavior only for duplicate processes, not the original one(s). 
> I am not activating ignoring other apps though, since you mentioned earlier this would not the desired behaviour.

This now starts a separate instance again.

> >https://bugzilla.mozilla.org/show_bug.cgi?id=1513336
> 
> Please confirm, that this is not happening anymore. 
> Not sure if this was introduced by our fix.

Yes, this was introduced by the earlier patch for this bug and is now fixed. We now open a new window in the existing instance.

::: toolkit/xre/nsNativeAppSupportCocoa.mm
@@ +197,5 @@
> +  // current user.
> +  NSString* notificationName = [[[NSBundle mainBundle] bundleIdentifier]
> +                                stringByAppendingString:
> +                                @".distributedNotification.commandLineArgs"];
> +  if (!shallProceedLikeNoRemote && !mozillaRestarting && [[NSRunningApplication

Instead of cramming `[[NSRunningApplication runningApplicationsWithBundleIdentifier:[[NSBundle mainBundle] bundleIdentifier]] count] > 1` into this `if` clause, let's have another local variable like `shallProceedLikeNoRemote` and `mozillaRestarting` for this.

Comment 87

[Yuri Z.]

Thanks for looking into this.

This now starts a separate instance again.

Can you let me know how exactly are you launching two instances? Using ./mach run, or by just clicking the icon in Dock? What arguments are being passed to each of the instances?

Comment 88

[Stephen A Pohl [:spohl]]

(In reply to Yuri from comment #87)

Thanks for looking into this.

This now starts a separate instance again.

Can you let me know how exactly are you launching two instances? Using ./mach run, or by just clicking the icon in Dock? What arguments are being passed to each of the instances?

I have followed the steps in bug 1513335:

(In reply to Markus Stange [:mstange] from comment #0)

Steps to reproduce:

1. Have a "Firefox Nightly" app icon in your dock, but don't have it
   running.
2. Launch a locally-compiled Firefox build from the command line. This
   instance of Nightly appears as a new icon in your Dock and uses its own
   Firefox profile.
3. Click the "Firefox Nightly" Dock icon.

The instance, launched in step 2 via ./mach run, is launched with the following command line arguments:

/Users/spohl/Documents/objdir-ff-release/dist/Nightly.app/Contents/MacOS/firefox -no-remote -foreground -profile /Users/spohl/Documents/objdir-ff-release/tmp/profile-default

Launching the instance in step 3 by clicking the Dock icon launches another instance with no command line arguments, as follows:

/Applications/Firefox Nightly.app/Contents/MacOS/firefox

Comment 89

[Yuri Z.]

Thanks Stephen. My other question would be – are you sure you have both instances (the one in Dock and the one you are running with ./mach run) built with the patch?

Note, that the /Applications/Firefox Nightly.app/Contents/MacOS/firefox must include the patch, the old version doesn't correctly handle IPC to re-activate the app and prevent it from starting.

I am having the same config and I verified that the second instance doesn't start - instead the original one gets activated once you click on a dock icon. Please confirm that you do - I will try to find out why this is happening.

Comment 90

[Stephen A Pohl [:spohl]]

(In reply to Yuri from comment #89)

I am having the same config and I verified that the second instance doesn't start - instead the original one gets activated once you click on a dock icon. Please confirm that you do - I will try to find out why this is happening.

You're correct, I'm seeing the same. Note that the running instance does not gain focus and is not brought to the foreground for me. We should make it so that we can still launch another instance, unless the instance is launched with the same profile.

Comment 91

[Stephen A Pohl [:spohl]]

(In reply to Stephen A Pohl [:spohl] from comment #90)

Note that the running instance does not gain focus and is not brought to the foreground for me.

I should clarify that each click on the Dock icon opens a new window in the existing, running instance that was launched from terminal via ./mach run.

Comment 92

[Yuri Z.]

Hi Stephen,

Note that the running instance does not gain focus and is not brought to the foreground for me

This is due to not activating ignoring other apps. We can activate the original app ignoring the currently focused app, though, if needed - then the original instance will be brought to foreground. Please confirm that this is what is actually wanted.

We should make it so that we can still launch another instance, unless the instance is launched with the same profile.

One can launch another instance by specifying --no-remote in the command line.
The patch does not deal with profiles currently (I think this must be done in another module - the one that actually displays the message "A copy of Firefox is already open. Only one copy of Firefox can be open at a time.").

I should clarify that each click on the Dock icon opens a new window in the existing, running instance that was launched from terminal via ./mach run.

I suppose this is the intended behavior?

Comment 93

[Stephen A Pohl [:spohl]]

(In reply to Yuri from comment #92)

Hi Stephen,

Note that the running instance does not gain focus and is not brought to the foreground for me

This is due to not activating ignoring other apps. We can activate the original app ignoring the currently focused app, though, if needed - then the original instance will be brought to foreground. Please confirm that this is what is actually wanted.

Yes, we need to either focus the existing instance and bring it to the foreground or we need to launch a new instance with the default profile (the current behavior is confusing, as described at the end of the description of bug 1513335).

We should make it so that we can still launch another instance, unless the instance is launched with the same profile.

One can launch another instance by specifying --no-remote in the command line.
The patch does not deal with profiles currently (I think this must be done in another module - the one that actually displays the message "A copy of Firefox is already open. Only one copy of Firefox can be open at a time.").

If a user has chosen to display the profile manager, the current behavior was to display the profile manager in the second instance to allow a user to select a different profile. With this patch, the Dock icon simply bounces and disappears and the existing instance has a new window opened, but isn't brought to the foreground. You can set the profile manager to display on startup by unchecking the right checkbox in the dialog that's displayed when launching firefox with the -p command line argument.

Comment 94

[Yuri Z.]

Attachment: patchv3.patch

Added activation of the original instance when a duplicate is launched, slight change of code formatting as per Stephen's comments.

Comment 95

[Stephen A Pohl [:spohl]]

Attachment: patchv3.patch

Thanks! I've updated the patch to remove some whitespace changes that have snuck in again. Let's land this and keep an eye out for any other potential fallout.

Comment 96

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4bdd668c63799c218c76873591ddc72598f1fc1
Bug 469990: Allow command line arguments to be handed off from a new Firefox/Thunderbird process to an existing one when necessary. r=spohl

Comment 97

[Razvan Maries]

Backed out for xpcshell perma fails on toolkit/mozapps/update/tests/unit_base_updater/marAppApplyUpdateSuccess.js.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=962e8a03ecba09cc14ff295977f78a76a6a5aa58&selectedJob=231708679

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231708679&repo=mozilla-inbound&lineNumber=7759

19:53:16 INFO - TEST-START | toolkit/mozapps/update/tests/unit_base_updater/marAppApplyUpdateSuccess.js
19:55:17 WARNING - TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/unit_base_updater/marAppApplyUpdateSuccess.js | xpcshell return code: 0
19:55:17 INFO - TEST-INFO took 120380ms
19:55:17 INFO - >>>>>>>
19:55:17 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
19:55:17 INFO - "11:53:16:974 | TEST-INFO | xpcshellUtilsAUS.js | [setupTestCommon : 738] start - general test setup"
19:55:17 INFO - TEST-PASS | toolkit/mozapps/update/tests/unit_base_updater/marAppApplyUpdateSuccess.js | setupTestCommon - [setupTestCommon : 740] gTestID should be 'undefined' (setupTestCommon should only be called once) - "undefined" === "undefined"
19:55:17 INFO - (xpcshell/head.js) | test pending (2)
19:55:17 INFO - "11:53:16:976 | TEST-INFO | xpcshellUtilsAUS.js | [setupTestCommon : 811] Updates Directory (UpdRootD) Path: /Users/cltbld/Library/Caches/Mozilla/updates/Users/cltbld/tasks/task_1551727841/build/tests/xpcshell/tests/toolkit/mozapps/update/tests/unit_base_updater/marAppApplyUpdateSuccess/dir"
19:55:17 INFO - "11:53:16:977 | TEST-INFO | xpcshellUtilsAUS.js | [setupTestCommon : 831] attempting to remove directory. Path: /Users/cltbld/Library/Caches/Mozilla/updates/Users/cltbld/tasks/task_1551727841/build/tests/xpcshell/tests/toolkit/mozapps/update/tests/unit_base_updater/marAppApplyUpdateSuccess/dir"
19:55:17 INFO - "11:53:16:977 | TEST-INFO | shared.js | [removeDirRecursive : 526] attempting to remove directory. Path: /Users/cltbld/Library/Caches/Mozilla/updates/Users/cltbld/tasks/task_1551727841/build/tests/xpcshell/tests/toolkit/mozapps/update/tests/unit_base_updater/marAppApplyUpdateSuccess/dir"
19:55:17 INFO - "11:53:16:978 | TEST-INFO | xpcshellUtilsAUS.js | [setupTestCommon : 843] finish - general test setup"
19:55:17 INFO - "11:53:16:978 | TEST-INFO | xpcshellUtilsAUS.js | [setupUpdaterTest : 2608] start - updater test setup"
19:55:17 INFO - "11:53:16:980 | TEST-INFO | xpcshellUtilsAUS.js | [SUT_TF_FE : 2624] start - setup test file: precomplete"

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f1f28ae9b0693826dd8fcfada19371b9fd9197

Comment 98

[Intermittent Failures Robot]

4 failures in 2818 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-inbound: 4

Platform breakdown:

• osx-10-10: 4

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=469990&startday=2019-03-04&endday=2019-03-10&tree=all

Comment 99

[Stephen A Pohl [:spohl]]

The failing test can be executed as follows:

./mach test toolkit/mozapps/update/tests/unit_base_updater/marAppApplyUpdateSuccess.js

One thing I've noticed in the console output is that firefox is executed as follows:

firefox -no-remote -test-process-updates

Comment 100

[Markus Stange [:mstange]]

Is Dave Townsend aware of the work that's happening here? His recent email to firefox-dev seems very relevant to the code that's being touched here. https://mail.mozilla.org/pipermail/firefox-dev/2019-March/006995.html

Comment 101

[Pascal Chevrel:pascalc]

On first landing the fix for this bug caused major functional regressions and the updated patch was also backed out last week. As Markus noted in comment #100, this is also likely to impact the work on Dedicated Profiles that Dave is doing now and that is planned for 67. It seems that fixing this P3 this week is unnecessary risk during our soft code freeze period.

Yuri, I suggest that you reland your patch after the final merge when we have bumped our mozilla-central repo to 68. Thanks!

Comment 102

[Dave Townsend [:mossop]]

Oooh, adding remote support to OSX? Nice. Sorry I wasn't aware of this earlier.

It would be great if it were possible to integrate this code into the remote service that both linux and windows now use (https://searchfox.org/mozilla-central/source/toolkit/components/remote) rather than nsNativeAppSupport, predominantly because having all this stuff in the same place helps finding it and having it happen at the same time and have similar behaviour on all platforms is beneficial, having things happen in different order on startup is a source of platform specific bugs.

Happy to try to help out wherever I can!

Comment 103

[Yuri Z.]

Attachment: thefix.patch

Hi Stephen,
Thanks for your suggestions. Not being a mozilla developer, your hints help a lot.
The test should have passed because it passes -no-remote to every instance, however for some reason the call [NSRunningApplication runningApplicationsWithBundleIdentifier] either takes too long comparing to the timeout set in test, or just never returns for some odd reason.
I kind of resolved this by making this call only in case -no-remote has not been passed (this doesn't break remote logic in any way). The test now passes.

Regarding Pascal's suggestion, when would be the right date to land the patch? I understand, we can't try it out on Nightly until that date, correct?

Dave, do you think we could do this migration (nsNativeAppSupport -> remote component) as a separate effort? Unfortunately, Mac support of "remote" turns out to get some hacks inside, so I am worried it may take another month or so to get this stuff debugged and working.

Stephen, what do you think?

Comment 104

[Dave Townsend [:mossop]]

(In reply to Yuri from comment #103)

Dave, do you think we could do this migration (nsNativeAppSupport -> remote component) as a separate effort? Unfortunately, Mac support of "remote" turns out to get some hacks inside, so I am worried it may take another month or so to get this stuff debugged and working.

By all means, don't want to hold things up.

Comment 105

[Yuri Z.]

Ping :)

Comment 106

[Stephen A Pohl [:spohl]]

(In reply to Yuri from comment #105)

Ping :)

Firmly penciled in for tomorrow morning. Thanks for your patience.

Comment 107

[Stephen A Pohl [:spohl]]

Attachment: Patch

Combined the patches. This looks good to me.

Comment 108

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f70c622344459f23a2ffa4080e7eb93ec33701e2

Comment 109

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1374f2163b67338d0d0540b51010899452bde96
Bug 469990: Allow command line arguments to be handed off from a new Firefox/Thunderbird process to an existing one when necessary. r=spohl

Comment 110

[Yuri Z.]

Thanks Stephen. Fingers crossed!

Comment 111

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b1374f2163b6

Comment 112

[Yuri Z.]

Hi Stephen, can I ask what are the next steps with the fix? Do we need to wait for it to get to nightly and then QA testing?

Comment 113

[Yuri Z.]

Attachment: thefix.patch

Updated as per Matthew's finding in https://bugzilla.mozilla.org/show_bug.cgi?id=1540821

Comment 114

[Matthew N. [:MattN]]

Comment on attachment 9054960 [details] [diff] [review]
thefix.patch

(In reply to Yuri from comment #113)

> Created attachment 9054960 [details] [diff] [review]
> thefix.patch
> 
> Updated as per Matthew's finding in https://bugzilla.mozilla.org/show_bug.cgi?id=1540821

Hi Yuri, once a bug is resolved you shouldn't attach further patches to it. You should attach that patch to bug 1540821.

Comment 115

[Yuri Z.]

Attached to 1540821. Please let me know if anything is incorrect.

Comment 116

[Stephen A Pohl [:spohl]]

(In reply to Yuri from comment #112)

Hi Stephen, can I ask what are the next steps with the fix? Do we need to wait for it to get to nightly and then QA testing?

Comment 111 shows that the patch landed in Nightly. It will show up in the next Nightly build, usually on the day following the landing. There is no automatic QA testing, unless it is specifically requested. We need to keep an eye out for bugs/regressions that get reported after the landing, such as has happened with bug 1540821. Thank you for fixing this so quickly. If there are no major regressions that require another backout of the patch, the fix will ride the train and ultimately land in release. You can get an idea of the timing by looking at the release calendar:

https://wiki.mozilla.org/Release_Management/Calendar

Comment 118

[Markus Keller]

Why did this bug end up in a mainstream release when regression bug 1553767 was already reported 2 month ago?

Comment 119

[Yuri Z.]

Hi Dave, could you please let me know whether this patch has already landed in Thunderbird, and which release, if so? (not sure where this can be checked)
Thank you

Comment 120

[Magnus Melin [:mkmelin]]

Flags on this bug say "firefox68: fixed", so it is available (also) in thunderbird version 68 and above.

Comment 121

[Yuri Z.]

Thanks Magnus