Closed Bug 667277 Opened 10 years ago Closed 10 years ago

Don't use already_AddRefed::get in nsAccessible::GetURI

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
mozilla7

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(1 file)

Attached patch Patch v1Splinter Review
This case happens to be correct, but in general, calling get() on an already_AddRefed pointer leaks.
Flags: in-testsuite-
Attachment #542011 - Flags: review?
Attachment #542011 - Flags: review? → review?(surkov.alexander)
(In reply to comment #0)
> Created attachment 542011 [details] [diff] [review] [review]
> Patch v1
> 
> This case happens to be correct, but in general, calling get() on an
> already_AddRefed pointer leaks.

Are you going to remove get() from already_AddRefed? If not then what's a purpose of this fix?
That's the goal, yes. See bug 663425. Even if we can't remove it entirely, it would be good to remove as much cases as possible, so people don't get used to using it.
(In reply to comment #3)
> That's the goal, yes. See bug 663425.

ok, I see.

> Even if we can't remove it entirely,
> it would be good to remove as much cases as possible, so people don't get
> used to using it.

no cases don't stop people from using it, everybody who knows xpcom and don't know about this bug will use it because it's even dangerous but elegant.
Blocks: 663425
Attachment #542011 - Flags: review?(surkov.alexander) → review+
Thanks.

http://hg.mozilla.org/mozilla-central/rev/24e4277f45c5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Is there any way I can verify this on the QA side? I visually inspected the code changes in the repo:
http://hg.mozilla.org/mozilla-central/file/922f27baed98/accessible/src/base/nsAccessible.cpp

Is that enough to mark this as Verified?

Thanks!
(In reply to Simona B [QA] from comment #6)

> Is there any way I can verify this on the QA side? I visually inspected the
> code changes in the repo:
> http://hg.mozilla.org/mozilla-central/file/922f27baed98/accessible/src/base/
> nsAccessible.cpp
> 
> Is that enough to mark this as Verified?

yes
Based on Comment 7 - marking this as Verified Fixed.

Thanks alexander!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.