Closed Bug 645552 Opened 9 years ago Closed 9 years ago

Thunderbird's tabmail does not honor onbeforeunload event handlers

Categories

(Thunderbird :: Toolbars and Tabs, defect, critical)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 5.0b1

People

(Reporter: protz, Assigned: protz)

Details

(Keywords: dataloss)

Attachments

(1 file, 2 obsolete files)

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.
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.
Severity: normal → critical
Keywords: dataloss
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.
Assignee: nobody → jonathan.protzenko
Status: NEW → ASSIGNED
Attachment #522466 - Flags: review?(bugzilla)
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.
Attachment #522466 - Flags: review?(bugzilla) → review+
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 :-).
Attachment #522466 - Attachment is obsolete: true
Attachment #525069 - Flags: review+
Attachment #525069 - Flags: feedback?(bugmail)
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 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.
Attachment #525069 - Flags: feedback?(bugmail) → feedback-
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)]
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!
Attachment #525069 - Attachment is obsolete: true
Attachment #525331 - Flags: review+
Attachment #525331 - Flags: feedback?(bugmail)
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.
Attachment #525331 - Flags: review?(bugzilla)
Attachment #525331 - Flags: review+
Attachment #525331 - Flags: feedback?(bugmail)
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 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 :-).
Er, no, Mark did change his email address!
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.
Attachment #525331 - Flags: review?(mbanner) → review+
http://hg.mozilla.org/comm-central/rev/95f70d4b2a83
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4
You need to log in before you can comment on or make changes to this bug.