785452 - Investigate restoring instance JSObject::doSomethingToThisObject methods to handles/roots

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

It might be possible to make e.g. obj->getProperty(...) work again when obj is a HandleObject or RootedObject, using the same tricks we do for methods on HandleValue and RootedValue.  JSObject has many subclasses so I don't know if this would work in exactly the same way, but object manipulating code is a good deal uglier after bug 782646 and it'd be nice to clean it up again.

Comment 1

[Brian Hackett [Laid off!]]

Unassigning.  After poking around and discussing with luke, the root issue here is that since JSObject is part of the API, any methods that Handle<JSObject> has need to be part of the API too which would require exposing all the getProperty etc. methods.  That may not be entirely bad, but could be avoided by making the object type entirely internal to the engine, e.g. a js::Object which we cast JSObject to at API boundaries.  Either way, making such a massive change a week before leaving for a couple months is probably not the best idea.

Comment 2

[Terrence Cole [:terrence]]

So, I ran into an extremely troublesome case today:
rootedFoo->anyMethodOnFoo(SomeMethodThatCanGC());

This crashes in some builds.  What is happening here is that the compiler is reordering RootedFoo::operator->() in front of SomeMethodThatCanGC() and storing the raw Foo* returned on the stack while calling SomeMethodThatCanGC.  The ordering of and lifetimes of temporaries is not defined, so C++ isn't going to make this easy.

Options:
1) Live with it.  Rewrite this code as:
     RootedFoo tmp(cx, SomeMethodThatCanGC());
     rootedFoo->anyMethodOnFoo(tmp);
We would need to audit our code for these, ensure future reviews look for them, and keep the dynamic analysis around to ensure we don't backslide.

2) Static all the things.  Rewrite this code as:
     Foo::anyMethodOnFoo(rootedFoo, SomeMethodThatCanGC());
   Ick.

3) Do what Brian is suggesting in this bug.  Don't rewrite this code, just have things work correctly.

Comment 3

[Terrence Cole [:terrence]]

Luke pointed out that the breaking code is inherently busted: if a method can GC, then it is fallible.  If the method is fallible, then we should not be able to use its result without checking it first.  We have to use (1) here to be correct at all.

This greatly reduces the impact, however it does leave one problem case.  Specifically, the broken function call can be hidden behind operator= syntax:

RootedFoo foo(cx);
foo->prop = MethodThatCanGC();  // *boom*
if (!foo->prop)
   return false;

This looks safe, but here's what actually happens:
//   RootedFoo::operator->  => return Foo* to stack
//   MethodThatCanGC()      => invalidates Foo* on stack
//   Foo::operator=         => uses invalid Foo* from stack as |this|

We could either (1) move all properties to getter/setter accesses, in which case we are back to the obvious "fallible methods on their own line" rule.  Or (2) we could ensure that we write all fallible calls to a temporary before making the assignment.

Comment 4

[Terrence Cole [:terrence]]

I think this turned out not to be a big deal in practice. It would be nice to have, but it would be hard enough to actually implement in practice that I don't think it's worth pursuing right now. Maybe the situation will change later but for now I'm going to close this.