Remove nsXPCWrappedJS's global object

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
5 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed, firefox66 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

10 months ago
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.
Priority: -- → P3

Updated

9 months ago
Blocks: war-on-xbl
Assignee

Comment 1

5 months ago
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.
Blocks: 1514210
No longer blocks: war-on-xbl
No longer depends on: war-on-xbl-in-content
Assignee

Comment 2

5 months ago
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.
(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.
Depends on: 1507895
Assignee

Comment 4

5 months ago
(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.
No longer depends on: 1507895
Assignee

Updated

5 months ago
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED

Comment 5

5 months ago
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/114c94d2677c
Remove the global stored in nsXPCWrappedJS. r=bzbarsky

Comment 6

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/114c94d2677c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Assignee

Comment 7

5 months ago
[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
Attachment #9033990 - Flags: approval-mozilla-beta?
Assignee

Comment 8

5 months ago
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.
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.
Blocks: 1508102
Comment on attachment 9033990 [details] [diff] [review]
Patch for beta

[Triage Comment]
Fixes scary-looking crashes in automation. Approved for 65.0b8.
Attachment #9033990 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.