Make XPCWrappedJS use the purple buffer and CAN_SKIP

RESOLVED FIXED in mozilla29

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
See bug 942528.
(Assignee)

Comment 1

4 years ago
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
(Assignee)

Comment 2

4 years ago
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).
(Assignee)

Comment 3

4 years ago
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.
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
(Assignee)

Comment 5

4 years ago
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
(Assignee)

Comment 6

4 years ago
Created attachment 8341950 [details] [diff] [review]
part 1 - Make XPCWrappedJS use the purple buffer. r=smaug
Attachment #8341950 - Flags: review?(bugs)
(Assignee)

Comment 7

4 years ago
Created attachment 8341951 [details] [diff] [review]
part 2 - Make XPCWrappedJS a proper skippable class. r=smaug
Attachment #8341951 - Flags: review?(bugs)
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Comment 9

4 years ago
"Normal" is too strong a word to use.
Summary: Make XPCWrappedJS into a more normal cycle collected class → Make XPCWrappedJS use the purple buffer and CAN_SKIP
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?
Attachment #8341950 - Flags: review?(bugs) → review+
(Assignee)

Comment 11

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

Sure.
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?
Attachment #8341951 - Flags: review?(bugs) → review-
(Assignee)

Comment 13

4 years ago
> 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).
(Assignee)

Comment 14

4 years ago
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 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.
Attachment #8341950 - Flags: review+ → review-
(Assignee)

Updated

4 years ago
Depends on: 947448
(Assignee)

Comment 16

4 years ago
Created attachment 8344215 [details] [diff] [review]
part 1 - Make XPCWrappedJS use the purple buffer.
Attachment #8344215 - Flags: review?(bugs)
Attachment #8344215 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 17

4 years ago
Created attachment 8344216 [details] [diff] [review]
part 2 - Make XPCWrappedJS a proper skippable class.
Attachment #8344216 - Flags: review?(bugs)
(Assignee)

Comment 18

4 years ago
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
(Assignee)

Updated

4 years ago
Attachment #8341950 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8341951 - Attachment is obsolete: true
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.
Attachment #8344215 - Flags: review?(bobbyholley+bmo)
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.
Attachment #8344215 - Flags: review?(bugs) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 21

4 years ago
Created attachment 8346856 [details] [diff] [review]
part 1 - Make XPCWrappedJS use the purple buffer. r=smaug

Addressed review comments.  Carrying forward smaug's r+.
(Assignee)

Updated

4 years ago
Attachment #8344215 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8346856 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/70b2d51d4f61
https://hg.mozilla.org/mozilla-central/rev/3b50a75aa431
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.