1495272 - Make sure calls to RecordReplayRegisterDeferredFinalize and DeferredFinalize line up

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

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.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Don't register deferred finalizer in BindingJSObjectCreator until initialization succeeds.

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.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Use DeferredFinalize in SweepTearOffs when recording/replaying.

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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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().

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/00c56e070d38
https://hg.mozilla.org/mozilla-central/rev/de02c78c82b7