354857 - XULRunner stub doesn't properly pass '-foreground' option, app restarts behind other applications

Status: VERIFIED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

After an app-initiated restart the apppops up behind other applications. After digging around a little I figured out that it was because no one was passing the -foreground command line option to XRE_main.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This bug is filed against Mac OS X. Shouldn't this be all or is it really Mac OS X only? I ask because of your bug 399580 comment 1.

Comment 2

[Matt Crocker]

Attachment: Patch to add -foreground when restarting.

SetupMacCommandLine(gRestartArgc, gRestartArgv) is not called in the normal startup case, so -foreground is never added.  Even if it were called, LaunchChild resets gRestartArgc/v for app initiated restarts.

I think LaunchChildMac is the appropriate place to add the param, as it seems like the kind of thing you'd want to do immediately before restarting.

Thoughts?

Comment 3

[Matt Crocker]

Comment on attachment 287030 [details] [diff] [review]
Patch to add -foreground when restarting.

Hi Mark, do you mind reviewing this tiny patch?  I'm told you're the best person to ask.

Thanks.

Comment 4

[Stephen Lau]

Attachment: mattc's patch updated for tip

This is just mattc's patch updated for tip... no functional code changes, it just needed to be updated since MacLaunchHelper.m moved to MacLaunchHelper.mm

Comment 5

[Benjamin Smedberg]

Comment on attachment 287030 [details] [diff] [review]
Patch to add -foreground when restarting.

This is not xulrunner-specific code. I'd like rob to look at this.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 345568 [details] [diff] [review]
mattc's patch updated for tip

I've had next to no exposure to this code. Since it is Mac specific could you take this review Dave?.

Comment 7

[Dave Townsend [:mossop] (PTO until July 20)]

Comment on attachment 345568 [details] [diff] [review]
mattc's patch updated for tip

Unless I'm missing something I think we are better off calling SetupMacCommandLine after clearing the arguments for the restart. Otherwise we have -foreground handling in two parts of the code that ends up adding multiple -foreground flags.

Comment 8

[Stephen Lau]

Attachment: Totally different patch to do what (I think) Mossop suggested

I think this implements what Mossop suggested.

Comment 9

[Dave Townsend [:mossop] (PTO until July 20)]

Comment on attachment 348109 [details] [diff] [review]
Totally different patch to do what (I think) Mossop suggested

Yep that works.

We should take this for 1.9.1, safe simple fix.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348109 [details] [diff] [review]
Totally different patch to do what (I think) Mossop suggested

a191=beltzner

Comment 11

[Dave Townsend [:mossop] (PTO until July 20)]

Should have landed this before the branch, I'll try to remember when the tree reopens

Comment 12

[Dave Townsend [:mossop] (PTO until July 20)]

Landed in mozilla-central: http://hg.mozilla.org/mozilla-central/rev/5567bece7eed

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dave, lets add the checkin-needed keyword meanwhile so it doesn't get lost.

Comment 14

[Dave Townsend [:mossop] (PTO until July 20)]

(In reply to comment #13)
> Dave, lets add the checkin-needed keyword meanwhile so it doesn't get lost.

It needs to bake on trunk before we can look to land on the branch

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Starting my todays debug build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081206 Minefield/3.2a1pre) shows following in the Error Console. Should this also be covered by this bug or is it another issue because it doesn't happen while restarting the browser?

Error: Warning: unrecognized command line flag -foreground

Source File: file:///Volumes/Daten/mozilla/source/obj/browser-i386-apple-darwin9.5.0/dist/MinefieldDebug.app/Contents/MacOS/components/nsBrowserContentHandler.js
Line: 713

Comment 16

[Dave Townsend [:mossop] (PTO until July 20)]

(In reply to comment #15)
> Starting my todays debug build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5;
> en-US; rv:1.9.2a1pre) Gecko/20081206 Minefield/3.2a1pre) shows following in the
> Error Console. Should this also be covered by this bug or is it another issue
> because it doesn't happen while restarting the browser?
> 
> Error: Warning: unrecognized command line flag -foreground
> 
> Source File:
> file:///Volumes/Daten/mozilla/source/obj/browser-i386-apple-darwin9.5.0/dist/MinefieldDebug.app/Contents/MacOS/components/nsBrowserContentHandler.js
> Line: 713

This is bug 369147

Comment 17

[Dave Townsend [:mossop] (PTO until July 20)]

Landed on the branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b4f285d733b6

Comment 18

[Aakash Desai [:aakashd]]

verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090413 Minefield/3.6a1pre ID:20090413031052

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090413 Minefield/3.6a1pre ID:20090413031052

Comment 19

[Aakash Desai [:aakashd]]

ack, here's the build ID for Shiretoko verification: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090413 Shiretoko/3.5b4pre ID:20090413031313