The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

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.
Attachment #634257 - Flags: review?(luke)
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

5 years ago
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.
Whiteboard: [js:t]

Comment 3

5 years ago
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?
Attachment #634257 - Flags: review?(luke) → review+

Comment 4

5 years ago
(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.
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.
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
Target Milestone: --- → mozilla16
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
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c710654ee747
Some compilers are dumb.  Made a tweak, repushed, now it appears to be sticking:

https://hg.mozilla.org/integration/mozilla-inbound/rev/13a8fa3afd28
https://hg.mozilla.org/mozilla-central/rev/13a8fa3afd28
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.