Closed Bug 1074377 Opened 9 years ago Closed 9 years ago

Telephony::EnumerateCallState ignores already_AddRefed return values.

Categories

(Firefox OS Graveyard :: RIL, defect)

Product:
Firefox OS Graveyard
Component:
RIL
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

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

[final] Should hold the already_AddRefed return value
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: 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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
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)
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
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.