545933 - deleting messages sometimes throws an uncaught exception in ClearMessagePane

Status: NEW

(Thunderbird :: Mail Window Front End, defect)

Description

[David :Bienvenu]

I've noticed this several times recently - deleting all the messages in my junk folders throws an exception in ClearMessagePane. This exception isn't caught, and we end up with a null gMsgDBView, and the 3 pane window is essentially broken. We can't load messages or switch folders. Catching the exception here:

function ClearMessagePane()
{
  // hide the message header view AND the message pane...
  HideMessageHeaderPane();
  gMessageNotificationBar.clearMsgNotifications();
  ClearPendingReadTimer();
  try {
    GetMessagePaneFrame().location.href = "about:blank";
  } catch(ex) {dump("error clearing message pane " + ex + ex.stack);}

fixes the problem. The href setter throws an exception, but I've forgotten what it was, and I can't reproduce the issue at the moment.

Comment 1

[Rain [:rain1] (inactive)]

IRC logs indicate that the exception is:

Error: uncaught exception: [Exception... "Component returned failure code: 0x805e0006 [nsIDOMLocation.href]"  nsresult: "0x805e0006 (<unknown>)"  location: "JS frame :: chrome://messenger/content/msgMail3PaneWindow.js :: ClearMessagePane :: line 882"  data: no]

Comment 2

[Blake Winton (:bwinton) (:☕️)]

So http://silver.warwickcompsoc.co.uk/mozilla/misc/nserror?0x805E0006 says that that error is:
Module          Severity        Number
CONTENT (25)	Failure (1)	6

And http://scotland.proximity.on.ca/dxr/mozilla-central/content/base/public/nsContentErrors.h.html#l59 says that Content Error 6 is:
NS_ERROR_CONTENT_BLOCKED

Which occurs, among other places, at http://mxr.mozilla.org/comm-central/source/mozilla/docshell/base/nsDocShell.cpp#7643

So I set a bunch of breakpoints, and ended up with the following backtrace:
(gdb) bt
#0  nsDocShell::InternalLoad (this=0x1f7558a0, aURI=0x1bfec200, aReferrer=0x1f038a44, aOwner=0x0, aFlags=1, aWindowTarget=0x1ff2bea0, aTypeHint=0x0, aPostData=0x0, aHeadersData=0x0, aLoadType=134217729, aSHEntry=0x0, aFirstParty=1, aDocShell=0x0, aRequest=0x0) at /Users/bwinton/Programming/thunderbird/reviews-central/mozilla/docshell/base/nsDocShell.cpp:7643
#1  0x152d9daa in nsDocShell::LoadURI (this=0x1f7558a0, aURI=0x1bfec200, aLoadInfo=0x1c180d20, aLoadFlags=0, aFirstParty=1) at /Users/bwinton/Programming/thunderbird/reviews-central/mozilla/docshell/base/nsDocShell.cpp:1369
#2  0x13dc7581 in nsLocation::SetURI (this=0x1597c070, aURI=0x1bfec200, aReplace=0) at /Users/bwinton/Programming/thunderbird/reviews-central/mozilla/dom/base/nsLocation.cpp:316
#3  0x13dc8af3 in nsLocation::SetHrefWithBase (this=0x1597c070, aHref=@0xbfffb96c, aBase=0x859e20, aReplace=0) at /Users/bwinton/Programming/thunderbird/reviews-central/mozilla/dom/base/nsLocation.cpp:595
#4  0x13dc8cf9 in nsLocation::SetHrefWithContext (this=0x1597c070, cx=0x1132600, aHref=@0xbfffb96c, aReplace=0) at /Users/bwinton/Programming/thunderbird/reviews-central/mozilla/dom/base/nsLocation.cpp:542
#5  0x13dc9120 in nsLocation::SetHref (this=0x1597c070, aHref=@0xbfffb96c) at /Users/bwinton/Programming/thunderbird/reviews-central/mozilla/dom/base/nsLocation.cpp:510
#6  0x003fa28e in NS_InvokeByIndex_P (that=0x1597c070, methodIndex=10, paramCount=1, params=0xbfffb53c) at /Users/bwinton/Programming/thunderbird/reviews-central/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179
#7  0x120f88bb in XPCWrappedNative::CallMethod (ccx=@0xbfffb8fc, mode=XPCWrappedNative::CALL_SETTER) at /Users/bwinton/Programming/thunderbird/reviews-central/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2727
#8  0x1210c0c6 in XPCWrappedNative::SetAttribute (ccx=@0xbfffb8fc) at xpcprivate.h:2550
#9  0x12105530 in XPC_WN_GetterSetter (cx=0x1132600, obj=0x1a05fce0, argc=1, argv=0x1149464, vp=0xbfffba10) at /Users/bwinton/Programming/thunderbird/reviews-central/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1792
#10 0x001141a7 in js_Invoke (cx=0x1132600, argc=1, vp=0x114945c, flags=2) at jsinterp.cpp:1388
[…]

And it looks like we get there because the return value of NS_CheckContentLoadPolicy is 0x80004003, or NS_ERROR_INVALID_POINTER.

I'll do some more digging after dinner.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Attachment: A diff to add some debug info (and a place to put a breakpoint).

So, I've tracked it down a little further, and if you place a breakpoint on the "\n\nAAAAAA\nrv5=%x\n" line, you can see that it's the
folder = do_QueryInterface(subFolder, &rv);
line which is causing the failure, because the subFolder's mRawPtr is null.

But I have no idea why _that_'s happening, so I'm going to post what I have, and hopefully someone else can take it and run.

Thanks,
Blake.

Comment 4

[David :Bienvenu]

I'm going to take this, because this bug makes (unpatched) builds pretty much unusable for me.

Comment 5

[David :Bienvenu]

This turned out to be an issue when you have multiple accounts on the same server, such that we need the username to correctly find a server from a URI.

The location code clears the username here:

 	necko.dll!nsStandardURL::SetUserPass(const nsACString_internal & input={...})  Line 1202	C++
 	msgbsutl.dll!nsMsgMailNewsUrl::SetUserPass(const nsACString_internal & aUserPass={...})  Line 429	C++
 	docshell.dll!nsDefaultURIFixup::CreateExposableURI(nsIURI * aURI=0x088e12f0, nsIURI * * aReturn=0x003c989c)  Line 142	C++
>	gklayout.dll!nsLocation::GetURI(nsIURI * * aURI=0x003c989c, int aGetInnermostURI=0x00000000)  Line 280 + 0x26 bytes	C++

in order to make a url that can be publicly used, but we need that info to figure out the server. So, we have a couple choices; make SetUserPass a noop for our mailnews urls (we never put passwords in the url, but we do put the user and server in them), or just handle the exception, given that this is pretty specific to the location.href code.

Comment 6

[David :Bienvenu]

Attachment: ameliorating patch - checked in

I'd like to land this for 3.1 beta 1, since it's an enormous pain for people with multiple accounts on the same server.

Comment 7

[Rain [:rain1] (inactive)]

This seems like it should be testable with mozmill, unless it only shows up for IMAP messages.

Comment 8

[David :Bienvenu]

the test case I have is imap

Comment 9

[David :Bienvenu]

also, the code that's returning the exception is imap code: nsImapService::NewURI, per https://bugzilla.mozilla.org/attachment.cgi?id=427516

Comment 10

[Rain [:rain1] (inactive)]

Comment on attachment 428335 [details] [diff] [review]
ameliorating patch - checked in

>-  GetMessagePaneFrame().location.href = "about:blank";
>+  try {
>+    GetMessagePaneFrame().location.href = "about:blank";
>+  } catch(ex) {dump("error clearing message pane " + ex + ex.stack);}

- Please add a comment explaining what's going wrong here.
- Please have the dump statement on a new line.
- We have an exception logging helper now, so logException(ex, false, "error clearing message pane") is preferred I guess.
- Per IRC discussion, a) this does not actually succeed in clearing the message pane, and b) extension developers will also hit this -- so please leave the bug open until we have a proper fix for the issue.

