819443 - DOMProxyHandler is being finalized on the background thread

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

I noticed this when looking at the crash stacks for GetXPConnect.

DOMProxyHandler::finalize calls nsXPConnect::GetRuntimeInstance(), which in turn calls GetXPConnect. But in this particular crash stack, the DOMProxyHandler is being finalized on the background thread, so we crash, because GetXPConnect does a MOZ_CRASH when it isn't called on the main thread.

https://crash-stats.mozilla.com/report/index/eeeb0b76-c9de-4e15-9afb-172ea2121207

I filed this as s-s because I don't know if this is just a particular problem with this class, in which case it is just a DOS and not a big deal, or a symptom of a broader problem with things being inappropriately finalized on the background thread.

Comment 1

[Andrew McCreight [:mccr8]]

Updating the summary to what is probably the real problem here...

Comment 2

[Andrew McCreight [:mccr8]]

To be clear, this is the DOMProxyHandler in NodeListBinding.cpp. I don't know if there are other ones or what.

Comment 3

[Andrew McCreight [:mccr8]]

Here's a similar crash where mozilla::dom::CSS2PropertiesBinding::DOMProxyHandler::finalize is being run on the background thread.

https://crash-stats.mozilla.com/report/index/8bcc8b7a-ffa5-47a9-9592-395a42121206

These are both from beta. I looked at all of the GetXPConnect() crashes on beta from the last week, and these were the only two like this, so it seems like it must be a rare combination of circumstances that causes this.

Comment 4

[Andrew McCreight [:mccr8]]

Marking sec-audit as there's no real evidence yet that it is anything worse than a DOS. Or, for that matter, that it isn't just a symptom of some weird memory corruption elsewhere.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix alloc kinds

This fixes some issues with allocation kinds that I saw while looking at code for this bug. I think it's harmless. We're missing a case for Ion code, but I don't think we ever call the function with that alloc kind. Better to fix it though.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: thread assert on background finalize

This asserts that we only run finalizers on the main thread. I wish we had a better thing to assert here, but the shape is already gone by the time we finalize.

In theory we could make this a release-mode assertion, but given the low incidence of the crash, it doesn't seem worth it.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 691494 [details] [diff] [review]
fix alloc kinds

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

Nice find!

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

These are just extra assertions, not a fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/97addca5ceae
https://hg.mozilla.org/integration/mozilla-inbound/rev/c940923e0efc

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

And a last-minute fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a58a0e5066

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/97addca5ceae
https://hg.mozilla.org/mozilla-central/rev/c940923e0efc
https://hg.mozilla.org/mozilla-central/rev/b7a58a0e5066

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm not working on this, so I unassigned myself.

Comment 12

[Andrew McCreight [:mccr8]]

FWIW, I don't see any crashes with GetXPConnect on 19+.  There are some on 18, but the ones I looked at didn't seem to have this particular problem.  So I'll just call this WFM...