Remove unnecessary uses of Expose*ToActiveJS

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Following on from bug 1297558 and bug 1306382, we can get rid of many calls to Expose*ToActiveJS.
(Assignee)

Comment 1

2 years ago
Created attachment 8801101 [details] [diff] [review]
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.
Assignee: nobody → jcoppeard
Attachment #8801101 - Flags: review?(continuation)
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.
Attachment #8801101 - Flags: review?(continuation) → review+
(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.
See Also: → bug 1310321
I just filed bug 1310321 for the CodeGen.py part. I can fix that.
(Assignee)

Comment 5

2 years ago
(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 :)
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.
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
status-firefox52: --- → fixed
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.