1287624 - Use RefPtr::forget() instead of RefPtr::swap() in more places

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

swap(T*&) is used much less than forget(T**). There are a number of places where the argument to swap is explicitly nulled out first, so we can safely convert them to forget to make them less weird.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Use RefPtr::forget() instead of ::swap() in dom/.

These uses all null the return value first, so there should be no change in behavior.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e87c342a58d

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Replace swap with forget in a few places.

The return argument was initialized to null in all of these, so it should not change the behavior.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8772563 [details] [diff] [review]
part 1 - Use RefPtr::forget() instead of ::swap() in dom/.

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

I wonder what a try run asserting a null previously-stored value in forget() would look like.

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> I wonder what a try run asserting a null previously-stored value in forget()
> would look like.

Yeah, I was thinking about filing a bug on that. In practice, that should always show up as a leak, but an assert would be easier to debug.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8772564 [details] [diff] [review]
part 2 - Replace swap with forget in a few places.

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

::: netwerk/base/nsInputStreamPump.cpp
@@ -65,5 @@
>      if (pump) {
>          rv = pump->Init(stream, streamPos, streamLen,
>                          segsize, segcount, closeWhenDone);
>          if (NS_SUCCEEDED(rv)) {
> -            *result = nullptr;

How are we guaranteed that *result is nullptr after removing this?

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] from comment #5)
> How are we guaranteed that *result is nullptr after removing this?

Sorry, what do you mean? In both cases, *result ends up with the value of pump, and pump ends up as null.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> (In reply to Nathan Froyd [:froydnj] from comment #5)
> > How are we guaranteed that *result is nullptr after removing this?
> 
> Sorry, what do you mean? In both cases, *result ends up with the value of
> pump, and pump ends up as null.

Your comment 1 said that the return values are always nulled first, so we can be confident replacing .swap() with .forget() is safe.  I didn't see that guarantee in this case.

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> Your comment 1 said that the return values are always nulled first, so we
> can be confident replacing .swap() with .forget() is safe.  I didn't see
> that guarantee in this case.

I just meant they are always nulled first before my patch, which is the composition of two semantics-preserving transformations:
1. I change the swap() to a forget(), and this does not change behavior because the return value is nulled first.
2. I can then remove the nulling of the return value because it is postdominated by the forget (which writes a new value and does not do anything with the old value), so it is a dead store.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d2485670bb2
part 1 - Use RefPtr::forget() instead of ::swap() in dom/. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/59409d62be61
part 2 - Replace swap with forget in a few places. r=froydnj

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d2485670bb2
https://hg.mozilla.org/mozilla-central/rev/59409d62be61