Make XPCWrappedNative always keep its JS reflector object alive

NEW
Unassigned

Status

()

Core
XPConnect
4 years ago
2 years ago

People

(Reporter: mccr8, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
Summary: Simplify XPCWrappedNative traversal → Simplify XPCWrappedNative cycle collector traversal
(Reporter)

Comment 1

4 years ago
The reason I want to do this is to enable dynamic soundness checking for ICC, which works like this:
1. At the start of an ICC, run a regular CC that doesn't collect anything, but just records what is garbage.
2. Finish the ICC.
3. Make sure that everything the ICC thinks is garbage is something the regular CC thought was garbage.

The problem is that if in between steps 1 and 2 an XPCWN drops in refcount to 1, then we no longer hold the JS object alive, which means that something held alive by the JS object may be seen as garbage by the CC, when it wasn't garbage at the start of the CC. (Note that this is a false positive from the checker.)

The specific instance of this I've seen is the XPCWN of a window which holds onto a JS object that keeps alive the XPCWN for 2 DOM prototypes and an nsXPCComponents.

---

The main thing I am interested in doing is getting rid of the thing in the XPCWN traverse method where we only traverse the mFlatJSObject if the refcount is greater than one (which really should be a call to HasExternalReference()).  That shouldn't be necessary, because the cycle collector can break cycles itself.  That's easy enough to do.

We want to keep the garbage collector and cycle collector in synch with regards to what they think is alive or not, so we have to also modify WrappedNativeJSGCThingTracer and WrappedNativeSuspecter.  If we just drop the HasExternalReference() check from those, then we end up with weird shutdown behavior, where things are not cleaned up immediately.  On IRC, Peter pointed out that XPCWNs can wrap non-cycle-collected natives, which seemed to match with what I'm seeing.  I have tried to work around that by only checking HasExternalReferences when the underlying object isn't CCed, which we can check by trying to QI to one of the magic CC interfaces.

Anyways, I'm not sure if the resulting code is really simpler.  But what we run in the CCed XPCWN case is a little simpler...

---

A minor cleanup I'd like to do is inlining XPCJSRuntime::SuspectWrappedNative, because there's no reason for it to exist as a separate entity.
(Reporter)

Updated

4 years ago
Depends on: 928655
(Reporter)

Updated

4 years ago
Depends on: 931283
(Reporter)

Updated

4 years ago
Depends on: 931285
(Reporter)

Comment 3

4 years ago
Created attachment 822807 [details] [diff] [review]
WIP

With the patches in bug 931283 and bug 931285, the RSS feed unit tests don't have an extra shutdown cycle collection with this patch.
Attachment #819239 - Attachment is obsolete: true
(Reporter)

Comment 4

4 years ago
A cheesier solution to this false positive would be to unmark gray the JS object if we're in the middle of a GC when we unroot the JS object.
(Reporter)

Comment 5

4 years ago
The only real point of this is to eliminate false positives from CC checking, which hasn't yet actually found any errors, so I think it can be delayed to the later polishing-for-enabling-by-default phase of things.
Blocks: 911246
No longer blocks: 850065
(Reporter)

Updated

4 years ago
No longer blocks: 911246
(Reporter)

Updated

4 years ago
Blocks: 956068
(Reporter)

Comment 6

3 years ago
Bug 942528 made XPCWNs use the normal refcounting, but there's still some odd behavior where the XPCWN doesn't keep alive its wrapper if the refcount drops to 1. This makes XPCWN less leaky. If we can fix the leaks some other way, then we might be able to make XPCWN into a normal SCRIPT_HOLDER class and remove the weird XPConnect tracing machinery.
Summary: Simplify XPCWrappedNative cycle collector traversal → Make XPCWrappedNative always keep alive its wrapper
(Reporter)

Updated

3 years ago
Summary: Make XPCWrappedNative always keep alive its wrapper → Make XPCWrappedNative always keep its JS reflector object alive
(Reporter)

Comment 7

3 years ago
I have a patch that does the simple thing, but it hits odd JS assertions in some tests, like in here:
  ./mach mochitest browser/devtools/webconsole/test/
I think there's some issue with when unlink is happening, and how much it is cleaning up.
(Reporter)

Comment 8

3 years ago
Created attachment 8635525 [details] [diff] [review]
Make XPCWrappedNative always keep alive its reflector. WIP
Attachment #822807 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Assignee: continuation → nobody
You need to log in before you can comment on or make changes to this bug.