Closed Bug 1573938 Opened 2 months ago Closed 2 months ago

Never collect wrapper JSObjects when recording/replaying

Categories

(Core :: Web Replay, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

Details

Attachments

(1 file)

There is some rather complicated logic used when recording/replaying to allow the garbage collection of JSObjects that wrap nsISupports things to interact with the recording in a deterministic way. These objects hold references to the nsISupports objects which are released shortly after the GC via the existing DeferredFinalize mechanism; when recording/replaying we make sure that references on an object are released at the same point when replaying as they were while recording, even if the associated JSObject hasn't been finalized yet. When replaying, this can lead to an object pointing to an nsISupports object whose reference has been released and may be garbage. The argument for why this is correct is that since the object was collected at this point while recording, it won't be used again while replaying so the dangling pointer can be left in place. This argument is wrong, though, as the dangling pointer can be accessed during finalization. In particular, XPCWrappedNative::FlatJSObjectFinalized needs to get the wrapper cache associated with its nsISupports, in order to clear the cache, and when replaying this might be operating on a dangling pointer.

The best way to fix this is I think to remove all this logic for ensuring that finalizers are called in consistent places, and just ensure that the associated objects are never collected by the JS GC. When recording/replaying we aren't able to guarantee that dead things will be collected, because the cycle collector is disabled, and given the short lived nature of a recorded tab it shouldn't matter all that much that we aren't able to collect these wrapper objects.

A related issue is that the wrapper caches themselves have some special record/replay related logic to make sure they behave deterministically wrt the JS GC. This is because if a cache is emptied by an object finalizer then refilling it can require interacting with the recording at different places between recording and replaying. Because the wrapper cache objects won't be collected by the GC, this specialized logic shouldn't be needed anymore.

The patch for this bug removes all of the record/replay logic used by deferred finalization and wrapper caches, replacing them with an API that holds onto the associated JSObject forever. This is called for both wrapper caches and objects that have nsISupports references; in many cases an object will satisfy both requirements so this is sort of a belt-and-suspenders fix, but from my understanding of the code there are cases where wrapper caches are used with objects that don't have nsISupports references, and where objects with nsISupports references are not associated with a wrapper cache. With the current implementation this will end up creating duplicate PersistentRootedObjects, though this will be easy to fix in followup if it turns out to be a performance problem.

Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de8dd803924f
Never collect wrapper JSObjects when recording/replaying, r=mccr8.
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.