645552 - Thunderbird's tabmail does not honor onbeforeunload event handlers

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Jonathan Protzenko [:protz]]

STR:
- open a Gmail content tab,
- compose a new message,
- type some text in the message body,
- close the tab.

In Firefox, this fires the "are you sure you want to navigate away from this page?" dialog. In Thunderbird, this closes the tab right away. I believe Thunderbird needs to do something regarding beforeunload event handlers, and possibly give them a chance to run before actually closing the tab.

Comment 1

[Wayne Mery (:wsmwk)]

the example you cite can lead to dataloss. (unless it's actually possible to undo the tab - but the user might not remember there's changed data in the closed tab)

onbeforeunload of course must be programmed in the page for this to have an effect. But if you indulge me to go on a tangent and cite a broader firefox situation, see my bug 370318 and its friends. It's easy for a tab to have data in it (a bug report for example) that was altered days or weeks ago and thus the user doesn't remember. Now, one can't totally dummy proof a browser. Still, I find the firefox position on this issue a bit disturbing. (as well as mconnor's lack of reply to bug 370318 comment 7)

I hope thunderbird might do better in this area than firefox, which as far as I know has no add-ons to mitigate the problem.

Comment 2

[Jonathan Protzenko [:protz]]

Attachment: A patch that does the right thing™

So this should basically fix the issue. The thing is we should offer tab types a chance to refuse being closed. I make this an optional argument of the tab type, and I add an implement of that new tryCloseTab method for both content tabs and chrome tabs.

In the case of these two tab types, what happens is the method asks the browser that holds the (chrome|content)Page, through its content viewer, if unload is permitted (nsIContentViewer.permitUnload()). That permitUnload function takes care of running onbeforeunload event listeners, and returns false if one of them canceled the unload.

I'd say this is pretty trivial and we don't need a test for this.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 522466 [details] [diff] [review]
A patch that does the right thing™

>+      // false means: we can't close the tab yet
>+      if (docShell && docShell.contentViewer && !docShell.contentViewer.permitUnload())
>+        return false;
>+      else
>+        return true;

It would be better to just do:

return !(docShell && docShell.contentViewer && !docShell.contentViewer.permitUnload());

I think we should have a test for this, as it is one of those features that although small, could easily get broken and no-one notice for ages. Plus I suspect the existing content-tab tests should provide a reasonable structure for this already. You can land it before the test, but I would like to get a test for this.

Comment 4

[Jonathan Protzenko [:protz]]

Attachment: New version with a tentative test

Andrew, could you please take a look at this? I think I'm doing everything right, but as soon as I enter the inner mozmill loop, and try to close the modal dialog through the doCommand() call, it comes back right away. I've got to be doing something wrong... calling target.doCommand() through the DOM Inspector does close the dialog properly, but doing the same thing through mozmill doesn't close the dialog.

I have no ideas right now, so I thought you might have one :-).

Comment 5

[Mark Banner (:standard8)]

Did you try doing a try/catch dump/alert in the callback function? I've found that errors typically get hidden and the effect is that the dialog does just get closed.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 525069 [details] [diff] [review]
New version with a tentative test

This isn't an odd mozmill issue per se; I gdb'ed it and the problem is that nsGlobalWindow::Close calls nsGlobalWindow::CanClose which calls DocumentViewerImpl::PermitUnload (and it's not calling it with aCallerClosesWindow=true).

THEN

your code goes and calls PermitUnload directly.

The box really is popping up twice.  Presumably you should be making sure that only one dude calls the method or that the first dude calls it in such a way that the second dude fast-paths out.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Here's the JS call stack of the second time if this helps at all:

0 onTryCloseTab(aTab = [object Object]) ["chrome://messenger/content/specialTabs.js":451]
    docShell = [xpconnect wrapped (nsISupports, nsIDocShell, nsIWebNavigation, nsIDocShellHistory, nsIInterfaceRequestor, nsIWebProgress) @ 0x7f6f72d716a0 (native @ 0x7f6f60791808)]
    this = [object Object]
1 closeTab(aNoUndo = undefined, aOptTabIndexNodeOrInfo = [object Object]) ["chrome://messenger/content/tabmail.xml":720]
    closeFunc = undefined
    tryCloseFunc = [function]
    tabNode = [object XULElement @ 0x7f6f6fcd4d60 (native @ 0x7f6f6078e580)]
    tab = [object Object]
    iTab = 2
    this = [object XULElement @ 0x7f6f6f0f1630 (native @ 0x7f6f6f603800)]
2 onDOMWindowClose(aEvent = [object Event @ 0x7f6f6aa9b7b0 (native @ 0x7f6f6a256460)]) ["chrome://messenger/content/specialTabs.js":596]
    this = [object XULElement @ 0x7f6f72032970 (native @ 0x7f6f6078ed80)]

Comment 8

[Jonathan Protzenko [:protz]]

Attachment: New version with a working test

Thanks a lot for your help debugging this. Actually, changing the way I close the tab fixed the issue (I'm now using mc.tabmail.closeTab). I'll just let you check that this is ok, then I guess I'll be able to commit this. Thanks again!

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 525331 [details] [diff] [review]
New version with a working test

I agree that changing the test should make the test pass, but it seems like window.close() being invoked inside a content window is still a thing that can happen and should also be tested; and I don't believe you've fixed that.  I say this because I believe we went out of our way to hook up the window.close() stuff, so we shouldn't pretend it's not a thing.

In any event, this is really Standard8's call, so I'm asking for review again because you've materially changed what is tested.  I would personally suggest we test both cases and make sure the prompt only comes up once.

Comment 10

[Jonathan Protzenko [:protz]]

Thanks. The problem with window.close from inside a content/chrome tab is something that's being tracked in bug 638493 (which I incidentally also reported, although I can't seem to figure out why I didn't remember it earlier). Long story short, I don't think anyone right now has a clue as to why it fails...

Comment 11

[Jonathan Protzenko [:protz]]

Comment on attachment 525331 [details] [diff] [review]
New version with a working test

I think the review requestee was wrong, which might explain Mark not giving his opinion on this. New attempt :-).

Comment 12

[Jonathan Protzenko [:protz]]

Er, no, Mark did change his email address!

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 525331 [details] [diff] [review]
New version with a working test


>+            // This gives contentTabs and chromeTabs a chance to run their inner
>+            // onbeforeunload event listeners, who might want to cancel the
>+            // tab's closing...
>+            let tryCloseFunc = tab.mode.tryCloseTab || tab.mode.tabType.tryCloseTab;
>+            if (tryCloseFunc && !tryCloseFunc.call(tab.mode.tabType, tab))
>+              return;

I don't think you need the comment here. If you want to keep it, then I think we can just make it a generic comment and not mention content/chromeTabs directly.

Comment 14

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/comm-central/rev/95f70d4b2a83