1480121 - Remove nsXPCWrappedJS's global object

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P3)

Description

[Jan de Mooij [:jandem]]

In bug 1478359 we're adding a global object pointer to nsXPCWrappedJS, but we can remove it once in-content XBL is eliminated.

See bug 1478359 comment 12.

Comment 1

[Jan de Mooij [:jandem]]

I think we should do this now:

(1) nsXPCWrappedJS has complicated GC behavior and we're seeing some oranges in this area.

(2) Due to the GC/CC machinery here, the global stored in nsXPCWrappedJS *must be* the object's global in the root-wrapper (implies non-CCW) case. If we do that, the global is redundant because we can just get it from the object when we need it.

(3) For the CCW case, it probably doesn't matter too much which chrome global we use so we can use the compartment's first global - we now have an API for that. This may also save some memory because it avoids keeping globals alive unnecessarily and matches what we do for WrappedNatives and CCWs now.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1480121 - Remove the global stored in nsXPCWrappedJS. r?bzbarsky

Reasons for doing this:

* nsXPCWrappedJS has complicated GC behavior and we're seeing some oranges in this area.

* Due to the GC/CC complexity, the global stored in nsXPCWrappedJS *must be* the
  object's global in the root-wrapper (implies non-CCW) case. If we do that, the
  global is redundant because we can just get it from the object when we need it.

* For the CCW case, it probably doesn't matter too much which chrome global we
  use so we can use the compartment's first global - we now have an API for that.
  This may also save some memory because it avoids keeping globals alive unnecessarily
  and matches what we do for WrappedNatives and CCWs now. Furthermore, bug 1478359
  comment 12 suggests CCWs can only show up here for in-content XBL and that's in the
  process of being removed.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> * For the CCW case, it probably doesn't matter too much which chrome global
> we
>   use so we can use the compartment's first global - we now have an API for
> that.
>   This may also save some memory because it avoids keeping globals alive
> unnecessarily
>   and matches what we do for WrappedNatives and CCWs now. Furthermore, bug
> 1478359
>   comment 12 suggests CCWs can only show up here for in-content XBL and
> that's in the
>   process of being removed.

Note that we do still support turning in-content XBL back on by setting the "dom.ua_widget.enabled" pref to false. So assuming this patch will break in-content XBL, let's block this on Bug 1507895.

We were planning to wait until the next merge after UA Widgets hit release (2019-01-28) to remove that pref, but we could discuss moving it up if there's a strong motivation for it.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> So assuming this patch will break
> in-content XBL, let's block this on Bug 1507895.

No it shouldn't break in-content XBL or affect it in any way. Right now the compartment changes are only for chrome compartments anyway, but even if we enabled for content and fixed other issues I still think the patch would be fine.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/114c94d2677c
Remove the global stored in nsXPCWrappedJS. r=bzbarsky

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/114c94d2677c

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch for beta

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1478359

User impact if declined: Potential crashes, intermittents

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Has been on Nightly for a few weeks. Just removes code for the most part.

String changes made/needed: None

Comment 8

[Jan de Mooij [:jandem]]

RyanVM asked about backporting this to beta because there was a signficant drop in intermittents after this landed. It's the same patch that landed on m-c but I had to add the js::GetFirstGlobalInCompartment API that landed in a different bug.

Comment 9

[Andrew McCreight [:mccr8]]

Oh this fixed those intermittent XPCWJS crashes we were seeing all over the place? Awesome! Yeah, this should definitely get uplifted. Those crashes looked like security issues.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9033990 [details] [diff] [review]
Patch for beta

[Triage Comment]
Fixes scary-looking crashes in automation. Approved for 65.0b8.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ef2cd013fe73