Terrence Cole [:terrence] Assignee
	Description • 8 years ago

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

	Terrence Cole [:terrence] Assignee
	Comment 1 • 8 years ago

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

	Terrence Cole [:terrence] Assignee
	Comment 2 • 8 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 3 • 8 years ago

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.)

	Terrence Cole [:terrence] Assignee
	Comment 4 • 8 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 8 years ago

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?

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 6 • 8 years ago

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

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

Looks good!

	Terrence Cole [:terrence] Assignee
	Comment 7 • 8 years ago

(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

	Terrence Cole [:terrence] Assignee
	Comment 8 • 8 years ago

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.

	Terrence Cole [:terrence] Assignee
	Comment 9 • 8 years ago

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.

	Terrence Cole [:terrence] Assignee
	Comment 10 • 8 years ago

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.

	Terrence Cole [:terrence] Assignee
	Comment 11 • 8 years ago

A version that has actually been compile tested.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 8 years ago

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....

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 8 years ago

I filed bug 1289129 on comment 12.

	Terrence Cole [:terrence] Assignee
	Comment 14 • 8 years ago

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

	Terrence Cole [:terrence] Assignee
	Comment 15 • 8 years ago

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

	Terrence Cole [:terrence] Assignee
	Comment 16 • 8 years ago

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

	Terrence Cole [:terrence] Assignee
	Comment 17 • 8 years ago

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

	Terrence Cole [:terrence] Assignee
	Comment 18 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 19 • 8 years ago

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

	Terrence Cole [:terrence] Assignee
	Comment 20 • 8 years ago

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

	Phil Ringnalda (:philor)
	Comment 21 • 8 years ago
bugherder

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

	Terrence Cole [:terrence] Assignee
	Comment 22 • 8 years ago

Clearing ni?.

	Terrence Cole [:terrence] Assignee
	Comment 23 • 8 years ago

Huh. Guess the checkbox isn't working?

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 8 years ago

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

	Terrence Cole [:terrence] Assignee
	Comment 25 • 8 years ago

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.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 26 • 8 years ago

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

Should help debug intermittent test failures, Beta50+

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 27 • 8 years ago
bugherder uplift

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