Closed Bug 1074377 Opened 10 years ago Closed 10 years ago

Telephony::EnumerateCallState ignores already_AddRefed return values.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S6 (10oct)
blocking-b2g 2.1+
Tracking Status
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)

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: nobody → szchen
Oh my bad.

Kyle,
May I have your review? Thanks.
Attachment #8497285 - Flags: review?(khuey)
Attachment #8497285 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/38444b94fd9e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
Please request aurora approval on this patch when you get a chance :)
Flags: needinfo?(szchen)
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)
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+
Eric or Szu-Yu, how can this bug be properly tested and verified against 2.1?
Flags: needinfo?(szchen)
Flags: needinfo?(echang)
Keywords: verifyme
You can test this by running a debug build and rejecting an incoming call.  The device should crash shortly after.
(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.
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)
You need a debug Gecko, not just VARIANT=userdebug.
Flags: needinfo?(szchen)
Verified okay after patch (export B2G_DEBUG=1)
Reproducible before patch.
Status: RESOLVED → VERIFIED
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.