1503725 - Crash in nsThread::MaybeRemoveFromThreadList

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-07336baa-dd9d-4e87-b852-e40620181031.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll void nsThread::MaybeRemoveFromThreadList xpcom/threads/nsThread.cpp:447
1 xul.dll nsThreadShutdownAckEvent::Run xpcom/threads/nsThread.cpp:294
2 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1245
3 xul.dll NS_ProcessPendingEvents xpcom/threads/nsThreadUtils.cpp:472
4 xul.dll static nsresult mozilla::ShutdownXPCOM xpcom/build/XPCOMInit.cpp:906
5 xul.dll void ScopedXPCOMStartup::~ScopedXPCOMStartup toolkit/xre/nsAppRunner.cpp:1429
6 xul.dll int XREMain::XRE_main toolkit/xre/nsAppRunner.cpp:4950
7 xul.dll XRE_main toolkit/xre/nsAppRunner.cpp:5014
8 xul.dll mozilla::BootstrapImpl::XRE_main toolkit/xre/Bootstrap.cpp:49
9 firefox.exe static int do_main browser/app/nsBrowserApp.cpp:233

=============================================================

this signature is newly popping up since nightly build 65.0a1 20181027100344 and subsequently uplifted to 64.0b5. the timing for this would point to involvement of bug 1500861.
the crashing address is indicating a uaf situation in many occasions.

Comment 1

[Eric Rahm [:erahm]]

I wonder if we actually need a lock when we call `isInList`. Although the crash address looks UAF, so perhaps were calling `isInList` on an already destroyed thread. I think maybe the problem is that we need to somehow cancel the `nsThreadShutdownAckEvent` when we timeout, otherwise it's referencing a bad `nsThreadShutdownContext`.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

(In reply to Eric Rahm [:erahm] from comment #1)
> I wonder if we actually need a lock when we call `isInList`. Although the
> crash address looks UAF, so perhaps were calling `isInList` on an already
> destroyed thread. I think maybe the problem is that we need to somehow
> cancel the `nsThreadShutdownAckEvent` when we timeout, otherwise it's
> referencing a bad `nsThreadShutdownContext`.

What's happening here is that we remove the contexts we intend to leak from mRequestedShutdownContexts - which means that they get freed. If one of the leaked threads happens to complete before we actually shutdown, it will crash on this.
We could make the shutdown context refcountable. Or to make sure that nsThreadShutdownAckEvent doesn't run.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Eric Rahm [:erahm] from comment #1)
> I wonder if we actually need a lock when we call `isInList`.

I think we do. We used to not, and there was a long comment explaining why, but after my last refactor some new races showed up.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1503725 - Do not deallocate nsThreadShutdownContext when leaking thread r?erahm!

Sometimes when we call ShutdownWithTimeout on a thread pool, the unresponsive
thread that we leak will actually complete before the main thread is done.
In that case, the thread will dereference the thread shutdown context, so
we must intentionally leak it too.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73a273670597c16d7c0644cc2d6d9b61c6fe8204

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/73a273670597

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9021989 [details]
Bug 1503725 - Do not deallocate nsThreadShutdownContext when leaking thread r?erahm!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1500861

User impact if declined: Possible UAF and crash at shutdown when a thread we intentionally leak becomes unstuck and completes before the main thread.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a very small change - for the threads that we do leak we also leak the shutdown context.

String changes made/needed:

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9021989 [details]
Bug 1503725 - Do not deallocate nsThreadShutdownContext when leaking thread r?erahm!

[Triage Comment]
Fixes a new crash introduced by bug 1500861. Approved for 64.0b7.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8016d0d1a391

Comment 10

[Bogdan Maris, Desktop Test Engineering]

We can verify it based on crash reports from Socorro.

Comment 11

[Mihai Boldan, Desktop QA [:mboldan]]

Based on the summary from Socorro, I'm marking this issue Verified Fixed since it appears that the crash was not encountered on Firefox 64b7, or on newer builds.