Closed Bug 947870 Opened 11 years ago Closed 11 years ago

Assertion in BluetoothOppManager::CreateFile

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: tzimmermann, Assigned: echou)

References

Details

(Keywords: regression)

Attachments

(2 files)

patch 1: v1: avoid assertion on receiving files via Bluetooth
8.14 KB, patch
gyeh
: review+
ben.tian
: feedback+
Details | Diff | Splinter Review
Patch 2 (v1): Add missing part in bluedroid
1.04 KB, patch
echou
: review+
Details | Diff | Splinter Review 
On m-c, I now constantly run into the assertion below. Ref is 0204febd3146. It looks like the phone tries to create a file before transferring data, but the CreateFile method doesn't support this.

STR:

 - pair with PC
 - transfer file from PC to phone
 - accept file transfer on phone

Expected:

 - file is transfered and stored on phone

Actual:

 - phone crashes immediately

>>>

mozilla@barney:~/Projects/mozilla/src/B2G-master-unagi$ ./run-gdb.sh attach 109
Attached; pid = 109
Listening on port 11111
prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-gdb -x /tmp/b2g.gdbinit.mozilla.2255 /home/mozilla/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/dist/bin/b2g
GNU gdb (GDB) 7.1-android-gg2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "--host=i686-linux-gnu --target=arm-elf-linux".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/mozilla/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/dist/bin/b2g...done.
Remote debugging from host 127.0.0.1
c
0x424b01be in JSObject::addPropertyInternal<(js::ExecutionMode)0> (cx=0x403634a0, obj=..., id=..., getter=0, setter=0x415c19dd <XPC_WN_OnlyIWrite_SetPropertyStub>, 
    slot=16777215, attrs=7, flags=0, shortid=0, spp=0x0, allowDictionary=true) at /home/mozilla/Projects/mozilla/src/mozilla-central/js/src/vm/Shape.cpp:595
595	        if (allowDictionary &&
(gdb) c
Continuing.
[New Thread 109.531]

Program received signal SIGSEGV, Segmentation fault.
0x4169f7f6 in mozilla::dom::bluetooth::BluetoothOppManager::CreateFile (this=0x4624d1a0)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/bluetooth/bluez/BluetoothOppManager.cpp:573
573	  MOZ_ASSERT(mPacketReceivedLength == mPacketLength);
(gdb) bt
#0  0x4169f7f6 in mozilla::dom::bluetooth::BluetoothOppManager::CreateFile (this=0x4624d1a0)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/bluetooth/bluez/BluetoothOppManager.cpp:573
#1  0x416a1fda in mozilla::dom::bluetooth::BluetoothOppManager::ConfirmReceivingFile (this=0x4624d1a0, aConfirm=<value optimized out>)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/bluetooth/bluez/BluetoothOppManager.cpp:474
#2  0x416a3f28 in mozilla::dom::bluetooth::BluetoothDBusService::ConfirmReceivingFile (this=<value optimized out>, aDeviceAddress=<value optimized out>, aConfirm=true, 
    aRunnable=0x44581860) at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/bluetooth/bluez/linux/BluetoothDBusService.cpp:3061
#3  0x4168a446 in mozilla::dom::bluetooth::BluetoothAdapter::ConfirmReceivingFile (this=<value optimized out>, aDeviceAddress=..., aConfirmation=true, aRv=...)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/bluetooth/BluetoothAdapter.cpp:814
#4  0x412955a2 in confirmReceivingFile (cx=0x45177090, obj=..., self=0x47733580, args=...)
    at /home/mozilla/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/dom/bindings/BluetoothAdapterBinding.cpp:1839
#5  0x4128bace in genericMethod (cx=0x45177090, argc=<value optimized out>, vp=<value optimized out>)
    at /home/mozilla/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/dom/bindings/BluetoothAdapterBinding.cpp:2158
