1451366 - [Spring Creators Update] After updating and launching Firefox the window is opened in the background

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect, P1)

Description

[Alexandru Simonca [:asimonca], Release Desktop QA]

[Note]
Make sure you have the latest Windows 10 update "Redstone 4" Version 1803 - OS build 17133.1

[Affected versions]:
- Firefox Beta 60.0b8
- Firefox Nightly 61.0a1 (20180403100105)

[Affected platforms]:
- Windows 10 x64 Redstone 4

[Steps to reproduce]:
1. Download and install an older version of Firefox Beta (e.g. 59.0b6)
2. Open some other apps and have the windows on screen.
3. Open the 59.0b6 build and go to About from the hamburger menu and update the build. 

[Expected result]:
- The build is updated and Firefox starts up in the foreground and in focus.

[Actual result]:
- The build is updated and Firefox starts up in the background (under the other windows) and not in focus.

[Regression Range]
This is not a recent regression. We will update the range shortly, in the eventuality that this is an actual regression.

Comment 1

[Molly Howell (she/her) (mostly inactive)]

Hmm, I have a Win10 insider machine I use Nightly on, it's been running 1803 for several days, and I don't recall seeing this there. I can't seem to reproduce it right now, either. In any case, I don't think this is related to install or update; those don't really impact how browser windows are opened on startup.

Comment 2

[Alexandru Simonca [:asimonca], Release Desktop QA]

I wasn't sure about that either. Thank you for your input, Matt. 
About reproducing the issue, we've had some problems with reproducing it, too, but we have managed to do it several times so I thought it was worth a bug.

Comment 3

[Andrew Overholt [:overholt]]

FWIW, I can reproduce this.

Comment 4

[Julien Cristau [:jcristau]]

Jim can you find someone to look at this?

Comment 5

[Jim Mathies [:jimm]]

Hey Adam, can you take a look here, see if you can reproduce. (I wonder if this has something to do with the change in bug 1447080.)

-> P1 until we figure out how serious this is.

Comment 6

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: after update Firefox appears in the background, then foreground

I've reproduced this a few times on a VM and on quantum ref hw. Occasionally I will also see the first run after a visible installation appear in the background.

I don't think this is specifically related to the new Windows 10 version, as I've seen this before (albeit rarely). Something about the timing may have changed to make it happen more reliably now, though. I can reproduce it most of the time using a build 17134 VM on the update from 59.0b6, though usually only the first time I try after startup?

The attached video shows the bug on the first update, then I pave over the install with 59.0b6 again and re-update and the bug does not happen that time.

Comment 7

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

the backgrounding shows up at about 1:18 in attachment 8972260 [details]

Comment 8

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 1451366: Part 1 - Wait after launch from update and restart

Review commit: https://reviewboard.mozilla.org/r/243244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243244/

Comment 9

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 1451366: Part 2 - Wait after launch from installer

Review commit: https://reviewboard.mozilla.org/r/243246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243246/

Comment 10

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 1451366: Part 3 - Wait after creating main process from Launcher

Review commit: https://reviewboard.mozilla.org/r/243248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243248/

Comment 11

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8974865 [details]
Bug 1451366: Part 2 - Wait after launch from installer

https://reviewboard.mozilla.org/r/243246/#review249284

::: browser/installer/windows/nsis/installer.nsi:896
(Diff revision 1)
>  
>    ClearErrors
>    ${GetParameters} $0
>    ${GetOptions} "$0" "/UAC:" $1
>    ${If} ${Errors}
> -    Exec "$\"$INSTDIR\${FileMainEXE}$\""
> +    ${ExecAndWaitForInputIdle} "$\"$INSTDIR\${FileMainEXE}$\""

We'll want this change in stub.nsi as well. No idea if it's actually affected by this problem, but no reason to leave it out.

::: toolkit/mozapps/installer/windows/nsis/common.nsh:8118
(Diff revision 1)
>  
>    Pop $1
>    Exch $0 ; return elapsed seconds
>  !macroend
> +
> +!define ExecAndWaitForInputIdle "!insertmacro ExecAndWaitForInputIdle_"

Add a documentation comment for the new macro, including why we we're interested in WaitForInputIdle anyway.

::: toolkit/mozapps/installer/windows/nsis/common.nsh:8133
(Diff revision 1)
> +
> +  ; STARTUPINFO
> +  System::Alloc 72
> +  Pop $1
> +  ; fill in STARTUPINFO.cb
> +  System::Call "*$1(i 72)"

Not that it really matters, but as the comments on SO point out, sizeof(STARTUPINFO) in a 32-bit module is 68. Needing to use 72 here instead is fishy.

::: toolkit/mozapps/installer/windows/nsis/common.nsh:8139
(Diff revision 1)
> +
> +  ; PROCESS_INFORMATION
> +  System::Call "*(i, i, i, i) i . r2"
> +
> +  ; CREATE_DEFAULT_ERROR_MODE is follwing NSIS myCreateProcess
> +  System::Call "kernel32::CreateProcess(i 0, t r0, i 0, i 0, i 0, i ${CREATE_DEFAULT_ERROR_MODE}, i 0, i 0, i r1, i r2) i . r0"

We should explicitly call CreateProcessW.

::: toolkit/mozapps/installer/windows/nsis/common.nsh:8149
(Diff revision 1)
> +    System::Call "user32::WaitForInputIdle(i $0, i 10000)"
> +    System::Call "kernel32::CloseHandle(i $0)"
> +    System::Call "kernel32::CloseHandle(i $1)"
> +  ${EndIf}
> +
> +  System::Free $1

$1 has (probably) been overwritten with hThread by this point; we can free the STARTUPINFO immediately after the CreateProcess call.

Comment 12

[Jim Mathies [:jimm]]

> WaitForInputIdle(hProcess, 10*1000);

Lets move this constant into a define at the top of the file, and please add a comment.

Comment 13

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8974864 [details]
Bug 1451366: Part 1 - Wait after launch from update and restart

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243244/diff/1-2/

Comment 14

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8974865 [details]
Bug 1451366: Part 2 - Wait after launch from installer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243246/diff/1-2/

Comment 15

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8974866 [details]
Bug 1451366: Part 3 - Wait after creating main process from Launcher

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243248/diff/1-2/

Comment 16

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8974865 [details]
Bug 1451366: Part 2 - Wait after launch from installer

https://reviewboard.mozilla.org/r/243246/#review249740

::: toolkit/mozapps/installer/windows/nsis/common.nsh:8153
(Diff revisions 1 - 2)
>  
>    ${If} $0 <> 0
> +    System::Free $1
>      System::Call "*$2(i . r0, i . r1)"
>      ; $0: hProcess, $1: hThread
> +    System::Free $2

Move both of these Free calls out of the ${If} block, with $1 before it and $2 after it; they've both been allocated and need to be free'd regardless of whether CreateProcessW succeeded.

Comment 17

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8974864 [details]
Bug 1451366: Part 1 - Wait after launch from update and restart

https://reviewboard.mozilla.org/r/243244/#review249746

Updater change loos good.

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8974866 [details]
Bug 1451366: Part 3 - Wait after creating main process from Launcher

https://reviewboard.mozilla.org/r/243248/#review249756

::: browser/app/LauncherProcessWin.cpp:189
(Diff revision 2)
>    }
>  
> +  // Keep the current process around until the callback process has created
> +  // its message queue, to avoid the launched process's windows being forced
> +  // into the background.
> +  ::WaitForInputIdle(process.get(), kWaitForInputIdleTimeoutMS);

Where is kWaitForInputIdleTimeoutMS defined?

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 8974864 [details]
Bug 1451366: Part 1 - Wait after launch from update and restart

https://reviewboard.mozilla.org/r/243244/#review249768

Comment 20

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8974865 [details]
Bug 1451366: Part 2 - Wait after launch from installer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243246/diff/2-3/

Comment 21

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 1451366: Part 1 - Wait after launch from update and restart

Review commit: https://reviewboard.mozilla.org/r/243898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243898/

Comment 22

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 1451366: Part 3 - Wait after creating main process from Launcher

Review commit: https://reviewboard.mozilla.org/r/243900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243900/

Comment 23

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8974865 [details]
Bug 1451366: Part 2 - Wait after launch from installer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243246/diff/3-4/

Comment 24

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Really sorry about the confusion here, I submitted to Review Board with only one part and it deleted the other parts... I don't see any way to force it to carry over the r+s.

Comment 25

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Part 1 - Wait after launch from update and restart

Review Board dropped this one somehow

Comment 26

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Part 1 - Wait after launch from update and restart

/me is bad at Bugzilla, sorry.

See comment 17 and comment 19 for jimm and mhowell's r+.

Comment 27

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Part 2 - Wait after launch from installer

See comment 16 for mhowell's r+

Comment 28

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Part 3 - Wait after creating main process from Launcher

See comment 18 for aklotz's r+

Comment 29

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Part 3 - Wait after creating main process from Launcher

(In reply to Adam Gashlin [:agashlin] from comment #28)
> Created attachment 8975661 [details] [diff] [review]
> Part 3 - Wait after creating main process from Launcher
> 
> See comment 18 for aklotz's r+

Mistakenly left out r=aklotz

Comment 30

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Try looks ok:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad7bb67faf3760e8e8a5d71f9dc05283a00e0552

I'm really sorry about the mess here, Review Board got confused when I accidentally pushed only one of the parts, so I finally just went with patches to avoid having to ask for reviews again. I've double-checked that these patches are what was reviewed, modulo nits.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2297c7b1314e
Part 1 - Wait after launch from update and restart, r=mhowell, jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/dee825ccdad1
Part 2 - Wait after launch from installer r=mhowell
https://hg.mozilla.org/integration/mozilla-inbound/rev/c03186fde011
Part 3 - Wait after creating main process from Launcher, r=aklotz

Comment 32

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/2297c7b1314e
https://hg.mozilla.org/mozilla-central/rev/dee825ccdad1
https://hg.mozilla.org/mozilla-central/rev/c03186fde011

Comment 33

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

This seems to be fixed, but I could only reproduce it sporadically. Can someone with a system where it reproduces regularly verify this?

Comment 34

[Alexandru Simonca [:asimonca], Release Desktop QA]

Hello and sorry for the late response.
I have tested this and it seems fixed. I tried installing and updating different versions of Nightly and from what I can observe anything that's passed 05/19 is working well and appearing in the foreground of other open windows. Marking this bug as VERIFIED FIXED.