1098398 - b2g memleak during stability testing after 48 hours

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect, P1)

Description

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: unified-memory-report-1415793153-234.json

Testcase:

1) Run gaiatest for call, camcorder, camera, sms, FM radio, video, music for 72 hours and we are seeing b2g memory growth of 30MB at some point although we are not running any apps.

I have captured a memory report when memory growth happens

Comment 1

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: dmd stack traces of memory growth

please note that we used v2.0 FFOS build for this testing.

Comment 2

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

[Blocking Requested - why for this release]:

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

schien and I looked at this today.  It appears that bluetooth has a static array of uuids[0] that is only appended to.  Presumably the testcase changes bluetooth settings a lot and results in us adding a lot of entries to this array.  This code was rewritten in bug 1048915 for 2.1 which appears to have fixed the bug, but that patch is likely to big to backport.

[0] http://mxr.mozilla.org/mozilla-b2g32_v2_0/source/dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp#59

Comment 4

[bhavana bajaj [:bajaj]]

NI thomas to help start investigation here given it may be related to Bluetooth.

Comment 5

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

Unfortunately, it seems to be a regression of Bug 1059136, which tries to fix MCTS API test case failures (bug 1058715). It's not relate to bug 1048915.

Due to v2.1 in the middle of transition for new bluetooth v2 API, Adapter Uuid property won't be exposed to certified app anymore, so v2.1 does not pick up this patch. However, later new API defers to v2.2, so this is why v2.1 is unaffected.

Comment 6

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

Attachment: bug1098398-v2.0.patch

Comment 7

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

Attachment: Bug 1098398 - Clear adapter uuids array to avoid memory leakage

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8522726 [details] [diff] [review]
Bug 1098398 - Clear adapter uuids array to avoid memory leakage

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

f=me with nit addressed.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +392,5 @@
>  
>        propertyValue = sAdapterBondedAddressArray;
>        BT_APPEND_NAMED_VALUE(props, "Devices", propertyValue);
>      } else if (p.type == BT_PROPERTY_UUIDS) {
> +      sAdapterUuidsArray.Clear();

nit: newline here.

Comment 9

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

Attachment: Bug 1098398 - Clear adapter uuids array to avoid memory leakage

Comment 10

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

Comment on attachment 8522735 [details] [diff] [review]
Bug 1098398 - Clear adapter uuids array to avoid memory leakage

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

Thank you.

2.1 and later are not affected AFAICT.

Comment 11

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

Comment on attachment 8522735 [details] [diff] [review]
Bug 1098398 - Clear adapter uuids array to avoid memory leakage

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):A regression of Bug 1059136, which tries to fix MCTS API test case failures (bug 1058715)
User impact if declined: Memory leakage once happen when switching off and on Bluetooth
Testing completed: Clear UUID array
Risk to taking this patch (and alternatives if risky): none, it only clear static UUID array which should be cleaned up 
String or UUID changes made by this patch:none

Comment 12

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

Attachment: Bug 1098398 - Clear adapter uuids array to avoid memory leakage, r=tzimmermann, f=btian

Comment 13

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

Comment on attachment 8522853 [details] [diff] [review]
Bug 1098398 - Clear adapter uuids array to avoid memory leakage, r=tzimmermann, f=btian

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):A regression of Bug 1059136, which tries to fix MCTS API test case failures (bug 1058715)
User impact if declined: Memory leakage once happen when switching off and on Bluetooth
Testing completed: Clear UUID array
Risk to taking this patch (and alternatives if risky): none, it only clear static UUID array which should be cleaned up 
String or UUID changes made by this patch:none

Comment 14

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #13)
> Comment on attachment 8522853 [details] [diff] [review]
> Bug 1098398 - Clear adapter uuids array to avoid memory leakage,
> r=tzimmermann, f=btian
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #):A regression of Bug 1059136, which
> tries to fix MCTS API test case failures (bug 1058715)
> User impact if declined: Memory leakage once happen when switching off and
> on Bluetooth
> Testing completed: Clear UUID array
> Risk to taking this patch (and alternatives if risky): none, it only clear
> static UUID array which should be cleaned up 
> String or UUID changes made by this patch:none

we restarted running automation with these changes and I will update asap.

Comment 15

[bhavana bajaj [:bajaj]]

Eill approve for branch uplift once this is on m-c or we have feedback from CAF.

Comment 16

[Nicholas Troast [:ntroast]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #13)
> Comment on attachment 8522853 [details] [diff] [review]
> Bug 1098398 - Clear adapter uuids array to avoid memory leakage,
> r=tzimmermann, f=btian
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #):A regression of Bug 1059136, which
> tries to fix MCTS API test case failures (bug 1058715)
> User impact if declined: Memory leakage once happen when switching off and
> on Bluetooth
> Testing completed: Clear UUID array
> Risk to taking this patch (and alternatives if risky): none, it only clear
> static UUID array which should be cleaned up 
> String or UUID changes made by this patch:none

This patch is causing gecko to crash continuously during boot. Can you please verify the patch?

Comment 17

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

I'm not sure why, but Bug 989976 seems to be hit after applying array clean-up. I saw SIGBUS error.

Comment 18

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

We probably need to uplift the patch set of Bug 989976 and also apply patch of this bug. Although they look irrelevant.

Comment 19

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

Attachment: Bug 1098398 - Clear adapter uuids array to avoid memory leakage, r=tzimmermann, f=btian

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):A regression of Bug 1059136, which tries to fix MCTS API test case failures (bug 1058715)
User impact if declined: Memory leakage once happen when switching off and on Bluetooth
Testing completed: Clear UUID array
Risk to taking this patch (and alternatives if risky): none, it only clear static UUID array which should be cleaned up 
String or UUID changes made by this patch:none

Comment 20

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

I've merged the Bug 989976 patch into this patch. Can you run again? Thanks.

Comment 21

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #20)
> I've merged the Bug 989976 patch into this patch. Can you run again? Thanks.

We are not seeing this issue in 24 hour testing but we are hitting some other crashes after 24 hours. Please land this patch if you are "OK" with it. Thanks a lot for your help :)

Comment 22

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

(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #21)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #20)
> > I've merged the Bug 989976 patch into this patch. Can you run again? Thanks.
> 
> We are not seeing this issue in 24 hour testing but we are hitting some
> other crashes after 24 hours. Please land this patch if you are "OK" with
> it. Thanks a lot for your help :)

I'm ok with it. Thanks for your information.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/34f9d6d2a5ac

If you want this on b2g30 for v1.4 as well, it'll need a separate approval request.

Comment 24

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

Shall we uplift to v1.4?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/34f9d6d2a5ac

Comment 26

[bhavana bajaj [:bajaj]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #24)
> Shall we uplift to v1.4?

shawn/ryan, if the patch is low risk enough and is not much work to backport please land this with a=bajaj on 1.4