1000961 - Make DBusReplyHandler use thread-safe ref-counting

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Eric Kramer [:Kramer]]

Description:
Making your phone "visible to all" through Settings > Bluetooth will cause the phone to become locked into the animated fox screen. 

User was unable to recover, resetting the phone through normal means (powering off or power cycling) did not work. Removing the battery and restarting the device places the user back in an infinite animated fox state.  

Repro Steps:
1) Update an Open_C to BuildID: 20140424040201
2) Open "Settings".
3) Open "Bluetooth" setting.
4) Turn on Bluetooth.
5) Activate the "Visible to all" option.

Actual:
The device crashes / resets to the animated fox screen. User cannot turn off, or power cycle, their phone. 

Expected:
Users are able to activate the Bluetooth options on their phone. 

Environmental Variables:
Device: Open_C Master MOZ
BuildID: 20140424040201
Gaia: d25852a189c9707b144eb5f82d08384eb066c0fd
Gecko: c8055a00235d
Version: 31.0a1
Firmware Version: P821A10-ENG_20140410


Keywords:
Bluetooth, Settings, Animated Fox

Notes:
We reproduced this issue on both an Open_C and a Buri, on Master Builds.


Repro frequency: 3/3 device, 100%
See attached: Will attach video shortly.

Comment 1

[Eric Kramer [:Kramer]]

This issue does not reproduce on Aurora build on Open_C.

1.4 Environmental Variables:
Device: Open_C 1.4 MOZ
BuildID: 20140424000203
Gaia: b6a41ee9e2934fe9692da6fb1cb310cb759e9870
Gecko: 2e9abf89c087
Version: 30.0a2
Firmware Version: P821A10-ENG_20140410

Comment 2

[Marcia Knous [:marcia]]

We need to get a logcat attached to this bug and/or a crash report. 

This could also be related the recent bug that was fixed - Bug 994411.

Comment 3

[Natalya Kot [:nkot]]

Attachment: logcat

Comment 4

[John Hammink]

I'm on the same gecko and gaia version (possibly different base image) and I'm not seeing the issue:

Gaia      d25852a189c9707b144eb5f82d08384eb066c0fd
Gecko     https://hg.mozilla.org/mozilla-central/rev/c8055a00235d
BuildID   20140424040201
Version   31.0a1
ro.build.version.incremental=eng.cuizhaohui.20140410.144352
ro.build.date=Thu Apr 10 14:45:37 CST 2014

Comment 5

[Eric Kramer [:Kramer]]

Added URL for video.

Comment 6

[Eric Kramer [:Kramer]]

Removed [Open_C] from the title due to this issue affecting Buri as well.

Comment 7

[J Zimbrick]

Note: This window is using non-engineering builds, as we are blocked from finding a regression window with engineering builds by bug 1000617, so the pushlog is larger than normal windows. 

b2g-inbound Regression Window:

Last Working Environmental Variables:
Device: Buri
BuildID: 20140424023002
Gaia: 4a4a078768423426c7c0fc580c8642bc735033a5
Gecko: 91946180b1e7
Version: 31.0a1
Base Image: v1.2-device.cfg

First Broken Environmental Variables:
Device: Buri
BuildID: 20140424053001
Gaia: 4b6c9d4929c57005ab142e8eb3dc6783d55c427c
Gecko: c30df002e626
Version: 31.0a1
Base Image: v1.2-device.cfg

Last Working  Gaia / First Broken Gecko: Issue DOES occur.
Gaia: 4a4a078768423426c7c0fc580c8642bc735033a5
Gecko: c30df002e626

First Broken Gaia / Last Working Gecko: Issue does NOT occur.
Gaia: 4b6c9d4929c57005ab142e8eb3dc6783d55c427c
Gecko: 91946180b1e7

Gecko Pushlog: http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=91946180b1e7&tochange=c30df002e626

Comment 8

[Jason Smith [:jsmith]]

bug 993286 looks very suspicious for causing this regression.

Ben - Can you confirm? If so, can you backout?

Comment 9

[Ben Tian (inactive)]

Bug 993286 only touches files for bluedroid, which are not built on Buri (BlueZ). Let me check this problem first and bring more info later.

Comment 10

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

Crash reason:  SIGSEGV
Crash address: 0x0

