798810 - Promotional content blurbs can overlap each other when downloading over dial-up/slow speeds

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Screenshot, showing overlapping content

Over a really slow connection, the stub installer's dynamically-changing promotional content (text/graphics) can overlap; see screenshot.

(To test this, I was in a VM and rate-limited my connection to dial-up speeds, essentially).

Comment 2

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

Attachment: patch rev1

Comment 3

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

Comment on attachment 670710 [details] [diff] [review]
patch rev1

Preemptively requesting aurora for this

Comment 4

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

Comment on attachment 670710 [details] [diff] [review]
patch rev1

I'm going to hold off on this one.

Comment 5

[Jason Smith [:jsmith]]

I can't reproduce this anymore on three different Windows operating systems. Our funnelcake beta testing didn't show this anymore either. Maybe bug 800112 fixed this?

Comment 6

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

I'm fairly certain I have a way to reproduce this and I was able to with all of the patches that landed so I'm going to reopen

Comment 7

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

Attachment: patch rev2

This is cleaner and just addresses this bug

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 671732 [details] [diff] [review]
patch rev2

Review of attachment 671732 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/windows/nsis/stub.nsi
@@ +1328,5 @@
>    Exec "$\"$0$\""
>  FunctionEnd
>  
> +Function DisplayDownloadError
> +  ${NSD_KillTimer} DisplayDownloadError

nit: I think we should add this call to .onUserAbort as well

@@ +1341,5 @@
> +    ${If} ${Errors}
> +      Call OpenManualDownloadURL
> +    ${Else}
> +      GetFunctionAddress $0 OpenManualDownloadURL
> +      UAC::ExecCodeSegment $0

Good call we don't want to do this when elevated.

On a side note, this will still launch elevated if the user right clicked on the installer to run as administrator, but if we were handling that we'd have other fixes too.  And it can not be handled easily.

Comment 9

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

(In reply to Brian R. Bondy [:bbondy] from comment #8)
> Comment on attachment 671732 [details] [diff] [review]
> patch rev2
> 
> Review of attachment 671732 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/installer/windows/nsis/stub.nsi
> @@ +1328,5 @@
> >    Exec "$\"$0$\""
> >  FunctionEnd
> >  
> > +Function DisplayDownloadError
> > +  ${NSD_KillTimer} DisplayDownloadError
> 
> nit: I think we should add this call to .onUserAbort as well
Good catch

> @@ +1341,5 @@
> > +    ${If} ${Errors}
> > +      Call OpenManualDownloadURL
> > +    ${Else}
> > +      GetFunctionAddress $0 OpenManualDownloadURL
> > +      UAC::ExecCodeSegment $0
> 
> Good call we don't want to do this when elevated.
> 
> On a side note, this will still launch elevated if the user right clicked on
> the installer to run as administrator, but if we were handling that we'd
> have other fixes too.  And it can not be handled easily.
Way back when we grabbed the user's desktop token to accomplish this in a few places but the token was never the same as what should have been used. I considered doing that for the installer but the amount of work and code to maintain doesn't seem worth it since the vast majority don't do this.

Comment 10

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #9)
> (In reply to Brian R. Bondy [:bbondy] from comment #8)
> > Comment on attachment 671732 [details] [diff] [review]
> > patch rev2
> > 
> > Review of attachment 671732 [details] [diff] [review]:
> > ----------------------------------------------------
> > 
> > On a side note, this will still launch elevated if the user right clicked on
> > the installer to run as administrator, but if we were handling that we'd
> > have other fixes too.  And it can not be handled easily.
> Way back when we grabbed the user's desktop token to accomplish this in a
> few places but the token was never the same as what should have been used. I
> considered doing that for the installer but the amount of work and code to
> maintain doesn't seem worth it since the vast majority don't do this.

Yup I think you could do it via grabbing the linked token, but limited user accounts would be an edge case, and yup not worth it, and yup the fix wouldn't be in this bug since there are multiple places where this happens.

Comment 11

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

Attachment: patch - updated to comments

Carrying forward r+

Thanks for the review!

Comment 12

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

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/610908ff1d38

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/610908ff1d38

Comment 14

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

Comment on attachment 671742 [details] [diff] [review]
patch - updated to comments

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Stub Installer bug 322206
User impact if declined: Content blurbs overlapped when showing download error msg
Testing completed (on m-c, etc.): baked on m-c and tested locally
Risk to taking this patch (and alternatives if risky): Little risk
String or UUID changes made by this patch: none

Comment 15

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

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/a232eca6b24c

Comment 16

[Jason Smith [:jsmith]]

Followup in bug 808118. Tough to reproduce this bug now, but it's still possible.

Comment 17

[Jason Smith [:jsmith]]

Verified per comment in bug 808118.