927663 - Roots registered with JS_AddMumbleRoot should be allowed to be NULL

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Attachment: It should be okay for roots registered with JS_Add***Root to be NULL.

If a root registered with JS_Add{,Named}{Object,String,Script}Root is null, the GC hurls itself from a cliff, crying out:

Assertion failure: thing, at /home/jimb/moz/dbg/js/src/gc/Marking.cpp:125

I dramatize only slightly.

This makes it annoying to manage rooted locations whose values can change: after every mutation you have to carefully check for transitions to and from null.

The attached patch fixes this, and tests that it is fixed.

Comment 1

[Jim Blandy :jimb]

Attachment: Bug 927633: It should be okay for roots registered with JS_Add***Root to be NULL.

Forgot to refresh patch; this one uses nullptr, not NULL.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 818138 [details] [diff] [review]
It should be okay for roots registered with JS_Add***Root to be NULL.

Thanks for the jsapi-test! Forwarding review. This is similar to bug 926779 but probably doesn't fix it; I wonder if they shouldn't have a common fix, somewhere deeper in the marking code.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 818139 [details] [diff] [review]
Bug 927633: It should be okay for roots registered with JS_Add***Root to be NULL.

Steve hit this exact issue earlier today. I'd guess that's what prompted this bug, except you didn't flag Steve for review.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 818138 [details] [diff] [review]
It should be okay for roots registered with JS_Add***Root to be NULL.

After forwarding the review this was in my queue still. Looks like a previous midair left me with review on an obsolete patch: an impressive failure mode even by bugzilla's standards.

Comment 5

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

I was fixing up the places in jsapi-tests that violate this, and even adding an assertion to catch them sooner, all on the assumption that the assertion is useful for catching errors (eg, when people call AddRoot with the address of a local variable and then pop it off the stack?) But I don't actually know that it is useful for catching errors; it was more of a "it *must* be useful, or somebody would have fixed it long ago!" which is a horrendously bad sort of logic.

But still, let me ask directly: do we think that this does not catch errors? Or are they errors that would be caught even after jimb's patch?

Comment 6

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink] from comment #5)
> I was fixing up the places in jsapi-tests that violate this, and even adding
> an assertion to catch them sooner, all on the assumption that the assertion
> is useful for catching errors (eg, when people call AddRoot with the address
> of a local variable and then pop it off the stack?) But I don't actually
> know that it is useful for catching errors; it was more of a "it *must* be
> useful, or somebody would have fixed it long ago!" which is a horrendously
> bad sort of logic.

My thought when I read your patch in bug 927204 was: well that's silly, why don't null roots just work? And I thought exactly what you thought: "surely /someone/ knew what they were doing when they designed the api like this?"

> But still, let me ask directly: do we think that this does not catch errors?
> Or are they errors that would be caught even after jimb's patch?

With this patch AddRoot interfaces will have the same semantics as Rooted. I think we're wrong and Jim is right.

Perhaps it would be clearer if these API's were called JS::AddRootedMumbleLocation or somesuch instead of AddMumbleRoot?

Comment 7

[Terrence Cole [:terrence]]

To be clear: I think we should take this as is.

Comment 8

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

Comment on attachment 818139 [details] [diff] [review]
Bug 927633: It should be okay for roots registered with JS_Add***Root to be NULL.

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

That assertion is causing me way too many problems right now, so I'll agree with you guys and say let's just do it. If we start noticing that some errors are harder to track down, we can figure out a different way of early-asserting.

Comment 9

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc405bc63b5

Comment 10

[Terrence Cole [:terrence]]

It looks like this got landed with bug 927633 in the title instead of bug 927663, Closing.