#6  0x42462e7c in js::CallJSNative (cx=0x45177090, native=0x4128ba15 <genericMethod>, args=...)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/js/src/jscntxtinlines.h:220
#7  0x424765e6 in js::Invoke (cx=0x45177090, args=..., construct=js::NO_CONSTRUCT) at /home/mozilla/Projects/mozilla/src/mozilla-central/js/src/vm/Interpreter.cpp:463
#8  0x42469f98 in Interpret (cx=0x45177090, state=<value optimized out>) at /home/mozilla/Projects/mozilla/src/mozilla-central/js/src/vm/Interpreter.cpp:2505
#9  0x42475efe in js::RunScript (cx=0x45177090, state=...) at /home/mozilla/Projects/mozilla/src/mozilla-central/js/src/vm/Interpreter.cpp:420
#10 0x4247657c in js::Invoke (cx=0x45177090, args=..., construct=js::NO_CONSTRUCT) at /home/mozilla/Projects/mozilla/src/mozilla-central/js/src/vm/Interpreter.cpp:482
#11 0x42476c42 in js::Invoke (cx=0x45177090, thisv=..., fval=..., argc=1, argv=0xbeebaee0, rval=...)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/js/src/vm/Interpreter.cpp:513
#12 0x42338186 in JS_CallFunctionValue (cx=0x45177090, objArg=<value optimized out>, fval=..., argc=1, argv=0xbeebaee0, rval=0xbeebaf68)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/js/src/jsapi.cpp:4990
#13 0x4131d4cc in mozilla::dom::EventHandlerNonNull::Call (this=<value optimized out>, cx=0x45177090, aThisObj=..., event=..., aRv=...)
    at /home/mozilla/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/dom/bindings/EventHandlerBinding.cpp:35
#14 0x416fa7ea in Call<nsISupports*> (this=0x4457f940, aEvent=0x443b82e0) at ../../../dist/include/mozilla/dom/EventHandlerBinding.h:59
#15 nsJSEventListener::HandleEvent (this=0x4457f940, aEvent=0x443b82e0) at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/src/events/nsJSEventListener.cpp:236
#16 0x418e1494 in nsEventListenerManager::HandleEventSubType (this=0x454e5d60, aListenerStruct=<value optimized out>, aDOMEvent=0x443b82e0, aCurrentTarget=0x443c8cc0, 
    aPusher=0xbeebb430) at /home/mozilla/Projects/mozilla/src/mozilla-central/content/events/src/nsEventListenerManager.cpp:934
#17 0x418e16b0 in nsEventListenerManager::HandleEventInternal (this=0x454e5d60, aPresContext=<value optimized out>, aEvent=0x47058a10, aDOMEvent=0xbeebb49c, 
    aCurrentTarget=0x443c8cc0, aEventStatus=0xbeebb4a0, aPusher=0xbeebb430)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/content/events/src/nsEventListenerManager.cpp:1011
#18 0x418de782 in nsEventListenerManager::HandleEvent (this=<value optimized out>, aVisitor=..., aCd=<value optimized out>, aPusher=0xbeebb430)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/content/events/src/nsEventListenerManager.h:326
#19 nsEventTargetChainItem::HandleEvent (this=<value optimized out>, aVisitor=..., aCd=<value optimized out>, aPusher=0xbeebb430)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/content/events/src/nsEventDispatcher.cpp:197
#20 0x418de8d6 in nsEventTargetChainItem::HandleEventTargetChain (aChain=..., aVisitor=..., aCallback=0x0, aCd=..., aPusher=0xbeebb430)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/content/events/src/nsEventDispatcher.cpp:292
#21 0x418df7ee in nsEventDispatcher::Dispatch (aTarget=<value optimized out>, aPresContext=0x0, aEvent=0x47058a10, aDOMEvent=<value optimized out>, aEventStatus=0xbeebb53c, 
    aCallback=0x0, aTargets=0x0) at /home/mozilla/Projects/mozilla/src/mozilla-central/content/events/src/nsEventDispatcher.cpp:609
