arni2033 Reporter
	Description • 9 years ago

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20151223030323
STR (there's 2 scenarios - A and B):
1.   Open attached "testcase 1" or "data:" url from the form above in a new tab, click on content area
2.   Open attached "testcase 1" or "data:" url from the form above in a new tab, click on content area
3.   Open a new tab
4.A) Middle-click the tab from Step 1. Middle-click the tab from Step 2.
4.B) Click close button in firefox window twice   OR   click (≡) -> Exit Nightly twice.
6.   Click the tab from Step 1 to switch to it
7.   Click "Stay on page" button. Click on content area
8.A) Middle-click the tab from Step 1.
8.B) Click close button in firefox window   OR   click (≡) -> Exit Nightly.

Result:       
 (A) The tab from Step 1 has closed   (B) the browser has closed

Expectations: 
 The browser should display a confirmation dialog in the tab from Step 1.

Note: STR are completely valid, because user may have many onbeforeunload tabs, and it's not obvious _how_ many. So user is forced to take a trip on the tab list. See also bug 1234936

This was regressed by bug 332195
> pushlog_url:   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f1f6a7ae585eb6ed1eb6caebdf4e491fbfa87fd4&tochange=66d59599465c708d4dc985c774c99ccc26aca69d

	arni2033 Reporter
	Comment 1 • 9 years ago

Oh, sorry, it's just wrong numbering. It'll be OK if we add a blank step. "Step 5: do nothing"

	:Gijs (he/him)
	Comment 2 • 9 years ago

This is because we track a tab's pending beforeunload state with a flag. But prompts spin the event loop, and there's 2 nested loops here:
1) for the first tab we're closing
2) for the second tab we're closing

because we close the first dialog first, we're still in the second tab's event loop and don't return from there, and because they're nested, we're still not returning from the first and the flag isn't being un-set on that tab.

This used to work because we'd force-switch you to a tab you were closing. When you switched away to the second tab you were closing, we'd automatically abort the first prompt (picking "stay on page" for that).

We have 2 options:

1) always switch to the tab when the prompt that's being shown is a beforeunload one
2) try to kill off the flag on the first tab when a prompt disappears. I don't know how reliably we can do the latter.

It certainly seems like the safest bet here is to just always focus the tab if it's being closed, and I'm already doing something similar in bug 1234936. But again, that kind of makes me sad, because of how onbeforeunload gets abused to trap people on pages.

	:Gijs (he/him)
	Comment 3 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #2)
> 2) try to kill off the flag on the first tab when a prompt disappears. I
> don't know how reliably we can do the latter.

This turns out to not be too hard because we can just pass the same information we do for the DOMModalDialogClosed event as we do for the DOMWillOpenModalDialog event.

The more unfortunate thing here is that that turned out not to fix the bug - because not only is the JS code "stuck" waiting to run to completion again, the native code in nsDocumentViewer is also "stuck", and so the member mInPermitUnloadPrompt bool remains set to true, and so calling permitUnload again just returns true.

I'm not entirely sure how to fix this, tbh. I'll think about it some more and try again tomorrow.

	:Gijs (he/him)
	Comment 4 • 9 years ago

I'm about to land the fix for bug 1234936 for the general beforeunload case, which will fix the immediate issue here.

... however, the more interesting thing here is that this is a fundamental issue with our tabmodal alerts. If you used the same STR as in comment #0 but with separate windows (instead of tabs), you would see the same issue (and bug 1234936 doesn't, and can't, fix that).

The good part about onbeforeunload is that all the code is under our control, so we could actually attempt to fix this specifically for beforeunload dialogs. I'm not sure it's actually possible to fix the generic issue. I'll file a separate bug for that.

	:Gijs (he/him)
	Comment 5 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #4)
> I'll file a separate bug for that.

This is already filed as bug 727801.

