Closed Bug 1287624 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

Details

Attachments

(2 files)

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.
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
Attachment #8772563 - Flags: review?(nfroyd)
The return argument was initialized to null in all of these, so it should not change the behavior.
Attachment #8772564 - Flags: review?(nfroyd)
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.
Attachment #8772563 - Flags: review?(nfroyd) → review+
(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 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?
Attachment #8772564 - Flags: review?(nfroyd) → review+
(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.
(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.
(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.
Pushed by amccreight@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/7d2485670bb2
https://hg.mozilla.org/mozilla-central/rev/59409d62be61
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.