1629204 - Startup crash in [@ nsMsgDBFolder::AddSubfolder]. infinite loop in nsMsgDBFolder::GetChildWithURI ? use-after-free

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Wayne Mery (:wsmwk)]

+++ This bug was initially created as a clone of Bug #1543183 +++

Still a strong crash in 68.7.0 and early crash counts indicate the crash rate may be unchanged. But it's too early to say definitively. Still very much a startup crash

bp-56a18da8-8e96-4704-8500-249060200411
xul.dll nsMsgDBFolder::AddSubfolder(nsTSubstring<char16_t> const&, nsIMsgFolder**) comm/mailnews/base/util/nsMsgDBFolder.cpp:3533
1 xul.dll nsMsgBrkMBoxStore::AddSubFolders(nsIMsgFolder*, nsCOMPtr<nsIFile>&, bool) comm/mailnews/local/src/nsMsgBrkMBoxStore.cpp:964
2 xul.dll nsMsgBrkMBoxStore::DiscoverSubFolders(nsIMsgFolder*, bool) comm/mailnews/local/src/nsMsgBrkMBoxStore.cpp:64
3 xul.dll nsMsgLocalMailFolder::GetSubFolders(nsISimpleEnumerator**) comm/mailnews/local/src/nsLocalMailFolder.cpp:191
4 xul.dll nsMsgDBFolder::WriteToFolderCache(nsIMsgFolderCache*, bool) comm/mailnews/base/util/nsMsgDBFolder.cpp:1314
5 xul.dll nsMsgAccountManager::WriteToFolderCache(nsIMsgFolderCache*) comm/mailnews/base/src/nsMsgAccountManager.cpp:1566
6 xul.dll nsMsgAccountManager::Shutdown() comm/mailnews/base/src/nsMsgAccountManager.cpp:168
7 xul.dll nsMsgAccountManager::Observe(nsISupports*, char const*, char16_t const*) comm/mailnews/base/src/nsMsgAccountManager.cpp:0
8 xul.dll nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) xpcom/ds/nsObserverList.cpp:66
9 xul.dll nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) xpcom/ds/nsObserverService.cpp:295
10 xul.dll nsXREDirProvider::DoShutdown() toolkit/xre/nsXREDirProvider.cpp:1092
11 xul.dll ScopedXPCOMStartup::~ScopedXPCOMStartup() toolkit/xre/nsAppRunner.cpp:1244
12 xul.dll XREMain::XRE_main(int, char** const, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4777

bp-24131bf9-c649-4f6b-824d-e6ff10200410 beta 76 Crash Address 0xffffffffffffffff

Comment 1

[Magnus Melin [:mkmelin]]

I now highly suspect this is involved: https://searchfox.org/comm-central/rev/6d5f1795357d3292b9a9f382896677460ce0f8ce/mailnews/local/src/nsLocalMailFolder.cpp#181-182. Note that it's a shutdown crash.

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: [causes failures, could give clues for fix] bug1629204_addsubfolders_crash.take2.patch

Do we gain anything from setting mInitialized = false on shutdown?
I didn't find what infinite looping the code talks about, but if we're shutting down, initialized is set to false and that code is called, this would happen. I guess we could try to track it down by removing this variable and see where it hangs.

Comment 3

[Wayne Mery (:wsmwk)]

Interesting that the crash is in shutdown when the majority uptimes are so short https://crash-stats.mozilla.org/signature/?signature=nsMsgDBFolder%3A%3AAddSubfolder&date=%3E%3D2020-04-10T12%3A53%3A00.000Z&date=%3C2020-04-11T12%3A53%3A00.000Z&_columns=date&_columns=version&_columns=platform&_columns=reason&_columns=address&_columns=uptime&_sort=uptime&_sort=-date&page=1

• one third is 6-11 seconds - arguably not enough time for a user to have moved the mouse in Thunderbird, IF the UI has even appeared
• one third 11-18 seconds
• one third 20-294 seconds

This may be an academic question, but what could be "forcing" many of the shutdowns, assuming most of these shutdowns are not by user choice? Thunderbird updates?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/237406c10003
Startup crash in [@ nsMsgDBFolder::AddSubfolder]. infinite loop in nsMsgDBFolder::GetChildWithURI. r=benc

Comment 5

[Magnus Melin [:mkmelin]]

Accidentally committed :/
Maybe we want to see how it holds?

Comment 6

[Geoff Lankow (:darktrojan)]

No, it doesn't hold. Blows up a bunch of tests. The good news is that the tests in question do weird things like remove all of the accounts and add new accounts. I think possibly returning to 0 accounts triggers the Shutdown function. I found reasonable workarounds for most cases but not all, and this shouldn't need working around.

Backed out: https://hg.mozilla.org/comm-central/rev/b8ffda5a11e3daa7042ff21083e8631f24b0fc5c

Comment 7

[Wayne Mery (:wsmwk)]

This may be an academic question, but what could be "forcing" many of the shutdowns, assuming most of these shutdowns are not by user choice? Thunderbird updates?

Actually, this is more than academic and is at least relevant to "steps to reproduce?". Does the stack reveal any reasons for such short uptimes? And if not, what reasonable theories explain how this might happen?

Comment 8

[Magnus Melin [:mkmelin]]

That is a good question. I don't see from the stack what would cause it.
I'm surprised the patch caused failures, but it could indeed be very good news since that should give us a test case to track this down. The tree is a bad state right now, but I think some tests that started failing was

comm/mail/components/extensions/test/browser/browser_ext_compose_onBeforeSend.js
comm/mail/components/extensions/test/browser/browser_ext_mailTabs.js

Comment 9

[Ben Campbell]

(In reply to Wayne Mery (:wsmwk) from comment #3)

This may be an academic question, but what could be "forcing" many of the shutdowns, assuming most of these shutdowns are not by user choice? Thunderbird updates?

I think the answer to that is here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1543183#c22
(using "File->Email link..." in Firefox. It runs Thunderbird with a pre-filled message compose window. Once the user has sent the email it closes down again. This seems to match the uptimes).

Comment 10

[Ben Campbell]

Just for completeness...
to run TB under a debugger in compose mode (just like "File->Email link..." in Firefox does):

./mach run --debug --profile obj-x86_64-pc-linux-gnu/tmp/profile-default -compose "to='[e-mail]',subject='testtesttest',body='Just a test...'

I wasn't able to replicate the bug. I suspect there are other factors involved (eg timing might depend on other configured accounts).

Comment 11

[Ben Campbell]

Attachment: 1629204-avoid-getsubfolders-1.patch

I suspect the real underlying cause is some other thread pulling the rug out from underneath us... but we shouldn't be doing folder discovery during shutdown anyway :- )

So I think this patch will fix the crash. It avoids initiating folder discovery when writing out the folder cache (which is one of the things we do during shutdown).

I've filed Bug 1633955 to handle some more substantial cleanup/refactoring.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9144216 [details] [diff] [review]
1629204-avoid-getsubfolders-1.patch

Review of attachment 9144216 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +1296,5 @@
>  
>    if (!deep) return rv;
>  
> +  // NOTE: using mSubFolders directly, as GetSubFolders() can cause folder
> +  // creation, which we don't need or want here (Bug 1629204).

I'll adjust the comment a bit. But looks good.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d7148a424674
Avoid performing folder discovery during shutdown. r=mkmelin

Comment 14

[Wayne Mery (:wsmwk)]

Only recently landed and didn't make a beta, so will consider after 77 beta and after ESR 68.8.0.

Comment 15

[Wayne Mery (:wsmwk)]

no 77 beta crashes so far, so pretty promising https://crash-stats.mozilla.org/signature/?release_channel=%21release&signature=nsMsgDBFolder%3A%3AAddSubfolder&date=%3E%3D2020-04-16T15%3A29%3A00.000Z&date=%3C2020-05-16T15%3A29%3A00.000Z#graphs

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9144216 [details] [diff] [review]
1629204-avoid-getsubfolders-1.patch

[Triage Comment]
Approved for ESR

Comment 17

[Rob Lemley [:rjl]]

Thunderbird 68.9.0:
https://hg.mozilla.org/releases/comm-esr68/rev/a6ffe45d0fc0

Comment 18

[Wayne Mery (:wsmwk)]

v.fixed for 68.9.0 - no crashes