The remaining issue here is for multiple windows, so let's just morph the bug to be about that. That's probably lower priority (dates back to bug 616853), and I'd prefer a general fix from bug 727801, so unassigning for now.

	:Gijs (he/him)
	Comment 6 • 9 years ago

So the only way you would run into issues like this after the fix from bug 1234936 lands is with multiple windows, viz:

1) open page with beforeunload handler in one window
2) open page with beforeunload handler in another window
3) try to close first page
4) try to close second page
5) select "stay on page" in first window
6) close that first page again

AR: no dialog, tab closes
ER: beforeunload dialog


This is an artifact of threading and how we implement tab-modal dialogs. I don't know that it's fixable in the general case - but that's bug 727801. It is in principle fixable for beforeunload because we control the code, so we could e.g. add an optional nsIRunnable (devolves to plain JS function for JS callers) to the API that we use to create prompts, and run it when the prompt is dismissed. As noted in comment #5, I'd prefer a general fix, and fixing this specifically for onbeforeunload is likely to be a bit time-consuming for relatively little gain...

	Brad Lassey [:blassey] (use needinfo?)
	Comment 7 • 9 years ago

Overholt, can you get an assignee

	Andrew Overholt [:overholt]
	Comment 8 • 9 years ago

Given comment 6, should we just WONTFIX this for beforeunload and go for bug 727801 instead?

	Brad Lassey [:blassey] (use needinfo?)
	Comment 9 • 9 years ago

go for it. Should probably nominate bug 727801 for tracking in that case though.

	:Gijs (he/him)
	Comment 10 • 9 years ago

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #9)
> go for it. Should probably nominate bug 727801 for tracking in that case
> though.

FWIW, I don't know that we can fix 727801 in the general case. Modal prompts use nested event loops. Spin up two and you can't break out of the 'outer' without breaking out of the inner. I don't know that we have the mechanisms right now to get a web-compatible alternative for this. See comments on that bug.

We can fix onbeforeunload because we control all the callsites - nothing in JS is specifically blocked on that dialog. It'd be good to consider if we actually have options to fix 727801 before wontfixing this and trying to get that fixed in the general case.

	Andrew Overholt [:overholt]
	Comment 11 • 9 years ago

smaug and I have spoken a bit about this and he has more info than I do.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 9 years ago

Gijs, so it the regression here fixed? Comment 4 seems to hint about that.

It is somewhat unclear to me what else there is to fix, and definitely unclear what we can do on platform level.
Even connected-browsing-context-event-loops wouldn't help here. One could open pages X and Y from domain A to two tabs so that they would be connected. Trying to close X but not clicking on beforeunload dialog but trying to close Y... what should happen?

Note, Chrome uses window modal dialog for beforeunload dialog, and so does Edge.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 9 years ago

Looks like Chrome has app-level modal dialog, which is what I'd expect us to get too, once we have multiple child processes.

	:Gijs (he/him)
	Comment 14 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #12)
> Gijs, so it the regression here fixed? Comment 4 seems to hint about that.

comment #0 was about having 2 tabs in the same window. In the 2-tab case, if you are on a tab that has a beforeunload dialog, and you unselect that tab by selecting another tab, we dismiss the beforeunload dialog with the cancel / "stay on page" result. That originally regressed in bug 332195, and is now fixed.

That doesn't happen in the case with multiple windows, because you can keep 2 tabs selected / open on a beforeunload dialog at the same time. The move to a tab-modal dialog happened in bug 616853 and so this is technically a regression from that bug.

I'm not sure which regression you mean, but I hope that helped clarify things. :-)

> It is somewhat unclear to me what else there is to fix, and definitely
> unclear what we can do on platform level.
> Even connected-browsing-context-event-loops wouldn't help here. One could
> open pages X and Y from domain A to two tabs so that they would be
> connected. Trying to close X but not clicking on beforeunload dialog but
> trying to close Y... what should happen?

Ideally, it should all "just work". So if you:

