1137336 - fix weak map tracing hazard due to function pointer

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

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

Function '_ZN2js11WeakMapBase16traceAllMappingsEPNS_13WeakMapTracerE|void js::WeakMapBase::traceAllMappings(js::WeakMapTracer*)' has unrooted 'c' of type 'class js::CompartmentsIterT<js::ZonesIter>' live across GC call js::WeakMapBase.traceMappings at js/src/jsweakmap.cpp:135

The problem here is js::TraceWeakMaps. It is called by https://dxr.mozilla.org/mozilla-central/source/xpcom/base/CycleCollectedJSRuntime.cpp#245 and another place in the same file, and it ends up calling the js::WeakMapTracer.callback function pointer.

I believe this is a false positive. But determining that is not straightforward. This is called by nsCycleCollector::FixGrayBits, which in fact *does* GC, but only after calling mJSRuntime->FixWeakMappingGrayBits(). So it's not like I can just say "the cycle collector won't GC while it's figuring out the gray bits."

I guess the trick would be to prove that the WeakMapTracer.callback won't GC. The easiest way to do that would be to make the API use a virtual function.

Comment 1

[Steve Fink [:sfink] [:s:]]

This probably shouldn't be sec-high, as it is very likely to be a false positive. In fact, I think I may "fix" this by adding in an AutoSuppressAnalysis, which will at least crash in debug builds if I am wrong and this can GC.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Explicitly disallow WeakMapTracer.callback from GCing

Assertion pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c75f426e82e0

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Explicitly disallow WeakMapTracer.callback from GCing

The one thing I'm not sure of is whether I should be using AutoAssertGCCallback instead.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8570563 [details] [diff] [review]
Explicitly disallow WeakMapTracer.callback from GCing

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

Yes, this is the right solution -- disallowing GC from a GC callback seems like a no-brainer.

Comment 5

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8224f00aec

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8570563 [details] [diff] [review]
Explicitly disallow WeakMapTracer.callback from GCing

Approval Request Comment
[Feature/regressing bug #]: unknown (as in, I didn't bother to dig it up)

[User impact if declined]: none. But if we backport the static analysis fixes that complain about this false positive, then we will have to ignore this failure for the hazard builds.

[Describe test coverage new/current, TreeHerder]: just now landed on mozilla-inbound

[Risks and why]: no change to release builds. For debug builds, adds an assertion that would indicate a serious problem (but is not expected to ever hit.)

[String/UUID change made/needed]: none

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d8224f00aec

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc28574bd2c5
https://hg.mozilla.org/releases/mozilla-beta/rev/ea414ee32231

Comment 9

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8570563 [details] [diff] [review]
Explicitly disallow WeakMapTracer.callback from GCing

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: it gives us the option of backporting bug 1120655, which is the updates to the analysis that mark this as a hazard (though it's believed to be a false positive)

User impact if declined: none. It might make it a little less confusing about what patch landed where, but it's not a huge deal either way.

Fix Landed on Version: it's been backported to 37

Risk to taking this patch (and alternatives if risky): the main risk is that the added debug assert finds out that this is a true positive, and we have to run around and fix something. That's not a problem with landing this patch, though. This patch does not change behavior in a release build.

String or UUID changes made by this patch: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ea414ee32231

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

bkerensa needs to be able to see this for building ESR.

Comment 12

[Benjamin Kerensa [:bkerensa]]

Lawrence wanted me to check per comment 11 I was unable to see these bugs and will need my privleges adjusted but do we want to respin on these?

Comment 13

[Al Billings [:abillings - ex-MoCo]]

(In reply to Liz Henry (:lizzard) from comment #11)
> bkerensa needs to be able to see this for building ESR.

Except this is "won't fix" for ESR31 per the flags. It is also a sec-other, which we don't take on ESR without a reason (we only take sec-high and sec-critical normally). There is no reason for anything to think we would take this on ESR31.