961551 - Clean up XPCWrappedJS::GetNewOrUsed some more

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Comment 1

[Andrew McCreight [:mccr8]]

This reduces the length of nsXPCWrappedJS::GetNewOrUsed by 28 lines, and makes the body just barely fit all on my screen at once.

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

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Add root wrappers to the map in XPCWrappedJS ctor.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Move addref out of XPCWrappedJS ctor.

This paves the way for smart pointerization.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Hold a strong ref to the root that is the result of Find in XPCWJS::GNOU.

This is trickier than it looks, because this change causes additional
refcount traffic on |root| in the case where FindOrFindInherited
succeeds. This is potentially bad because if a WJS has a refcount of 1,
and no weak references to it, then if it goes to a refcount of 2 then
back to 1 it will be deleted.  A WJS can get in this state if it
is a root with a refcount of 2, and has a weak reference, then
its refcount drops to 1, then later the weak reference goes away.

However, in this case, there can be no other WJS in the chain,
or the refcount would be greater than 1, so FindOrFindInherited
must end up returning |root|, so |wrapper == root|, and the
assignment to |wrapper| will make root go to a refcount of 3,
so the release of |root| on exit will only cause the refcount to
go to 2, so |root| won't be deleted.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Remove release_root from XPCWJS::GetNewOrUsed.

GetNewOrUsed now always releases |root| when |root| is non-null, so
release_root can be eliminated.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Smart pointerize root.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 6 - Smart pointerize wrapper.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 7 - Smart pointerize rootClasp.

Also, move the declaration of |clasp| down to where it is used.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 8 - Use the code at the end of XPCWJS::GetNewOrUsed to build a new root wrapper.

The code at the end of nsXPCWrappedJS::GetNewOrUsed will now correctly
construct a root wrapper when |root == nullptr|, so we can consolidate
the two places that construct |wrapper|.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 9 - Split scope of |wrapper|, plus other cleanup.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8362680 [details] [diff] [review]
part 1 - Add root wrappers to the map in XPCWrappedJS ctor.

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

::: js/xpconnect/src/XPCWrappedJS.cpp
@@ +411,5 @@
>      NS_ADDREF_THIS();
>  
> +    if (IsRootWrapper()) {
> +        nsXPConnect::GetRuntimeInstance()->GetWrappedJSMap()->Add(cx, this);
> +     } else {

Indentation

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8362687 [details] [diff] [review]
part 8 - Use the code at the end of XPCWJS::GetNewOrUsed to build a new root wrapper.

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

Clever.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8362688 [details] [diff] [review]
part 9 - Split scope of |wrapper|, plus other cleanup.

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

::: js/xpconnect/src/XPCWrappedJS.cpp
@@ +368,1 @@
>      wrapper = new nsXPCWrappedJS(cx, jsObj, clasp, root);

How about combining these two lines into a declare + assign?

Comment 14

[Bobby Holley (:bholley)]

Also - I'm not sure if this is a new thing, but I'm kind of uncomfortable with writing |r=bholley| in the commit message of unreviewed patches that get published on bugzilla, since these things could float around and accidentally get committed. I always add the r= afterwards (using an interactive rebase) as a sanity-check to make sure that all patches I'm committing were r+ed. And I think doing this is important. Thoughts?

Comment 15

[Andrew McCreight [:mccr8]]

> How about combining these two lines into a declare + assign?

It made the line a tiny bit wide, but I can do that.

Comment 16

[Andrew McCreight [:mccr8]]

> Thoughts?

I'm not sure exactly what your concern is. Are you worried that somebody will accidentally commit a patch that is floating around in their git repo or that somebody will commit a patch that somebody else uploaded to bugzilla?  I assume the former.  Anyways, I'm pretty methodical with my patch landing, so I'm not really concerned about that personally (including looking at each bug and making sure I've addressed all review comments).  Though maybe I could write out a more explicit check list to follow.

Comment 17

[Andrew McCreight [:mccr8]]

addressed comments

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d4bc1ae518c7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e6c31c9c36bb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b402626107f8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/802f26863b48
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ff626945d531
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0a216f68f6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc6b8213fd2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad4a8494e6c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a8129fa38c35

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d4bc1ae518c7
https://hg.mozilla.org/mozilla-central/rev/e6c31c9c36bb
https://hg.mozilla.org/mozilla-central/rev/b402626107f8
https://hg.mozilla.org/mozilla-central/rev/802f26863b48
https://hg.mozilla.org/mozilla-central/rev/ff626945d531
https://hg.mozilla.org/mozilla-central/rev/6a0a216f68f6
https://hg.mozilla.org/mozilla-central/rev/0dc6b8213fd2
https://hg.mozilla.org/mozilla-central/rev/1ad4a8494e6c
https://hg.mozilla.org/mozilla-central/rev/a8129fa38c35