Closed
Bug 1074377
Opened 9 years ago
Closed 9 years ago
Telephony::EnumerateCallState ignores already_AddRefed return values.
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 verified, b2g-v2.2 verified)
People
(Reporter: khuey, Assigned: aknow)
References
Details
(Keywords: assertion, regression)
Attachments
(1 file, 1 obsolete file)
1.21 KB,
patch
|
aknow
:
review+
khuey
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
At [0] we call Telephony::CreateCall and ignore the return value. But the return value is an already_AddRefed, which asserts[2] in its destructor that it was used. So in a debug build hitting this code path will crash reliably. In an opt build it will (presumably leak). [0] http://hg.mozilla.org/mozilla-central/annotate/66ab05e2a667/dom/telephony/Telephony.cpp#l603 [1] http://hg.mozilla.org/mozilla-central/annotate/66ab05e2a667/dom/telephony/Telephony.h#l186 [2] http://hg.mozilla.org/mozilla-central/annotate/ce9a0b34225e/xpcom/glue/nsCOMPtr.h#l176
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → szchen
Assignee | ||
Comment 1•9 years ago
|
||
Oh my bad. Kyle, May I have your review? Thanks.
Attachment #8497285 -
Flags: review?(khuey)
Reporter | ||
Updated•9 years ago
|
Attachment #8497285 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8497285 -
Attachment is obsolete: true
Attachment #8499376 -
Flags: review+
Assignee | ||
Comment 3•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=daa468bbbcf6
Keywords: checkin-needed
Comment 4•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/38444b94fd9e
Keywords: checkin-needed
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/38444b94fd9e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
Comment 6•9 years ago
|
||
Please request aurora approval on this patch when you get a chance :)
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → fixed
status-firefox33:
--- → wontfix
status-firefox34:
--- → affected
status-firefox35:
--- → fixed
Flags: needinfo?(szchen)
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8499376 [details] [diff] [review] [final] Should hold the already_AddRefed return value Approval Request Comment [Feature/regressing bug #]: 1027513 [User impact if declined]: crash in debug build or memory leak in opt build [Describe test coverage new/current, TBPL]: none [Risks and why]: very low risk [String/UUID change made/needed]: none
Attachment #8499376 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(szchen)
Reporter | ||
Comment 8•9 years ago
|
||
Comment on attachment 8499376 [details] [diff] [review] [final] Should hold the already_AddRefed return value a=me
Attachment #8499376 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 10•9 years ago
|
||
Eric or Szu-Yu, how can this bug be properly tested and verified against 2.1?
Reporter | ||
Comment 11•9 years ago
|
||
You can test this by running a debug build and rejecting an incoming call. The device should crash shortly after.
Reporter | ||
Comment 12•9 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11) > You can test this by running a debug build and rejecting an incoming call. > The device should crash shortly after. And FWIW, I have verified that on master.
Comment 13•9 years ago
|
||
Hi Kyle, the debug build is the 'VARIANT=userdebug' set in .userconfig? I tried to run it but it does no crash(v2.1). (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11) > > You can test this by running a debug build and rejecting an incoming call. > > The device should crash shortly after. > > And FWIW, I have verified that on master.
Flags: needinfo?(echang)
Reporter | ||
Comment 14•9 years ago
|
||
You need a debug Gecko, not just VARIANT=userdebug.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(szchen)
Comment 15•9 years ago
|
||
Verified okay after patch (export B2G_DEBUG=1) Reproducible before patch.
Status: RESOLVED → VERIFIED
Comment 16•9 years ago
|
||
refer to comment 10, 12 and 15, mark v2.1 and v2.2 status as verified
You need to log in
before you can comment on or make changes to this bug.
Description
•