1. try to close x
2. try to close y
3. confirm closing x

x should be closed. Right now x will be in limbo until you confirm/reject the dialog for y.

> Note, Chrome uses window modal dialog for beforeunload dialog, and so does
> Edge.

In which case you never have to worry about the double dialog to begin with. That seems like strictly worse UX, though, and additionally, they still need to solve this issue for alert() and other things that they do use tab-modal dialogs for...

(In reply to Olli Pettay [:smaug] from comment #13)
> Created attachment 8737962 [details]
> a testcase for new window
> 
> Looks like Chrome has app-level modal dialog, which is what I'd expect us to
> get too, once we have multiple child processes.

How do multiple child processes alter the situation here? The dialog itself is in the parent process, I'm fairly sure...

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #14)
> I'm not sure which regression you mean, but I hope that helped clarify
> things. :-)
This bug was filed as a regression bug. So you're saying that regression is fixed, the regression caused by
bug 332195.
So why are we keeping this bug open still?


> In which case you never have to worry about the double dialog to begin with.
> That seems like strictly worse UX,
But that is what the sane event loop handling requires for web pages. connected browsing contexts should use the same
loop so one shouldn't even be able to get the second beforeunload prompt when there is already one open for the given
connect-browsing-contexts. JS shouldn't run in that context while waiting for beforeunload prompt to be closed.

> 
> How do multiple child processes alter the situation here? The dialog itself
> is in the parent process, I'm fairly sure...
I wouldn't want to block all the web sites running any main thread JS while waiting for one to be closed.

	:Gijs (he/him)
	Comment 16 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #15)
> (In reply to :Gijs Kruitbosch from comment #14)
> > I'm not sure which regression you mean, but I hope that helped clarify
> > things. :-)
> This bug was filed as a regression bug. So you're saying that regression is
> fixed, the regression caused by
> bug 332195.
> So why are we keeping this bug open still?

Because the separate-window case is still broken. I can close this bug and file a new one, but that doesn't seem very productive...

> > In which case you never have to worry about the double dialog to begin with.
> > That seems like strictly worse UX,
> But that is what the sane event loop handling requires for web pages.
> connected browsing contexts should use the same
> loop so one shouldn't even be able to get the second beforeunload prompt
> when there is already one open for the given
> connect-browsing-contexts. JS shouldn't run in that context while waiting
> for beforeunload prompt to be closed.

Sure, but the same argument can be made about alert(), and that has the same problem. That's why bug 727801 was originally filed...

> > How do multiple child processes alter the situation here? The dialog itself
> > is in the parent process, I'm fairly sure...
> I wouldn't want to block all the web sites running any main thread JS while
> waiting for one to be closed.

I'm confused, because we already don't do this, due to how the tab modal dialogs spin the event loop. To me this sounds like you both want us to not spin the event loop, and that you want to make sure the tab-modal dialogs run in the content process so we don't have to spin an event loop but just "hang" the content process until the user responds to the dialog in that content process (which, fwiw, sounds like a scary and regression-prone thing to do).

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #16)
> 
> Because the separate-window case is still broken. I can close this bug and
> file a new one, but that doesn't seem very productive...
But the reason why this bug was filed has been fixed, right? If so, makes sense to close this one deal with other issues elsewhere.


> I'm confused, because we already don't do this, due to how the tab modal
> dialogs spin the event loop. To me this sounds like you both want us to not
> spin the event loop, 
We should not spin event loop in the child process, correct. That is causing many issues. Same issue with modal dialogs and sync XHR.


> and that you want to make sure the tab-modal dialogs
> run in the content process so we don't have to spin an event loop but just
> "hang" the content process until the user responds to the dialog in that
> content process (which, fwiw, sounds like a scary and regression-prone thing
> to do). 
No. Parent process should show the modal dialog and spin event loop, but child process would initiate that dialog
using sync call, so nothing would happen there.