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

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[arni2033]

>>>   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 2

[Andrew Overholt [:overholt]]

ni(mconley) given regression from bug 1177310

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Yikes. Yeah, I'll look at this.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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/

Comment 5

[:Gijs (he/him)]

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?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

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

[:Gijs (he/him)]

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...

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb222bbb283

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Bonus points are my favourite: Bug 1256989.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2eb222bbb283

Comment 11

[arni2033]

Fixed on:   Win7_64, Nightly 48, 32bit, ID 20160317030235

Comment 12

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 14

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

remote:   https://hg.mozilla.org/releases/mozilla-esr45/rev/6345a3c7c1bc

Comment 16

[Andrei Vaida [:avaida]]

Setting the flag for verification, as the fix here could use a bit of Exploratory Testing.

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

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.