Biju Reporter
	Description • 14 years ago

Thanks Thanks Thanks Thanks Thanks Thanks Thanks Thanks for bug 59314 FIX
I noticed a Race Condition when closing inactive tab with alerts in OnBeforeUnload 

Steps:-
1. Go to attachment 477005 [details]
2. Create a new tab
3. Activate / go to the new tab
4. Click [x] on the attachment 477005 [details] tab
   -- nothing happens
5. Click [x] again

Result:
Some times you will see race condition 
Other times you may see window modal dialog
some other times long running script and on "stop script" a tab that is unable to close.

Expected result
May be bug 598824 solution 

Will bug 598824 handle this situation also ?

	Biju Reporter
	Comment 1 • 14 years ago

May be this depend on Bug 391834 as on attachment 492119 [details] this problem dont occur.

	Biju Reporter
	Comment 2 • 14 years ago

Retested on new profile on Win Vista this is what I see
On first time run you see win modal dialog
Second run of all the 5 STR, see win modal dialog again
On Third run of all the 5 STR, you get script too long, choose stop script 

now try to close firefox, you may have to try multiple times

Once you succeed quitting firefox, restart firefox
you get 
  "Firefox is already running, but is not responding. To open a new window, 
   you must first close the existing Firefox process, or restart your system."

	Dave Townsend [:mossop]
	Comment 3 • 14 years ago

Dolske, this seems problematic. Previously we would have switched to the tab when the alert happened so maybe that is the cause. What do you think?

	Dave Townsend [:mossop]
	Comment 4 • 14 years ago

Actually it looks like this is breaking something in tabbrowser, it made it impossible for me to close one of my tabs after playing with it for a bit.

	Biju Reporter
	Comment 5 • 14 years ago

.

   this also an issue with TAB-CANDY aka PANORAMA

STR:-
(you may want to copy these steps to notepad before starting)
1. Go to attachment 477005 [details] (call it tab ABC)
2. Create a new tab (call it tab XYZ)
3. click PANORAMA button
4. create a new PANORAMA group
5. Drag tab ABC (tab for attachment 477005 [details]) to the new group
6. Close that new group by clicking [X] on the group
7. select tab XYZ in PANORAMA to Zoom it.
8. Exit firefox
9. re-start firefox

Result:-
User see dialog
   Firefox is already running, but is not responding.
   To open a new window, you must first close the existing Firefox process,
   or restart your system.

Again on attachment 492119 [details] (Bug 391834)
this is not an issue, because the onbeforeunload dialog popup showup as soon as user comes out of TAB-CANDY

	Biju Reporter
	Comment 6 • 14 years ago

there is almost similar situation in Bug 616838

	Justin Dolske [:Dolske] Assignee
	Comment 7 • 14 years ago

I can't seem to reproduce a problem with the original STR, but can get some problems with comment 5's STR.

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 8 • 14 years ago

Is 626455 a dupe of this bug?

	Mihai Sucan [:msucan]
	Comment 9 • 14 years ago

Michael: bug 626455 doesn't seem to be a dupe of this bug. It seems it may require a more elaborate fix.

Similarly for bug 616838 which I am not always able to reproduce.

As for the problem reported in this bug:

- in comment #0: I cannot repro the steps. It did happen for me once, while I was playing with the TC but not by following those steps, and I wasn't able to repro again. So, maybe we are missing something those steps.

- in comment #5: I can always repro the issue.

I found another way to reliably repro the bug:

1. Make sure you have only one Firefox window open with a tab. Doesn't matter what page.
2. Open attachment 477005 [details] in a new tab.
3. Quit Firefox. Ctrl-Q
4. See alert(onbeforeunload).
5. See confirmation to leave/stay on the page.
6. Choose leave page.

Expected result: Firefox quits.

Actual result: Firefox seems to quit (the window disappears), but its process continues to run in the background. Never quits.

The root cause: Firefox tries to quit, but it also tries to show the alert() from onunload. The tight loop in nsPrompter.js never ends:

        while (args.promptActive)
            thread.processNextEvent(true);

Will attach a proposed fix.

	Mihai Sucan [:msucan]
	Comment 10 • 14 years ago

Patch for a proposed fix.

This fixes the issue for repro steps given in comment 5 and comment 9. I expect it also fixes the problem with repro steps in comment 0, but I was not able to repro.

What the proposed solution does:

- for onunload and onpagehide we do not use tabmodal dialogs, we switch back to use window modal dialogs.

I believe that the proposed solution to not allow alerts/confirm/prompts in onbeforeunload/onunload/onpagehide is not ideal (see bug 391834). While that would avoid the hangs and the related issues, it also prevents acceptable use-cases for prompts in web apps.

Window modal dialogs are always shown, unlike tabmodal dialogs which disappear when the tab closes. When Firefox quits they don't show at all, no longer causing any hangs.

I have not included a test, because I am not sure if this is an acceptable fix. Please let me know. Thanks!

Another thing to take into consideration: is there a way to check if the tab can be made visible/focused? Because that's the main problem with tabmodal dialogs - they are not visible, and the user can't dismiss those dialogs. That's what also affects Panorama usage. How I can check that? If I can do that, I expect to fix bug 626455 as well. Thanks!

	Biju Reporter
	Comment 11 • 14 years ago

Thanks a lot !!!