Comment 11

[David :Bienvenu]

taking off b1 blocking list. patch addressing comments has been checked in.

Comment 12

[David :Bienvenu]

What we need now is a way to make cloning those imap uri's not fail. Making SetUserPass a noop is the most straightforward fix but I'm worried about the privacy concerns.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

We're resetting the blocking flag for 3.1 on this bug and instead setting the wanted-thunderbird+ flag. We have too many blocking-3.1 bugs, to the point where it doesn't mean much, and managing the list is making it hard to actually work on closing bugs, which helps no one.

Thunderbird 3.1's primary purpose is to allow us to offer a prompted major update to Thunderbird 2 users, to ensure their continued ability to safely use Thunderbird.  Thunderbird 2 is built on an outdated version of Gecko, and our long-term ability to maintain the users' safety for Thunderbird 2 users is limited.

If you think this bug meets the requirements below, please renominate with a detailed explanation of how it meets the following two criteria, and we will reconsider.  To qualify, this bug must either:

a) make the upgrade experience from TB2 very painful for a large number of users

or

b) be a new, reproducible, severe quality issue (eg dataloss, frequent crashes)

Just because this bug doesn't block TB3.1 doesn't mean it can't or won't make the release.  Once they're done with their blockers (if any), we encourage developers to keep working on non-blocking bugs, and to try to land them as early in the cycle as possible, as non-blocking bugs will become increasingly difficult to land in the later stages of the cycle.

Comment 14

[Wayne Mery (:wsmwk)]

(In reply to David :Bienvenu from comment #12)
> What we need now is a way to make cloning those imap uri's not fail. Making
> SetUserPass a noop is the most straightforward fix but I'm worried about the
> privacy concerns.

perhaps "[needs patch to making clearing message pane work]" should be a new bug, if one is needed at all