1538779 - Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P5)

Description

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

I don't know if this is really desirable, but it was bothering me that we needed a separate type for Rooted<StackGCVector<Value>> v(cx) avoid v(cx, cx) or v(cx, StackedGCVector<Value>(cx). Since i wasted a bunch of time recently on a harder template metaprogramming problem (and eventually gave up), I thought this seemed easier so maybe I should try it out. What do you guys think?

Comment 1

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

Attachment: Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible

This is what it ended up as. I tried to use std::is_constructible<T>, but it produced some weird errors that I didn't want to figure out.

Comment 2

[Jon Coppeard (:jonco)]

Oh, neat! This has also bugged me for ages, but I couldn't see how to do it.

Comment 3

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

Attachment: Bug 1538779 - Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible r=jonco

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6f935d90da4
Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible r=jwalden

Comment 5

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=busted&group_state=expanded&revision=d6f935d90da47f542743715481bdec6679b64f94&selectedJob=242949180

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=242949191&repo=autoland&lineNumber=15985

Backout link: https://hg.mozilla.org/integration/autoland/rev/38a860da33822840712ea52dc1fe2c07eab05287

Comment 6

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

Ugh. The shell build succeeds, but the browser build fails. It's the fault of the update for Waldo's review comment: dispatching to Rooted(cx, SafelyInitialized<T>()) rather than repeating the code. And that revealed a rat.

The issue is that GCPolicy<T> now requires an isValid() function, which I just recently discovered and documented in bug 1534440. Except that it turns out that I'm wrong -- few if any of the Gecko specializations of GCPolicy include that member, and everything works just fine.

It turns out that isValid is only required if you use the Rooted(cx, S&& initial) constructor. If you don't, then no body with a call to isValid() is ever generated. And apparently that's the case with all of the Gecko users.

jonco: I can see 3 ways to proceed: (1) add isValid to everything in Gecko; (2) revert to my previous version that conditionally requires isValid and update the docs; or (3) do messy template magic to test for the existence of isValid to essentially default isValid to { return true; }.

#2 would be no better nor worse than the status quo, but it does make for a bit of a surprising footgun when someone attempts to do Rooted r(cx, mytype(...)) and it fails to compile.

#3 is nicer, except that I've already added a bunch of confusing goop to Rooted's code so I'm not sure if it's a good idea to add more. It can at least be done externally though (by defining a GCPolicyWithIsValid struct that does the weird defaulting stuff, and then just s/GCPolicy::isValid/GCPolicyWithIsValid::isValid/ in the actual Rooted body.)

I'll put up a patch for #3 and see what jonco thinks of it.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #6)
There are only four uses in Gecko. I'd say just define it for them.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2a3fb166dfb
Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible r=jonco

Comment 9

[Cristian Brindusan [:cbrindusan]]

Backed out changeset f2a3fb166dfb (Bug 1538779) for nojit build bustages at backup-point-bug1315634.js and hazard build bustages.

Backout: https://hg.mozilla.org/integration/autoland/rev/aec4b19308a9b4eb59e2b988840af1d8996825c8

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=f2a3fb166dfba14d854a49668aeca6cce6bb3b55&selectedJob=243368748

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=243368748&repo=autoland&lineNumber=3823
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=243368659&repo=autoland&lineNumber=2768

Comment 10

[Narcis Beleuzu [:NarcisB]]

There were also assertion failures (detail::GCPolicyWithIsValid<T>::isValid(ptrRootingAPI)) on RootingAPI.h
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=243377112&repo=autoland&lineNumber=893

Comment 11

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

Yeah, apparently "it compiles" is not a good enough test for this.

The problem stems from

typename = decltype(T(std::declval<RootingContext>()))

This is supposed to be a test for the existence of a constructor T::T(cx). I thought it was equivalent to code like

T dummy(cx);

which correctly fails. But it's actually more like

decltype(T(cx)) dummy;

which somehow succeeds. (And initializes a JSAtom* to the value of a JSContext*, which then fails the isValid check.) I could use

new T(cx);

which would (correctly) fail, but that only makes sense for pointers, and making it work for other things would be reinventing std::is_constructible. So at this point, I'll just use std::is_constructible, which now... just works. I have no idea why that failed for me last time I used this.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e57ad617753c
Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible r=jonco

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/e57ad617753c