1479644 - Don't clear wrapper cache during finalization when replaying

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

This patch fixes a kind of tricky issue that arises when finalizing wrappers while replaying.  The ClearWrapper method finds the wrapper cache which might contain the wrapper by doing a QI, but when replaying the reference held on the object being QI'ed may have already been released (we released it at the same time as we did while recording, and the wrapper may be finalized at a later point while replaying) and it may have already been freed.

The fix for this is simply to not do any clearing in this method when replaying.  This call to ClearWrapper is only effectful if the particular wrapper is still in the wrapper cache, and when replaying wrapper caches retain strong references on their contents via the recordreplay SetWeakPointerJSRoot method, making sure the wrapper is not collected prematurely, so there is no way the wrapper cache both (a) still exists, and (b) still references the wrapper being finalized.

Comment 1

[Boris Zbarsky [:bzbarsky]]

What about the "ClearWrapper(T* p, nsWrapperCache* cache, JSObject* obj)" overload?  In that one, "p" and "cache" reference the same object, and if it might be dead already then we don't want to cache->ClearWrapper(), right?

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

Oops, yes, the overload needs to check this as well (somehow I thought this was a helper method and not an overloaded version of the function below it).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8996354 [details] [diff] [review]
patch

So just to make sure I understand...

When replaying, wrappercached things all keep their wrappers alive (is this  true?).  So if we're getting finalized at all, that means "p" is dead (even if it hasn't actually been deleted yet, it's at least unreachable)?

Basically, I'd like the comments to make it clear why we're not ending up with a dangling pointer if we skip clearing.

r=me with that.

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> Comment on attachment 8996354 [details] [diff] [review]
> patch
> 
> So just to make sure I understand...
> 
> When replaying, wrappercached things all keep their wrappers alive (is this 
> true?).  So if we're getting finalized at all, that means "p" is dead (even
> if it hasn't actually been deleted yet, it's at least unreachable)?
> 
> Basically, I'd like the comments to make it clear why we're not ending up
> with a dangling pointer if we skip clearing.
> 
> r=me with that.

Yes, nsWrapperCaches keep references on their wrappers alive.  If the wrapper is getting finalized, it means that the wrapper cache is not pointing to this wrapper anymore.  It could be that either the wrapper cache has been destroyed, or it is pointing to another wrapper (or null).  The reference to "p" may or may not have been released yet; when replaying, we release the references at the same point that we did while recording (so that side effects in p's destructor happen execute at consistent points).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a30f347973
Don't clear wrapper cache during finalization when replaying, r=bz.

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d9a30f347973