Closed
Bug 1048581
Opened 10 years ago
Closed 10 years ago
[MobileConnection] Crash in WrapNewBindingObjectHelper during stability test
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(blocking-b2g:2.0+, 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)
376.41 KB,
text/plain
|
Details | |
581.42 KB,
text/plain
|
Details | |
1.05 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
5.82 KB,
patch
|
edgar
:
review+
|
Details | Diff | Splinter Review |
6.37 KB,
patch
|
Details | Diff | Splinter Review |
[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) ]
Comment 1•10 years ago
|
||
Comment 2•10 years ago
|
||
Updated•10 years ago
|
Whiteboard: [b2g-crash][CR 704025] → [caf priority: p1][b2g-crash][CR 704025]
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 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•10 years ago
|
Summary: Crash in WrapNewBindingObjectHelper during stability test → [MobileConnection] Crash in WrapNewBindingObjectHelper during stability test
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Updated•10 years ago
|
Whiteboard: [caf priority: p1][b2g-crash][CR 704025] → [caf-crash 321][caf priority: p1][b2g-crash][CR 704025]
Assignee | ||
Comment 5•10 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•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 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•10 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•10 years ago
|
Attachment #8469031 -
Flags: review?(bugs) → review+
Comment 10•10 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 You don't need + mProvider = nullptr; + mVoice = nullptr; + mData = nullptr; in destructor.
Attachment #8469059 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Address review comment #10. Thanks for the review, Olli.
Attachment #8469059 -
Attachment is obsolete: true
Attachment #8469757 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
Try result: https://tbpl.mozilla.org/?tree=Try&rev=5d58c9aa969f
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/67c2719b7fc4 https://hg.mozilla.org/integration/b2g-inbound/rev/a55ad5e6580b
Comment 15•10 years ago
|
||
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)
Comment 16•10 years ago
|
||
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
Comment 17•10 years ago
|
||
Even with bug 1030002 applied, part 2 will need additional rebasing for b2g32 as well.
Updated•10 years ago
|
Flags: needinfo?(echen)
Keywords: branch-patch-needed
Assignee | ||
Updated•10 years ago
|
Keywords: branch-patch-needed
Comment 19•10 years ago
|
||
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
Comment 20•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/fed262164fd1 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/4d869581586d
Updated•10 years ago
|
Whiteboard: [caf-crash 321][caf priority: p1][b2g-crash][CR 704025] → [caf-crash 321][caf priority: p3][b2g-crash][CR 704025]
Comment 21•10 years ago
|
||
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
Updated•10 years ago
|
Flags: in-moztrap?(bzumwalt)
Comment 22•10 years ago
|
||
No STR is present to create test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(bzumwalt)
Flags: in-moztrap-
Comment 23•10 years ago
|
||
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.
Description
•