Thread 4 (crashed)
 0  libxul.so!mozilla::ipc::DBusReplyHandler::Release() [DBusUtils.h : 64 + 0x4]
     r4 = 0x4408f380    r5 = 0x40331080    r6 = 0x437ffd60    r7 = 0x437ffdf0
     r8 = 0x437ffd58    r9 = 0x437ffd88   r10 = 0x00000000    fp = 0x00000000
     sp = 0x437ffd10    lr = 0x4088beb3    pc = 0x40f98de0
    Found by: given as instruction pointer in context
 1  libxul.so!ProcessRemainingDeviceAddressesTask::~ProcessRemainingDeviceAddressesTask [nsAutoPtr.h : 894 + 0x3]
     r4 = 0x44018e90    r5 = 0x44018e90    r6 = 0x437ffd60    r7 = 0x437ffdf0
     r8 = 0x437ffd58    r9 = 0x437ffd88   r10 = 0x00000000    fp = 0x00000000
     sp = 0x437ffd28    pc = 0x412ccb6f
    Found by: call frame info
 2  libxul.so!ProcessRemainingDeviceAddressesTask::~ProcessRemainingDeviceAddressesTask [BluetoothDBusService.cpp:3b166b8add93 : 2722 + 0x5]
     r4 = 0x44018e90    r5 = 0x44018e90    r6 = 0x437ffd60    r7 = 0x437ffdf0
     r8 = 0x437ffd58    r9 = 0x437ffd88   r10 = 0x00000000    fp = 0x00000000
     sp = 0x437ffd30    pc = 0x412ccb91
    Found by: call frame info
 3  libxul.so!MessageLoop::RunTask(Task*) [message_loop.cc:3b166b8add93 : 358 + 0x7]
     r4 = 0x437ffde4    r5 = 0x44018e90    r6 = 0x437ffd60    r7 = 0x437ffdf0
     r8 = 0x437ffd58    r9 = 0x437ffd88   r10 = 0x00000000    fp = 0x00000000
     sp = 0x437ffd38    pc = 0x40d5e16d
    Found by: call frame info
 4  libxul.so!MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) [message_loop.cc:3b166b8add93 : 365 + 0x5]
     r4 = 0x00000001    r5 = 0x437ffd50    r6 = 0x437ffd60    r7 = 0x437ffdf0
     r8 = 0x437ffd58    r9 = 0x437ffd88   r10 = 0x00000000    fp = 0x00000000
     sp = 0x437ffd48    pc = 0x40d5eec7
    Found by: call frame info

Comment 11

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

Attachment: stack trace

Comment 12

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

Attachment: 7fc5d895-639a-8d55-3c45f19f-74c0c4fe.dmp

Comment 13

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

gaia="73b5d6a8773aa048054119bf5b3ca0d005b5494e"
gecko="154dafc7f594b4845466d3c68f3b116536949eef"

Comment 14

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

Add Thomas for checking ipc part.
Hello Thomas,
Can you help to check in parallel? This bug seems to be consistently happened.

Comment 15

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

Oh, recent change on DBusReplyHandler is Bug 999884 - Make DBusReplyHandler not use mozilla::RefCounted; r=smaug.

Comment 16

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

