1306579 - Remove unnecessary uses of Expose*ToActiveJS

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Following on from bug 1297558 and bug 1306382, we can get rid of many calls to Expose*ToActiveJS.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1306579-remove-expose-calls

Patch to remove use of Expose*ToActiveJS() when reading out of a Heap<T> since this now happens automatically.

I simplified some uses to call exposeToActiveJS() on the Heap<T> which conditionally does the expose if the contents are non-null.

I also removed nsIScriptContext::GetWindowProxyPreserveColor since it was not used.

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8801101 [details] [diff] [review]
bug1306579-remove-expose-calls

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

Can you make calling ExposeActiveObjectToActiveJS a compile error? That might be neat. Maybe it isn't worthwhile.

It would be worth taking a look at CodeGen.py, too. I see 201 instances of ExposeObjectToActiveJS in WebIDL codegenned files. There are only three instances of it. I can fix that if you want.

The code looks like the same sort of thing you are fixing here:
  JS::ExposeObjectToActiveJS(result);
  args.rval().setObject(*result);

::: dom/base/nsFrameMessageManager.cpp
@@ +1904,5 @@
>  void
>  nsMessageManagerScriptExecutor::MarkScopesForCC()
>  {
>    for (uint32_t i = 0; i < mAnonymousGlobalScopes.Length(); ++i) {
> +    mAnonymousGlobalScopes[i].exposeToActiveJS();

Much nicer.

::: dom/base/nsGlobalWindow.cpp
@@ -2648,5 @@
>      // We're reusing the inner window, but this still counts as a navigation,
>      // so all expandos and such defined on the outer window should go away. Force
>      // all Xray wrappers to be recomputed.
> -    JS::Rooted<JSObject*> rootedObject(cx, GetWrapperPreserveColor());
> -    JS::ExposeObjectToActiveJS(rootedObject);

Well, that's odd...

::: dom/promise/PromiseCallback.cpp
@@ +145,1 @@
>    JS::ExposeValueToActiveJS(aValue);

Can you get rid of this call for aValue here and below? I'd guess that the Rooted<> value call would do an expose.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Can you get rid of this call for aValue here and below? I'd guess that the
> Rooted<> value call would do an expose.

I don't mean you necessarily need to fix this, given that it is in old promises code we don't even compile any more; I was just curious about whether it would be ok.

Comment 4

[Andrew McCreight [:mccr8]]

I just filed bug 1310321 for the CodeGen.py part. I can fix that.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #2)

> ::: dom/promise/PromiseCallback.cpp
> @@ +145,1 @@
> >    JS::ExposeValueToActiveJS(aValue);
> 
> Can you get rid of this call for aValue here and below? I'd guess that the
> Rooted<> value call would do an expose.

As noted in bug 1310321 there's no read barrier on Rooted.

> I just filed bug 1310321 for the CodeGen.py part. I can fix that.

Cheers :)

Comment 6

[Boris Zbarsky [:bzbarsky]]

It's not clear to me how a gray value would enter ResolvePromiseCallback::Call as aValue.  I suspect that can't happen now that Heap has a read barrier.

But it's also not worth worrying about too much, because all this code is #ifndef SPIDERMONKEY_PROMISE and we should instead aim to remove it or something.

Comment 7

[Jon Coppeard (:jonco)]

Ugh, I pushed this with the wrong bug number (bug 1297558):

https://hg.mozilla.org/integration/mozilla-inbound/rev/1538850bba0f
https://hg.mozilla.org/mozilla-central/rev/1538850bba0f