Closed Bug 927601 Opened 7 years ago Closed 2 years ago

Make XPCWrappedNative always keep its JS reflector object alive

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED INACTIVE

People

(Reporter: mccr8, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Summary: Simplify XPCWrappedNative traversal → Simplify XPCWrappedNative cycle collector traversal
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.
Depends on: 928655
Depends on: 931283
Depends on: 931285
Attached patch WIP (obsolete) — Splinter Review
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
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.
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: IncrementalCC
No longer blocks: 911246
Blocks: 956068
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
Summary: Make XPCWrappedNative always keep alive its wrapper → Make XPCWrappedNative always keep its JS reflector object alive
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.
Assignee: continuation → nobody
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.