Closed Bug 1547923 Opened 7 months ago Closed 6 months ago

Investigate automatically exposing the result of calling nsIGlobalObject::GetGlobalJSObject to JS

Categories

(Core :: DOM: Core & HTML, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: jonco, Assigned: bzbarsky)

References

Details

Attachments

(7 files)

Currently some implementations of nsIGlobalObject::GetGlobalJSObject can return a gray object. This is problematic in places where this is immediately passed into the JS API.

Previously some instances have been fixed by adding JS::ExposeObjectToActiveJS calls (e.g. in bug 1522817) but it would be better if this wasn't required.

It should be possible to do this automatically. It may be necessary to add a version of this method that doesn't do this if that is useful somewhere, or maybe we could add boolean method to indicate whether the global exists.

There is also a comment in nsIGlobalObject.h describing further work that should be done if we make this change:

  // GetGlobalJSObject may return a gray object.  If this ever changes so that
  // it stops doing that, please simplify the code in FindAssociatedGlobal in
  // BindingUtils.h that does JS::ExposeObjectToActiveJS on the return value of
  // GetGlobalJSObject.  Also, in that case the JS::ExposeObjectToActiveJS in
  // AutoJSAPI::InitInternal can probably be removed.  And also the similar
  // calls in XrayWrapper and nsGlobalWindow.

https://searchfox.org/mozilla-central/source/dom/base/nsIGlobalObject.h#77-83

Summary: Investigate automatically exposing the result of calling nsGlobalObject::GetGlobalJSObject to JS → Investigate automatically exposing the result of calling nsIGlobalObject::GetGlobalJSObject to JS
Attached file Callsite audit

Jon, could you take a look at the js::NotifyAnimationActivity bits in HTMLCanvasElement::InvalidateCanvasContent and HTMLMediaElement::SeekToNextFrame and comment about whether those need exposure?

Flags: needinfo?(jcoppeard)

I don't think these ones strictly need to expose, but I feel like we should make this simple and always expose when calling into the JS engine.

Flags: needinfo?(jcoppeard)

Promise::Compartment is unused.

The callers that want to call AutoJSAPI::Init can pass it an nsIGlobalObject,
which is actually more efficient, since passing a JSObject just gets an
nsIGlobalObject from it and passes that.

This can be used in things like assertions or some other rare circumstances
where not exposing the object to active JS is OK.

Consumers that just care about this boolean state should use this instead of
getting the JSObject* directly.

See callsite audit in https://bugzilla.mozilla.org/attachment.cgi?id=9061976
for details on why the remaining GetGlobalJSObject callers should switch to the
"always expose" behavior.

Blocks: 1548613
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57f7cff9a4b1
part 1.  Remove unused callers of GetGlobalJSObject.  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/64fb8460f0f8
part 2.  Remove Promise::GlobalJSObject.  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/358e6f08d9c5
part 3.  Add nsIGlobalObject::GetGlobalJSObjectPreserveColor().  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/2d60837b8f3c
part 4.  Add nsIGlobalObject::HasJSGlobal().  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/9437efdc6fc3
part 5.  Start using nsIGlobalObject::GetGlobalJSObjectPreserveColor where possible.  r=mccr8,jonco
https://hg.mozilla.org/integration/autoland/rev/a0971c8a28a9
part 6.  Make nsIGlobalObject::GetGlobalJSObject always expose to active JS.  r=mccr8
Assignee: nobody → bzbarsky
You need to log in before you can comment on or make changes to this bug.