Closed
Bug 1287624
Opened 5 years ago
Closed 5 years ago
Use RefPtr::forget() instead of RefPtr::swap() in more places
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
Details
Attachments
(2 files)
6.57 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
3.15 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•5 years ago
|
||
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)
Assignee | ||
Comment 2•5 years ago
|
||
The return argument was initialized to null in all of these, so it should not change the behavior.
Attachment #8772564 -
Flags: review?(nfroyd)
![]() |
||
Comment 3•5 years ago
|
||
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+
Assignee | ||
Comment 4•5 years ago
|
||
(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•5 years ago
|
||
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+
Assignee | ||
Comment 6•5 years ago
|
||
(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•5 years ago
|
||
(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.
Assignee | ||
Comment 8•5 years ago
|
||
(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
Comment 10•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7d2485670bb2 https://hg.mozilla.org/mozilla-central/rev/59409d62be61
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•