#22 0x418df9a6 in nsEventDispatcher::DispatchDOMEvent (aTarget=0x443c8cc0, aEvent=0x47058a10, aDOMEvent=0x443b82e0, aPresContext=0x0, aEventStatus=0xbeebb53c)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/content/events/src/nsEventDispatcher.cpp:676
#23 0x41688da8 in nsWindowRoot::DispatchEvent (this=<value optimized out>, aEvt=<value optimized out>, aRetVal=0xbeebb56f)
---Type <return> to continue, or q <return> to quit---
    at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/base/nsWindowRoot.cpp:80
#24 0x4164ce72 in mozilla::dom::DOMRequest::DispatchEvent (this=0xbeebb53c, evt=0x443b82e0, _retval=0xbeebb56f)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/base/DOMRequest.h:31
#25 0x4164f800 in mozilla::dom::DOMRequest::FireEvent (this=<value optimized out>, aType=..., aBubble=false, aCancelable=false)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/base/DOMRequest.cpp:186
#26 0x4164fa22 in mozilla::dom::DOMRequest::FireSuccess (this=0x443c8cc0, aResult=<value optimized out>)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/base/DOMRequest.cpp:126
#27 0x416b61e4 in PostResultEvent::Run (this=<value optimized out>) at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/devicestorage/nsDeviceStorage.cpp:1960
#28 0x40e7d544 in nsThread::ProcessNextEvent (this=0x40302550, mayWait=<value optimized out>, result=0xbeebb6af)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/threads/nsThread.cpp:612
#29 0x40e321f0 in NS_ProcessNextEvent (thread=0x40302550, mayWait=false) at /home/mozilla/Projects/mozilla/src/mozilla-central/xpcom/glue/nsThreadUtils.cpp:263
#30 0x4102ac54 in mozilla::ipc::MessagePump::Run (this=0x40301d90, aDelegate=0x4034d0c0) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/glue/MessagePump.cpp:85
#31 0x4101cd02 in MessageLoop::RunInternal (this=0x4034d0c0) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:222
#32 0x4101cd1a in MessageLoop::RunHandler (this=0x4034d0c0) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:215
#33 MessageLoop::Run (this=0x4034d0c0) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:189
#34 0x41561c52 in nsBaseAppShell::Run (this=0x443fcca0) at /home/mozilla/Projects/mozilla/src/mozilla-central/widget/xpwidgets/nsBaseAppShell.cpp:161
#35 0x41f273a6 in nsAppStartup::Run (this=0x4435c1c0) at /home/mozilla/Projects/mozilla/src/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:268
#36 0x41ef393c in XREMain::XRE_mainRun (this=0xbeebb994) at /home/mozilla/Projects/mozilla/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:3978
#37 0x41ef6cda in XREMain::XRE_main (this=0xbeebb994, argc=<value optimized out>, argv=<value optimized out>, aAppData=0x22170)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:4046
#38 0x41ef6e74 in XRE_main (argc=1, argv=0xbeebdb84, aAppData=0x22170, aFlags=<value optimized out>)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:4254
#39 0x00009a34 in do_main (argc=1, argv=0xbeebdb84) at /home/mozilla/Projects/mozilla/src/mozilla-central/b2g/app/nsBrowserApp.cpp:163
#40 main (argc=1, argv=0xbeebdb84) at /home/mozilla/Projects/mozilla/src/mozilla-central/b2g/app/nsBrowserApp.cpp:256
(gdb) print mPacketLength 
$1 = 24573
(gdb) print mPacketReceivedLength 
$2 = 0
(gdb)
Blocks: 932728
I'll check tomorrow. Thanks, Thomas.
Assignee: nobody → echou
I did some grepping through the source code. Just before calling CreateFile, there is an assertion that mPacketReceivedLength == 0 at line 466 after user confirmation. So we are supposed to only call this function on the first PUT. mPacketLength is set up as a result of line 852, but user confirmation is only asked afterwards. So the assertion doesn't seem to make sense.

