1610406 - Crash in [@ PLDHashTable::Search | mozilla::dom::DocumentOrShadowRoot::GetElementById]

Status: VERIFIED FIXED

(Thunderbird :: General, defect)

Description

[Wayne Mery (:wsmwk)]

#2 crash for 73.0b1

Regression in nightly. bp-2736a504-a4fc-4a77-876b-461d80200106 is the earliest buildid 20200105105422 73.0a1

Top 10 frames of crashing thread:

0 xul.dll PLDHashTable::Search xpcom/ds/PLDHashTable.cpp:501
1 xul.dll mozilla::dom::DocumentOrShadowRoot::GetElementById dom/base/DocumentOrShadowRoot.cpp:98
2 xul.dll nsMsgWindow::GetMessageWindowDocShell comm/mailnews/base/src/nsMsgWindow.cpp:76
3 xul.dll nsImapProtocol::SetupWithUrl comm/mailnews/imap/src/nsImapProtocol.cpp:825
4 xul.dll nsImapProtocol::LoadImapUrl comm/mailnews/imap/src/nsImapProtocol.cpp:2099
5 xul.dll nsImapIncomingServer::LoadNextQueuedUrl comm/mailnews/imap/src/nsImapIncomingServer.cpp:521
6 xul.dll nsresult `anonymous namespace'::SyncRunnable2<nsIImapMailFolderSink, const char*, nsIImapUrl*>::Run comm/mailnews/imap/src/nsSyncRunnableHelpers.cpp:121
7 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1241
8 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
9 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87

Comment 1

[Magnus Melin [:mkmelin]]

It would have been "caused" by bug 1603649 where the caller was introduced: https://searchfox.org/comm-central/rev/009c65e87d240a61404a57362bdf7aa514894fd5/mailnews/base/src/nsMsgWindow.cpp#76-78

Comment 2

[Wayne Mery (:wsmwk)]

Pretty bad for beta, so we should respin beta with a fix for this. Of course, first need a fix.

Comment 3

[Magnus Melin [:mkmelin]]

Unfortunately probably a bug over in m-c so re-spinning is harder, crashing at https://searchfox.org/mozilla-central/rev/f98dad153b59a985efd4505912588d4651033395/xpcom/ds/PLDHashTable.cpp#501 via https://searchfox.org/mozilla-central/rev/f98dad153b59a985efd4505912588d4651033395/dom/base/DocumentOrShadowRoot.cpp#132

Comment 4

[Magnus Melin [:mkmelin]]

Or well, if wanted we could back out the last changeset from bug 1603649 (use the earlier approach) on beta.

Comment 5

[Wayne Mery (:wsmwk)]

FWIW, there are so far zero Firefox crashes, and zero Mac crashes.

Comment 6

[Jorg K (CEST = GMT+2)]

Add a null check to the M-C code and get the reviewer interested that way?

Comment 7

[Wayne Mery (:wsmwk)]

As long as we get the solution quicker rather than later :) ... Who is driving?

he Mac crash is PLDHashTable::Search | nsDocShell::QueryInterface ? bp-e86ab8a5-e63d-4c26-ab45-0b3910200131

Comment 8

[Magnus Melin [:mkmelin]]

Maybe we want to add a null check @ DocumentOrShadowRoot.cpp#132?
But, I wonder how that ever happens. If we don't get an element with the expected id, things will not work properly at all.

Comment 9

[Wayne Mery (:wsmwk)]

Now #1 crash for beta

Comment 10

[Ben Campbell]

How odd. The QueryInterface crash is exclusive to Mac, and the GetElementById is windows. The code path on the m-c side looks like it should be a pretty solid and well-tested one (I mean, GetElementById() is pretty fundamental, right?). I'm currently leaning toward it being a threading issue - maybe TB being shut down while IMAP operations are still pending (that dom being whisked away at just the wrong time, say)... But unless I can replicate it, it's going to be a matter of trying to work out and understand the whole code path, and where races/clashes could be occurring.

Comment 11

[Wayne Mery (:wsmwk)]

Too bad we don't have a graph that shows recent topcrashes against each other over time - we could clearly see when a signature trails off and a new one begins. Or that it's not happening to more nightly users - we'd have a more clear picture of the regression range :) But given the crash rate I'd say the checkin range should be 2020-01-01 to 2020-01-05, or very late December.

So you're of the opinion it's not likely to be an m-c checkin? Nor bug 1603649 whose checkin was 2019-12-20?

Comment 12

[Ben Campbell]

I did think we could mitigate it with an m-c patch to check for null... but it's worse than that - I think it's accessing freed memory which may have been overwritten by something else. So a null check doesn't help.

The crash is happening in the main thread, in response to an event that was dispached to the main threads message queue by the IMAP thread.
My best theory so far is that the IMAP thread posts a LoadNextQueuedUrl() request to the main thread. But before it's serviced, shutdown is initiated. So when the request is eventually processed by the main thread, the messagewindow and its docshell have been (at least partly) torn down. Kaboom.
Tricky to confirm without having steps to replicate the crash, of course.
But if so, the cheesy mitigation is to modify all the IMAPs dispatch-to-main-thread handlers (there's a proxy class to dispatch-to-main a whole bunch of IMAP functions), to detect that shutdown has started and if so, just exit/fail without doing anything. There are already some "we're shutting down" flags in the IMAP code, so hopefully I can use one of them, once I figure out how they're used currently.
So that's my current plan and approach. Stay tuned.

The proper solution is to check over all the IMAP threading and shutdown handling, making sure it's all nice and clean and safe, but that's a bigger task ;-)

I suspect some of these other PLDHashTable::Search() crashes might have a similar cause:
https://bugzilla.mozilla.org/buglist.cgi?quicksearch=PLDHashTable%3A%3ASearch

Comment 13

[Ben Campbell]

Attachment: 1610406-remove-extra-if.patch

nsMsgWindow::GetMessageWindowDocShell() seems to have a superfluous 'if' statement in it (leftover from some recent changes I think). This patch clears it up.

Comment 14

[Ben Campbell]

Attachment: 1610406-add-docshell-destruction-check.patch

After lots of false starts, I discovered that there's a isBeingDestroyed() method on nsIDocShell. This patch adds a check in nsMsgWindow::GetMessageWindowDocShell() so it'll hopefully abort before crashing. There are also a couple of tweaks in nsImapProtocol::SetupWithUrl() to check for such an error before plowing on blindly...

I can't say for sure that this'll reduce the crashes, but I guess there's only one way to tell...

Comment 15

[Ben Campbell]

And some more notes, just while they're fresh in my head:

• It seems bonkers that any nsMsgWindow-related code is present down in nsImapProtocol in the first place.
• nsImapProtocol::SetupWithUrl() is doing this nsMsgWindow hackery to set up notification callbacks on the mock channel.
• nsImapProtocol::SetupWithUrl() has special handling to deal mockchannels vs real channels. This kind of skirts the whole point of using mock channels - the mockchannel should probably be a less leaky abstraction.

So I think the cleanup steps would be:

1. ruthlessly track down and refactor real/mockchannel checks and better encapsulate the mock behaviour.
2. go over the mockchannel notification callback nsMsgWindow hackery and see what it's really trying to do, and see if it can be done /without/ tying the nsImapProtocol system to the GUI side.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9127764 [details] [diff] [review]
1610406-add-docshell-destruction-check.patch

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

::: mailnews/base/src/nsMsgWindow.cpp
@@ +77,5 @@
> +      // but really, we shouldn't even get here in such cases.
> +      bool doomed;
> +      rootShell->IsBeingDestroyed(&doomed);
> +      if (doomed) {
> +        return NS_ERROR_FAILURE;

Maybe we should remove NS_ERROR_ILLEGAL_DURING_SHUTDOWN instead to perhaps make errors easier to understand if someone runs into them?

Comment 17

[Ben Campbell]

Attachment: 1610406-add-docshell-destruction-check-2.patch

Thanks Magnus - seems like there's a perfect error code for all kinds of surprising occasions!
There seems to be a little bustage currently, so I'll hold off the checkin-needed for now.

Comment 18

[Wayne Mery (:wsmwk)]

So this linux signature as well?
PLDHashTable::Search | <name omitted> | nsMsgWindow::GetMessageWindowDocShell bp-391bc935-1256-4557-9937-52a1a0200220

Comment 19

[Ben Campbell]

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

So this linux signature as well?
PLDHashTable::Search | <name omitted> | nsMsgWindow::GetMessageWindowDocShell bp-391bc935-1256-4557-9937-52a1a0200220

Looks like it, yes. In that case it's coming in from javascript, but the actual crash looks exactly the same.

Comment 20

[Ben Campbell]

Damn it, the patch breaks all the imap unit tests!
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=079b46908cf84690b92396efb55b5c0ca105952b

Hopefully I've just done something stupid and screwed up the patch.
If we're unlucky, we'll find out that all the tests were relying on the docshell being in the shutting-down state the whole time or something stupid like that... :-(
Gah...
More investigation.

Comment 21

[Ben Campbell]

Attachment: 1610406-add-docshell-destruction-check-3.patch

Revised patch. Removed some returncode checks I had added. The imap code was ignoring the returncode and assuming the returned pointer was null on failure. Should probably be tightened up, but for now I just went through and made sure the pointer would in fact be null.
Also added a comment on what the offending code is actually doing (it wasn't at all obvious to me, and it's still not clear it should be doing it anyway!).

Try build here - will check back in on it later today and add checkin flags if it looks clean.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bf32cf94e26d4f0eea6ab887da62d5220a153238

Comment 22

[Ben Campbell]

Last try build failed, but this one looks clean:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=095f3079e980e7bd6b366282b56c341a6301c98c
(there's one mochitest fail, but that seems unrelated to this this patch).
I'll flag the checkin-needed now because I think we need to get this into the wild and see if it helps.
But I'm definitely open to following it up - Magnus, want to give this patch another quick once-over and see if there's anything extra you'd like me to chase up?

Comment 23

[Wayne Mery (:wsmwk)]

If one of you gets me a Mac try build I will test it.

I can very reliably reproduce PLDHashTable::Search | nsDocShell::QueryInterface on Mac beta

1. mailnews.show_send_progress=true mailnews.sendInBackground=false
2. send a large message
3. don't use "Cancel" but click to close the compose window "X".
   bp-c0cd6cee-42b7-4460-8583-390ae0200223

Magnus see comment 22.

Comment 24

[Wayne Mery (:wsmwk)]

I am unable to reproduce this on windows TB74

Btw, the steps above are from bug 1608733 which the user cites for Mac using version 68.

Comment 25

[Magnus Melin [:mkmelin]]

Let's get it checked in and you can check it on nightly.

Comment 26

[Ben Campbell]

Can't seem to get the repro steps in comment 23 to trigger the bug on Linux (with the latest code, but without my patch), which is a shame.

In any case, I just kicked off a new try build including windows and mac:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3d0ac805d17841266b11ef18638217df8b510431

bug 1608733 Is the same crash, but coming in from a different code path. My patch should make the offending function bail out before it crashes, but it's possible that the calling code doesn't gracefully handle the error code So even if the patch works, the repro steps might give you a different crash :-) But it'd be progress.

Comment 27

[Ben Campbell]

Scratch that - I did get the comment 23 repro steps working!
I'm still a little fuzzy on the exact steps, frantically closing down windows and confirmation dialogs before the sending finished (and I'm running I3, which doesn't even have "X" close buttons on the windows), but it did trigger.

After applying the patch and trying to trigger the bug again, I haven't yet managed to get it to crash down in PLDHashTable, which is very promising.
(I'm running a debug build and I do get a assert triggering down in cairo, via MOZ_gdk_display_close(). But I think that's out of scope for this bug)

Comment 28

[Wayne Mery (:wsmwk)]

I tested with https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Btg4TvEJSSW2PxaqoR69Ew/runs/0/artifacts/public/build/target.dmg - no crash

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/947de04af434
Remove superfluous 'if' in nsMsgWindow::GetMessageWindowDocShell(). r=mkmelin
https://hg.mozilla.org/comm-central/rev/ebcd40d4aaab
Make nsMsgWindow::GetMessageWindowDocShell() fail if docshell is being destroyed. r=mkmelin

Comment 31

[Wayne Mery (:wsmwk)]

Comment on attachment 9128338 [details] [diff] [review]
1610406-add-docshell-destruction-check-3.patch

Yes, let's see how this addresses the regression on beta

Comment 32

[Rob Lemley [:rjl]]

Thunderbird 74.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/75481e886915
Thunderbird 74.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/3cfca362ac8c

Comment 33

[Wayne Mery (:wsmwk)]

There are no crashes for buildid 20200227011511 aka 74.0b2
v.fixed