944492 - Make XPCWrappedJS use the purple buffer and CAN_SKIP

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

See bug 942528.

Comment 1

[Andrew McCreight [:mccr8]]

Turns out, the existing traverse method for XPCWrappedJS is only correct when the wrapper doesn't have any weak references.

https://tbpl.mozilla.org/?tree=Try&rev=77bb1d9d50b6

Comment 2

[Andrew McCreight [:mccr8]]

I should also turn CanSkipWrappedJS into a normal skippable method, and maybe bake the logic about weak references into the skippable method (eg if we're a root wrapper and we have a weak reference, we're definitely alive).

Comment 3

[Andrew McCreight [:mccr8]]

CanSkipWrappedJS seems a little overly complicated for what it has to do.  It has this isRootWrappedJS stuff, but as far as I can see, it is only being called on things in the mWrappedJSRoots list, which I'd think would all have isRootWrappedJS. Olli, am I missing something?  Of course, now I need that extra logic.

Comment 4

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

I don't think you're missing anything. It is just safe to call that method even without non-root stuff. The fact that the object is GetRootWrapper() doesn't in general mean that it is in  mWrappedJSRoots

Comment 5

[Andrew McCreight [:mccr8]]

Previous try run looked good.

Turned into a normal skippable class, with the CanSkip stuff rewritten a bit:
  https://tbpl.mozilla.org/?tree=Try&rev=784769303c99

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Make XPCWrappedJS use the purple buffer. r=smaug

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make XPCWrappedJS a proper skippable class. r=smaug

Comment 8

[Andrew McCreight [:mccr8]]

I broke up the giant conditional in CanSkipWrappedJS() a bit to make it easier to understand.  I also changed around what it does a little so that should be carefully looked at.

Comment 9

[Andrew McCreight [:mccr8]]

"Normal" is too strong a word to use.

Comment 10

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

Comment on attachment 8341950 [details] [diff] [review]
part 1 - Make XPCWrappedJS use the purple buffer. r=smaug

a bit scary stuff, given the complexity of this stuff.
Could we land this early in the next cycle?

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #10)
> Could we land this early in the next cycle?

Sure.

Comment 12

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

Comment on attachment 8341951 [details] [diff] [review]
part 2 - Make XPCWrappedJS a proper skippable class. r=smaug

>+nsXPCWrappedJS::CanSkip()
>+{
>+    if (!nsCCUncollectableMarker::sGeneration)
>+        return false;
>+
>+    // If this wrapper holds a gray object, need to trace it.
>+    JSObject *obj = GetJSObjectPreserveColor();
>+    if (obj && xpc_IsGrayGCThing(obj))
>+        return false;
>+
>+    if (IsSubjectToFinalization())
>+        return false;
>+
>+    // The extra ref from the weak reference keeps the wrapper alive.
>+    if (HasWeakReferences())
>+        return true;
I don't understand this. If we have a weak ref and a strong ref, why can we skip?

Comment 13

[Andrew McCreight [:mccr8]]

> I don't understand this. If we have a weak ref and a strong ref, why can we skip?

There's an extra AddRef in the XPCWJS constructor, and XPCWJS::Release drops the extra ref if there's no weak reference.  Therefore, if anything has a "weak reference" to an XPCWJS (in the SupportsWeakRef sense), the XPCWJS won't be destroyed, so it is definitely alive and we don't need to add it to the CC graph (aside from the usual tracing JS stuff, which the earlier conditions are supposed to handle).

Comment 14

[Andrew McCreight [:mccr8]]

Though I'm not entirely what ends up destroying the XPCWJS when it does have a weak reference, and a refcount of 1.  I should figure that out.  That could be a problem just with part 1.

Comment 15

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

Comment on attachment 8341950 [details] [diff] [review]
part 1 - Make XPCWrappedJS use the purple buffer. r=smaug

Yeah, I think this isn't good either.
Having a weakref pointing to XPCWJS shouldn't affect to whether it is deleted or not.

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Make XPCWrappedJS use the purple buffer.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make XPCWrappedJS a proper skippable class.

Comment 18

[Andrew McCreight [:mccr8]]

I don't know if you want to look at part 1 or not, Bobby.  I consolidated and clarified the documentation of XPCWrappedJS lifetime.  These patches should not change the behavior.

green L64 debug try run: https://tbpl.mozilla.org/?tree=Try&rev=bbbd2b63b1ef

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8344215 [details] [diff] [review]
part 1 - Make XPCWrappedJS use the purple buffer.

Review of attachment 8344215 [details] [diff] [review]:
-----------------------------------------------------------------

This comment was very informative. Thank you for writing it!

I don't know enough about this code to review it without spending several hours figuring it out myself. I'll let smaug review.

Comment 20

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

Comment on attachment 8344215 [details] [diff] [review]
part 1 - Make XPCWrappedJS use the purple buffer.


>+// When a wrapper is subject to finalization, the wrapper has a refcount of 1. It is
>+// now owned exclusively by its JS object. Either it will have GetNewOrUsed called on it,
>+// which will bring its refcount up to 2 and change the wrapper back to the
>+// rooting state, or it will stay alive until the JS object dies. If the JS object
>+// dies, then when XPCJSRuntime::FinalizeCallback calls FindDyingJSObjects
>+// it will find the wrapper and call Release() in it, destroying the wrapper.
>+// Otherwise, the wrapper will stay alive, even if it no longer has a weak reference
>+// to it.
Also getting non-weakRef from the weakref ends up increasing refcnt from 1 to 2.
No need for GetNewOrUsed

>+NS_IMETHODIMP_(void)
>+nsXPCWrappedJS::DeleteCycleCollectable(void)
>+{
>+    delete(this);

I think without () is more common 
delete this;

thanks for the explanation.

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Make XPCWrappedJS use the purple buffer. r=smaug

Addressed review comments.  Carrying forward smaug's r+.

Comment 22

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/70b2d51d4f61
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3b50a75aa431

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70b2d51d4f61
https://hg.mozilla.org/mozilla-central/rev/3b50a75aa431