1754867 - Nullptr crash due to reentrancy in ~AutoTaskDispatcher

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Byron Campen [:bwc]]

See bug 1748333 (comment 13 and comment 33)

Comment 1

[Byron Campen [:bwc]]

This looks like just a nullptr crash, and it is pretty hard to trigger. Rate on crash-stats is pretty low:

https://crash-stats.mozilla.org/signature/?signature=mozilla%3A%3AAutoTaskDispatcher%3A%3ADispatchTasksFor&date=%3E%3D2022-01-10T20%3A47%3A00.000Z&date=%3C2022-02-10T20%3A47%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#reports

Comment 2

[Byron Campen [:bwc]]

There's a few things we could blame here:

1. This code results in ~AutoTaskDispatcher going reentrant and calling mTailDispatcher.DispatchTasksFor, because Maybe::reset invokes the d'tor first, and then marks itself as containing Nothing:

https://searchfox.org/mozilla-central/rev/9f61d854547cedbde0773b2893e4f925352be3b3/xpcom/threads/AbstractThread.cpp#196
https://searchfox.org/mozilla-central/rev/9f61d854547cedbde0773b2893e4f925352be3b3/mfbt/Maybe.h#639,642

This is pretty inconsistent with the way things like unique_ptr work, which is basically how we're using mTailDispatcher. I think that this is probably something we need to fix, one way or another, even if there are other things we should be fixing here.

2. It is probably not ideal that we are setting up a situation where reentrancy could occur in the first place. It is harder to say what we could do to fix this, though.

Comment 3

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d76839bf1a0d8a2002ce7cb73babd1e5bbb348ef
https://treeherder.mozilla.org/#/jobs?repo=try&revision=467107ab96add2b7eb9964ac9f1b2751c1ef5248
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8654ce42f50ac3d20d3f28832c95055f866cf596

Comment 4

[Byron Campen [:bwc]]

Attachment: Bug 1754867: Use unique_ptr instead of Maybe for mTailDispatcher to avoid reentrancy. r?nika

The core of the problem here is that Maybe::reset invokes the d'tor first, and
then clears the value, whereas unique_ptr::reset does the opposite.

Comment 5

[Byron Campen [:bwc]]

webrtc jobs look ok, although this change could effect pretty much any feature.

Comment 6

[Byron Campen [:bwc]]

Trying to determine whether anything has regressed in mochitest writ large:

https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&revision=d742e72b4e1f3a6c9583801f45b180f5caf86670

Here's the baseline push:

https://treeherder.mozilla.org/jobs?repo=try&duplicate_jobs=visible&revision=2ba69bced06adce1d6968022f34fb6ea473f37a2

So many oranges...

Comment 7

[Byron Campen [:bwc]]

Here's a failure caused by this bug on try.

https://treeherder.mozilla.org/jobs?repo=try&revision=46272de3f29201f032e7aebbff0786d8cef1a7a2&selectedTaskRun=GhM1Z3ttRs23CyAb-IoL4Q.0

Comment 9

[Byron Campen [:bwc]]

Seeing a lot more failures in test-windows7-32-qr/opt-mochitest-browser-chrome-fis-e10s-5 (including several novel-looking failures at the end of js/xpconnect/tests/browser):

With changes:
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=O9J8VfX1SMO2rix7BmGq0g.0&revision=d742e72b4e1f3a6c9583801f45b180f5caf86670

Without changes:
https://treeherder.mozilla.org/jobs?repo=try&duplicate_jobs=visible&revision=2ba69bced06adce1d6968022f34fb6ea473f37a2&selectedTaskRun=TUnn-v3iTM-DvwkRIns2pQ.0

Here's a try push focusing on js/xpconnect/tests/browser. Maybe I can get better logging of this problem on some other platform.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9572c441295de22e708579498b95715e7f228d7

Comment 10

[Byron Campen [:bwc]]

Yeah, that only seems to happen on windows7 jobs, and it does not appear that we have any windows7 debug jobs even with --full...

Comment 11

[Byron Campen [:bwc]]

Any idea why this patch might be causing a new hang-on-finish in the xpconnect bc tests?

https://treeherder.mozilla.org/jobs?repo=try&revision=c9572c441295de22e708579498b95715e7f228d7&selectedTaskRun=dhoRVCkFTESygEZfZopjLw.0

Comment 12

[Nika Layzell [:nika] (ni? for response)]

I'm not sure why that would be happening. I do notice that the log has the line [task 2022-02-14T23:20:25.127Z] 23:20:25 INFO - GECKO(3384) | [Exiting due to channel error. (https://treeherder.mozilla.org/logviewer?job_id=367873398&repo=try&lineNumber=1955) which suggests that the parent process crashed in some way & lost connection to a child process unexpectedly (child processes log that when the parent process disappears unexpectedly & they quick-exit), though that doesn't line up super well with the application hanging.

Given that this failure is occuring on opt builds, we wouldn't have deadlock detection running, so one of the most likely options here is that we're encountering a deadlock somewhere in the parent process. Unfortunately I don't really have any idea where that would be.

Comment 13

[Byron Campen [:bwc]]

Hmm. The work in progress over in bug 1755318 makes this happen far more often on try:

https://treeherder.mozilla.org/jobs?repo=try&revision=46df533e014f3fda1f17c0d5f5cc0b447e658f52

I think this will have to block bug 1755318.

Comment 14

[Byron Campen [:bwc]]

Seems that the issue does not reproduce when I hack win7 opt to be more in line with a debug build:

https://treeherder.mozilla.org/jobs?repo=try&revision=a7a34bddcf240948fa697573bcc7a8c25da1bc59

It is possible that disabling assertions prevents the issue, but when assertions are left enabled, we hit assertions and crash before we even run any tests.

It may be that I need to selectively enable just the debugging features I want (eg; just deadlock detection).

Comment 15

[Byron Campen [:bwc]]

Actually, there's another possibility; maybe the rebase I just did fixed the issue. Here's a try push for that:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=427fca1b66223e6904e0603f2884b4fdabeb6599

Edit: Well that's a lucky break. A rebase has either stopped the issue, or made it so uncommon that it doesn't show up in 20 runs.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68192b01f87d
Use unique_ptr instead of Maybe for mTailDispatcher to avoid reentrancy. r=nika

Comment 17

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/68192b01f87d

Comment 18

[Intermittent Failures Robot]

1 failures in 3281 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform and build breakdown:

• linux1804-64-qr: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1754867&startday=2022-02-14&endday=2022-02-20&tree=all