I don't know why this started failing now, but I can remove the assertion and everything works as before.
The assertion should be MOZ_ASSERT(mPacketReceivedLength == 0) as in ConfirmReceivingFile(), since mPacketReceivedLength is reset immediately each time a complete PUT packet is received [1]. The cause is recent bug 932543 fix [2] that rewrote the logic to check data size before memcpy.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluez/BluetoothOppManager.cpp#861
[2] https://hg.mozilla.org/releases/mozilla-b2g18/rev/97e2069ba2a0
Mark this as a regression of bug 932543
Keywords: regression
* Rename "mPacketReceivedLength" to "mPutPacketReceivedLength"
* Reset mPutPacketReceivedLength to 0 in ReplyToPut()
* Ensure a complete packet has being received at the beginning of CreateFile()
Attachment #8345099 - Flags: review?(gyeh)
Attachment #8345099 - Flags: feedback?(btian)
Tested on my Unagi.
Comment on attachment 8345099 [details] [diff] [review]
patch 1: v1: avoid assertion on receiving files via Bluetooth

Review of attachment 8345099 [details] [diff] [review]:
-----------------------------------------------------------------

Please also revise the variable name in [1], and sync the fix to bluedroid OPP manager.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluez/BluetoothOppManager.cpp#765

::: dom/bluetooth/bluez/BluetoothOppManager.cpp
@@ +461,5 @@
>  BluetoothOppManager::ConfirmReceivingFile(bool aConfirm)
>  {
>    NS_ENSURE_TRUE(mConnected, false);
>    NS_ENSURE_TRUE(mWaitingForConfirmationFlag, false);
>  

Should we keep MOZ_ASSERT(mPacketReceivedLength == mPacketLength) here?
(In reply to Ben Tian [:btian] from comment #7)
> Comment on attachment 8345099 [details] [diff] [review]
> patch 1: v1: avoid assertion on receiving files via Bluetooth
> 
> Review of attachment 8345099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please also revise the variable name in [1], and sync the fix to bluedroid
> OPP manager.
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluez/
> BluetoothOppManager.cpp#765
> 
> ::: dom/bluetooth/bluez/BluetoothOppManager.cpp
> @@ +461,5 @@
> >  BluetoothOppManager::ConfirmReceivingFile(bool aConfirm)
> >  {
> >    NS_ENSURE_TRUE(mConnected, false);
> >    NS_ENSURE_TRUE(mWaitingForConfirmationFlag, false);
> >  
> 
> Should we keep MOZ_ASSERT(mPacketReceivedLength == mPacketLength) here?

Yes we could but I think it's not really necessary since it has been checked in CreateFile(). I can readd it if you think we should keep it. Just don't like to add too many assertions. :)
> > Should we keep MOZ_ASSERT(mPacketReceivedLength == mPacketLength) here?
> 
> Yes we could but I think it's not really necessary since it has been checked
> in CreateFile(). I can readd it if you think we should keep it. Just don't
> like to add too many assertions. :)
Agree. Please keep the assertion removed.
Comment on attachment 8345099 [details] [diff] [review]
patch 1: v1: avoid assertion on receiving files via Bluetooth

Review of attachment 8345099 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great to me. Thanks.
Attachment #8345099 - Flags: review?(gyeh) → review+
Attachment #8345099 - Flags: feedback?(btian) → feedback+
https://hg.mozilla.org/mozilla-central/rev/67bcb37ba888
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Reopen as the bug still happens on bluedroid.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Remove the reset in bluedroid as in bluez.
Attachment #8351138 - Flags: review?(echou)
Comment on attachment 8351138 [details] [diff] [review]
Patch 2 (v1): Add missing part in bluedroid

Review of attachment 8351138 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this. My bad.
Attachment #8351138 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/20798ccad862
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: