799710 - [stub installer] In the timeframe between the download phase being complete and installing phase starting, a noticeable hang can sometimes be seen

Status: RESOLVED FIXED

(Firefox :: Installer, defect, P1)

Description

[Jason Smith [:jsmith]]

During installation of firefox, more commonly on slower performing machines, I've seen cases of where a hang can occur during the timeframe between when downloading has completed for the stub, but installing has not started. The more slower the machine is, the more obvious the issue stands out. If it's a fast machine, you might not even notice the issue.

Generally, we shouldn't hang and instead always give users the perception of progress.

Comment 1

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

Some intense operations for a system will do this in general. For example, in the full installer copying xul.dll due to its size and with the stub launching the full installer. I think I can improve the stub's experience in relation to this bug but I doubt it is possible to fix this entirely.

Comment 2

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

So I got a brand-new Lenovo X230 machine with an i7 cpu and 8GB of memory. I can see this with the latest Aurora stub installer, which totally stuck at this step. The download has been finihsed but it doesn't continue to installing for about 5 minutes now. I'm running it as a non-admin user. Not sure if that might be the reason here.

Is it related to the issue as reported here or shall I file a new bug?

Comment 5

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

If we assume that the perceived hang is the stub UI being unresponsive, this points to long monolithic activity on the main thread. As a measure of how prevalent hangs might be, I looked at the timing of the preinstall phase in the stub ping where we do two activities which might take up considerable time:
- open a handle on the installer (which might cause a delay due to an on-demand virus scanner kicking in)
- verify the installer's certificate (which may just take time due to a large file)

preinstall phase covers these lines:
https://dxr.mozilla.org/mozilla-central/rev/96cb95af530477edb66ae48d98c18533476e57bb/browser/installer/windows/nsis/stub.nsi#1554-1626

As noted in comment 1 launching the full installer might also be part of this hang, but that is not covered in the preinstall phase.

21% of pings include a preinstall time of at least one second
https://sql.telemetry.mozilla.org/queries/2132#3915
which likely produces a visible hang, such as the progress bar animation pausing. 9.3% are over 2 seconds
https://sql.telemetry.mozilla.org/queries/2294#4197

Comment 7

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

I might have stumbled onto another possible source of this hang. In InetBgDl [0], when its Reset function is invoked, there's a wait for the download thread to exit normally that hangs the main thread for up to 10 seconds (the thread is forcibly aborted after that). In the duplicate bug 1351618 there's a GIF of a hang that lasts very close to 10 seconds, and I just ran into a similar looking hang while working on bug 1429814, so I think this could be related. It's just a theory for now, and removing that wait might involve major surgery to InetBgDl, so it needs further investigation before we really do anything.

[0] https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp#99

Comment 8

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

Specifically, the things to investigate would be whether the plugin is being unloaded by NSIS after the download completes, because that's the only way that Reset would be getting invoked at this point in the install process, and whether the download thread really could even still be running when that happens.

Comment 9

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

Attachment: Bug 799710 - Move installer certificate validation to its own thread.

We occasionally get reports of UI unresponsiveness immediately following the
download phase of the stub installer. The longest operation that runs on the
main thread during this phase is validating the code signature of the full
installer. This patch moves that work (which is done in a native NSIS plugin)
to a separate thread. Hopefully this helps resolve the hangs.

I've also converted the build files for the plugin from Visual C++ 6 to 2017,
just to avoid the inconvenience of needing to pull up VC6 to build it.

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

Comment 10

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

I'm amending the above patch to add a timeout to the certificate check wait; if the background thread hangs somehow, we should eventually show an error instead of just spinning forever.

Comment 11

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

Comment on attachment 8973010 [details]
Bug 799710 - Move installer certificate validation to its own thread.

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

Comment 12

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

Comment on attachment 8973010 [details]
Bug 799710 - Move installer certificate validation to its own thread.

https://reviewboard.mozilla.org/r/241544/#review247924

Looks good, two minor nits to take or leave:

nit: might be nice to warn ifndef UNICODE

::: other-licenses/nsis/Contrib/CertCheck/CertCheck.cpp:64
(Diff revision 2)
> +// after each call from the script.
> +UINT_PTR __cdecl
> +NSISPluginCallback(NSPIM event)
> +{
> +  if (event == NSPIM_UNLOAD){
> +    if (WaitForSingleObject(gCheckThread, 0) != WAIT_OBJECT_0) {

nit: check for null gCheckThread

Comment 13

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

Comment on attachment 8973010 [details]
Bug 799710 - Move installer certificate validation to its own thread.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3af4e6789a7a
Move installer certificate validation to its own thread. r=agashlin

Comment 15

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

I never was able to reproduce this bug myself, so I could use some help from someone who can to verify that this patch actually helps the problem.

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/3af4e6789a7a

Comment 17

[Camelia Badau [:cbadau], Desktop Test Engineering]

I'm not able to reproduce this issue since the Stub Installer build is always the latest version of Firefox, now Firefox 62 Beta 15. 
I've tested on latest S

Comment 18

[Camelia Badau [:cbadau], Desktop Test Engineering]

I'm not able to reproduce this issue since the Stub Installer always brings the latest version of Firefox, now Firefox 62 Beta 15. 
I didn't see the issue using Firefox 62 Beta 15 Stub Installer.

Comment 19

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

This issue happens within the stub installer itself, so which Firefox version the stub pulls down isn't important; old stub installers should still be able to reproduce the bug (assuming it can be reproduced at all, which, as I said earlier, I was never able to do).

Comment 20

[Camelia Badau [:cbadau], Desktop Test Engineering]

I've tried with old stub installers, but unfortunately I wasn't able to reproduce the issue.