Closed Bug 1451366 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
Widget: Win32
Version:
60 Branch
Platform:
x86_64
Windows 10
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 + wontfix
firefox61 - wontfix
firefox62 - verified

People

(Reporter: asimonca, Assigned: agashlin)

References

Details

Attachments

(4 files, 7 obsolete files)

after update Firefox appears in the background, then foreground
6.39 MB, video/webm
Details
Part 1 - Wait after launch from update and restart
2.94 KB, patch
agashlin
: review+
Details | Diff | Splinter Review
Part 2 - Wait after launch from installer
4.79 KB, patch
agashlin
: review+
Details | Diff | Splinter Review
Part 3 - Wait after creating main process from Launcher
1.04 KB, patch
agashlin
: review+
Details | Diff | Splinter Review 
[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.
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.
Component: Installer → Widget: Win32
Product: Firefox → Core
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.
FWIW, I can reproduce this.
Jim can you find someone to look at this?
Flags: needinfo?(jmathies)
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.
Flags: needinfo?(jmathies) → needinfo?(agashlin)
Priority: -- → P1
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.
Flags: needinfo?(agashlin)
the backgrounding shows up at about 1:18 in attachment 8972260 [details]
Assignee: nobody → agashlin
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.
Attachment #8974865 - Flags: review?(mhowell) → review-
> WaitForInputIdle(hProcess, 10*1000);

Lets move this constant into a define at the top of the file, and please add a comment.
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.
Attachment #8974865 - Flags: review?(mhowell) → review+
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.
Attachment #8974864 - Flags: review?(mhowell) → review+
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?
Attachment #8974866 - Flags: review?(aklotz) → review+
Comment on attachment 8974864 [details]
Bug 1451366: Part 1 - Wait after launch from update and restart

https://reviewboard.mozilla.org/r/243244/#review249768
Attachment #8974864 - Flags: review?(jmathies) → review+
Attachment #8974864 - Attachment is obsolete: true
Attachment #8974866 - Attachment is obsolete: true
Attachment #8975652 - Attachment is obsolete: true
Attachment #8975652 - Flags: review?(mhowell)
Attachment #8975652 - Flags: review?(jmathies)
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.
Review Board dropped this one somehow
Attachment #8975655 - Flags: review+
/me is bad at Bugzilla, sorry.

See comment 17 and comment 19 for jimm and mhowell's r+.
Attachment #8975655 - Attachment is obsolete: true
Attachment #8975656 - Flags: review+
See comment 16 for mhowell's r+
Attachment #8974865 - Attachment is obsolete: true
Attachment #8975658 - Flags: review+
See comment 18 for aklotz's r+
Attachment #8975653 - Attachment is obsolete: true
Attachment #8975653 - Flags: review?(aklotz)
Attachment #8975661 - Flags: review+
(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
Attachment #8975661 - Attachment is obsolete: true
Attachment #8975670 - Flags: review+
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.
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
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
Keywords: checkin-needed
This seems to be fixed, but I could only reproduce it sporadically. Can someone with a system where it reproduces regularly verify this?
Flags: needinfo?(alexandru.simonca)
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.
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
Flags: qe-verify+
Flags: needinfo?(alexandru.simonca)
See Also: → 1554490
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: