505974 - Crash [@ nsMsgAccountManager::OnItemRemoved] deleting a local folder which had a saved search subfolder

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.1) Gecko/20090715 Lightning/1.0pre Thunderbird/3.0b3

(found while poking around Mozmill) Looks like a null crash.

Run search-window/test-search-window.js then delete the SearchWindowA folder. TB then crashes.

OR:

Create a new local folder, put a message inside. Run a saved search inside the folder for that message, saving as a saved search. Delete the whole local folder (including the saved search subfolder). TB then crashes.

bp-4a0c664b-870a-4d39-8c13-1701c2090723 (My Mac crash)
bp-0cd37460-3a47-4842-bb7d-8bd992090716 (another Linux folk's crash about a week ago)

xref bug 474247


Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
nsMsgAccountManager::OnItemRemoved (this=0x12bccd80, parentItem=0x129b4f60, item=0xe589270) at /Users/skywalker/comm-central/mailnews/base/src/nsMsgAccountManager.cpp:3246
3246              parent->PropagateDelete(savedSearch, PR_TRUE, nsnull);
(gdb) bt
#0  nsMsgAccountManager::OnItemRemoved (this=0x12bccd80, parentItem=0x129b4f60, item=0xe589270) at /Users/skywalker/comm-central/mailnews/base/src/nsMsgAccountManager.cpp:3246
#1  0x0903daa1 in nsMsgMailSession::OnItemRemoved (this=0xe589270, aParentItem=0x129b4f60, aItem=0x1435fd30) at /Users/skywalker/comm-central/mailnews/base/src/nsMsgMailSession.cpp:197
#2  0x0900e388 in nsMsgDBFolder::NotifyItemRemoved (this=0x129b4f40, aItem=0x1435fd30) at /Users/skywalker/comm-central/mailnews/base/util/nsMsgDBFolder.cpp:4722
#3  0x0900ac16 in nsMsgDBFolder::PropagateDelete (this=0x129b4f40, folder=0x1435fd30, deleteStorage=0, msgWindow=0x1430d190) at /Users/skywalker/comm-central/mailnews/base/util/nsMsgDBFolder.cpp:3280
#4  0x090d4859 in nsMsgLocalMailFolder::CopyFolderLocal (this=0x143612b0, srcFolder=0x1435fd30, isMoveFolder=1, msgWindow=0x1430d190, listener=0x0) at /Users/skywalker/comm-central/mailnews/local/src/nsLocalMailFolder.cpp:2093
#5  0x0905d2b7 in nsMsgCopyService::DoNextCopy (this=0x12f9fef0) at /Users/skywalker/comm-central/mailnews/base/src/nsMsgCopyService.cpp:330
#6  0x0905d51a in nsMsgCopyService::DoCopy (this=0x12f9fef0, aRequest=0x13ce83f0) at /Users/skywalker/comm-central/mailnews/base/src/nsMsgCopyService.cpp:263
#7  0x0905d85d in nsMsgCopyService::CopyFolders (this=0x12f9fef0, folders=0x1090c910, dstFolder=0x143612d0, isMove=1, listener=0x0, window=0x1430d190) at /Users/skywalker/comm-central/mailnews/base/src/nsMsgCopyService.cpp:571
#8  0x090d32ff in nsMsgLocalMailFolder::DeleteSubFolders (this=0x129b4f40, folders=0x1090c910, msgWindow=0x1430d190) at /Users/skywalker/comm-central/mailnews/local/src/nsLocalMailFolder.cpp:1028
#9  0x002c8138 in NS_InvokeByIndex_P (that=0x129b4f60, methodIndex=28, paramCount=2, params=0xbfffd34c) at /Users/skywalker/comm-central/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179
#10 0x096a0179 in ~AutoJSSuspendNonMainThreadRequest [inlined] () at /Users/skywalker/comm-central/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2454
#11 0x096a0179 in ~AutoJSSuspendNonMainThreadRequest [inlined] () at xpcprivate.h:3618
#12 0x096a0179 in XPCWrappedNative::CallMethod (ccx=@0xbfffd174, mode=XPCWrappedNative::CALL_METHOD) at /Users/skywalker/comm-central/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2073
#13 0x096a43aa in XPC_WN_CallMethod (cx=0xa71800, obj=0x0, argc=0, argv=0xe589270, vp=0x0) at /Users/skywalker/comm-central/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590
#14 0x0017b74a in js_Invoke (cx=0xa71800, argc=2, vp=0xa728d4, flags=34) at jsinterp.cpp:1386
#15 0x0016b67e in js_Interpret (cx=0xa71800) at /Users/skywalker/comm-central/mozilla/js/src/jsinterp.cpp:5179
#16 0x0017b757 in js_Invoke (cx=0xa71800, argc=1, vp=0xa72824, flags=32) at jsinterp.cpp:1394
#17 0x0017bd24 in js_InternalInvoke (cx=0xa71800, obj=0x138f5580, fval=173211424, flags=0, argc=1, argv=0xa72820, rval=0xbfffdb54) at jsinterp.cpp:1447
#18 0x0013eb18 in JS_CallFunctionValue (cx=0xa71800, obj=0x0, fval=0, argc=0, argv=0x0, rval=0x0) at /Users/skywalker/comm-central/mozilla/js/src/jsapi.cpp:5187
#19 0x0adc71f6 in nsJSContext::CallEventHandler (this=0x12920ab0, aTarget=0x136325d0, aScope=0x128bb600, aHandler=0xa52ff20, aargv=0x1056a7f0, arv=0xbfffdcf8) at /Users/skywalker/comm-central/mozilla/dom/src/base/nsJSEnvironment.cpp:2035
#20 0x0ae00e10 in nsJSEventListener::HandleEvent (this=0x13651e80, aEvent=0x1057261c) at /Users/skywalker/comm-central/mozilla/dom/src/events/nsJSEventListener.cpp:247
#21 0x0ac9277d in nsEventListenerManager::HandleEventSubType (this=0x13670f10, aListenerStruct=0x13670f34, aListener=0x13651e80, aDOMEvent=0x1057261c, aCurrentTarget=0x136325d0, aPhaseFlags=6) at /Users/skywalker/comm-central/mozilla/content/events/src/nsEventListenerManager.cpp:1098
#22 0x0ac92d73 in nsEventListenerManager::HandleEvent (this=0x13670f10, aPresContext=0xa7f800, aEvent=0xbfffe0bc, aDOMEvent=0xbfffe004, aCurrentTarget=0x136325d0, aFlags=6, aEventStatus=0xbfffe008) at /Users/skywalker/comm-central/mozilla/content/events/src/nsEventListenerManager.cpp:1206
#23 0x0acb232c in nsCOMPtr<nsISupports>::operator= () at nsCOMPtr.h:236
#24 0x0acb232c in nsEventTargetChainItem::HandleEvent (this=0x14fb8820, aVisitor=@0xbfffdffc, aFlags=6, aMayHaveNewListenerManagers=1) at /Users/skywalker/comm-central/mozilla/content/events/src/nsEventDispatcher.cpp:237
#25 0x0acb2561 in nsEventTargetChainItem::HandleEventTargetChain (this=0x14fb8920, aVisitor=@0xbfffdffc, aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=1) at /Users/skywalker/comm-central/mozilla/content/events/src/nsEventDispatcher.cpp:300
#26 0x0acb32a3 in nsEventDispatcher::Dispatch (aTarget=0x136325d0, aPresContext=0xa7f800, aEvent=0xbfffe0bc, aDOMEvent=0x0, aEventStatus=0xbfffe100, aCallback=0x0) at /Users/skywalker/comm-central/mozilla/content/events/src/nsEventDispatcher.cpp:514
#27 0x0aaa72f1 in PresShell::HandleDOMEventWithTarget (this=0x0, aTargetContent=0x136325d0, aEvent=0xbfffe0bc, aStatus=0xbfffe100) at /Users/skywalker/comm-central/mozilla/layout/base/nsPresShell.cpp:6363
#28 0x0abfdba8 in ~nsCOMPtr [inlined] () at nsCOMPtr.h:2074
#29 0x0abfdba8 in ~nsCOMPtr [inlined] () at /Users/skywalker/comm-central/mozilla/layout/xul/base/src/nsXULPopupManager.cpp:469
#30 0x0abfdba8 in ~nsXULCommandEvent [inlined] () at /Users/skywalker/comm-central/mozilla/layout/xul/base/src/nsXULPopupManager.cpp:1152
#31 0x0abfdba8 in ~nsXULCommandEvent [inlined] () at nsCOMPtr.h:1147
#32 0x0abfdba8 in nsXULMenuCommandEvent::Run (this=0x1434dc30) at /Users/skywalker/comm-central/mozilla/layout/xul/base/src/nsXULPopupManager.cpp:2074
#33 0x002b90fa in nsThread::ProcessNextEvent (this=0x416b90, mayWait=0, result=0xbfffe1ac) at /Users/skywalker/comm-central/mozilla/xpcom/threads/nsThread.cpp:510
#34 0x00279967 in NS_ProcessPendingEvents_P (thread=0x416b90, timeout=20) at nsThreadUtils.cpp:180
#35 0x0987b5f2 in nsBaseAppShell::NativeEventCallback (this=0x4439d0) at /Users/skywalker/comm-central/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:121
#36 0x0984631a in nsAppShell::ProcessGeckoEvents (aInfo=0x4439d0) at /Users/skywalker/comm-central/mozilla/widget/src/cocoa/nsAppShell.mm:405
#37 0x92f5a595 in CFRunLoopRunSpecific ()
#38 0x92f5ac78 in CFRunLoopRunInMode ()
#39 0x9469428c in RunCurrentEventLoopInMode ()
#40 0x94693fde in ReceiveNextEventCommon ()
#41 0x94693f19 in BlockUntilNextEventMatchingListInMode ()
#42 0x926a9d0d in _DPSNextEvent ()
#43 0x926a95c0 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#44 0x926a25fb in -[NSApplication run] ()
#45 0x09845a6a in nsAppShell::Run (this=0x4439d0) at /Users/skywalker/comm-central/mozilla/widget/src/cocoa/nsAppShell.mm:720
#46 0x007aa0f7 in nsAppStartup::Run (this=0xbffff128) at /Users/skywalker/comm-central/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:193
#47 0x00067db8 in XRE_main (argc=1, argv=0xbffff790, aAppData=0x40e610) at /Users/skywalker/comm-central/mozilla/toolkit/xre/nsAppRunner.cpp:3298
#48 0x00002ebf in main (argc=1, argv=0xbffff790) at /Users/skywalker/comm-central/mail/app/nsMailApp.cpp:103
(gdb) l
3241              nsCOMPtr<nsIMsgFolder> parent;
3242              rv = savedSearch->GetParent(getter_AddRefs(parent));
3243              NS_ENSURE_SUCCESS(rv, rv);
3244              db = nsnull;
3245              dbFolderInfo = nsnull;
3246              parent->PropagateDelete(savedSearch, PR_TRUE, nsnull);
3247            }
3248            else
3249            {
3250            // remove leading '|' we added (or one after |folderURI, if first URI)
(gdb) p parent
$1 = {
  <nsCOMPtr_base> = {
    mRawPtr = 0x0
  }, <No data fields>}
(gdb)

Comment 1

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

We may not be generating a notification that the virtual folder was created that we should be generating...

Comment 2

[David :Bienvenu]

I suspect the issue is that the parent was deleted before the child, and I think the parent pointer is a weak ref...which means GetParent is not returning an error...

Comment 3

[David :Bienvenu]

Attachment: make GetParent return an error on null, and get rid of GetParentMsgFolder

This makes GetParent return an error if there's no parent. While I was there, I noticed GetParentMsgFolder is the exact same function, and no longer needed, so I got rid of it.

It's possible there will be some js code that throws an exception that didn't before, so it would be good to make this change early in the beta 4 cycle.

Comment 4

[[e-mail]]

Comment on attachment 391403 [details] [diff] [review]
make GetParent return an error on null, and get rid of GetParentMsgFolder

>   _isSpecialFolder: function DBViewWrapper_isSpecialFolder(
>       aMsgFolder, aFlags, aCheckAncestors)
>   {
>     if (!aMsgFolder)
>       return false;
>     else if ((aMsgFolder.flags & aFlags) == 0) {
>-      let parentMsgFolder = aMsgFolder.parentMsgFolder;
>+      let parentMsgFolder = aMsgFolder.parent;
> 
>-      if (parentMsgFolder && aCheckAncestors)
>+      if (parentMsgFolder && aCheckAncestors && !parentMsgFolder.isServer)
This caller isn't expecting parent to throw, but then shouldn't its caller be using msgFolder.isSpecialFolder(flags, checkAncestors) ?

Comment 5

[[e-mail]]

Comment on attachment 391403 [details] [diff] [review]
make GetParent return an error on null, and get rid of GetParentMsgFolder

> function IsInTrashFolder(aFolder)
> {
>   if (!aFolder)
>     return false;
>   if (aFolder.flags & Components.interfaces.nsMsgFolderFlags.Trash)
>     return true;
>-  return IsInTrashFolder(aFolder.parentMsgFolder);
>+  return IsInTrashFolder(aFolder.parent);
> }
Or indeed this one... its callers should probably do folder.isSpecialFolder(nsMsgFolderFlags.Trash, true)

Comment 6

[David :Bienvenu]

(In reply to comment #4)

> This caller isn't expecting parent to throw, but then shouldn't its caller be
> using msgFolder.isSpecialFolder(flags, checkAncestors) ?

oh yeah, I wonder if that didn't exist when Andrew wrote that code, or if he didn't know about it...

Comment 7

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

(In reply to comment #6)
> oh yeah, I wonder if that didn't exist when Andrew wrote that code, or if he
> didn't know about it...

I was just porting the existing code which was using the pure JS function.  It looks like the C++ method existed but the JS callers were never adapted to use it, nor the JS method annotated with a comment telling people to use the C++.  I think the JS method has recently been removed and its callers with it.

Comment 8

[David :Bienvenu]

Comment on attachment 391403 [details] [diff] [review]
make GetParent return an error on null, and get rid of GetParentMsgFolder

there are other places that need to be fixed

Comment 9

[[e-mail]]

(In reply to comment #8)
> (From update of attachment 391403 [details] [diff] [review])
> there are other places that need to be fixed
Yeah, a patched build turns out to be fairly broken :-(

Comment 10

[David :Bienvenu]

yeah, I'm going to punt on fixing getParent not to throw an exception at this point, though I will try to land some of the cleanup.

Comment 11

[David :Bienvenu]

Attachment: proposed fix

This gets rid of the parentMsgFolder attribute, and fixes this bug. Asking for r=neil, except on the tb ui parts, and standard8's sr, and r= for the tb ui part.

Comment 12

[[e-mail]]

Comment on attachment 392353 [details] [diff] [review]
proposed fix

>-  /* handy accessor when we want a msg folder */
>-  readonly attribute nsIMsgFolder parentMsgFolder;
Might want to comment that parent now throws for servers and orphans.

>     // Return if there is still nothing in aUrl or the folder is in Trash.
>-    if (!aUrl.length || IsInTrashFolder(aFolder))
>+    if (!aUrl.length || (aFolder &&
>+                         aFolder.isSpecialFolder(Ci.nsMsgFolderFlags.Trash, true)))
Nit: We already assume aFolder is non-null so no need to check explicitly.

Comment 13

[David :Bienvenu]

Comment on attachment 392353 [details] [diff] [review]
proposed fix

Standard8 is swamped - hoping Neil can do the sr, since he did the r already.

Comment 14

[[e-mail]]

Comment on attachment 392353 [details] [diff] [review]
proposed fix

Technically you're still missing a Tb r= ;-)

Comment 15

[David :Bienvenu]

(In reply to comment #12)
> (From update of attachment 392353 [details] [diff] [review])
> >-  /* handy accessor when we want a msg folder */
> >-  readonly attribute nsIMsgFolder parentMsgFolder;
> Might want to comment that parent now throws for servers and orphans.

I removed that part of the original patch. It broke too many things.
> 
> >     // Return if there is still nothing in aUrl or the folder is in Trash.
> >-    if (!aUrl.length || IsInTrashFolder(aFolder))
> >+    if (!aUrl.length || (aFolder &&
> >+                         aFolder.isSpecialFolder(Ci.nsMsgFolderFlags.Trash, true)))
> Nit: We already assume aFolder is non-null so no need to check explicitly.

I'll fix that.

Comment 16

[David :Bienvenu]

fix checked in - the TB-only changes were very simple, so Neil's review is fine with me.

Comment 17

[Ludovic Hirlimann [:Usul]]

https://litmus.mozilla.org/show_test.cgi?id=9504