1283634 - Assert that no CC gray objects pass through the JSAPI interface

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: assert_not_gray_in_interface-v0.diff

This piggy backs on the cross-compartment assertions.

I found one weird instance where we're compiling an optimized stub against a gray expando. Because of the compilation, GC is suppressed, so this isn't a safety issue. If we do "fix" it however, it looks like we can browse about with the assertion on. 

I'm going to make sure that try is also mostly green before asking for review because handling the failures could be a fairly large grab bag: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc465ed474aa

Comment 1

[Terrence Cole [:terrence]]

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

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8766951 [details] [diff] [review]
assert_not_gray_in_interface-v0.diff

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

I thought I had a try run here already -- and that it was green -- but I'm not seeing it in the bug, so I've given it another run. The 62% complete run is not all orange, so I'm going to go ahead and ask for review.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8766951 [details] [diff] [review]
assert_not_gray_in_interface-v0.diff

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

You should get review for the DOM changes from Boris, who would know about the perf implications.

Also, I came across GetGlobalForObjectCrossCompartment() in bug 1283636, which was getting a gray object passed into it. So this is certainly not complete. ;) (I hit that during mach package.)

Comment 4

[Terrence Cole [:terrence]]

Ah found it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc465ed474aa&filter-tier=1&selectedJob=23189213

You are quite right! Looks like there are a couple failures that need patching before we can land. Since there are perf implications I guess I'll split this into multiple patches.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8766951 [details] [diff] [review]
assert_not_gray_in_interface-v0.diff

This is probably OK (though I _think_ this does not improve anything material other than allowing the new asserts, unless I misunderstand what the illegal things to do with gray objects are), but don't we need similar things various places in the JS engine itself that read reserved slots?  If not, why not?

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8766951 [details] [diff] [review]
assert_not_gray_in_interface-v0.diff

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

Looks good!

Comment 7

[Terrence Cole [:terrence]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> Comment on attachment 8766951 [details] [diff] [review]
> assert_not_gray_in_interface-v0.diff
> 
> This is probably OK (though I _think_ this does not improve anything
> material other than allowing the new asserts, unless I misunderstand what
> the illegal things to do with gray objects are), but don't we need similar
> things various places in the JS engine itself that read reserved slots?  If
> not, why not?

We very well might! I know that we've added quite a few of these in the past as they've come up, mostly in the debugger. We also have a generic read barrier that exposes as well for the general case: [1].

1- https://dxr.mozilla.org/mozilla-central/source/js/src/gc/Heap.h#1303-1304

Comment 8

[Terrence Cole [:terrence]]

After discussing on IRC, it seems like the majority of slot reads do not need a read barrier. It is only reads of things which may be involved in CC cycles. In which case it is only reads performed by Gecko that generally need this, so it doesn't make sense to do it as part of HeapSlot. As to how to assert this... I'm less sure.

Comment 9

[Terrence Cole [:terrence]]

I think we need a defense in depth approach for this. There are a number of places where we can assert the "no black to CC-gray" invariant directly -- HeapSlot::set, Rooted::Rooted, and probably a few others -- however, the existence of the JIT's means that we are going to have a hard time covering all of them. We should do these ASAP. I'm not sure why they don't already exist, honestly. This bug would then be our depth. In the case where we miss a direct assertion, this can act as something of a backstop.

Comment 10

[Terrence Cole [:terrence]]

Attachment: assert_not_gray_in_interface-v1.diff

Boris is handling the fallout in dependent bugs, so I've stripped the ExposeToActiveJS: they were at too high a level anyway. This patch also adds manual gray checks for JS_EnterCompartment, JSAutoCompartment, and JSAutoNullableCompartment, as those obviously don't check for compartment mismatches automatically.

Carrying the r+ because we have general consensus on the approach and insurance that there will be adequate testing before it lands.

Comment 11

[Terrence Cole [:terrence]]

Attachment: assert_not_gray_in_interface-v2.diff

A version that has actually been compile tested.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Well, with that the try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=177a3dbb5da1 is very thoroughly orange.

I spot-checked a few failures and they're from AutoJSAPI::Init; presumably the incoming aGlobal is gray.  Now the thing is, AutoJSAPI::Init does NOT root the incoming global per se.  It takes a strong ref to the incoming nsIGlobalObject, which keeps things alive via CC.  And it enters the compartment of aGlobal; I'm not sure whether that makes aGlobal a GC root from the JS engine point of view....

Comment 13

[Boris Zbarsky [:bzbarsky]]

I filed bug 1289129 on comment 12.

Comment 14

[Terrence Cole [:terrence]]

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

Comment 15

[Terrence Cole [:terrence]]

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

Comment 16

[Terrence Cole [:terrence]]

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

Comment 17

[Terrence Cole [:terrence]]

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

Comment 18

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ce0ae49d9775024d53a6d4aeb71b5038075719
Bug 1283634 - Assert that no gray objects get passed into JSAPI; r=jonco,r=bz

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for mda failures like https://treeherder.mozilla.org/logviewer.html#?job_id=33105669&repo=mozilla-inbound in https://hg.mozilla.org/integration/mozilla-inbound/rev/f0df70a13eb8

Comment 20

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ea439b8ccb891d50723cc91bf329c1bf9e38217
Bug 1283634 - Assert that no gray objects get passed into JSAPI; r=jonco,r=bz

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5ea439b8ccb8

Comment 22

[Terrence Cole [:terrence]]

Clearing ni?.

Comment 23

[Terrence Cole [:terrence]]

Huh. Guess the checkbox isn't working?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Do we want to consider nominating this for Beta uplift now that the issues have mostly shaken out?

Comment 25

[Terrence Cole [:terrence]]

Comment on attachment 8773853 [details] [diff] [review]
assert_not_gray_in_interface-v2.diff

Approval Request Comment
[Feature/regressing bug #]: The Cycle Collector.
[User impact if declined]: See risks.
[Describe test coverage new/current, TreeHerder]: A full cycle on TH.
[Risks and why]:  None: this adds a single debug-only assertion. The issue it is tracking has been present since ~Firefox 4. Uplifting this will keep our assertion coverage the same between beta, alpha, and trunk, which will allow us to much more easily notice changes in the orange level of the remaining issues so that we know what further fixes need to be uplifted. Taking this patch has zero risk for the release and will potentially let us fix the long-standing issue one release sooner.
[String/UUID change made/needed]: None.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8773853 [details] [diff] [review]
assert_not_gray_in_interface-v2.diff

Should help debug intermittent test failures, Beta50+

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f5ca6b83927d