Telephony::EnumerateCallState ignores already_AddRefed return values.

VERIFIED FIXED in Firefox 34

Status

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: aknow)

Tracking

({assertion, regression})

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

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)
https://hg.mozilla.org/mozilla-central/rev/38444b94fd9e
Status: NEW → RESOLVED
Closed: 5 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.