765990 - Never use Rooted as a temporary, only as a local variable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

Using it as a temporary introduces various conversion ambiguities that requires an |operator Handle<T>| to placate clang.  It's unclear if this is a spec thing or a clang quirk, but either way it doesn't really matter: there's extra complexity to handle.  The extra conversion operator also introduces problems because it can make conversion ambiguous, in certain circumstances.

Another problem with using it as a temporary is that, used in concert with a local variable, you get actually wrong behavior -- see bug 759895.

In sum, we should only use Rooted as a local variable, not as a temporary.  Here's a patch implementing this.  It tends to bitrot pretty easily, although generally not too horribly each time, so it'd be good to get it reviewed reasonably soon.

Comment 1

[Brian Hackett [Laid off!]]

I don't buy this argument.  Re: bug 759895, if anyone writes code like that there will be an assert botch whenever that code executes, which seems pretty minor to me.  The fact that clang requires an extra one line method to make it happy doesn't seem that relevant.  And these new variables are adding a ton of cruft to the code and make it much harder for me to read than when the Rooted<X> are constructed as temporaries.

Comment 2

[Luke Wagner [:luke]]

I think this is a good patch.  We have two ambiguous ways to go from a Rooted to a Handle; it is no surprise there is an ambiguity and I'm surprised GCC accepts it.

Stylistically, I'm happy to see all these RootedVars be given their own statements, not just b/c of the LIFO hazard, but because they make the callsites harder to read.  Apparently the IM folks have already adopted the style of having separate declarations for Rooted so this patch would bring more uniformity to SM.

cc'ing more people who may wish to state their preference.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 634257 [details] [diff] [review]
Patch

Given that IM is already doing this and it seems to avoid never-ending clang headaches, I don't see any reason not to do this.

r+ with two changes, already discussed:

Could you use the .tbl to auto-generate something that returns atom handles instead of using fromMarkedLocation?

Could you remove the 'const' from the Handle(Rooted &) constructor so that we can catch accidental use of temporaries in GCC?

Comment 4

[Luke Wagner [:luke]]

(In reply to Luke Wagner [:luke] from comment #3)
> Could you use the .tbl to auto-generate something that returns atom handles
> instead of using fromMarkedLocation?

Or just use Rooted, your choice.

Comment 5

[Brian Hackett [Laid off!]]

The Rooted class should also have JS_GUARD_OBJECT_NOTIFIER, which will I believe give a compile error somewhere other than clang if someone uses Rooted as a temporary in the future.

Comment 6

[Jeff Walden [:Waldo]]

I used Rooted for now for the atomState atom uses.  The .tbl thing is something we should do, but it seems non-trivial and tangential enough to warrant its own patch and its own bug.

I also removed const as mentioned, which will also address comment 5's concern (temporaries passed to non-const ref parameters are a very well-known, widely-implemented compile error, no need to double down on the errors with guard macros).

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe1b4d570df

Comment 7

[Jeff Walden [:Waldo]]

Sigh, forgot that building the shell alone wouldn't be enough for this change.  Fixed the couple other users outside the engine, verified it built fine, repushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5a8d617bff

Comment 8

[Ed Morley [:emorley]]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c710654ee747

Comment 9

[Jeff Walden [:Waldo]]

Some compilers are dumb.  Made a tweak, repushed, now it appears to be sticking:

https://hg.mozilla.org/integration/mozilla-inbound/rev/13a8fa3afd28

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/13a8fa3afd28