Last Comment Bug 765990 - Never use Rooted as a temporary, only as a local variable
: Never use Rooted as a temporary, only as a local variable
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- minor (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-06-18 17:51 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-06-22 03:46 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (124.88 KB, patch)
2012-06-18 17:51 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-18 17:51:42 PDT
Created attachment 634257 [details] [diff] [review]

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 User image Brian Hackett (:bhackett) 2012-06-18 17:58:46 PDT
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 User image Luke Wagner [:luke] 2012-06-19 11:00:07 PDT
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 User image Luke Wagner [:luke] 2012-06-20 13:18:55 PDT
Comment on attachment 634257 [details] [diff] [review]

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 User image Luke Wagner [:luke] 2012-06-20 13:19:39 PDT
(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 User image Brian Hackett (:bhackett) 2012-06-20 15:16:47 PDT
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 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-20 15:34:56 PDT
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).
Comment 7 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-21 01:52:31 PDT
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:
Comment 8 User image Ed Morley [:emorley] 2012-06-21 02:08:05 PDT
Comment 9 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-21 11:24:15 PDT
Some compilers are dumb.  Made a tweak, repushed, now it appears to be sticking:
Comment 10 User image Ed Morley [:emorley] 2012-06-22 03:46:38 PDT

Note You need to log in before you can comment on or make changes to this bug.