(In reply to comment #10)
> it also fixes the problem with repro steps in comment 0
I am also unable to reproduce this using STR at comment 0

> - for onunload and onpagehide we do not use tabmodal dialogs, 
> we switch back to use window modal dialogs.
This will be OK if it has option
"Prevent this page from creating additional dialogs"
Otherwise it will be equivalent of reverting bug 59314 and bug 61098 FIX

> I believe that the proposed solution to not allow alerts/confirm/prompts in
> onbeforeunload/onunload/onpagehide is not ideal (see bug 391834). While that
> would avoid the hangs and the related issues, it also prevents acceptable
> use-cases for prompts in web apps.
see bug 578828, we really dont need even onbeforeunload prompt.
It is there only for legacy sites. 
Having alerts/confirm/prompts is equivalent to reverting bug 588292

	Mihai Sucan [:msucan]
	Comment 12 • 14 years ago

(In reply to comment #11)
> Thanks a lot !!!
> 
> (In reply to comment #10)
> > it also fixes the problem with repro steps in comment 0
> I am also unable to reproduce this using STR at comment 0
> 
> > - for onunload and onpagehide we do not use tabmodal dialogs, 
> > we switch back to use window modal dialogs.
> This will be OK if it has option
> "Prevent this page from creating additional dialogs"
> Otherwise it will be equivalent of reverting bug 59314 and bug 61098 FIX

Correct. It still has the option "prevent this page from creating additional dialogs".

> > I believe that the proposed solution to not allow alerts/confirm/prompts in
> > onbeforeunload/onunload/onpagehide is not ideal (see bug 391834). While that
> > would avoid the hangs and the related issues, it also prevents acceptable
> > use-cases for prompts in web apps.
> see bug 578828, we really dont need even onbeforeunload prompt.
> It is there only for legacy sites. 
> Having alerts/confirm/prompts is equivalent to reverting bug 588292

I really can't comment on that. Having read the comments of those bugs already, I see that people have very strong contradicting opinions on such changes. Hence, I chose to not go into the "politics" of making a choice over removing or not removing alerts/prompts/confirms from those events. The patch I submitted keeps the same functionality, while avoiding (some) browser hangs.

If I find a way to improve the patch, to fix bug 626455 as well, I think it's the best solution at the moment. It would leave the (hard) decision of removing support for alerts from onbeforeunload/onunload/onpagehide ... for some time later, in a future Firefox release.

	Mihai Sucan [:msucan]
	Comment 13 • 14 years ago

Updated the proposed patch.

In a similar approach to what I presented yesterday, this new update attempts to also fix the related bug 626455 by not using tabmodal dialogs when the TabView (panorama) feature is visible. Doing this avoids the hang reported by the user, and it does allow the user to choose to not close the tab (picking the "stay on page" option). If he chooses "leave page", Firefox quits as expected.

Comments are welcome. Thanks!

	Justin Dolske [:Dolske] Assignee
	Comment 14 • 14 years ago

Comment on attachment 505389 [details] [diff] [review]
updated proposal

>--- a/toolkit/components/prompts/src/nsPrompter.js
...
>+    try {
>+      Services.ww.openWindow(domWin, uri, "_blank", "centerscreen,chrome,modal,titlebar", args);
>+    } catch (ex) {
>+      // The openWindow() call can fail when Firefox is about to quit.
>+    }

Do a Cu.reportError() in the catch.


>         if (allowTabModal && this.domWin) {
>             let tabPrompt = PromptUtils.getTabModalPrompt(this.domWin);
>-            if (tabPrompt) {
...
>+            if (tabPrompt && !docShell.isInUnload && !TabViewVisible) {

Can you describe what was causing the brokenness in comment 0/5/9, and how this fixes it? Or did you just find that whatever it was this fixes it? I'd like to better understand what the problem was.

Seems like what you're doing here is forcing a fallback to a window-modal prompt, and relying on it failing? We probably ought to just return/throw right here when .isInUnload is true -- I think there are existing bugs on file about suppressing alerts in cases like these.

	Biju Reporter
	Comment 15 • 14 years ago

(In reply to comment #14)
> here when .isInUnload is true -- I think there are existing bugs on file about
> suppressing alerts in cases like these.

Yes, that seems to be working, see attachment onunload.html.
we may need .isInBeforeUnload situation

I feel if bug 620615 fix resolves this issue also that will be a better choices

	Biju Reporter
	Comment 16 • 14 years ago

with more testing options

	Mihai Sucan [:msucan]
	Comment 17 • 14 years ago

(In reply to comment #14)
> Comment on attachment 505389 [details] [diff] [review]
> updated proposal
> 
> >--- a/toolkit/components/prompts/src/nsPrompter.js
> ...
> >+    try {
> >+      Services.ww.openWindow(domWin, uri, "_blank", "centerscreen,chrome,modal,titlebar", args);
> >+    } catch (ex) {
> >+      // The openWindow() call can fail when Firefox is about to quit.
> >+    }
> 
> Do a Cu.reportError() in the catch.

Will do.


> >         if (allowTabModal && this.domWin) {
> >             let tabPrompt = PromptUtils.getTabModalPrompt(this.domWin);
> >-            if (tabPrompt) {
> ...
> >+            if (tabPrompt && !docShell.isInUnload && !TabViewVisible) {
> 
> Can you describe what was causing the brokenness in comment 0/5/9, and how this
> fixes it? Or did you just find that whatever it was this fixes it? I'd like to
> better understand what the problem was.

Sure.

The brokenness in comments 0, 5 and 9 is caused by the fact the code shows a tab modal dialog that is displayed in the web page in question. The web page is removed from the tab bar, or hidden from the user, by Panorama. The user never gets to see the page prompt, and Firefox gets stuck waiting for user action.

Based on logging, I determined that the tab modal dialog waits endlessly in:

        while (args.promptActive)
            thread.processNextEvent(true);

(from nsPrompter.js)


> Seems like what you're doing here is forcing a fallback to a window-modal
> prompt, and relying on it failing? We probably ought to just return/throw right
> here when .isInUnload is true -- I think there are existing bugs on file about
> suppressing alerts in cases like these.

It's not entirely relying on the window modal prompt to failing. I just brought back the old behavior, for such cases when tab modal dialogs hang the UI. The window modal dialogs won't typically fail, because they work in the case of Panorama (see bug 626455 which is also fixed by this patch).

Yes, there are bugs about removing alerts from onbeforeunload/onunload/onpagehide, but I decided against doing such a "disputed" change, especially this late in the Firefox release cycle. If you want, I can do it, I can Cu.reportError() when isInUnload is true, and not show any dialog.

I am not sure if the above solves two important issues:

1. onbeforeunload. The nsIDocShell .isInUnload is false during the event and I am not sure how to check what is the currently executing event. Thoughts?

2. alerts that are shown based on timers, or other events, in tabs. They can prevent closing / cause hangs, in Panorama, or Firefox quitting. We still need to show these using window modal dialogs, for when Panorama is visible - like the current patch does.

Thanks for looking into the patch! Will wait for your answer, before I update the patch. Please let me know how you want me to proceed.

	Biju Reporter
	Comment 18 • 14 years ago

(In reply to comment #15)
> I feel if bug 620615 fix resolves this issue also that will be a better choices

fix at bug 620615 did not solve this, so we may have to go with this patch

	Justin Dolske [:Dolske] Assignee
	Comment 19 • 14 years ago

(In reply to comment #17)

> The brokenness in comments 0, 5 and 9 is caused by the fact the code shows a
> tab modal dialog that is displayed in the web page in question. The web page is
> removed from the tab bar, or hidden from the user, by Panorama. The user never
> gets to see the page prompt, and Firefox gets stuck waiting for user action.

Ok, so that's the core issue. Sounds like we're trying to show a prompt somewhere when it will never be seen.

> Based on logging, I determined that the tab modal dialog waits endlessly in:
> 
>         while (args.promptActive)
>             thread.processNextEvent(true);

That's expected and normal; we normally loop there until the prompt is dismissed. (Of course, if you can't dismiss the prompt, it'll never stop looping).


> The
> window modal dialogs won't typically fail, because they work in the case of
> Panorama (see bug 626455 which is also fixed by this patch).

Not sure why they would be different for Panorama. I wonder if Panorama should be listening for DOMWillOpenModalDialog so that it can switch to the appropriate tabgroup when requested.

 
> I am not sure if the above solves two important issues:
> 
> 1. onbeforeunload. The nsIDocShell .isInUnload is false during the event and I
> am not sure how to check what is the currently executing event. Thoughts?

I think there are existing bugs on disallowing dialogs during onbeforeunload, but I need to think about this some more.

> 2. alerts that are shown based on timers, or other events, in tabs. They can
> prevent closing / cause hangs, in Panorama, or Firefox quitting. We still need
> to show these using window modal dialogs, for when Panorama is visible - like
> the current patch does.

I don't understand why you think those need to be window-modal. It might happen to avoid some problem, but fundamentally there's no reason why we can't make tab-modal prompts work correctly in these cases.

	Mihai Sucan [:msucan]
	Comment 20 • 14 years ago

(In reply to comment #19)
> (In reply to comment #17)
> 
> > The brokenness in comments 0, 5 and 9 is caused by the fact the code shows a
> > tab modal dialog that is displayed in the web page in question. The web page is
> > removed from the tab bar, or hidden from the user, by Panorama. The user never
> > gets to see the page prompt, and Firefox gets stuck waiting for user action.
> 
> Ok, so that's the core issue. Sounds like we're trying to show a prompt
> somewhere when it will never be seen.

Correct.

> > Based on logging, I determined that the tab modal dialog waits endlessly in:
> > 
> >         while (args.promptActive)
> >             thread.processNextEvent(true);
> 
> That's expected and normal; we normally loop there until the prompt is
> dismissed. (Of course, if you can't dismiss the prompt, it'll never stop
> looping).

Yep, it's never dismissed.


> > The
> > window modal dialogs won't typically fail, because they work in the case of
> > Panorama (see bug 626455 which is also fixed by this patch).
> 
> Not sure why they would be different for Panorama. I wonder if Panorama should
> be listening for DOMWillOpenModalDialog so that it can switch to the
> appropriate tabgroup when requested.

The tabbrowser listens for the event and it selects the tab. The tabbrowser sometimes "communicates" with Panorama, and I think it might be appropriate to do this in tabbrowser: it can toggle off Panorama.

> > I am not sure if the above solves two important issues:
> > 
> > 1. onbeforeunload. The nsIDocShell .isInUnload is false during the event and I
> > am not sure how to check what is the currently executing event. Thoughts?
> 
> I think there are existing bugs on disallowing dialogs during onbeforeunload,
> but I need to think about this some more.

Will wait for your thoughts on this.

> > 2. alerts that are shown based on timers, or other events, in tabs. They can
> > prevent closing / cause hangs, in Panorama, or Firefox quitting. We still need
> > to show these using window modal dialogs, for when Panorama is visible - like
> > the current patch does.
> 
> I don't understand why you think those need to be window-modal. It might happen
> to avoid some problem, but fundamentally there's no reason why we can't make
> tab-modal prompts work correctly in these cases.

Agreed. :) I did this change because it seemed to be the least invasive one.

- we could cancel quits, and focus the first tab found with a tabmodal dialog, so the user can decide what to do.

- we could hide Panorama when a tabmodal dialog shows, and properly focus the tab (and group).

Or, the easy way is to just prevent any kind of dialog in onbeforeunload, onunload and onpagehide. I looked into how to do this, and I know how for onunload and onpagehide (in nsPrompter I just don't show any prompt when nsIDocShell .isUInUnload is true). However, I don't know where/how to check if the currently executing event is onbeforeunload.

So, please let me know what you believe it's best to do, and I'll update the patch accordingly. Thanks!

	Mihai Sucan [:msucan]
	Comment 21 • 14 years ago

Alternative fix. This is a different approach to fixing this bug, while waiting for further feedback.

This patch fixes the issue for repro steps given in comment 5, comment 9, and bug 626455. I expect it also fixes the problem with repro steps in comment 0, but I was not able to repro.


The approach differs now by focusing on making things work in the failing cases:

- when a tab is closing, we no longer show a tab prompt, because the prompt is only going to get stuck (hangs the browser because the page is no longer visible).

- when the window is closing / the browser is quitting ... no longer show tab prompts, again avoiding hangs. 

Tab modal prompts are not replaced with window modal prompts, they are dropped / skipped.

- when a tab is showing a prompt, focus it - switch away from Panorama and activate the correct group. This is actually optional - we could remove this chunk of code, but I believe it helps with focusing the selected tab, instead of what is happening now (nothing happens in Panorama).

Feedback is welcome. Thanks!

	Johnathan Nightingale [:johnath]
	Comment 22 • 14 years ago

ETA on feedback here, Dolske? Could this just become a straight review? This is one of our last beta hardblockers.

	Dão Gottwald [:dao]
	Comment 23 • 14 years ago

Comment on attachment 508795 [details] [diff] [review]
alternative fix

>       <handler event="DOMWillOpenModalDialog" phase="capturing">
>         <![CDATA[
>           if (!event.isTrusted)
>             return;
> 
>           // We're about to open a modal dialog, make sure the opening
>           // tab is brought to the front.
>           this.selectedTab = this._getTabForContentWindow(event.target.top);
>+
>+          if (window.TabView && TabView.isVisible()) {
>+            let tabViewItem = this.selectedTab._tabViewTabItem;
>+            if (!tabViewItem) {
>+              TabView.hide();
>+              return;
>+            }
>+
>+            let win = TabView.getContentWindow();
>+
>+            win.UI.setActiveTab(tabViewItem);
>+            if (tabViewItem.parent)
>+              win.GroupItems.setActiveGroupItem(tabViewItem.parent);
>+            else {
>+              win.GroupItems.setActiveOrphanTab(tabViewItem);
>+              win.GroupItems.setActiveGroupItem(null);
>+            }
>+
>+            TabView.hide();
>+          }

I'd prefer not to mingle tab candy logic into tabbrowser.xml like this. It seems like this should be handled in the tab candy window when the TabSelect event occurs while it's open.

>       <method name="_tabOnTabClose">
>         <parameter name="aEvent"/>
>         <body><![CDATA[
>           var menuItem = aEvent.target.mCorrespondingMenuitem;
>           if (menuItem)
>             this.removeChild(menuItem);
>+          aEvent.originalTarget._tabClosed = true;
>         ]]></body>
>       </method>

This is the tabbrowser-alltabs-popup binding, where this certainly doesn't belong.

	Mihai Sucan [:msucan]
	Comment 24 • 14 years ago

(In reply to comment #23)
> Comment on attachment 508795 [details] [diff] [review]
> alternative fix
> 
> >       <handler event="DOMWillOpenModalDialog" phase="capturing">
> >         <![CDATA[
> >           if (!event.isTrusted)
> >             return;
> > 
> >           // We're about to open a modal dialog, make sure the opening
> >           // tab is brought to the front.
> >           this.selectedTab = this._getTabForContentWindow(event.target.top);
> >+
> >+          if (window.TabView && TabView.isVisible()) {
> >+            let tabViewItem = this.selectedTab._tabViewTabItem;
> >+            if (!tabViewItem) {
> >+              TabView.hide();
> >+              return;
> >+            }
> >+
> >+            let win = TabView.getContentWindow();
> >+
> >+            win.UI.setActiveTab(tabViewItem);
> >+            if (tabViewItem.parent)
> >+              win.GroupItems.setActiveGroupItem(tabViewItem.parent);
> >+            else {
> >+              win.GroupItems.setActiveOrphanTab(tabViewItem);
> >+              win.GroupItems.setActiveGroupItem(null);
> >+            }
> >+
> >+            TabView.hide();
> >+          }
> 
> I'd prefer not to mingle tab candy logic into tabbrowser.xml like this. It
> seems like this should be handled in the tab candy window when the TabSelect
> event occurs while it's open.

Agreed. This is an optional part of the code, as explained in comment 21. Also, I imagine that if the approach is acceptable, then I can do cleanup / improvements like moving this code into browser-tabview.js - I was thinking it belongs there.

I looked into TabView's UI onTabSelect when I worked on the patch, but the method description made me "not touch it": it is called when the user switches from one tab to another outside of the TabView UI. This is not the same as handling the switching out of TabView when a specific tab shows a prompt. Nonetheless, I agree the method can be expanded to handle this case as well. Perhaps we should ping Ian Gilman about this, if Dolske agrees we should follow this approach in fixing the bug.


> >       <method name="_tabOnTabClose">
> >         <parameter name="aEvent"/>
> >         <body><![CDATA[
> >           var menuItem = aEvent.target.mCorrespondingMenuitem;
> >           if (menuItem)
> >             this.removeChild(menuItem);
> >+          aEvent.originalTarget._tabClosed = true;
> >         ]]></body>
> >       </method>
> 
> This is the tabbrowser-alltabs-popup binding, where this certainly doesn't
> belong.

Ouch. Looked through the code more now. Setting tab._tabClosed = true might fit better in _beginRemoveTab where the TabClose event is also dispatched.

Thanks for your feedback Dao!

	Dão Gottwald [:dao]
	Comment 25 • 14 years ago

> I looked into TabView's UI onTabSelect when I worked on the patch, but the
> method description made me "not touch it": it is called when the user switches
> from one tab to another outside of the TabView UI. This is not the same as
> handling the switching out of TabView when a specific tab shows a prompt.

I don't think it needs to know anything about prompts. It should just handle tab selection not triggered by the UI while the UI is up.

> Ouch. Looked through the code more now. Setting tab._tabClosed = true might fit
> better in _beginRemoveTab where the TabClose event is also dispatched.

There's already a _removingTabs array, so you may not need this.

	Mihai Sucan [:msucan]
	Comment 26 • 14 years ago

Updated the patch based on feedback from Dao. Thanks!

Changes:

- moved tab._tabClosed to the _beginRemoveTab() method.

Dao: I think we need this, in case we keep stale references to tab objects. The _removingTabs array is spliced in _endRemoveTab(), which means code that may execute later might not be able to determine if the tab is closed or not. If you have ideas on how to improve this code without adding _tabClosed, please let me know.

- investigated better what TabView UI.onTabSelect() does. It seems to serve the exact purpose I had, but there was one tiny error. When DOMWillOpenModalDialog fires for the currently selectedTab, the UI.onTabSelect() method is not called (since the selectedTab did not change), so TabView is not hidden. Changed the code to call onTabSelect() manually when selectedTab == the previously selected tab. So, now, when a tabprompt shows in the selected tab, while the user is in panorama, the tab is zoomed into. (it worked in all other cases, actually - tabprompts were correctly causing TabView to zoom into their respective tabs)

The code is now only two liners in DOMWillOpenModalDialog. Not sure where else I should put it into. So, I did leave it there. Please let me know if this is fine.

Feedback for improvements is welcome! Thanks!

	Dão Gottwald [:dao]
	Comment 27 • 14 years ago

Comment on attachment 510560 [details] [diff] [review]
alternative fix (updated)

>+              canShowPrompt : function() {
>+                let tab = self._getTabForContentWindow(browser.contentWindow);
>+                return tab && !tab._tabClosed && !self._windowIsClosing &&

OK, so you can't use _removingTabs because the tab isn't in there anymore, but this should mean it's not in the tab bar either. The tab is removed before the browser. So how could _getTabForContentWindow possibly find that tab?

>       <handler event="DOMWillOpenModalDialog" phase="capturing">
>         <![CDATA[
>           if (!event.isTrusted)
>             return;
> 
>           // We're about to open a modal dialog, make sure the opening
>           // tab is brought to the front.
>+          let previousTab = this.selectedTab;
>           this.selectedTab = this._getTabForContentWindow(event.target.top);
>+          if (previousTab == this.selectedTab &&
>+              window.TabView && TabView.isVisible()) {
>+            let win = TabView.getContentWindow();
>+            win.UI.onTabSelect(this.selectedTab);
>+          }

I still don't like this and would prefer if you filed a separate bug on it.

	Mihai Sucan [:msucan]
	Comment 28 • 14 years ago

(In reply to comment #27)
> Comment on attachment 510560 [details] [diff] [review]
> alternative fix (updated)
> 
> >+              canShowPrompt : function() {
> >+                let tab = self._getTabForContentWindow(browser.contentWindow);
> >+                return tab && !tab._tabClosed && !self._windowIsClosing &&
> 
> OK, so you can't use _removingTabs because the tab isn't in there anymore, but
> this should mean it's not in the tab bar either. The tab is removed before the
> browser. So how could _getTabForContentWindow possibly find that tab?

Thought of only relying on _getTabForContentWindow() to fail, but I was not sure if that's going to work. _beginRemoveTab() does not remove the tab and the browser from the this.browsers and this.tabs arrays. That only happens in _endRemoveTab() which may be too late for canShowPrompt(). If you believe I can rely on _getTabForContentWindow() returning null, then I can remove tab._tabClosed.


> >       <handler event="DOMWillOpenModalDialog" phase="capturing">
> >         <![CDATA[
> >           if (!event.isTrusted)
> >             return;
> > 
> >           // We're about to open a modal dialog, make sure the opening
> >           // tab is brought to the front.
> >+          let previousTab = this.selectedTab;
> >           this.selectedTab = this._getTabForContentWindow(event.target.top);
> >+          if (previousTab == this.selectedTab &&
> >+              window.TabView && TabView.isVisible()) {
> >+            let win = TabView.getContentWindow();
> >+            win.UI.onTabSelect(this.selectedTab);
> >+          }
> 
> I still don't like this and would prefer if you filed a separate bug on it.

Sure. I can do that, but it is related to the issue we are having here, in this bug - this is why I tackled the problem. Shall I file a separate bug specific to this portion of code and submit this as a patch there? Should it be a bug for the Firefox TabCandy component?

Thanks again for looking into the patch, and for your comments!

	Dão Gottwald [:dao]
	Comment 29 • 14 years ago

(In reply to comment #28)
> Thought of only relying on _getTabForContentWindow() to fail, but I was not
> sure if that's going to work. _beginRemoveTab() does not remove the tab and the
> browser from the this.browsers and this.tabs arrays. That only happens in
> _endRemoveTab() which may be too late for canShowPrompt().

If canShowPrompt is called before _endRemoveTab, then the tab is going to be in _removingTabs array.

> If you believe I can
> rely on _getTabForContentWindow() returning null, then I can remove
> tab._tabClosed.

I think it might return a bogus tab rather than null, depending on whether the browser is still in the browsers array.

> Sure. I can do that, but it is related to the issue we are having here, in this
> bug - this is why I tackled the problem.

It seemed only remotely related to me, but maybe I'm missing something. Didn't you say it was optional?

	Dão Gottwald [:dao]
	Comment 30 • 14 years ago

(In reply to comment #29)
> > If you believe I can
> > rely on _getTabForContentWindow() returning null, then I can remove
> > tab._tabClosed.
> 
> I think it might return a bogus tab rather than null, depending on whether the
> browser is still in the browsers array.

I just checked this. The browsers array is built based on the tabs node list, so _getTabForContentWindow will reliably return null once the tab isn't in the _removingTabs array anymore.

	Justin Dolske [:Dolske] Assignee
	Comment 31 • 14 years ago

Comment on attachment 510560 [details] [diff] [review]
alternative fix (updated)

So, I think this is generally ok (modulo the stuff dao already f-'d). I'd like to poke around to see if there's a better way to determine this kind of state from the content document instead of checking tabbrowser properties... But the current way seems fine.

	Johnathan Nightingale [:johnath]
	Comment 32 • 14 years ago

(In reply to comment #31)
> Comment on attachment 510560 [details] [diff] [review]
> alternative fix (updated)
> 
> So, I think this is generally ok (modulo the stuff dao already f-'d). I'd like
> to poke around to see if there's a better way to determine this kind of state
> from the content document instead of checking tabbrowser properties... But the
> current way seems fine.

So... [needs review]? Or [needs updated patch]?

	Mihai Sucan [:msucan]
	Comment 33 • 14 years ago

(In reply to comment #29)
> > Sure. I can do that, but it is related to the issue we are having here, in this
> > bug - this is why I tackled the problem.
> 
> It seemed only remotely related to me, but maybe I'm missing something. Didn't
> you say it was optional?

Yes, I did say that, but one of the things discussed in the comments above, with Dolske, was that it would be good to properly give focus to the tabs that show prompts - to avoid the issue of users not seeing their action is needed. Ultimately, yes, it's optional.

Will attach an updated patch with that removed, and without tab._tabClosed.

Thanks Dao and Justin for your feedback!

	Mihai Sucan [:msucan]
	Comment 34 • 14 years ago

Updated the patch as requested by Dao.

I checked again the STR from comment 0: cannot repro.

Comment 5: WFM in mozilla-central as well (it seems this has been fixed already?).

Comment 9: still fails in m-c, but it works as expected when this patch is applied. The problem in bug 626455 is no longer fixed by this patch (for some reason). A short investigation shows that Panorama calls removeTab() twice for the same tab, and that breaks tabbrowser badly, ending in an infinite loop that endlessly calls _endRemoveTab(). If I find the problem, I'll submit a patch in that bug - since the issue seems to be sufficiently distinct.

	Dão Gottwald [:dao]
	Comment 35 • 14 years ago

Comment on attachment 511006 [details] [diff] [review]
alternative fix (update 2)

>+  window.browserIsShuttingDown = true;

Please rename this. E.g.: window.windowIsClosing = true;
It's too bad that the function is called BrowserShutdown, since it doesn't actually shut down the browser.

>+              canShowPrompt : function() {
>+                let tab = self._getTabForContentWindow(browser.contentWindow);
>+                return tab && !self._windowIsClosing &&
>+                       !window.browserIsShuttingDown;

I'd recommend rewriting these lines like this:

return !window.windowIsClosing &&
       !self._windowIsClosing &&
       self.browsers.indexOf(browser) > -1;

By the way, are you sure that self._windowIsClosing is actually needed here?

	Mihai Sucan [:msucan]
	Comment 36 • 14 years ago

(In reply to comment #35)
> Comment on attachment 511006 [details] [diff] [review]
> alternative fix (update 2)
> 
> >+  window.browserIsShuttingDown = true;
> 
> Please rename this. E.g.: window.windowIsClosing = true;
> It's too bad that the function is called BrowserShutdown, since it doesn't
> actually shut down the browser.

Yeah, I named the property because the function is called BrowserShutdown.


> >+              canShowPrompt : function() {
> >+                let tab = self._getTabForContentWindow(browser.contentWindow);
> >+                return tab && !self._windowIsClosing &&
> >+                       !window.browserIsShuttingDown;
> 
> I'd recommend rewriting these lines like this:
> 
> return !window.windowIsClosing &&
>        !self._windowIsClosing &&
>        self.browsers.indexOf(browser) > -1;

Sure, that looks good, and equivalent to what I have. I'll update the patch accordingly.


> By the way, are you sure that self._windowIsClosing is actually needed here?

Yes, because there's logic in tabbrowser that seems to prevent further tab closes when the window is closing - to fast-path the execution. If that happens, canShowPrompt() must return false - since any attempt to show a prompt would be futile. Please correct me if my understanding of the code is wrong.


Thanks for your continued feedback, Dao!

	Mihai Sucan [:msucan]
	Comment 37 • 14 years ago

Updated the patch based on Dão's feedback.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 38 • 14 years ago

Just sayin', this is the last front end hard blocker ...

	Justin Dolske [:Dolske] Assignee
	Comment 39 • 14 years ago

Comment on attachment 511043 [details] [diff] [review]
alternative fix (update 3)

This doesn't seem to fix the problem described in comment 0. Well, maybe it fixes the "sometimes window-modal, sometimes tab-modal" issue, I always seem to get a tab-modal prompt so it's hard to say.

But after clicking [x] on the tab a second time, closing another tab hangs the browser: I still get a slow-script warning for tabbrowser.xml, and from that point on the browser is broken.

Looking into this...

	Justin Dolske [:Dolske] Assignee
	Comment 40 • 14 years ago

The tabbrowser.xml slow-script is an infinite loop, _endRemoveTab() has become confused and is looping over a 1-element _removingTabs, calling _endRemoveTab() on it to no effect.

The problem actually starts earlier, and is a result of closing a tab which performs a blocking call [like alert()] in its onbeforeunload handler... The first time you click the testcase's close button, tabbrowser's _beginRemoveTab() calls docShell.contentViewer.permitUnload() -- this is where the onbeforeunload event is fired. The second time you click the close button, we're still blocked halfway through the first call of _beginRemoveTab(), but we're now running through it again. permitUnload() immediately returns here (it explicitly does so if called recursively), and so the tab is closed. That results in the alert() being torn down, the first call to permitUnload() returning, and the original invocation of _beginRemoveTab() finishing execution.

I haven't looked at _exactly_ what's going wrong in tabbrowser, but it seems quite likely that it's just not expecting this to have happened.

So, a few approaches to fixing this:

1) Fix tabbrowser to deal with this case. Needs more investigation (which I'll do, because I want to understand this better). But there are other places in the browser that call permitUnload(), and similar work might be needed to prevent similar problems in those codepaths.

2) Block all prompt requests while running content |beforeunload| event handlers. This is the long-term goal, but is kind of late to do now. [OTOH, if we're ever going to do this, there isn't much remedy other than to tell affected sites to stop doing that.]

3) Disallow tab-modal requests while running content |beforeunload| event handlers. This sidesteps the problem by returning to the previous state of affairs, where the double _beginRemoveTab()/permitUnload() invocation problem described above can't happen [because the prompt is window-modal, and you can't close the tab while it's showing.]

#3 seems like the safest approach at the moment. It's unfortunate that abusive sites could annoy users with a modal prompt here, but at least the fix from bug 61098 makes it possible to for the user to escape.

	Justin Dolske [:Dolske] Assignee
	Comment 41 • 14 years ago

A little quick and dirty, but this eliminates the problematic behavior per my last comment. I should probably write a test. :)

This patch probably ought to instead add a new interface to nsIContentViewer, to peek at its mPermitUnload (instead of stuffing it into the window), but it seemed less of a hassle to add bits to nsPIDOMWindow than to add a new nsIContentViewer_MOZILLA_2_0 interface to nsDocumentViewer.cpp. Forgive me for I am just a simple front-end hacker. :)

Net upshot of this patch is that any standard DOM prompt opened from a |beforeunload| handler will be window-modal instead of tab-modal.

	Mihai Sucan [:msucan]
	Comment 42 • 14 years ago

(In reply to comment #39)
> Comment on attachment 511043 [details] [diff] [review]
> alternative fix (update 3)
> 
> This doesn't seem to fix the problem described in comment 0. Well, maybe it
> fixes the "sometimes window-modal, sometimes tab-modal" issue, I always seem to
> get a tab-modal prompt so it's hard to say.
> 
> But after clicking [x] on the tab a second time, closing another tab hangs the
> browser: I still get a slow-script warning for tabbrowser.xml, and from that
> point on the browser is broken.
> 
> Looking into this...

Weird that I cannot repro the problem in comment 0.

(In reply to comment #40)
> The tabbrowser.xml slow-script is an infinite loop, _endRemoveTab() has become
> confused and is looping over a 1-element _removingTabs, calling _endRemoveTab()
> on it to no effect.
> 
> [...]

Yep, exactly what I noticed in bug 626455 comment 8.


> So, a few approaches to fixing this:
> 
> 1) Fix tabbrowser to deal with this case. Needs more investigation (which I'll
> do, because I want to understand this better). But there are other places in
> the browser that call permitUnload(), and similar work might be needed to
> prevent similar problems in those codepaths.

I tried this, but it leads to no end. Besides, the _beginRemoveTab() function still stays locked/blocked on the content viewer permitUmload() call forever. Fixing this case would actually only fix the problem of "infinite loop" inside tabbrowser. We could make it behave nicer (don't get stuck), but the error would simply be hidden.


> 3) Disallow tab-modal requests while running content |beforeunload| event
> handlers. This sidesteps the problem by returning to the previous state of
> affairs, where the double _beginRemoveTab()/permitUnload() invocation problem
> described above can't happen [because the prompt is window-modal, and you can't
> close the tab while it's showing.]
> 
> #3 seems like the safest approach at the moment. It's unfortunate that abusive
> sites could annoy users with a modal prompt here, but at least the fix from bug
> 61098 makes it possible to for the user to escape.

Yeah, best choice. It works better like this, indeed.

	Mihai Sucan [:msucan]
	Comment 43 • 14 years ago

Comment on attachment 512108 [details] [diff] [review]
Patch v.1

This patch doesn't fix the hang in comment 9. So, perhaps we need both patches? Or we want a different solution for comment 9 than the approach I had in mind with the patch I submitted.

Nonetheless, I think we need to fix the case in comment 9 as well.

	Nochum Sossonko [:Natch]
	Comment 44 • 14 years ago

I think the best way to fix this is to focus the tab when the user tries to close the tab the first time, then close it (skip permitUnload entirely) if the user attempts to close it again. This is how Firefox works today (try it with two windows, one with an onbeforeunload handler and hit File->Exit from the other window. Then hit Exit again and FF will exit disregarding the onbeforeunload handler).

The reasoning is simple: the user must see the onbeforeunload prompt because it warns the user of a potential loss of data. However, once the user has seen it, it should be the user prerogative to completely ignore it.

	Nochum Sossonko [:Natch]
	Comment 45 • 14 years ago

Oh, and I missed one thing, the onbeforeunload handler must be called when the window is closed, so the tab tear down (with a return from the alert call so that permitUnload may continue) must happen before the window is closed.

	Nochum Sossonko [:Natch]
	Comment 46 • 14 years ago

The problem seems to lie here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/nsPrompter.js#468

Throwing an exception when the window is closed without addressing the prompt is wrong, IMO. It should just return null/void/false whatever is appropriate. That may fix the issue here and cause the onbeforeunload prompt to show properly.

	Nochum Sossonko [:Natch]
	Comment 47 • 14 years ago

This is how the flow should work...the [x] button on the tab now effectively cancels the prompt on the page and will bring up the onbeforeunload handler.

	Johnny Stenback (:jst)
	Comment 48 • 14 years ago

Comment on attachment 512108 [details] [diff] [review]
Patch v.1

I think I would prefer the new nsIContentViewer interface rather than changing nsPIDOMWindow, which is technically an interface as well. Other than that this looks good to me.

	Justin Dolske [:Dolske] Assignee
	Comment 49 • 14 years ago

(In reply to comment #43)

> This patch doesn't fix the hang in comment 9.

Sigh. This is while rambling bugs that cover multiple issues should be avoided.

I think I can just tweak my current patch to fix this case as well. This hang stack is conveniently going through nsDocumentViewer::PageHide, (where |pagehide| is fired), so we can just disable tab-modal prompts from this path in the same way.

	Justin Dolske [:Dolske] Assignee
	Comment 50 • 14 years ago

(In reply to comment #44)
> I think the best way to fix this is to focus the tab when the user tries to
> close the tab the first time, then close it (skip permitUnload entirely) 

Uhh, we can't skip PermitUnload because that's where the normal Stay On Page / Leave Page dialog gets invoked.

(In reply to comment #46)
> nsPrompter.js#468
> 
> Throwing an exception when the window is closed without addressing the prompt
> is wrong, IMO.

I don't see how that's related, the exception gets thrown at content. Chrome code wouldn't see it, and shouldn't have any effect on this bug.

	Justin Dolske [:Dolske] Assignee
	Comment 51 • 14 years ago

Tweaked version of Patch v.1 that does not hang with the comment 9 testcase.

Will address review comments next.

	Nochum Sossonko [:Natch]
	Comment 52 • 14 years ago

(In reply to comment #50)
> (In reply to comment #44)
> > I think the best way to fix this is to focus the tab when the user tries to
> > close the tab the first time, then close it (skip permitUnload entirely) 
> 
> Uhh, we can't skip PermitUnload because that's where the normal Stay On Page /
> Leave Page dialog gets invoked.

I meant, bypass the call to permitUnload to allow the first to complete properly, but it's irrelevant really to this bug.
> (In reply to comment #46)
> > nsPrompter.js#468
> > 
> > Throwing an exception when the window is closed without addressing the prompt
> > is wrong, IMO.
> 
> I don't see how that's related, the exception gets thrown at content. Chrome
> code wouldn't see it, and shouldn't have any effect on this bug.

It gets thrown during the execution of the onbeforeunload code which halts the execution at that point. See the difference by just commenting out that line and rebuilding.

Basically, the flow is as follows:

1) User hits [x], _beginRemoveTab calls permitUnload
2) permitUnload runs onbeforeunload js checking for a return value
3) during the execution (before a return value is hit) the js code throws up a prompt
4) the user hits [x] again to close the tab
5) it gets hairy

with my patch, it goes like this:

5) _beginRemoveTab senses that it is trying to tear down this tab already and is being blocked, so it tears down all the prompts (without throwing an exception)
6) the js code in onbeforeunload continues and gets the return value and throws up the "Leave Page" prompt.

	Justin Dolske [:Dolske] Assignee
	Comment 53 • 14 years ago

Now with less laziness. :)

	Justin Dolske [:Dolske] Assignee
	Comment 54 • 14 years ago

Comment on attachment 512392 [details] [diff] [review]
Patch v.3

(Also, will write some tests.)

	Justin Dolske [:Dolske] Assignee
	Comment 55 • 14 years ago

(In reply to comment #52)

> > Uhh, we can't skip PermitUnload because that's where the normal Stay On Page /
> > Leave Page dialog gets invoked.
> 
> I meant, bypass the call to permitUnload to allow the first to complete
> properly, but it's irrelevant really to this bug.

It's not, because PermitUnload fires beforeunload, and a tab-modal alert there can cause the same kinds of problems. We have to call it eventually.

	Justin Dolske [:Dolske] Assignee
	Comment 56 • 14 years ago

Moving this to a final+ blocker, I don't think there's anything here that requires beta coverage. The problem and fix is well understood.

	Johnny Stenback (:jst)
	Comment 57 • 14 years ago

Comment on attachment 512392 [details] [diff] [review]
Patch v.3

+  PRBool allowTabModal = PR_TRUE;
+  if (mDocShell) {
+    nsCOMPtr<nsIContentViewer> cv;
+    mDocShell->GetContentViewer(getter_AddRefs(cv));
+    nsCOMPtr<nsIContentViewer_MOZILLA_2_0_BRANCH> cv2 = do_QueryInterface(cv);
+    if (cv2)
+      cv2->GetIsTabModalPromptAllowed(&allowTabModal);
+  }

This is repeated 3 times in this file, please add a bool GetIsTabModalPromptAllowed() method to nsGlobalWindow, and call that in the 3 places where this code appears.

r=jst with that.

	(no longer active)
	Comment 58 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/ecb0354f7c09

	Biju Reporter
	Comment 59 • 14 years ago

(In reply to comment #58)
> http://hg.mozilla.org/mozilla-central/rev/ecb0354f7c09

Just curious, still we have 
+  PRBool allowTabModal = PR_TRUE;
+  if (mDocShell) {
+    nsCOMPtr<nsIContentViewer> cv;
+    mDocShell->GetContentViewer(getter_AddRefs(cv));
+    nsCOMPtr<nsIContentViewer_MOZILLA_2_0_BRANCH> cv2 = do_QueryInterface(cv);
+    if (cv2)
+      cv2->GetIsTabModalPromptAllowed(&allowTabModal);
+  }
repeated 3 times, is that what jst wanted...

	Dão Gottwald [:dao]
	Comment 60 • 14 years ago

(In reply to comment #59)
> repeated 3 times, is that what jst wanted...

no...

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 61 • 14 years ago

How about closing this hardblocker and file a follow up for the consolidation?

	Dave Townsend [:mossop]
	Comment 62 • 14 years ago

I've addressed the review comments in http://hg.mozilla.org/mozilla-central/rev/104a6d17e013

	(no longer active)
	Comment 63 • 14 years ago

Sorry, I missed the status whiteboard tag regarding the nits before landing.  :(