[MobileConnection] Crash in WrapNewBindingObjectHelper during stability test

RESOLVED FIXED in Firefox 34, Firefox OS v2.0

Status

Firefox OS
RIL
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Greg Grisco, Assigned: edgar)

Tracking

(Depends on: 1 bug, {crash})

unspecified
2.1 S2 (15aug)
ARM
Gonk (Firefox OS)
crash
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

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

Details

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

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
[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) ]
Created attachment 8467380 [details]
EXTRA file attachment
Created attachment 8467381 [details]
decoded minidump

Updated

3 years ago
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
(Assignee)

Comment 4

3 years ago
(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)
(Assignee)

Updated

3 years ago
Summary: Crash in WrapNewBindingObjectHelper during stability test → [MobileConnection] Crash in WrapNewBindingObjectHelper during stability test
blocking-b2g: 2.0? → 2.0+

Updated

3 years ago
Whiteboard: [caf priority: p1][b2g-crash][CR 704025] → [caf-crash 321][caf priority: p1][b2g-crash][CR 704025]
(Assignee)

Comment 5

3 years ago
(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).
(Assignee)

Comment 6

3 years ago
Created attachment 8469031 [details] [diff] [review]
Part 1: Voice() and Data() won't need to check mProvider is valid or not, v1
(Assignee)

Comment 7

3 years ago
Created attachment 8469059 [details] [diff] [review]
Part 2: MobileConnection shouldn't be shutdown if JS still keeps a ref to it, v1
(Assignee)

Comment 8

3 years ago
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)
(Assignee)

Comment 9

3 years ago
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)

Updated

3 years ago
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+
(Assignee)

Comment 11

3 years ago
Created attachment 8469757 [details] [diff] [review]
Part 2: MobileConnection shouldn't be shutdown if JS still keeps a ref to it, v2, r=smaug

Address review comment #10.

Thanks for the review, Olli.
Attachment #8469059 - Attachment is obsolete: true
Attachment #8469757 - Flags: review+
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1047246
(Assignee)

Comment 13

3 years ago
Try result: https://tbpl.mozilla.org/?tree=Try&rev=5d58c9aa969f
(Assignee)

Comment 14

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/67c2719b7fc4
https://hg.mozilla.org/integration/b2g-inbound/rev/a55ad5e6580b
https://hg.mozilla.org/mozilla-central/rev/67c2719b7fc4
https://hg.mozilla.org/mozilla-central/rev/a55ad5e6580b
Status: NEW → RESOLVED
Last Resolved: 3 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.
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
status-firefox32: --- → wontfix
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
Even with bug 1030002 applied, part 2 will need additional rebasing for b2g32 as well.
Flags: needinfo?(echen)
Keywords: branch-patch-needed
(Assignee)

Comment 18

3 years ago
Created attachment 8470446 [details] [diff] [review]
[b2g32_v2.0] Part 2: MobileConnection shouldn't be shutdown if JS still keeps a ref to it, v2, r=smaug, a=2.0+

Patch for b2g32_v2.0
Flags: needinfo?(echen)
(Assignee)

Updated

3 years ago
Keywords: branch-patch-needed
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
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/fed262164fd1
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/4d869581586d
status-b2g-v2.0: affected → fixed

Updated

3 years ago
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
Depends on: 1062031
You need to log in before you can comment on or make changes to this bug.