1246697 - Perma-crash when using PersistentRooted<Traceable>

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

When I re-implemented PersistentRooted's guts last week, I forgot that the reset method instantiates a new T to overwrite the value. This is now crashing in ConcreteTraceable's constructor. Given that we don't know T in this list, we need to do something similar to what we do for traceable for reset as well. Or change our contract for Traceable interaction. This will require some thought.

Comment 1

[Terrence Cole [:terrence]]

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

Comment 2

[Terrence Cole [:terrence]]

Attachment: new_semantics_for_PersistentRooted_Traceable-v0.diff

This establishes new, slightly more restrictive, semantics for PersistentRooted<Traceable>. I used jsapi-tests to ensure that both positive and negative conditions on both types worked as expected before and after. Try run above should verify that none of our browser paths are doing anything more adventurous.

One thing I noticed in writing this is that the documentation for PersistentRooted indicates it is context independent. This is not currently true and we should fix it. I think I've already filed a bug for this.

Comment 3

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

Comment on attachment 8717714 [details] [diff] [review]
new_semantics_for_PersistentRooted_Traceable-v0.diff

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

::: js/src/gc/RootMarking.cpp
@@ +137,5 @@
>      FinishPersistentRootedChain<jsid>(heapRoots_[JS::RootKind::Id]);
>      FinishPersistentRootedChain<Value>(heapRoots_[JS::RootKind::Value]);
> +
> +    // Note that we do not finalize the Traceable list as we do not know how to
> +    // safely clear memebers. We instead assert that none escape the RootLists.

I am imagining that memebers are bears that strike in the middle of the night, spray painting memes onto your house's exterior walls.

::: js/src/jsapi-tests/testGCExactRooting.cpp
@@ +154,5 @@
> +//        container.str() = JS_NewStringCopyZ(cx, "Hello");
> +//        sContainer.init(rt, container);
> +//        return true;
> +//    }
> +//    END_TEST(testGCPersistentRootedTraceableCannotOutliveRuntime)

Rather than comment out this whole test, maybe leave it in, but clear the value before returning? (With a comment "// uncommenting this line should trigger an assertion.") This at least runs through the "expected" path.

It'd be better to have a crash test type thing here, but that feels like going too far for this bug since we don't have test infrastructure for that.

::: js/src/jspubtd.h
@@ +307,5 @@
> +        // of the held thing, so we simply cannot do this without accruing
> +        // extra overhead and complexity for all users for a case that is
> +        // unlikely to ever be used in practice. For this reason, the following
> +        // assertion disallows usage of PersistentRooted<Traceable> that
> +        // outlives the RootLists.

Just for my own edification, what would this take? I guess you could set it up so that if your struct has a finish() ("clear()"?), then it would get invoked automagically.

Or we could wire it up so that we memcpy &GCPolicy<T>::initial()?

Comment 4

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink, :s:] from comment #3)
> Comment on attachment 8717714 [details] [diff] [review]
> new_semantics_for_PersistentRooted_Traceable-v0.diff
> 
> ::: js/src/jsapi-tests/testGCExactRooting.cpp
> @@ +154,5 @@
> > +//        container.str() = JS_NewStringCopyZ(cx, "Hello");
> > +//        sContainer.init(rt, container);
> > +//        return true;
> > +//    }
> > +//    END_TEST(testGCPersistentRootedTraceableCannotOutliveRuntime)
> 
> Rather than comment out this whole test, maybe leave it in, but clear the
> value before returning? (With a comment "// uncommenting this line should
> trigger an assertion.") This at least runs through the "expected" path.

Good idea!

> It'd be better to have a crash test type thing here, but that feels like
> going too far for this bug since we don't have test infrastructure for that.

My thought as well!

> ::: js/src/jspubtd.h
> @@ +307,5 @@
> > +        // of the held thing, so we simply cannot do this without accruing
> > +        // extra overhead and complexity for all users for a case that is
> > +        // unlikely to ever be used in practice. For this reason, the following
> > +        // assertion disallows usage of PersistentRooted<Traceable> that
> > +        // outlives the RootLists.
> 
> Just for my own edification, what would this take? I guess you could set it
> up so that if your struct has a finish() ("clear()"?), then it would get
> invoked automagically.

We'd need to expand DispatchWrapper to contain both a trace and a finish method. Which effectively means we'd need a second copy of DispatchWrapper for PersistentRooted or something substantially more complicated if we want to share. Then we'd need a GCPolicy<T>::finish on /all/ types used with a PersistentRooted... even if ~100% of users do not live past their owner and thus never actually call the new code. I think this is the real sticking point, since it would require us to add a ton of complex code that is just going to bit-rot.

> Or we could wire it up so that we memcpy &GCPolicy<T>::initial()?

I think that would leak any allocated Vector or Map data that had been allocated by T, no?

Comment 5

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/510574b085e2771059bb5c9b01f1eb0b08850998
Bug 1246697 - Use simpler semantics for PersistentRooted<Traceable>; r=sfink

Comment 6

[Terrence Cole [:terrence]]

Note: the try run above is a bit messy (55 failures), but I verified manually that none of them relate to this patch and that they all either showed up on m-i on qbase, or were not run there.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/510574b085e2