Closed Bug 1255511 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- affected
firefox46 --- verified
firefox47 --- verified
firefox48 --- verified
firefox-esr38 --- unaffected
firefox-esr45 --- fixed

People

(Reporter: arni2033, Assigned: mconley)

References

Details

(Keywords: regression, Whiteboard: btpp-active)

Attachments

(2 files)

>>>   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
Component: Untriaged → DOM: Events
Product: Firefox → Core
ni(mconley) given regression from bug 1177310
Flags: needinfo?(mconley)
Whiteboard: btpp-followup-2016-03-18
Yikes. Yeah, I'll look at this.
Assignee: nobody → mconley
Flags: needinfo?(mconley)
Whiteboard: btpp-followup-2016-03-18 → btpp-active
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 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)
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)
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 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+
See Also: → 1256989
Bonus points are my favourite: Bug 1256989.
https://hg.mozilla.org/mozilla-central/rev/2eb222bbb283
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Fixed on:   Win7_64, Nightly 48, 32bit, ID 20160317030235
Status: RESOLVED → VERIFIED
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+
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.
You need to log in before you can comment on or make changes to this bug.