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
Status: RESOLVED FIXED
[js:t]
:
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)
:
:
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-18 17:51:42 PDT
Created attachment 634257 [details] [diff] [review]
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 (: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 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 Luke Wagner [:luke] 2012-06-20 13:18:55 PDT
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] 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 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 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).

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe1b4d570df
Comment 7 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:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5a8d617bff
Comment 8 Ed Morley [:emorley] 2012-06-21 02:08:05 PDT
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c710654ee747
Comment 9 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:

https://hg.mozilla.org/integration/mozilla-inbound/rev/13a8fa3afd28
Comment 10 Ed Morley [:emorley] 2012-06-22 03:46:38 PDT
https://hg.mozilla.org/mozilla-central/rev/13a8fa3afd28

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