980039 - MOZ_ASSERT(aWindow) in BluetoothAdapter::Create (aWindow=0x0, aValue=...) at BluetoothAdapter.cpp:291

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Gregor Wagner [:gwagner]]

Seen on current gaia/gecko trunk with debug gecko.

STR:
Device with unregistered SIM card. (not sure if this is important here)
open dialer
call a number
No network connection shows up. If it doesn't hit the assertion the first time, click ok and dial again.

Program received signal SIGSEGV, Segmentation fault.
0xb53dd5d4 in mozilla::dom::bluetooth::BluetoothAdapter::Create (aWindow=0x0, aValue=...) at ../../../dom/bluetooth/BluetoothAdapter.cpp:291
291	  MOZ_ASSERT(aWindow);
(gdb) bt
#0  0xb53dd5d4 in mozilla::dom::bluetooth::BluetoothAdapter::Create (aWindow=0x0, aValue=...) at ../../../dom/bluetooth/BluetoothAdapter.cpp:291
#1  0xb53df7fc in GetAdapterTask::ParseSuccessfulReply (this=0xb0d98b80, aValue=<optimized out>) at ../../../dom/bluetooth/BluetoothManager.cpp:58
#2  0xb53e09b0 in mozilla::dom::bluetooth::BluetoothReplyRunnable::Run (this=0xb0d98b80) at ../../../dom/bluetooth/BluetoothReplyRunnable.cpp:71
#3  0xb4c04572 in ProcessNextEvent (result=0xbee57dbf, mayWait=false, this=0xb3d2e780) at ../../../xpcom/threads/nsThread.cpp:643
#4  nsThread::ProcessNextEvent (this=0xb3d2e780, mayWait=<optimized out>, result=0xbee57dbf) at ../../../xpcom/threads/nsThread.cpp:567
#5  0xb4bbe87c in NS_ProcessNextEvent (thread=0xb3d2e780, mayWait=<optimized out>) at ../../../xpcom/glue/nsThreadUtils.cpp:263
#6  0xb4d9eb04 in mozilla::ipc::MessagePump::Run (this=0xb3d01b50, aDelegate=0xbee57f18) at ../../../ipc/glue/MessagePump.cpp:95
#7  0xb4d9050a in MessageLoop::RunInternal (this=0xbee57f18) at ../../../ipc/chromium/src/base/message_loop.cc:226
#8  0xb4d90522 in RunHandler (this=0xbee57f18) at ../../../ipc/chromium/src/base/message_loop.cc:219
#9  MessageLoop::Run (this=0xbee57f18) at ../../../ipc/chromium/src/base/message_loop.cc:193
#10 0xb52d1806 in nsBaseAppShell::Run (this=0xb30d3340) at ../../../widget/xpwidgets/nsBaseAppShell.cpp:164
#11 0xb5b557d6 in XRE_RunAppShell () at ../../../toolkit/xre/nsEmbedFunctions.cpp:679
#12 0xb4d9ec1e in mozilla::ipc::MessagePumpForChildProcess::Run (this=0xb3d01b50, aDelegate=0xbee57f18) at ../../../ipc/glue/MessagePump.cpp:253
#13 0xb4d9050a in MessageLoop::RunInternal (this=0xbee57f18) at ../../../ipc/chromium/src/base/message_loop.cc:226
#14 0xb4d90522 in RunHandler (this=0xbee57f18) at ../../../ipc/chromium/src/base/message_loop.cc:219
#15 MessageLoop::Run (this=0xbee57f18) at ../../../ipc/chromium/src/base/message_loop.cc:193
#16 0xb5b556ba in XRE_InitChildProcess (aArgc=5, aArgv=<optimized out>, aProcess=<optimized out>) at ../../../toolkit/xre/nsEmbedFunctions.cpp:516
#17 0x00008862 in main (argc=6, argv=0xbee58a14) at ../../../ipc/app/MozillaRuntimeMain.cpp:149

Comment 1

[Jamin Liu [:jaliu][:Jamin]]

I've tried to reproduce several times and I hadn't seen the same MOZ_ASSERT() was triggered by BluetoothAdapter.

However, a very similar MOZ_ASSERT() was hit few times.