I revert patch of Bug 999884 (http://hg.mozilla.org/mozilla-central/rev/c81b25ae61c0). No longer crash. Please backout the patch of bug 999884.

Comment 17

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

Hi Shawn

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #14)
> Add Thomas for checking ipc part.
> Hello Thomas,
> Can you help to check in parallel? This bug seems to be consistently
> happened.

I think this might again be caused by non-threadsafe ref-counting. The DBus reply handler gets allocated on the main thread near BluetoothDBusService.cpp:2775 and free'd on the I/O thread near BluetoothDBusService.cpp:2731.

Can you replace NS_INLINE_DECL_REFCOUNTING at DBusUtils.h:64 by NS_INLINE_DECL_THREADSAFE_REFCOUNTING and test again?

Comment 18

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #17)
> Hi Shawn
> 
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #14)
> > Add Thomas for checking ipc part.
> > Hello Thomas,
> > Can you help to check in parallel? This bug seems to be consistently
> > happened.
> 
> I think this might again be caused by non-threadsafe ref-counting. The DBus
> reply handler gets allocated on the main thread near
> BluetoothDBusService.cpp:2775 and free'd on the I/O thread near
> BluetoothDBusService.cpp:2731.
> 
> Can you replace NS_INLINE_DECL_REFCOUNTING at DBusUtils.h:64 by
> NS_INLINE_DECL_THREADSAFE_REFCOUNTING and test again?

You are right. Changing to threadsafe version can fix it.

Comment 19

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

Attachment: Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

How odd. Bug 999884 didn't change refcounting from threadsafe to non-threadsafe.
Sounds like we need some fix to all the branches using DBusReplyHandler.

Comment 21

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

Comment on attachment 8412459 [details] [diff] [review]
Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting

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

Thanks.

Comment 22

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

Attachment: Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting, r=echou, f=tzimmermann

Comment 23

[(no longer active)]

Non-threadsafe refcounting bugs are potentially security sensitive, please request sec-approval before landing.

Comment 24

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #20)
> How odd. Bug 999884 didn't change refcounting from threadsafe to
> non-threadsafe.
> Sounds like we need some fix to all the branches using DBusReplyHandler.

It isn't that odd, one of the main reasons Ehsan is converting things away from RefCounted is that it has no threadsafety assertions.

Comment 25

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #20)
> How odd. Bug 999884 didn't change refcounting from threadsafe to
> non-threadsafe.

Yeah this bug was there before my patch, it was just masked.

> Sounds like we need some fix to all the branches using DBusReplyHandler.

Indeed.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] from comment #24)
> (In reply to Olli Pettay [:smaug] from comment #20)
> > How odd. Bug 999884 didn't change refcounting from threadsafe to
> > non-threadsafe.
> > Sounds like we need some fix to all the branches using DBusReplyHandler.
> 
> It isn't that odd,
It is still a bit odd. We don't crash when using non-threadsafe RefCounted but do crash with
non-threadsafe xpcom refcounting.
Object layout ends up being a bit different so perhaps that is enough to mask the issue
in RefCounted case.

Comment 27

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #26)
> (In reply to Andrew McCreight [:mccr8] from comment #24)
> > (In reply to Olli Pettay [:smaug] from comment #20)
> > > How odd. Bug 999884 didn't change refcounting from threadsafe to
> > > non-threadsafe.
> > > Sounds like we need some fix to all the branches using DBusReplyHandler.
> > 
> > It isn't that odd,
> It is still a bit odd. We don't crash when using non-threadsafe RefCounted
> but do crash with
> non-threadsafe xpcom refcounting.

The "crash" is just us hitting the MOZ_CRASH which checks whether you're on the right thread when Release-ing the object.  Note that thread-safety assertions are on on non-debug builds on Nightly: <http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#46>

> Object layout ends up being a bit different so perhaps that is enough to
> mask the issue
> in RefCounted case.

No, the thing that masks the issue on RefCounted is that it lacks any thread safety assertions whatsoever, so you would race with other threads while manipulating the refcount without anyone catching you, and if you lose the race you will end up with a use after free.

Comment 28

[(no longer active)]

Shawn, have you verified that this object is thread-safe in places other than the refcounting?

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #27)
> The "crash" is just us hitting the MOZ_CRASH which checks whether you're on
> the right thread when Release-ing the object.  Note that thread-safety
> assertions are on on non-debug builds on Nightly:
> <http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.
> h#46>
Aha! we have that MOZ_CRASH enabled in b2g nighties, but not in Firefox nighties because the check is just
too slow and disturbs profiling FF nightlies. B2G stuff needs special build anyway for proper profiling.

Comment 30

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

I cannot add :echou, :tzimmermann in cc list. Can anyone help?

Comment 31

[Andrew McCreight [:mccr8]]

It looks to me like they are already in the CC list for this bug.

Comment 32

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

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #28)
> Shawn, have you verified that this object is thread-safe in places other
> than the refcounting?

I have checked the use of all objects which are instances of DBusReplyHandler or its subclass such as BluetoothArrayOfDevicePropertiesReplyHandler. All of those are not used outside IO thread.

To be more careful, ni? Thomas, who has revised this part of code for many times, to confirm.

Comment 33

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

Comment on attachment 8412477 [details] [diff] [review]
Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting, r=echou, f=tzimmermann

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Hit use-after-delete when turning on bluetooth due to non-thread safe ref counting.

Which older supported branches are affected by this flaw?
Only master branch.
If not all supported branches, which bug introduced the flaw?
Bug 999884 reveals the potential non-thread safe ref counting problem.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This only happens on master branch.
How likely is this patch to cause regressions; how much testing does it need?
No regression, just change to xpcom thread-safe ref-counting.

Comment 34

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

> To be more careful, ni? Thomas, who has revised this part of code for many
> times, to confirm.

The overall code should be thread safe. The reason why this bug exists is that I somehow assumed ref-counting to be thread-safe as well.

Almost all calls to libdbus are on the I/O thread and calls to libdbus acquire an internal lock, so you cannot run two of them concurrently.

For our data structures, I spend a lot of time to make sure that objects are only accessed on a single thread, or are owned by only a single thread at a time. The exception are places like the one we just fixed, where ownership changes. All data structures are build upon DBusReplyHandler, so they should all get fixed by the attached patch.

Comment 35

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

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> 
> Hit use-after-delete when turning on bluetooth due to non-thread safe ref
> counting.
> 
> Which older supported branches are affected by this flaw?

We only see this on master, but the bug was introduced by bug 884840, which landed ~9 month ago. All active branches should be affected.

> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?

Trivial.

Comment 36

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8412477 [details] [diff] [review]
Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting, r=echou, f=tzimmermann

sec-approval+ for trunk.

Comment 37

[Ben Tian (inactive)]

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

Comment 38

[Ed Morley [:emorley]]

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

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this patch.

Comment 40

[Natalya Kot [:nkot]]

Verified Fixed on the latest trunk - enabling BT does not cause the crash.

Open_C master m-c
BuildID: 20140430040206
Gaia: badc73ee7f108fa631150bded0cc57e92aad810e
Gecko: e19812f56952
Version: 32.0a1
P821A10-ENG_20140410

Comment 41

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #35)
> > [Security approval request comment]
> > How easily could an exploit be constructed based on the patch?
> > 
> > Hit use-after-delete when turning on bluetooth due to non-thread safe ref
> > counting.
> > 
> > Which older supported branches are affected by this flaw?
> 
> We only see this on master, but the bug was introduced by bug 884840, which
> landed ~9 month ago. All active branches should be affected.
> 
Hi Thomas,
Based on Comment 27, Refcounted is not thread-safe. I think we need to land patch of bug 999884 first and land the patch (attachment 8412477 [details] [diff] [review]) of bug 1000961. Because DBusReplyHandler still uses Refcounted class on other branches, right?

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Otherwise, can we just roll that change into a branch patch here?

Comment 43

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

> Based on Comment 27, Refcounted is not thread-safe. I think we need to land
> patch of bug 999884 first and land the patch (attachment 8412477 [details] [diff] [review]
> [diff] [review]) of bug 1000961. Because DBusReplyHandler still uses
> Refcounted class on other branches, right?

Inheriting from |AtomicRefCounted| instead of |RefCounted| should work.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

So IIUC, we can land the trunk patch on Aurora (since bug 999884 landed there already), and we'll want a Beta patch with comment 43 addressed. And barring any unforeseen issues, the Beta patch should uplift to the other B2G release branches per the usual security uplift policy.

Shawn, you'll still need to do the Aurora/Beta approvals when ready.

Comment 45

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

Attachment: Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting (v1.3/1.4)

Comment 46

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

I've verified with v.1.3 beta patch (Attachment #8417790 [details] [diff]) on debug build, and did some basic tests, looks fine.

Comment 47

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

Attachment: Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting (v1.3/1.4), r=echou

Comment 48

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

Attachment: Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting (v1.3/1.4), r=echou

Comment 49

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

Attachment: Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting (v1.3/1.4)

Comment 50

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

Attachment: Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting, r=echou

Comment 51

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

Comment on attachment 8418535 [details] [diff] [review]
Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting, r=echou

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 884840
User impact if declined:
Non-thread safe ref counting could cause crash
Testing completed (on m-c, etc.): on 1.3
Risk to taking this patch (and alternatives if risky): No risk
String or IDL/UUID changes made by this patch: Nothing changed

Comment 52

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

Comment on attachment 8418535 [details] [diff] [review]
Bug 1000961 - Make DBusReplyHandler use thread-safe ref-counting, r=echou

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 884840
User impact if declined:
Non-thread safe ref counting could cause crash
Testing completed (on m-c, etc.): on 1.3
Risk to taking this patch (and alternatives if risky): No risk
String or IDL/UUID changes made by this patch: Nothing changed

Comment 53

[Ryan VanderMeulen [:RyanVM]]

So we're clear here, per comment 44, I landed the trunk patch (attachment 8412477 [details] [diff] [review]) on Aurora and attachment 8418535 [details] [diff] [review] on all other branches. If that's not correct, scream now :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9378a757f74
https://hg.mozilla.org/releases/mozilla-beta/rev/306759b08bbf
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/c74289fa0407
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7a0be20beae7

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/306759b08bbf