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

VERIFIED FIXED in mozilla7

Status

()

Core
Disability Access APIs
--
trivial
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla7
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 542011 [details] [diff] [review]
Patch v1

This case happens to be correct, but in general, calling get() on an already_AddRefed pointer leaks.
Flags: in-testsuite-
Attachment #542011 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #542011 - Flags: review? → review?(surkov.alexander)
nsCOMPtr?

Comment 2

6 years ago
(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?
(Assignee)

Comment 3

6 years ago
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.

Comment 4

6 years ago
(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

Updated

6 years ago
Attachment #542011 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 5

6 years ago
Thanks.

http://hg.mozilla.org/mozilla-central/rev/24e4277f45c5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7

Comment 6

6 years ago
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!

Comment 7

6 years ago
(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

Comment 8

6 years ago
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.