Closed Bug 469990 Opened 16 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
Widget: Cocoa
Platform:
All
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: hans.mustermann, Assigned: mozilla)

References

Details

(Keywords: platform-parity)

Attachments

(1 file, 23 obsolete files)

Patch
(deleted), patch
spohl
: review+
Details | Diff | Splinter Review
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 mmetzger@gmx.de 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
Version: unspecified → 2.0
hans, what happens if you use /Applications/Shredder.app/Contents/MacOS/thunderbird-bin -compose to:mmetzger@gmx.de 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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted-thunderbird3+
Keywords: qawanted
For exactly the same problem with -remote and -edit on Thunderbird and SeaMonkey, see Bug 472891.
Metzger says "Adding "to:" does not change anything."
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
This problem still exists on MacOS 10.6.7 using Thunderbird 5.0b2.
So is anything happening with this issue?
Please can this be changed to Intel Architecture and update TB version to latest available as unresolved in latest Thunderbird 10.0.2
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 ...
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.
Thunderbird 24.4.0 on MacOSX 10.9.2 (intel) Still doesn't work. Could someone update bug's platform and version?
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.
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...
(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.
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.
Patches welcome. Likely needs some changes in this file - http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/xre/nsCommandLineServiceMac.cpp
Version: 2.0 → Trunk
Bug is still reproducible on x86 hardware.
Hardware: PowerPC → All
Whiteboard: See comment 17 for possible file to be modified
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.
Keywords: qawanted → pp
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?)
Flags: needinfo?(vseerror)
Thanks for volunteering. Jorg can advise on submitting a patch.
Assignee: nobody → mozilla
Flags: needinfo?(vseerror) → needinfo?(jorgk)
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 ;-)
Flags: needinfo?(jorgk)
Attached patch Fix for the issue (obsolete) (deleted) — — Splinter Review
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.
Flags: needinfo?(jorgk)
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?
Component: Message Compose Window → Widget: Cocoa
Flags: wanted-thunderbird3+
Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(jorgk)
Product: Thunderbird → Core
Attached patch Updated to comply to coding guidelines (obsolete) (deleted) — — Splinter Review
Thanks Jorg, I updated the patch, trying to comply to coding guidelines.
Attachment #9028105 - Attachment is obsolete: true
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!
Attached patch Fix - Cleaner formatting (obsolete) (deleted) — — Splinter Review
Attachment #9028141 - Attachment is obsolete: true
Comment on attachment 9028264 [details] [diff] [review] Fix - Cleaner formatting Adding to my review queue.
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #9028264 - Flags: review?(spohl.mozilla.bugs)
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
Attachment #9028264 - Flags: review?(spohl.mozilla.bugs)
Attached patch cleaned up formatting (obsolete) (deleted) — — Splinter Review
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.
Attachment #9028264 - Attachment is obsolete: true
Flags: needinfo?(spohl.mozilla.bugs)
Attached patch 7-line unified patch (obsolete) (deleted) — — Splinter Review
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.
Attachment #9028494 - Attachment is obsolete: true
Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(spohl.mozilla.bugs)
(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
Magnus, got it, thanks. Please see my last attachment.
Attached patch patch generated with "diff = -p -U 8" (obsolete) (deleted) — — Splinter Review
Stephen, can you please re-review the patch? Thank you.
Attachment #9028514 - Attachment is obsolete: true
Attachment #9028619 - Flags: review?(spohl.mozilla.bugs)
A kind ping :)
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.
Attachment #9028619 - Flags: review?(spohl.mozilla.bugs)
Attached patch Code review Iteration fixes (obsolete) (deleted) — — Splinter Review
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).
Attachment #9028619 - Attachment is obsolete: true
Attachment #9029503 - Flags: review?(spohl.mozilla.bugs)
(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.
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #9029503 - Flags: review?(spohl.mozilla.bugs)
Attached patch workingMacRemote.patch (obsolete) (deleted) — — Splinter Review
Changed the activation policy per your request.
Attachment #9029514 - Flags: review?(spohl.mozilla.bugs)
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 ..."
Attachment #9029514 - Flags: review?(spohl.mozilla.bugs)
Attached patch workingMacRemote.patch (obsolete) (deleted) — — Splinter Review
Hopefully this should be it?
Attachment #9029503 - Attachment is obsolete: true
Attachment #9029514 - Attachment is obsolete: true
Attachment #9029710 - Flags: review?(spohl.mozilla.bugs)
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.
Attachment #9029710 - Flags: review?(spohl.mozilla.bugs) → review+
Attached patch workingMacRemote.patch (obsolete) (deleted) — — Splinter Review
Updated
Attachment #9029710 - Attachment is obsolete: true
Flags: needinfo?(spohl.mozilla.bugs)
Attached patch Patch (obsolete) (deleted) — — Splinter Review
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.
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(mozilla)
Attachment #9029820 - Flags: review+
Looks good, thanks Stephen
Flags: needinfo?(mozilla) → needinfo?(spohl.mozilla.bugs)
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.
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!
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(mozilla)
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.
Attached patch workingMacRemote.patch (obsolete) (deleted) — — Splinter Review
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.
Attachment #9029819 - Attachment is obsolete: true
Flags: needinfo?(mozilla) → needinfo?(spohl.mozilla.bugs)
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?
(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.
Flags: needinfo?(spohl.mozilla.bugs)
I believe MacAutoReleasePool.h is spelled correctly, unless the filename changed since I checked out the code last time...
(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.
Attached patch workingMacRemote.patch (obsolete) (deleted) — — Splinter Review
I see. My bad. In any case, updated it in case you prefer it to be done right.
Attachment #9029870 - Attachment is obsolete: true
Flags: needinfo?(spohl.mozilla.bugs)
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
Attached patch Patch (obsolete) (deleted) — — Splinter Review
As landed.
Attachment #9029820 - Attachment is obsolete: true
Attachment #9029876 - Attachment is obsolete: true
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #9029976 - Flags: review+
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?
(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).
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.
(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.
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.
Whiteboard: See comment 17 for possible file to be modified
(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!
Attached image Error Message (obsolete) (deleted) —
Ran commmand from terminal with preceeding /Application/ with the result shown
Attached image Error Message (obsolete) (deleted) —
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.
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.
Attachment #9030021 - Attachment is obsolete: true
Attachment #9030022 - Attachment is obsolete: true
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.
https://hg.mozilla.org/mozilla-central/rev/7bd4fa70b728
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Depends on: 1513335
Depends on: 1513336
Depends on: 1513341
Let's get this backed out of beta as well. Thanks!
Flags: needinfo?(ryanvm)
Status: RESOLVED → REOPENED
status-firefox66: --- → affected
Resolution: FIXED → ---
Target Milestone: mozilla65 → ---
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.
Flags: needinfo?(mozilla)
https://hg.mozilla.org/releases/mozilla-beta/rev/978402f68f3c
Status: REOPENED → ASSIGNED
status-firefox65: fixed → wontfix
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/590bdef6b312
Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox66: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: qe-verify+

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.

Flags: needinfo?(mozilla) → needinfo?(spohl.mozilla.bugs)

(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.

Flags: needinfo?(spohl.mozilla.bugs)
Priority: -- → P3
Attached patch fixv2.patch (obsolete) (deleted) — — Splinter Review

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.

Flags: needinfo?(spohl)
Flags: needinfo?(spohl) → needinfo?(spohl.mozilla.bugs)

Ping

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

Attached patch fixv2.patch (obsolete) (deleted) — — Splinter Review

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.

Attachment #9029976 - Attachment is obsolete: true
Attachment #9042355 - Attachment is obsolete: true
Flags: needinfo?(spohl.mozilla.bugs)
Attached patch fixv2.patch (obsolete) (deleted) — — Splinter Review

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

Attachment #9046389 - Attachment is obsolete: true
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.
Attachment #9046393 - Flags: review-

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?

Flags: needinfo?(spohl.mozilla.bugs)

(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

Flags: needinfo?(spohl.mozilla.bugs)

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.

Flags: needinfo?(spohl.mozilla.bugs)

(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.

Flags: needinfo?(spohl.mozilla.bugs)

(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.

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?

Flags: needinfo?(spohl.mozilla.bugs)

(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.

Flags: needinfo?(spohl.mozilla.bugs)
Attached patch patchv3.patch (obsolete) (deleted) — — Splinter Review

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

Flags: needinfo?(spohl.mozilla.bugs)
Attached patch patchv3.patch (obsolete) (deleted) — — Splinter Review

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.

Attachment #9046393 - Attachment is obsolete: true
Attachment #9047342 - Attachment is obsolete: true
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #9048172 - Flags: review+
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

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

Flags: needinfo?(mozilla)

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

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

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!

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!

Attached patch thefix.patch (obsolete) (deleted) — — Splinter Review

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?

Flags: needinfo?(mozilla) → needinfo?(spohl.mozilla.bugs)

(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.

Ping :)

(In reply to Yuri from comment #105)

Ping :)

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

Attached patch Patch (deleted) — — Splinter Review

Combined the patches. This looks good to me.

Attachment #9048172 - Attachment is obsolete: true
Attachment #9051611 - Attachment is obsolete: true
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #9054237 - Flags: review+
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

Thanks Stephen. Fingers crossed!

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

Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Flags: qe-verify+

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?

Flags: needinfo?(spohl.mozilla.bugs)
Depends on: 1540821
Attached patch thefix.patch (obsolete) (deleted) — — Splinter Review

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

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.
Attachment #9054960 - Attachment is obsolete: true

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

(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

Flags: needinfo?(spohl.mozilla.bugs)
Regressions: 1542316
No longer depends on: 1540821
Regressions: 1540821, 1543627, 1542316
Regressions: 1553767
Regressions: 1565331
Regressions: 1565597

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

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

Flags: needinfo?(dtownsend)

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

Flags: needinfo?(dtownsend)

Thanks Magnus

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: