Closed Bug 1199653 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.5+, firefox43 fixed)

RESOLVED FIXED
FxOS-S7 (18Sep)
blocking-b2g 2.5+
Tracking Status
firefox43 --- fixed

People

(Reporter: gwagner, Assigned: yrliou)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file gdb bt
on aries with debug gecko:

Open music app, close music app via task manager.
blocking-b2g: --- → 2.5+
Jocelyn will help check this bug. Assign to her first.
Assignee: nobody → joliu
Set QA wanted to have a branch check and see if it's a regression first.
Keywords: qawanted
This is with a debug gecko. I don't think QA tests with debug gecko.
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
Flags: needinfo?(anygregor)
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.
Flags: needinfo?(anygregor)
(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.
Forgot to clean the flag since it's a debug build bug.
Keywords: qawanted
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
(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]
>
btw, I couldn't reproduce it on the release build using flame-kk m-c.
Hi Thomas,

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

Thanks,
Jocelyn
Attachment #8658078 - Flags: review?(tzimmermann)
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.
Attachment #8658078 - Flags: review?(tzimmermann) → feedback+
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
Attachment #8658078 - Flags: review?(shuang)
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.
Attachment #8658078 - Flags: review?(shuang) → review+
https://hg.mozilla.org/mozilla-central/rev/d0998dc22964
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S7 (18Sep)
You need to log in before you can comment on or make changes to this bug.