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

VERIFIED FIXED in Firefox 18

Status

()

Firefox
Installer
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: stephend, Assigned: rstrong)

Tracking

18 Branch
Firefox 19
x86
Windows 7
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox18 fixed)

Details

(Whiteboard: [stub=])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 668810 [details]
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).

Updated

6 years ago
Whiteboard: [stub+]

Updated

6 years ago
Whiteboard: [stub+] → [stub=]

Updated

6 years ago
Duplicate of this bug: 800059
Created attachment 670710 [details] [diff] [review]
patch rev1
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #670710 - Flags: review?(netzen)
Attachment #670710 - Flags: review?(netzen) → review+
Comment on attachment 670710 [details] [diff] [review]
patch rev1

Preemptively requesting aurora for this
Attachment #670710 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #670710 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 670710 [details] [diff] [review]
patch rev1

I'm going to hold off on this one.
Attachment #670710 - Flags: review+
Attachment #670710 - Flags: approval-mozilla-aurora+
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?
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 671732 [details] [diff] [review]
patch rev2

This is cleaner and just addresses this bug
Attachment #670710 - Attachment is obsolete: true
Attachment #671732 - Flags: review?(netzen)
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.
Attachment #671732 - Flags: review?(netzen) → review+
(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.
(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.
Created attachment 671742 [details] [diff] [review]
patch - updated to comments

Carrying forward r+

Thanks for the review!
Attachment #671732 - Attachment is obsolete: true
Attachment #671742 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/610908ff1d38
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19

Updated

6 years ago
Keywords: verifyme
QA Contact: jsmith
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
Attachment #671742 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #671742 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/a232eca6b24c
status-firefox18: --- → fixed
(Reporter)

Updated

6 years ago
QA Contact: jsmith → jbecerra
Followup in bug 808118. Tough to reproduce this bug now, but it's still possible.
Depends on: 808118
Keywords: verifyme
QA Contact: jbecerra → jsmith

Updated

6 years ago
Whiteboard: [stub=] → [stub=], [qa verification blocked]
No longer depends on: 808118
Verified per comment in bug 808118.
Status: RESOLVED → VERIFIED
Whiteboard: [stub=], [qa verification blocked] → [stub=]
You need to log in before you can comment on or make changes to this bug.