1199653 - Moz_Crash() when closing the music app at PContentChild::SendPBluetoothConstructor

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Gregor Wagner [:gwagner]]

Attachment: gdb bt

on aries with debug gecko:

Open music app, close music app via task manager.

Comment 1

[Ben Tian (inactive)]

Jocelyn will help check this bug. Assign to her first.

Comment 2

[Jocelyn Liu [:jocelyn] [:joliu]]

Set QA wanted to have a branch check and see if it's a regression first.

Comment 3

[Gregor Wagner [:gwagner]]

This is with a debug gecko. I don't think QA tests with debug gecko.

Comment 4

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi Gregor,

AFAIK, both release build and debug build will crash for MOZ_CRASH, right?

Could you provide more detail information for this bug so I could try to reproduce from my end?
1) Do you encounter this in both debug and release build? or debug build only?
2) What gecko/gaia version are you using?
3) aries-kk or aries-l?
3) What's the reproduce rate?
4) Is the reproduce step only like below?
    Step1. Enable BT in settings
    Step2. Open music app
    Step3. Close music app from task manager

Thanks,
Jocelyn

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

Looks like it crashed when we trying to send a ipc request while the content process is shutting down.
Try to reproduce it but haven't had luck to hit it yet.

There's an another bug in our code that we shouldn't unregister signal handler when it's not registered. (In unlink)
But I think this ipc crash bug can still be hit even that has been fixed.

Comment 6

[Gregor Wagner [:gwagner]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #4)
> Hi Gregor,
> 
> AFAIK, both release build and debug build will crash for MOZ_CRASH, right?

I dont know.

> 
> Could you provide more detail information for this bug so I could try to
> reproduce from my end?
> 1) Do you encounter this in both debug and release build? or debug build
> only?

Only tried with debug gecko

> 2) What gecko/gaia version are you using?

Just tried current b-i and gaia tip and its still hitting the MOZ_CRASH

> 3) aries-kk or aries-l?

aries-kk

> 3) What's the reproduce rate?

100%

> 4) Is the reproduce step only like below?
>     Step1. Enable BT in settings
>     Step2. Open music app
>     Step3. Close music app from task manager

Yep

It's an app shutdown crash.

Comment 7

[Jocelyn Liu [:jocelyn] [:joliu]]

Forgot to clean the flag since it's a debug build bug.

Comment 8

[Bruce Sun [:brsun]]

|BluetoothService| would be deleted by |KillClearOnShutdown()|[1][2]. It would be better to unregister the signal handler earlier.

Probably we could solve this issue by notifying observers in |Cleanup()|[3], and then handle the unregister stuff (ex. [4]) in |Notify()| (ex. [5]) function.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothService.cpp?from=BluetoothService.cpp&offset=0#617
[2] https://dxr.mozilla.org/mozilla-central/source/xpcom/base/ClearOnShutdown.h?from=KillClearOnShutdown#101
[3] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothService.cpp?from=BluetoothService.cpp#231
[4] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothAdapter.cpp?case=true&from=BluetoothAdapter.cpp#57
[5] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothAdapter.cpp?case=true&from=BluetoothAdapter.cpp#480

Comment 9

[Jocelyn Liu [:jocelyn] [:joliu]]

Basically we could open any app that will access bluetooth, attach gdb on the content process, then observed MOZ_CRASH by closing the app.

In this case, we observed XPCOM_SHUTDOWN in content process, but |sInShutdown| in |BluetoothService.cpp| [1] won't become true since we overwrite |HandleShutdown| in ServiceChildProcess.cpp [2].
We will create a new service instance during shutdown, trying to send ipc messages, then failed to send. (we will receive "Closed channel: cannot send/recv" since the channel state is |ChannelClosed|.[3])
MOZ_CRASH is then hit because of that.[4]

IMHO, sInShutdown in the content process's instance should be true when we observed XPCOM_SHUTDOWN.
I have tried to set the flag for both parent and content process before calling into |HandleShutdown|, applications can be exited normally .

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothService.cpp?offset=1300#82
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/ipc/BluetoothServiceChildProcess.cpp?from=BluetoothServiceChildProcess.cpp&offset=0#639
[3] https://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#1675-1676
[4] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PContentChild.cpp?case=true&from=PContentChild.cpp#2095-2097

Comment 10

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #9)
> In this case, we observed XPCOM_SHUTDOWN in content process, but
> |sInShutdown| in |BluetoothService.cpp| [1] won't become true since we
> overwrite |HandleShutdown| in ServiceChildProcess.cpp [2].
I missed one line of description here.
If content process issues any requests during shutdown, we will...

> We will create a new service instance during shutdown, trying to send ipc
> messages, then failed to send. (we will receive "Closed channel: cannot
> send/recv" since the channel state is |ChannelClosed|.[3])
> MOZ_CRASH is then hit because of that.[4]
>

Comment 11

[Jocelyn Liu [:jocelyn] [:joliu]]

btw, I couldn't reproduce it on the release build using flame-kk m-c.

Comment 12

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1199653 - Correctly set |sInShutdown| in BluetoothService for content processes.

Hi Thomas,

Could you help to review my patch?
Please kindly see my explanation in Comment 9 and Comment 10.

Thanks,
Jocelyn

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8658078 [details] [diff] [review]
Bug 1199653 - Correctly set |sInShutdown| in BluetoothService for content processes.

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

Hi,

your patch seems to make sense according to your explanation, but I don't know enough about the shutdown logic to review.

Comment 14

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8658078 [details] [diff] [review]
Bug 1199653 - Correctly set |sInShutdown| in BluetoothService for content processes.

Thanks for your feedback, Thomas.

Hi Shawn,

Could you help to review my patch?
Please kindly see my explanation in Comment 9 and Comment 10.

Thanks,
Jocelyn

Comment 15

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

Comment on attachment 8658078 [details] [diff] [review]
Bug 1199653 - Correctly set |sInShutdown| in BluetoothService for content processes.

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

Hi Jocelyn,
Thanks for fixing this long existing bug.

::: dom/bluetooth/common/BluetoothService.cpp
@@ +630,5 @@
>      return HandleSettingsChanged(aSubject);
>    }
>  
>    if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
> +    sInShutdown = true;

Please add comments to explain why we move |sInShutdown| here, since it's easy to forget.

Comment 16

[Jocelyn Liu [:jocelyn] [:joliu]]

Thanks for your time, Shawn.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=199ed394e17f

Comment 17

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1199653 - Correctly set |sInShutdown| in BluetoothService for content processes. f=tzimmermann, r=shuang

https://treeherder.mozilla.org/#/jobs?repo=try&revision=199ed394e17f
try looks ok, failed items seem all exist before my change.

Comment 18

[Pulsebot]

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

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d0998dc22964