MOZ_ASSERT(aWindow) was triggered in mozilla::dom::bluetooth::BluetoothDevice::Create
It locates at ../../../dom/bluetooth/BluetoothDevice.cpp:168

I'll take a look.
Thanks.

Comment 2

[Jamin Liu [:jaliu][:Jamin]]

Hi Gregor,
Thank you for reporting.

May I ask few questions about STR?
1. Was Bluetooth enabled and under discovering when it hit the MOZ_ASSERT?
2. You said "click ok and dial again", does it means cancel the first call and redail again?
3. Did it hit the MOZ_ASSERT when the phone was connected to other Bluetooth device?

Thanks.

Comment 3

[Gregor Wagner [:gwagner]]

(In reply to Jamin Liu [:jaliu] from comment #2)
> Hi Gregor,
> Thank you for reporting.
> 
> May I ask few questions about STR?
> 1. Was Bluetooth enabled and under discovering when it hit the MOZ_ASSERT?

Yes it was enabled afaik but I will have to test again to make sure.

> 2. You said "click ok and dial again", does it means cancel the first call
> and redail again?

My device had an unregistered SIM. So when I dial a number I get the overlay that says
that I can't make phone calls and this overlay just has an OK button.

> 3. Did it hit the MOZ_ASSERT when the phone was connected to other Bluetooth
> device?

It was not connected.

> 
> Thanks.

Comment 4

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [patch 1] Unregister Bluetooth signal handler when BT objects were disconnected from owner window. (v0)

Comment 5

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [patch 2] Stop to create BT adapter if BT manager was disconnected from owner window. (v0)

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8389752 [details] [diff] [review]
[patch 1] Unregister Bluetooth signal handler when BT objects were disconnected from owner window. (v0)

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

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +172,4 @@
>  {
>    MOZ_ASSERT(aWindow);
>    MOZ_ASSERT(IsDOMBinding());
>  

Can you elaborate that why we need to remove |BindToOwner|?

Comment 7

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #6)
> Comment on attachment 8389752 [details] [diff] [review]
> [patch 1] Unregister Bluetooth signal handler when BT objects were
> disconnected from owner window. (v0)
> 
> Review of attachment 8389752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +172,4 @@
> >  {
> >    MOZ_ASSERT(aWindow);
> >    MOZ_ASSERT(IsDOMBinding());
> >  
> 
> Can you elaborate that why we need to remove |BindToOwner|?


Sorry, I should provide an explanation before.
 
I removed BindToOwner() from BluetoothManager since we already called it through initialization list of constructor.

Please refer to [1], ": nsDOMEventTargetHelper(aWindow)" would bind BluetoothManager to owner.
[1] http://dxr.mozilla.org/mozilla-central/source/dom/events/nsDOMEventTargetHelper.h#37

It did the same thing in BluetoothAdapter and BluetoothDevice.

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8389752 [details] [diff] [review]
[patch 1] Unregister Bluetooth signal handler when BT objects were disconnected from owner window. (v0)

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

Make sense to me. But can you explain the problem in detail?

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +199,5 @@
> +  nsDOMEventTargetHelper::DisconnectFromOwner();
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE_VOID(bs);
> +  bs->UnregisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_ADAPTER), this);

Will |UnregisterBluetoothSignalHandler()| be called twice? Since BluetoothAdapter dtor also call |UnregisterBluetoothSignalHandler|.

::: dom/bluetooth/BluetoothManager.cpp
@@ +116,5 @@
> +  nsDOMEventTargetHelper::DisconnectFromOwner();
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE_VOID(bs);
> +  bs->UnregisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_MANAGER), this);

ditto.

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8389753 [details] [diff] [review]
[patch 2] Stop to create BT adapter if BT manager was disconnected from owner window. (v0)

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

It's better to change the patch title and add more detail explanation. Or just combine this patch with patch 1. Thanks.

Comment 10

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #8)
> Comment on attachment 8389752 [details] [diff] [review]
> [patch 1] Unregister Bluetooth signal handler when BT objects were
> disconnected from owner window. (v0)
> 
> Review of attachment 8389752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Make sense to me. But can you explain the problem in detail?
> 
Sure.
I think I should create a different bug since Attachment #8389752 [details] [diff] (patch 1) tries to solve another similar MOZ_ASSERT() problem and it's not directly match the situation of this bug.
Please refer to Bug 988111.

> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +199,5 @@
> > +  nsDOMEventTargetHelper::DisconnectFromOwner();
> > +
> > +  BluetoothService* bs = BluetoothService::Get();
> > +  NS_ENSURE_TRUE_VOID(bs);
> > +  bs->UnregisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_ADAPTER), this);
> 
> Will |UnregisterBluetoothSignalHandler()| be called twice? Since
> BluetoothAdapter dtor also call |UnregisterBluetoothSignalHandler|.

Yes, it would be called twice if it was disconnected from the owner window.
As far as I know, it wouldn't cause any problem to unregister BluetoothSignal handler twice.

The observer of BluetoothSignal should be removed from observer list whether it's been destructed or been disconnected from its owner windows.

Since javascript use garbage collection to free memory, BluetoothManager/BluetoothAdapter could still exist for a while even the refCount = 0. I think we should unregister the observer when it's not used by gaia, even it hasn't been destructed.

> 
> ::: dom/bluetooth/BluetoothManager.cpp
> @@ +116,5 @@
> > +  nsDOMEventTargetHelper::DisconnectFromOwner();
> > +
> > +  BluetoothService* bs = BluetoothService::Get();
> > +  NS_ENSURE_TRUE_VOID(bs);
> > +  bs->UnregisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_MANAGER), this);
> 
> ditto.

Comment 11

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8389752 [details] [diff] [review]
[patch 1] Unregister Bluetooth signal handler when BT objects were disconnected from owner window. (v0)

Abandon Attachment #8389752 [details] [diff] since it's been moved to Bug 988111.

Comment 12

[Jamin Liu [:jaliu][:Jamin]]

Root cause:
When Gaia dev call manager.getDefaultAdapter(), a adapter would be created through a BluetoothReplyRunnable 'GetAdapterTask'.

The owner windows of BluetoothManager would be passed to Bluetoothadapter in GetAdapterTask::ParseSuccessfulReply().

If the nsPIDOMWindow was cleaned up during the 'getDefaultAdapter' process, the MOZ_ASSERT would be hit.
P.S. It's hard to reproduce.

Solution:
Don't create BT adapter if its owner windows was cleaned up.
It's meaningless to create a adapter for a BluetoothManager which was disconnected from its owner window. (v0)

Comment 13

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Don't create BT adapter if the owner window of BT manager was cleaned up. (v1)

Comment 14

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8396939 [details] [diff] [review]
Don't create BT adapter if the owner window of BT manager was cleaned up. (v1)

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

LGTM, thanks!

Comment 15

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8396939 [details] [diff] [review]
Don't create BT adapter if the owner window of BT manager was cleaned up. (v1)

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

r=me with nit addressed.

::: dom/bluetooth/BluetoothManager.cpp
@@ +51,5 @@
>        SetError(NS_LITERAL_STRING("BluetoothReplyTypeError"));
>        return false;
>      }
>  
> +    if(!mManagerPtr->GetOwner()) {

nit: please separate "if" and "("

Comment 16

[Jamin Liu [:jaliu][:Jamin]]

> ::: dom/bluetooth/BluetoothManager.cpp
> @@ +51,5 @@
> >        SetError(NS_LITERAL_STRING("BluetoothReplyTypeError"));
> >        return false;
> >      }
> >  
> > +    if(!mManagerPtr->GetOwner()) {
> 
> nit: please separate "if" and "("

Thank you.
I've fixed it and put it on the try server.

Comment 17

[Jamin Liu [:jaliu][:Jamin]]

Attachment: (final) Don't create BT adapter if the owner window of BT manager was cleaned up. (v1) r=echou, f=shuang

It looks fine on try server.
https://tbpl.mozilla.org/?tree=Try&rev=69fd7e86401e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8462e3d47529

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8462e3d47529

Comment 20

[Firefox Commit Bot]

The following changeset is now in Firefox Nightly:

> 8462e3d47529 Bug 980039 - Don't create BT adapter if the owner window of BT manager was cleaned up. r=echou, f=shuang

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central