1310147 - There are too many ways to assert that GC doesn't happen

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Currently we have:

 - AutoAssertOnGC
 - AutoAssertNoAlloc
 - AutoCheckCannotGC

This is pretty confusing.  Maybe we can get away with a single class that does the following:

 - asserts on GC thing allocation (as this has the potential for GC)
 - asserts on GC (to catch API calls to force GC)
 - is checked by the hazard analysis

Comment 1

[Jon Coppeard (:jonco)]

Another different is that AutoAssertOnGC asserts in release builds too.

Comment 2

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

Related are AutoSuppressGCAnalysis, the new AutoRequireNoGC, and AutoAssertNoNurseryAlloc.

With respect to the hazard analysis, there are three options:

 - 1. Suppress the analysis. Needed for when the analysis can't figure out something on its own.

 - 2. Ignored by the analysis. In order for a hazard to be reported, you need something that the analysis recognizes as GC'ing (not hard, since the analysis conservatively assumes things can GC when in doubt; it is very rare for something to be able to GC without the analysis knowing), and an unrooted pointer kept live. These tend to be for documentation -- you put in an AutoAssertOnGC to inform the reader that they don't have to consider the possibility of a GC while reading the code.

 - 3. Checked by the analysis. I think this is the most common case.

 - 4. A base class that could be either #1 or #3, for use as a token to promise to a callee that you won't be GC'ing in a region around the call. 

I don't *think* we need #2.

I'm not sure whether AutoAssertNoAlloc would be needed independently. I guess it would be for cases where it's ok to GC to but not ok to alloc? I don't know of where that would be useful, and I notice that two of the users have comments claiming it's about GC, not alloc. So it seems like AutoAssertNoAlloc can probably go away.

I thought at some point one of these was statically checked but not asserted, because we couldn't write to the state required to mark the need to dynamically assert. It would have been a zero-arg constructor variant, I guess.

If we need to distinguish release asserts vs debug-only asserts, perhaps it would be better to use a template parameter instead of a whole separate class name?

Comment 3

[Jon Coppeard (:jonco)]

The point of AutoAssertNoAlloc was not to check for allocation specifically, but for potential GC sites.  However it looks like we call GCRuntime::verifyIsSafeToGC from GCRuntime::checkAllocatorState anyway, so I think this can just go.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1310147-rename-assert-on-gc

Patch to rename AutoAssertOnGC to AutoAssertNoGC, since it doesn't just assert on GC but in places where we allocate GC things too.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1310147-remove-assert-no-alloc

Patch to remove AutoAssertNoAlloc and replace it with AutoAssertNoGC since this has the same effect.

Comment 6

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

Comment on attachment 8804796 [details] [diff] [review]
bug1310147-remove-assert-no-alloc

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

::: js/src/jsgc.cpp
@@ +6992,5 @@
> +  : gc(&rt->gc), gcNumber(rt->gc.gcNumber())
> +{
> +    gc->enterUnsafeRegion();
> +}
> +

I think you can just change the JSContext* constructor to just take a JSRuntime* now instead of adding an overload -- JSContext is a subclass of JSRuntime nowadays.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df256c6bf29f
Rename AutoAssertOnGC to AutoAssertNoGC r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc3d6c32ed80
Remove AutoAssertNoAlloc and replace with AutoAssertNoGC r=sfink

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #6)
> I think you can just change the JSContext* constructor to just take a
> JSRuntime* now instead of adding an overload -- JSContext is a subclass of
> JSRuntime nowadays.

I tried this and it caused problems because this header is included in the browser where it can't see that these are related.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df256c6bf29f
https://hg.mozilla.org/mozilla-central/rev/cc3d6c32ed80

Comment 10

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

(In reply to Jon Coppeard (:jonco) from comment #8)
> (In reply to Steve Fink [:sfink] [:s:] from comment #6)
> > I think you can just change the JSContext* constructor to just take a
> > JSRuntime* now instead of adding an overload -- JSContext is a subclass of
> > JSRuntime nowadays.
> 
> I tried this and it caused problems because this header is included in the
> browser where it can't see that these are related.

Doh! Ok. Well, my first comment was going to be that we can use delegating constructors now, so the implementation of AutoAssertNoGC(JSContext*) could be

  JS::AutoAssertNoGC::AutoAssertNoGC(JSContext* cx) : AutoAssertNoGC(cx) {}

(but that's a minor point.)

Comment 11

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

>   JS::AutoAssertNoGC::AutoAssertNoGC(JSContext* cx) : AutoAssertNoGC(cx) {}

Doh!

    JS::AutoAssertNoGC::AutoAssertNoGC(JSContext* cx) : AutoAssertNoGC((JSRuntime*)cx) {}

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #10)
> Well, my first comment was going to be that we can use delegating
> constructors now

Neat, I didn't know we had these now!