Link with download attribute triggers onbeforeunload

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: evilpie, Assigned: Benjamin)

Tracking

(Blocks 1 bug)

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

7 years ago
Bug 676619 made this more obvious, but even before clicking on a link that opens a file download triggers the onbeforeunload event.
Reporter

Updated

7 years ago
Depends on: 676619
A stacktrace would be nice here.
> even before clicking on a link that opens a file download triggers the onbeforeunload
> event.

onbeforeunload fires before we start the GET to the server in that case (has to, per spec, iirc).  We don't know we're going to end up with a download at that point.

But for @download, I agree we know it up front and shouldn't fire beforeunload.
Assignee

Comment 3

7 years ago
Per http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#navigating-across-documents beforeonload has to be fired before any network requests.
Summary: File download triggers onbeforeunload → Link with download attribute triggers onbeforeunload
Comment on attachment 689083 [details] [diff] [review]
don't ask to unload the document when we know it's a download

I'm mostly offline today (independence day). Will look at this hopefully tomorrow.
Attachment #689083 - Flags: feedback?(evilpies)
Comment on attachment 689083 [details] [diff] [review]
don't ask to unload the document when we know it's a download

mTiming->NotifyBeforeUnload() is then a bit wrong.
If we don't do unload, we shouldn't change beforeunload.
Attachment #689083 - Flags: review?(bugs)
Attachment #689083 - Flags: review-
Attachment #689083 - Flags: feedback?(evilpies)
Assignee

Comment 7

7 years ago
(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 689083 [details] [diff] [review]
> don't ask to unload the document when we know it's a download
> 
> mTiming->NotifyBeforeUnload() is then a bit wrong.
> If we don't do unload, we shouldn't change beforeunload.

It seems this is a preexisting problem with javascript urls, though?
Sure, but downloading js urls isn't common (since it isn't supported in all the browsers).
The patch would make the problem significantly worse - and fixing the problem should be almost trivial.
Assignee

Comment 9

7 years ago
Posted patch don't invoke timing (obsolete) — Splinter Review
Attachment #689083 - Attachment is obsolete: true
Attachment #689755 - Flags: review?(bugs)
Assignee

Comment 10

7 years ago
Tweak.
Attachment #689755 - Attachment is obsolete: true
Attachment #689755 - Flags: review?(bugs)
Attachment #689819 - Flags: review?(bugs)
Comment on attachment 689819 [details] [diff] [review]
don't invoke timing for downloads

Could you file a followup to add tests for performance timing in case
download happens.
Attachment #689819 - Flags: review?(bugs) → review+
Assignee

Comment 13

7 years ago
See bug 819605.

Comment 14

7 years ago
https://hg.mozilla.org/mozilla-central/rev/f5a438e93854
Assignee: nobody → benjamin
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/40324fd7e5db

Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 16

7 years ago
https://hg.mozilla.org/mozilla-central/rev/af0f5575a153
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.