Closed Bug 1283634 Opened 3 years ago Closed 3 years ago

Assert that no CC gray objects pass through the JSAPI interface

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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 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.
Attachment #8766951 - Flags: review?(jcoppeard)
Attachment #8766951 - Flags: review?(continuation)
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.)
Attachment #8766951 - Flags: review?(continuation) → review?(bzbarsky)
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 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?
Attachment #8766951 - Flags: review?(bzbarsky) → review+
Comment on attachment 8766951 [details] [diff] [review]
assert_not_gray_in_interface-v0.diff

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

Looks good!
Attachment #8766951 - Flags: review?(jcoppeard) → review+
(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
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.
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.
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.
Attachment #8766951 - Attachment is obsolete: true
Attachment #8773848 - Flags: review+
A version that has actually been compile tested.
Attachment #8773848 - Attachment is obsolete: true
Attachment #8773853 - Flags: review+
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....
Blocks: 1280591
Depends on: 1291928
https://hg.mozilla.org/mozilla-central/rev/5ea439b8ccb8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Clearing ni?.
Huh. Guess the checkbox isn't working?
Flags: needinfo?(terrence)
Do we want to consider nominating this for Beta uplift now that the issues have mostly shaken out?
Flags: needinfo?(terrence.d.cole)
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.
Flags: needinfo?(terrence.d.cole)
Attachment #8773853 - Flags: approval-mozilla-beta?
Comment on attachment 8773853 [details] [diff] [review]
assert_not_gray_in_interface-v2.diff

Should help debug intermittent test failures, Beta50+
Attachment #8773853 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.