Closed Bug 1495272 Opened 1 year ago Closed 1 year ago

Make sure calls to RecordReplayRegisterDeferredFinalize and DeferredFinalize line up

Categories

(Core :: Web Replay, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: bhackett1024, Assigned: bhackett1024)

Details

Attachments

(2 files)

There have been some crashes using Web Replay (sample below) where the IDs associated with RegisterTrigger calls on newly created objects do not match up between recording and replaying.  The only way this should be able to happen is if the pointer being registered was previously registered with a different ID without being unregistered, and the only way that should be able to happen is if a RecordReplayRegisterDeferredFinalize call was made on the old pointer without a corresponding DeferredFinalize call (which does the unregistering).

https://crash-stats.mozilla.com/report/index/360d77b1-c277-4962-84cc-7a9fb0180904

I looked through the calls to RecordReplayRegisterDeferredFinalize to see if there are ways this could happen, and found a couple potential bugs, which the attached patch fixes.
Currently, when BindingJSObjectCreator creates a JS object it registers a deferred finalizer for it immediately, at the same point where the JS object's reserved slot is filled in.  If initialization fails (can this happen for reasons other than OOM?) then the destructor clears this reserved slot and DeferredFinalize will never be called.  The attached patch registers the deferred finalizer at the point where initialization succeeds and the JS object has acquired the reference on the native pointer.
Attachment #9013132 - Flags: review?(bzbarsky)
PCWrappedNative::InitTearOff registers a deferred finalizer for the tearoff it initializes, at the same point where the tearoff's native pointer is set.  DeferredFinalize should be used when this native pointer is cleared, but XPCWrappedNative::SweepTearOffs is able to clear this pointer directly.  The attached patch changes SweepTearOffs to use DeferredFinalize instead when recording/replaying.
Attachment #9013133 - Flags: review?(bugs)
Comment on attachment 9013133 [details] [diff] [review]
Part 2 - Use DeferredFinalize in SweepTearOffs when recording/replaying.

Ok, this is similar to https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/js/xpconnect/src/XPCWrappedNative.cpp#782-789
Attachment #9013133 - Flags: review?(bugs) → review+
Comment on attachment 9013132 [details] [diff] [review]
Part 1 - Don't register deferred finalizer in BindingJSObjectCreator until initialization succeeds.

>+    forget(T** aResult)
>     {
>+      *aResult = mNative;

Should set mNative null as well, for actual forget() semantics.

Please add documentation for OwnedNative:mNative saying that the JS object owns the native but we need to keep track of the pointer because we need it in InitializationSucceeded().
Attachment #9013132 - Flags: review?(bzbarsky) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00c56e070d38
Part 1 - Don't register deferred finalizer in BindingJSObjectCreator until initialization succeeds, r=bz.
https://hg.mozilla.org/integration/mozilla-inbound/rev/de02c78c82b7
Part 2 - Use DeferredFinalize in SweepTearOffs when recording/replaying, r=smaug.
https://hg.mozilla.org/mozilla-central/rev/00c56e070d38
https://hg.mozilla.org/mozilla-central/rev/de02c78c82b7
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.