887077 - JS::CompileOptions incorrectly roots 'element' member

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

The current declaration is:

    HandleObject element;

which is totally bogus for a structure element; Handles are only appropriate for function arguments. This means that any CompileOptions instance whose lifetime isn't stack-like, and that has its setElement member called, will probably end up with its 'element' Handle pointing to freed stack space.

Fortunately, there are no calls to setElement in the browser at the moment.

Comment 1

[Jim Blandy :jimb]

Attachment: Properly root JS::CompileOptions::element.

Comment 2

[Jim Blandy :jimb]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=ab9ccff25227

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 767523 [details] [diff] [review]
Properly root JS::CompileOptions::element.

Review of attachment 767523 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +5184,5 @@
> +}
> +
> +JS::CompileOptions &
> +CompileOptions::setElement(HandleObject e) {
> +    /* Roots aren't allowed to be NULL, so we need to manage things here. */

Where does it break when they're NULL? I tried to trace it through, but it seems like the writeBarrierPre does a NULL check before doing anything, and then it calls 
rt->gcRootsHash.put unconditionally with &element.

@@ +5186,5 @@
> +JS::CompileOptions &
> +CompileOptions::setElement(HandleObject e) {
> +    /* Roots aren't allowed to be NULL, so we need to manage things here. */
> +    if (e)
> +        JS_AddNamedObjectRoot(cx, &element, "JS::CompileOptions::element");

It looks like adding roots is idempotent, so this is ok. We ought to have a comment to that effect in jsapi.h.

::: js/src/jsapi.h
@@ +3880,4 @@
>  
>  /* Options for JavaScript compilation. */
>  struct JS_PUBLIC_API(CompileOptions) {
> +    JSContext *cx;

Given that the whole problem here is that CompileOptions is sometimes on the stack, sometimes on the heap, how do you know it's safe to store a cx here when it's on the heap? Is it guaranteed that the cx will outlive the CompileOptions?

@@ +3903,5 @@
> +    /*
> +     * These members are dynamic roots, registered explicitly by the constructor/destructor.
> +     * You'll need to use Handle::fromMarkedLocation to get Handles for them.
> +     */
> +    JSObject *element;

So it seems to me that you need a copy constructor here, since the default will do a bitwise copy of element and result in an unrooted pointer in the copy. So you need to explicitly add the new &element to the root set as well.

And that results in quite a bit of logic in something named 'CompileOptions' to implement all the root juggling. For that reason, I think I'd rather see the separate DynamicRoot class you were talking about to encapsulate all this, even if it's just for this one use. Only I have to bikeshed the name.

  I do not like "DynamicRoot"
  I do not think that it is goot
  A "dynamic" could be a stack
  I think you need another tack
  I would not like it in a suit
  I do not think that it is cute
  I would not wear it as a coat
  I would not like it with a goat
  "AutoRoot" would be just fine
  Except that name's already mine
  I do not like DynamicRoot
  I do not like, give it the boot!

Well, more specifically there's already an AutoRooter, which still has the LIFO restriction. It could be renamed, though. It's sort of a CustomRooted or RootedExternal or something.

Also, on the larger question -- it sounds like the CompileOptions can outlive the element right now? This code keeps it alive, but what does it mean when that happens? You'd end up with a script compiled for a dead element of some sort? Is this going to leak weird stuff or prevent some sort of C++-level finalization or something?

In general, structures that can appear on either the stack or the heap are very problematic for GGC. I'd want to be sure we really need this one before going to too much effort.

Comment 4

[Jim Blandy :jimb]

(In reply to Steve Fink [:sfink] from comment #3)
> Where does it break when they're NULL? I tried to trace it through, but it
> seems like the writeBarrierPre does a NULL check before doing anything, and
> then it calls 
> rt->gcRootsHash.put unconditionally with &element.

js::gc::MarkRuntime
-> MarkObjectRoot
   -> MarkRoot<Object>
      -> MarkInternal
         -> CheckMarkedThing
            -> JS_ASSERT(thing)

> It looks like adding roots is idempotent, so this is ok. We ought to have a
> comment to that effect in jsapi.h.

Yes; I checked this. The comment is a good suggestion.

> Given that the whole problem here is that CompileOptions is sometimes on the
> stack, sometimes on the heap, how do you know it's safe to store a cx here
> when it's on the heap? Is it guaranteed that the cx will outlive the
> CompileOptions?

No. Rats. Thanks for catching this. :(

> @@ +3903,5 @@
> > +    /*
> > +     * These members are dynamic roots, registered explicitly by the constructor/destructor.
> > +     * You'll need to use Handle::fromMarkedLocation to get Handles for them.
> > +     */
> > +    JSObject *element;
> 
> So it seems to me that you need a copy constructor here, since the default
> will do a bitwise copy of element and result in an unrooted pointer in the
> copy. So you need to explicitly add the new &element to the root set as well.

Right --- I realized this yesterday and fixed it in my version of the patch, but didn't update the bug. Sorry about that.

> And that results in quite a bit of logic in something named 'CompileOptions'
> to implement all the root juggling. For that reason, I think I'd rather see
> the separate DynamicRoot class you were talking about to encapsulate all
> this, even if it's just for this one use. Only I have to bikeshed the name.
> 
>   I do not like "DynamicRoot"
>   I do not think that it is goot
>   A "dynamic" could be a stack
>   I think you need another tack
>   I would not like it in a suit
>   I do not think that it is cute
>   I would not wear it as a coat
>   I would not like it with a goat
>   "AutoRoot" would be just fine
>   Except that name's already mine
>   I do not like DynamicRoot
>   I do not like, give it the boot!
> 
> Well, more specifically there's already an AutoRooter, which still has the
> LIFO restriction. It could be renamed, though. It's sort of a CustomRooted
> or RootedExternal or something.

This was very funny, but unhelpful. :) Naming complaints are so troublesome in general that they must always be accompanied by specific suggestions --- and suggestions good enough that you would be happy to see them implemented, not just "throwing things out there".</grr>

> Also, on the larger question -- it sounds like the CompileOptions can
> outlive the element right now? This code keeps it alive, but what does it
> mean when that happens? You'd end up with a script compiled for a dead
> element of some sort? Is this going to leak weird stuff or prevent some sort
> of C++-level finalization or something?

No doubt. Perhaps it should be a weak reference? Do we even have those?

> In general, structures that can appear on either the stack or the heap are
> very problematic for GGC. I'd want to be sure we really need this one before
> going to too much effort.

Well, perhaps it should be a weak reference; I'm wary in general of the debugger entraining all sorts of potentially big structures, and elements entrain their whole document. But we definitely want this information --- it's going to allow the debugger to relate dynamically added source code to the way it entered the page. "Where did this code come from?" Better backtraces, better development experience, the whole nine yards.

Comment 5

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #4)
> > And that results in quite a bit of logic in something named 'CompileOptions'
> > to implement all the root juggling. For that reason, I think I'd rather see
> > the separate DynamicRoot class you were talking about to encapsulate all
> > this, even if it's just for this one use. Only I have to bikeshed the name.
> > 
> >   I do not like "DynamicRoot"
> >   I do not think that it is goot
> >   A "dynamic" could be a stack
> >   I think you need another tack
> >   I would not like it in a suit
> >   I do not think that it is cute
> >   I would not wear it as a coat
> >   I would not like it with a goat
> >   "AutoRoot" would be just fine
> >   Except that name's already mine
> >   I do not like DynamicRoot
> >   I do not like, give it the boot!

Would you like it "ExplicitRoot"?
Or shall I give 'element' the boot?
Would you, with a pointer weak?
Shall I make some other tweak?

Comment 6

[Jim Blandy :jimb]

This isn't even "dogerel"; it's "dogs-versus-cats-erel"...

Comment 7

[Steve Fink [:sfink] [:s:]]

(In reply to Jim Blandy :jimb from comment #4)
> (In reply to Steve Fink [:sfink] from comment #3)
> > Given that the whole problem here is that CompileOptions is sometimes on the
> > stack, sometimes on the heap, how do you know it's safe to store a cx here
> > when it's on the heap? Is it guaranteed that the cx will outlive the
> > CompileOptions?
> 
> No. Rats. Thanks for catching this. :(

I'm not sure what to do about this. Seems like it's a problem for any scheme.

I looked briefly at letting Add*Root take a runtime instead of a cx, but remember it looking hard.

> > Well, more specifically there's already an AutoRooter, which still has the
> > LIFO restriction. It could be renamed, though. It's sort of a CustomRooted
> > or RootedExternal or something.
> 
> This was very funny, but unhelpful. :) Naming complaints are so troublesome
> in general that they must always be accompanied by specific suggestions ---
> and suggestions good enough that you would be happy to see them implemented,
> not just "throwing things out there".</grr>

I only have time to spend 15 minutes or being funny; I don't have enough time to spend the 1 or 2 minutes it would take to be helpful.

Now I'm thinking "AutoAddRoot", to connect it with the Add*Root functions. Or perhaps AutoNamedRoot, especially since that works a little better if you were to later switch the implementation to a doubly-linked list instead of using the Add*Root hashtable.

Note that any RAII class won't nest since Add*Root is idempotent. The first Remove does it for everyone. We may need to return a boolean from the Remove functions so you can assert.

I guess a doubly-linked list could hang off the runtime, which would fix the cx issue too.

> > Also, on the larger question -- it sounds like the CompileOptions can
> > outlive the element right now? This code keeps it alive, but what does it
> > mean when that happens? You'd end up with a script compiled for a dead
> > element of some sort? Is this going to leak weird stuff or prevent some sort
> > of C++-level finalization or something?
> 
> No doubt. Perhaps it should be a weak reference? Do we even have those?

Not really. We have WeakMap plus a number of manually-implemented weak pointers. I guess a WeakMap would work, but they're not really set up for heavy use right now. I just filed bug 887915 to see if it makes sense to add something general.

If this were weak, what behavior would you want if the element goes away? Would it pass a null value to the JS caller, which would end up displaying it as " (deleted)" or something? Could you copy out enough information to be useful? 

> > In general, structures that can appear on either the stack or the heap are
> > very problematic for GGC. I'd want to be sure we really need this one before
> > going to too much effort.
> 
> Well, perhaps it should be a weak reference; I'm wary in general of the
> debugger entraining all sorts of potentially big structures, and elements
> entrain their whole document. But we definitely want this information ---
> it's going to allow the debugger to relate dynamically added source code to
> the way it entered the page. "Where did this code come from?" Better
> backtraces, better development experience, the whole nine yards.

Oh! I don't want to be discouraging, because I would love love love to have this. And it seems like there's no fundamental reason why it can't work, just lots of annoying superficial problems. I should really stare at the code so I can understand the case where CompileOptions is heap-allocated. Then maybe I could be more helpful.

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] from comment #7)
> I looked briefly at letting Add*Root take a runtime instead of a cx, but
> remember it looking hard.

Doesn't look at all hard to me right now.  Such an JS_AddRoot(JSRuntime*, Value*) wouldn't be able to report OOM if it failed, due to lack of context, which might complicate things slightly.  Other than that this seems trivial to me.

Comment 9

[Steve Fink [:sfink] [:s:]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> (In reply to Steve Fink [:sfink] from comment #7)
> > I looked briefly at letting Add*Root take a runtime instead of a cx, but
> > remember it looking hard.
> 
> Doesn't look at all hard to me right now.  Such an JS_AddRoot(JSRuntime*,
> Value*) wouldn't be able to report OOM if it failed, due to lack of context,
> which might complicate things slightly.  Other than that this seems trivial
> to me.

Yeah, I think it was the OOM report that I got hung up on.

Oh! Sure enough, there's already AddValueRootRT. Just need AddObjectRootRT and friends. So it seems we already don't care.

Comment 10

[Jim Blandy :jimb]

The doubly-linked list approach seems perfect. I'll give that a shot.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This doesn't seem like a good approach to me. As far as I can tell, all uses of CompileOptions in the browser are already on the stack. Could we just add a MOZ_STACK_CLASS annotation to CompileOptions and then make the |element| field have type RootedObject?

Even if we do want to heap-allocate CompileOptions, storing a JSContext inside doesn't seem kosher to me. The lifetime of JSContext may not be longer than the heap-allocated CompileOptions.

Finally, adding things as GC roots has always been a bit problematic. For one thing, these roots aren't known to the cycle collector and so they can lead to leaks. If we actually do heap-allocate CompileOptions and store them in some refcounted place, this could be a problem.

Comment 12

[Jim Blandy :jimb]

(In reply to Bill McCloskey (:billm) from comment #11)
> This doesn't seem like a good approach to me. As far as I can tell, all uses
> of CompileOptions in the browser are already on the stack. Could we just add
> a MOZ_STACK_CLASS annotation to CompileOptions and then make the |element|
> field have type RootedObject?

This one is in the heap:
http://dxr.mozilla.org/mozilla-central/source/js/src/ion/AsmJSModule.h#l326

If we could get rid of that use, we could make CompileOptions MOZ_STACK_CLASS. I haven't looked into what's entailed.


> Even if we do want to heap-allocate CompileOptions, storing a JSContext
> inside doesn't seem kosher to me. The lifetime of JSContext may not be
> longer than the heap-allocated CompileOptions.

That's been pointed out, in comment 3. The doubly-linked list approach doesn't require holding a cx.


> Finally, adding things as GC roots has always been a bit problematic. For
> one thing, these roots aren't known to the cycle collector and so they can
> lead to leaks. If we actually do heap-allocate CompileOptions and store them
> in some refcounted place, this could be a problem.

Right, because they're roots, not traced. I really don't understand the context of HandleDynamicLinkFailure at all.

Comment 13

[Jim Blandy :jimb]

I believe that PostLinkFailureInfo gets held by asm.js modules, so it's got an arbitrary lifetime. Putting a root in there is definitely wrong.

Comment 14

[Luke Wagner [:luke]]

Heh, yeah, I just saw that root in CompileOptions in PostLinkFailureInfo and it is removed by the patches in bug 880538 (hopefully to reviewed by jorendorff soon...).

Comment 15

[Eddy Bruel [:ejpbruel]]

Attachment: Make CompileOptions a MOZ_STACK_CLASS

Luke tells me it is now safe to turn CompileOptions into a MOZ_STACK_CLASS, so here is a patch that does just that. Flagging luke for review since he's familiar with the problem.

Comment 16

[Steve Fink [:sfink] [:s:]]

Comment on attachment 781236 [details] [diff] [review]
Make CompileOptions a MOZ_STACK_CLASS

We should check with bhackett too, since he needed to have some of this stuff on the heap for off-thread compilation iirc.

Comment 17

[Brian Hackett [Laid off!]]

Comment on attachment 781236 [details] [diff] [review]
Make CompileOptions a MOZ_STACK_CLASS

Review of attachment 781236 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, off thread parsing allocates CompileOptions on the heap.

Comment 18

[Eddy Bruel [:ejpbruel]]

(In reply to Brian Hackett (:bhackett) from comment #17)
> Comment on attachment 781236 [details] [diff] [review]
> Make CompileOptions a MOZ_STACK_CLASS
> 
> Review of attachment 781236 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, off thread parsing allocates CompileOptions on the heap.

Any way we can avoid that?

Comment 19

[Brian Hackett [Laid off!]]

(In reply to Eddy Bruel [:ejpbruel] from comment #18)
> (In reply to Brian Hackett (:bhackett) from comment #17)
> > Comment on attachment 781236 [details] [diff] [review]
> > Make CompileOptions a MOZ_STACK_CLASS
> > 
> > Review of attachment 781236 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Yeah, off thread parsing allocates CompileOptions on the heap.
> 
> Any way we can avoid that?

Not really.  We need to access the CompileOptions to parse/emit on another thread, and those CompileOptions are created on the main thread, so there's really no way to transfer the data between the two without putting it on the heap.

Can you use JS_AddRoot and JS_RemoveRoot in the elements setter and dtor to manage rooting on |elements|?  With off thread parsing the heap CompileOptions will only be read off thread; the ctor/dtor and all writing happen on the main thread.

Comment 20

[Jim Blandy :jimb]

To address this, I'm working on a patch that splits CompileOptions into three classes:

- A non-instantiable base class, ReadOnlyCompileOptions, from which one can read options, but in which one may not not store them. JS::Compile and JS::Evaluate take const references to these, as does most internal use of CompileOptions.

- A subclass, CompileOptions, marked MOZ_STACK_CLASS, which refers to option values owned by something that can guarantee that their lifespan will exceed that of the CompileOptions itself.

- A second subclass of ReadOnlyCompileOptions, OwningCompileOptions, which makes copies of / roots / bumps the reference counts of all the option values stored in it. This can be kept on the heap, for cases like off-thread compilation.

Comment 21

[Jim Blandy :jimb]

Attachment: Use getter functions to retrieve ownable resources from CompileOptions, instead of direct data member access.

Comment 22

[Jim Blandy :jimb]

Attachment: Split CompileOptions into ReadOnlyCompileOptions, CompileOptions, and OwningCompileOptions.

This is the real meat.

I've separated out the patches that just change all the uses to the appropriate derived classes for ease of review; but this patch and the next two need to be applied as a unit.

Comment 23

[Jim Blandy :jimb]

Attachment: Use ReadOnlyCompileOptions in preference to CompileOptions where possible.

Comment 24

[Jim Blandy :jimb]

Attachment: Use OwningCompileOptions for off-main-thread compilation.

Comment 25

[Terrence Cole [:terrence]]

Comment on attachment 824763 [details] [diff] [review]
Use getter functions to retrieve ownable resources from CompileOptions, instead of direct data member access.

Review of attachment 824763 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: js/src/jsobj.h
@@ +451,5 @@
>  
>      /*
>       * We allow the prototype of an object to be lazily computed if the object
>       * is a proxy. In the lazy case, we store (JSObject *)0x1 in the proto field
> +     * of the object's TypeObject. We offer three ways of getting th e prototype:

I guess this hunk is spurious?

Comment 26

[Terrence Cole [:terrence]]

Comment on attachment 824764 [details] [diff] [review]
Split CompileOptions into ReadOnlyCompileOptions, CompileOptions, and OwningCompileOptions.

Review of attachment 824764 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: js/src/jsapi.cpp
@@ +4348,5 @@
> +    : ReadOnlyCompileOptions(),
> +      runtime(GetRuntime(cx)),
> +      elementRoot(GetRuntime(cx))
> +{
> +    elementPtr = elementRoot.address();

And this can use fromMarkedLocation. (see below)

@@ +4385,5 @@
> +        copy = JS_strdup(cx, f);
> +        if (!copy)
> +            return false;
> +    } else {
> +        copy = nullptr;

Just initialize copy to nullptr and leave off the else block.

@@ +4405,5 @@
> +        copy = js_strdup(cx, s);
> +        if (!copy)
> +            return false;
> +    } else {
> +        copy = nullptr;

Ditto.

::: js/src/jsapi.h
@@ +3402,2 @@
>  {
> + protected:

Two spaces before protected.

@@ +3406,5 @@
>      const char *filename_;
>      const jschar *sourceMapURL_;
> +
> +    // A pointer to some properly rooted location that refers to the element.
> +    JSObject * const *elementPtr;

I think this should be a Handle, actually. Both cases have a clear path to Rooted/Persistent with identical lifetime.

@@ +3446,5 @@
>      JSPrincipals *principals() const { return principals_; }
>      JSPrincipals *originPrincipals() const;
>      const char *filename() const { return filename_; }
>      const jschar *sourceMapURL() const { return sourceMapURL_; }
> +    JSObject *element() const { return *elementPtr; }

To be 100% clear: this will stay exactly the same, despite changing elementPtr to Handle. 

i.e. Copy by value to force the caller to re-root/heapify so that a reference to the rooted doesn't leak past the lifetime of the embedded root, just as you are doing now.

@@ +3487,5 @@
> + * or indirectly, by a JavaScript object: if any value that this roots ever
> + * comes to refer to the object that owns this, then the whole cycle, and
> + * anything else it entrains, will never be freed.
> + */
> +class JS_PUBLIC_API(OwningCompileOptions): public ReadOnlyCompileOptions

One space before the :.

@@ +3546,5 @@
> + * filename; source map URL) that are owned by something else. If you
> + * create an instance of this type, it's up to you to guarantee that
> + * everything you store in it will outlive it.
> + */
> +class MOZ_STACK_CLASS JS_PUBLIC_API(CompileOptions): public ReadOnlyCompileOptions

One space before the :.

@@ +3561,5 @@
> +        principals_ = rhs.principals();
> +        originPrincipals_ = rhs.originPrincipals();
> +        filename_ = rhs.filename();
> +        sourceMapURL_ = rhs.sourceMapURL();
> +        elementPtr = elementRoot.address();

And this should auto-convert.

Comment 27

[Terrence Cole [:terrence]]

Comment on attachment 824766 [details] [diff] [review]
Use ReadOnlyCompileOptions in preference to CompileOptions where possible.

Review of attachment 824766 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: js/src/jsapi.cpp
@@ +4471,5 @@
>      return script;
>  }
>  
>  JSScript *
> +JS::Compile(JSContext *cx, HandleObject obj, const ReadOnlyCompileOptions &options_, const char *filename)

Despite continuing disagreement (including from me), the standard style here is optionsArg instead of options_.

@@ +4838,5 @@
>  
>  static const unsigned LARGE_SCRIPT_LENGTH = 500*1024;
>  
>  extern JS_PUBLIC_API(bool)
> +JS::Evaluate(JSContext *cx, HandleObject obj, const ReadOnlyCompileOptions &options_,

Ditto.

@@ +4897,5 @@
>      return ok;
>  }
>  
>  extern JS_PUBLIC_API(bool)
> +JS::Evaluate(JSContext *cx, HandleObject obj, const ReadOnlyCompileOptions &options_,

Ditto.

Comment 28

[Terrence Cole [:terrence]]

Comment on attachment 824767 [details] [diff] [review]
Use OwningCompileOptions for off-main-thread compilation.

Review of attachment 824767 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ r=me

Comment 29

[Jim Blandy :jimb]

(In reply to Terrence Cole [:terrence] from comment #25)
> I guess this hunk is spurious?

Certainly was... sorry about that.

Comment 30

[Jim Blandy :jimb]

Okay, I've made the revisions Terrence requested, except that we made the element getter an abstract virtual member function, instead of turning the data member into a Handle; construction ordering issues make that unworkable.

Comment 31

[Jim Blandy :jimb]

One last try push because I'm completely paranoid:
https://tbpl.mozilla.org/?tree=Try&rev=9fadfe5ec39f

Comment 32

[Jim Blandy :jimb]

Try push looked good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb08dc14d68
https://hg.mozilla.org/integration/mozilla-inbound/rev/528cc63b3e0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/74a8ba38863b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cbb15a6a12e

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2fb08dc14d68
https://hg.mozilla.org/mozilla-central/rev/528cc63b3e0a
https://hg.mozilla.org/mozilla-central/rev/74a8ba38863b
https://hg.mozilla.org/mozilla-central/rev/3cbb15a6a12e