Closed Bug 1048581 Opened 10 years ago Closed 10 years ago

[MobileConnection] Crash in WrapNewBindingObjectHelper during stability test

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

(blocking-b2g:2.0+, firefox32 wontfix, firefox33 wontfix, firefox34 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S2 (15aug)
blocking-b2g 2.0+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: ggrisco, Assigned: edgar)

References

Details

(Keywords: crash, Whiteboard: [caf-crash 321][caf priority: p3][b2g-crash][CR 704025])

Crash Data

Attachments

(5 files, 1 obsolete file)

[Blocking Requested - why for this release]:

We have seen this crash twice so far.  

Crashed with signature:

[@ mozilla::dom::WrapNewBindingObjectHelper, true>::Wrap(JSContext*, nsRefPtrmozilla::dom::MobileConnectionInfo const&, JS::MutableHandleJS::Value) | mozilla::dom::MozMobileConnectionBinding::get_voice | mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) | js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) ]
Attached file decoded minidump
Whiteboard: [b2g-crash][CR 704025] → [caf priority: p1][b2g-crash][CR 704025]
MozMobileConnection.webidl has:

   readonly attribute MozMobileConnectionInfo voice;

But the actual implementation does:

215 MobileConnection::Voice() const
216 {
217   if (!mProvider) {
218     return nullptr;

If that codepath is hit, you will get the observed crash, because bindings trust the IDL's claim that the getter never returns null.

Either the IDL needs to say that null can be returned or the implementation needs to never return null.
Blocks: 898445
Component: JavaScript Engine → RIL
Flags: needinfo?(echen)
Product: Core → Firefox OS
(In reply to On vacation Aug 5-18.  Please do not request review. from comment #3)
> MozMobileConnection.webidl has:
> 
>    readonly attribute MozMobileConnectionInfo voice;
> 
> But the actual implementation does:
> 
> 215 MobileConnection::Voice() const
> 216 {
> 217   if (!mProvider) {
> 218     return nullptr;
> 
> If that codepath is hit, you will get the observed crash, because bindings
> trust the IDL's claim that the getter never returns null.
> 
> Either the IDL needs to say that null can be returned or the implementation
> needs to never return null.

Ah, yes, we should not return null in such case, throwing an exception seems better.
Assignee: nobody → echen
Flags: needinfo?(echen)
Summary: Crash in WrapNewBindingObjectHelper during stability test → [MobileConnection] Crash in WrapNewBindingObjectHelper during stability test
blocking-b2g: 2.0? → 2.0+
Whiteboard: [caf priority: p1][b2g-crash][CR 704025] → [caf-crash 321][caf priority: p1][b2g-crash][CR 704025]
(In reply to Edgar Chen [:edgar][:echen] from comment #4)
> (In reply to On vacation Aug 5-18.  Please do not request review. from
> comment #3)
> > MozMobileConnection.webidl has:
> > 
> >    readonly attribute MozMobileConnectionInfo voice;
> > 
> > But the actual implementation does:
> > 
> > 215 MobileConnection::Voice() const
> > 216 {
> > 217   if (!mProvider) {
> > 218     return nullptr;
> > 
> > If that codepath is hit, you will get the observed crash, because bindings
> > trust the IDL's claim that the getter never returns null.
> > 
> > Either the IDL needs to say that null can be returned or the implementation
> > needs to never return null.
> 
> Ah, yes, we should not return null in such case, throwing an exception seems
> better.

Hmm, after review current code again, I think we don't even need to check mProvider is valid or not for Voice() and Data() given that MobileConnection maintains these information in mVoice and mData. We could just return them directly.

But mVoice/mData will be set to null after MobileConnection::Shutdown is called and currently we have a potential problem on this (Please see bug 1047246).
Comment on attachment 8469031 [details] [diff] [review]
Part 1: Voice() and Data() won't need to check mProvider is valid or not, v1

MobileConnection keeps a ref to voice/data info, mVoice/mData, instead of getting them from provider service. So we don't need to check mProvider in Voice()/Data().

Hi Olli, could you help to review this?
Attachment #8469031 - Flags: review?(bugs)
Comment on attachment 8469059 [details] [diff] [review]
Part 2: MobileConnection shouldn't be shutdown if JS still keeps a ref to it, v1

MobileConnection::Shutdown is currently called by MobileConnectionArray::DropConnections() when unlink runs. But if JS still keeps a ref to MobileConnection object, it should not be shutdown (Please see bug 1047246). This is one possibility which cause this crash (returns a nullptr).

As the discussion in bug 1047246, the shutdown should be called in:
1). MobileConnection dtor.
2). MobileConnection unlink.
3). DisconnectFromOwner.

And also I rewrite the Shutdown() to make it's logic more correct.

Hi Olli, could you help to review this? Thank you.
Attachment #8469059 - Flags: review?(bugs)
Attachment #8469031 - Flags: review?(bugs) → review+
Comment on attachment 8469059 [details] [diff] [review]
Part 2: MobileConnection shouldn't be shutdown if JS still keeps a ref to it, v1

You don't need
+  mProvider = nullptr;
+  mVoice = nullptr;
+  mData = nullptr;
in destructor.
Attachment #8469059 - Flags: review?(bugs) → review+
Address review comment #10.

Thanks for the review, Olli.
Attachment #8469059 - Attachment is obsolete: true
Attachment #8469757 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/67c2719b7fc4
https://hg.mozilla.org/mozilla-central/rev/a55ad5e6580b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S2 (15aug)
This depends on bug 1030002, which isn't on b2g32. Per smaug, we can't backport this safely without it.
Even with bug 1030002 applied, part 2 will need additional rebasing for b2g32 as well.
Flags: needinfo?(echen)
Observed on: 

Device: msm8610
Gonk Version: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.058
Moz BuildID: 20140807000201
B2G Version: 2.0
Gecko Version: 32.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=8cc28fd31905a0ea2b2e15d13e80a0eab2feb1ba
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=f7bd772b1e42774708a4ede13b149a1706a59b25
Whiteboard: [caf-crash 321][caf priority: p1][b2g-crash][CR 704025] → [caf-crash 321][caf priority: p3][b2g-crash][CR 704025]
Observed on: 

Device: msm8226
Gonk Version: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.060
Moz BuildID: 20140810000201
B2G Version: 2.0
Gecko Version: 32.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=de28796a8956a48bb98ca67df6a33e0622d642d1
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=2b27becae85092d46bfadcd4fb5605e82e1e1093
Flags: in-moztrap?(bzumwalt)
No STR is present to create test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(bzumwalt)
Flags: in-moztrap-
Observed on: 

Device: msm8226
Gonk Version: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.066
Moz BuildID: 20140810160202
B2G Version: 2.0
Gecko Version: 32.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=de28796a8956a48bb98ca67df6a33e0622d642d1
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=2b27becae85092d46bfadcd4fb5605e82e1e1093
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: