Onbeforeunload dialogs appear twice for each tab when I exit/restart Firefox

VERIFIED FIXED in Firefox 46

Status

()

VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: arni2033, Assigned: mconley)

Tracking

({regression})

Trunk
mozilla48
regression
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox45 affected, firefox46 verified, firefox47 verified, firefox48 verified, firefox-esr38 unaffected, firefox-esr45 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160308030418
STR:
1. Open attached "testcase 1"
2. Click on that page
3.A) Press F10 -> File -> Exit
3.B) Click Australis Menu (≡) -> [?] -> "Restart with Add-ons Disabled"
3.C) Open about:preferences -> General, uncheck "multi-process", click "OK", click "OK"
4. Click "Leave Page"
5. Click "Stay on Page"

AR:  After Step 4 the dialog blinks. After Step 5 browser closes/restarts.
ER:  After Step 4 browser should close/restart. Step 5 shouldn't be possible.

This is regression from bug 1177310. Yep. ESR 45 is affected.
Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=45708ae19e28b92403549a6e42324f6cfdcb8f67&tochange=15ded62d213cb2e01feebcd21af52be74a535983
Comment hidden (spam)

Updated

3 years ago
Component: Untriaged → DOM: Events
Product: Firefox → Core
ni(mconley) given regression from bug 1177310
Flags: needinfo?(mconley)
Whiteboard: btpp-followup-2016-03-18
(Assignee)

Comment 3

3 years ago
Yikes. Yeah, I'll look at this.
Assignee: nobody → mconley
Flags: needinfo?(mconley)
Whiteboard: btpp-followup-2016-03-18 → btpp-active
(Assignee)

Comment 4

3 years ago
Created attachment 8730854 [details]
MozReview Request: Bug 1255511 - Skip beforeunload prompts once nsIAppStartup shuttingDown returns true. r?Gijs

When the application is asked to quit, all windows are checked to see if
they can close (which will spawn the permit unload dialogs). Once the
quit is granted, quit-application-granted is fired, and SessionStore
does async window flushing, and closes each window. In the async
window flushing loop, it's not necessary to re-ask each window
to see if they can close.

Review commit: https://reviewboard.mozilla.org/r/40191/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40191/
Attachment #8730854 - Flags: review?(gijskruitbosch+bugs)

Comment 5

3 years ago
Comment on attachment 8730854 [details]
MozReview Request: Bug 1255511 - Skip beforeunload prompts once nsIAppStartup shuttingDown returns true. r?Gijs

https://reviewboard.mozilla.org/r/40191/#review36719

Can we instead teach whatever code is currently checking skipNextCanClose to also not bother checking for (before)unload after quit-application-granted fires?

That would address two things that worry me about this patch, namely:
1) if we ever try to close windows post-quit-application-granted somewhere other than in session store, we'll hit the same problem;
2) if this function ever gets called (hi add-ons!) by something that's not in the quit-application-granted path, we might do bad things.

Does that make sense or am I missing something?
Attachment #8730854 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

3 years ago
Attachment #8730854 - Attachment description: MozReview Request: Bug 1255511 - Skip beforeunload prompts during async window flush on shutdown. r?Gijs → MozReview Request: Bug 1255511 - Skip beforeunload prompts once nsIAppStartup shuttingDown returns true. r?Gijs
Attachment #8730854 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 6

3 years ago
Comment on attachment 8730854 [details]
MozReview Request: Bug 1255511 - Skip beforeunload prompts once nsIAppStartup shuttingDown returns true. r?Gijs

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

Comment 7

3 years ago
Comment on attachment 8730854 [details]
MozReview Request: Bug 1255511 - Skip beforeunload prompts once nsIAppStartup shuttingDown returns true. r?Gijs

https://reviewboard.mozilla.org/r/40191/#review36761

Nice, thanks!

Bonus points if you file a follow-up to write a marionette test for this...
Attachment #8730854 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

3 years ago
See Also: → bug 1256989
(Assignee)

Comment 9

3 years ago
Bonus points are my favourite: Bug 1256989.

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2eb222bbb283
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Reporter)

Comment 11

3 years ago
Fixed on:   Win7_64, Nightly 48, 32bit, ID 20160317030235
Status: RESOLVED → VERIFIED
(Assignee)

Comment 12

3 years ago
Comment on attachment 8730854 [details]
MozReview Request: Bug 1255511 - Skip beforeunload prompts once nsIAppStartup shuttingDown returns true. r?Gijs

Approval Request Comment
[Feature/regressing bug #]:

Bug 1177310

[User impact if declined]:

Users that visit sites that use onbeforeunload to give the user the option to cancel closing the tab / quitting the browser will see that prompt twice when attempting to quit the browser.

[Describe test coverage new/current, TreeHerder]:

Unfortunately, full-browser quit is hard to test for now. We don't have great automated test coverage here.

[Risks and why]: 

This patch is pretty innocent. We already had code to skip redundant onbeforeunload dialogs, but now we enter that handling if we're in the "shutting down" state, which occurs if the user has already clicked through all of the onbeforeunload dialogs once.

[String/UUID change made/needed]:

None.
Attachment #8730854 - Flags: approval-mozilla-esr45?
Attachment #8730854 - Flags: approval-mozilla-beta?
Attachment #8730854 - Flags: approval-mozilla-aurora?
Comment on attachment 8730854 [details]
MozReview Request: Bug 1255511 - Skip beforeunload prompts once nsIAppStartup shuttingDown returns true. r?Gijs

Fix was verified, Aurora47+, Beta46+, ESR45+
Attachment #8730854 - Flags: approval-mozilla-esr45?
Attachment #8730854 - Flags: approval-mozilla-esr45+
Attachment #8730854 - Flags: approval-mozilla-beta?
Attachment #8730854 - Flags: approval-mozilla-beta+
Attachment #8730854 - Flags: approval-mozilla-aurora?
Attachment #8730854 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 14

3 years ago
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/f971cddcd1d0
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/2f6f69a19150

Waiting on ESR45 tree to open to land there.
status-firefox46: affected → fixed
status-firefox47: affected → fixed
Flags: needinfo?(mconley)
(Assignee)

Comment 15

3 years ago
remote:   https://hg.mozilla.org/releases/mozilla-esr45/rev/6345a3c7c1bc
status-firefox-esr45: affected → fixed
Flags: needinfo?(mconley)
Setting the flag for verification, as the fix here could use a bit of Exploratory Testing.
Flags: qe-verify+
Reproduced the initial issue on Windows 10 x86 using Firefox 45.0.1.

Exploratory testing was performed around this fix to ensure nothing else was broken on Windows 7 x64 and Windows 10x86.
Confirming as fixed for:
*Firefox 46 beta 6
*Latest 47.0a2 Aurora
*Latest 48.0a1 Nightly.
status-firefox46: fixed → verified
status-firefox47: fixed → verified
status-firefox48: fixed → verified
You need to log in before you can comment on or make changes to this bug.