941122 - nsXBLProtoImplMethod and nsXBLProtoImplProperty don't call ExposeObjectToActiveJS when they should

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jim Blandy :jimb]

nsXBLProtoImplMethod and nsXBLProtoImplProperty both use nsXBLMaybeCompiled to hold onto JSFunction objects. They participate in cycle collection via their Trace methods, meaning that these function objects are sometimes marked gray. However, those classes do not properly call JS::ExposeObjectToActiveJS before using their JSFunctions, and as a result SpiderMonkey is passed gray objects to operate on - and SpiderMonkey should never see gray objects.

At the moment, this doesn't happen to cause problems. However, a proposed patch in bug 637572 to make cloned JSScripts refer to their ScriptSourceObjects via a cross-compartment-wrapper, rather than cloning them too - a change that ought to have little consequence - does lead to js::CloneScript being passed gray objects.

Comment 1

[Jim Blandy :jimb]

Attachment: Make nsXBLProtoImplMethod and nsXBLProtoImplProperty handle gray objects properly.

This patch adds calls to JS::ExposeObjectToActiveJS to those methods of nsXBLProtoImplMethod and nsXBLProtoImplProperty that seem to need them.

It might be more appropriate to change nsXBLMaybeCompiled to do the exposure, since it's already concerned with barriers. However, we would need to make some corresponding changes to nsXBLProtoImplMethod and nsXBLProtoImplProperty to avoid triggering exposure when it's not appropriate --- for example, in their Trace methods. (Exposing there leaks the world, because marking the thing gray marks the JSFunctions black...)

Comment 2

[Jim Blandy :jimb]

... and with that prior patch applied, bug 637572's patches work as expected.

Comment 3

[Andrew McCreight [:mccr8]]

Nice find!

> However, we would need to make some corresponding changes to nsXBLProtoImplMethod and nsXBLProtoImplProperty to avoid triggering exposure

The usual way to handle this is to make a PreserveColor variant of these methods, but I don't know how many of them there are, or how many places they are used.

Comment 4

[Jim Blandy :jimb]

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

Comment 5

[Jim Blandy :jimb]

Attachment: nsXBLProtoImplMethod-expose-gray.patch

Here's a revision of the patch that puts the read barrier in nsXBLMaybeCompiled, and adds a non-barrier "...PreserveColor" accessor. That needed to be threaded through various places, but it does seem that the new patch is concerned with marking up unusual uses, instead of normal uses.

Since the GetJSFunction member function is not quite as trivial as it used to be, I changed nsXBLProtoImplProperty::InstallMember not to hit it quite as hard as it used to.

Try: https://tbpl.mozilla.org/?tree=Try&rev=442165e54b5d

Comment 6

[Jim Blandy :jimb]

Attachment: Make nsXBLProtoImplMethod and nsXBLProtoImplProperty handle gray objects properly.

Folded in the assertion that catches the bug. (This is included in the try push.)

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8336294 [details] [diff] [review]
Make nsXBLProtoImplMethod and nsXBLProtoImplProperty handle gray objects properly.

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

Nice!  smaug should look at this, too, as I don't know anything about XBL in particular.  Though he's not listed as an XBL peer either...

::: content/xbl/src/nsXBLMaybeCompiled.h
@@ +39,5 @@
>      uintptr_t unmasked = mUncompiled & ~BIT_UNCOMPILED;
>      return reinterpret_cast<UncompiledT*>(unmasked);
>    }
>  
> +  // Return the compiled JS function to which we refer. For the benefit of

This comment doesn't seem necessary.

@@ +50,5 @@
> +      JS::ExposeObjectToActiveJS(mCompiled);
> +    return mCompiled;
> +  }
> +
> +  // Return the compiled JS function to which we refer. Do not prepare it

The first two sentences don't seem very useful.

Comment 8

[Andrew McCreight [:mccr8]]

> I changed nsXBLProtoImplProperty::InstallMember not to hit it quite as hard as it used to.

This shouldn't really be a big deal.  Only the first call will do any real work.  Bill made a lot of effort to ensure that the fast path for the expose function is fast.  (At least on the scale of something like InstallMember.)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8336294 [details] [diff] [review]
Make nsXBLProtoImplMethod and nsXBLProtoImplProperty handle gray objects properly.


> 
>+  // Return the compiled JS function to which we refer. For the benefit of
>+  // cycle collection, prepare it to be exposed to active use by
>+  // SpiderMonkey.
>   JSObject* GetJSFunction() const
>   {
>     MOZ_ASSERT(IsCompiled(), "Attempt to get uncompiled function as compiled");
>+    if (mCompiled)
>+      JS::ExposeObjectToActiveJS(mCompiled);
if (expr) {
  stmt;
}

Comment 10

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/725dad9a964b

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/725dad9a964b