Last Comment Bug 667277 - Don't use already_AddRefed::get in nsAccessible::GetURI
: Don't use already_AddRefed::get in nsAccessible::GetURI
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla7
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on:
Blocks: 663425
  Show dependency treegraph
 
Reported: 2011-06-26 04:00 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-08-31 07:34 PDT (History)
4 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (707 bytes, patch)
2011-06-26 04:00 PDT, :Ms2ger (⌚ UTC+1/+2)
surkov.alexander: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-06-26 04:00:43 PDT
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.
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-26 08:34:06 PDT
nsCOMPtr?
Comment 2 alexander :surkov 2011-06-27 21:35:02 PDT
(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?
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-06-28 09:17:14 PDT
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 alexander :surkov 2011-06-28 19:09:04 PDT
(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.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-07-03 08:08:41 PDT
Thanks.

http://hg.mozilla.org/mozilla-central/rev/24e4277f45c5
Comment 6 Simona B [:simonab] 2011-08-31 07:28:23 PDT
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 alexander :surkov 2011-08-31 07:30:57 PDT
(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 Simona B [:simonab] 2011-08-31 07:34:27 PDT
Based on Comment 7 - marking this as Verified Fixed.

Thanks alexander!

Note You need to log in before you can comment on or make changes to this bug.