814341 - [BLUETOOTH] Sending large file would crash the whole system

Status: VERIFIED FIXED

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

Description

[Walter Chen[:ypwalter][:wachen]]

*Environment:
Phone - Unagi
https://releases.mozilla.com/b2g/
2012-11-21 build

*How to reproduce:
  1. Connect cell phone to a computer from settings/bluetooth
  2. Choose to send a large file(>100MB) from PC
  3. accept it from phone

*Expected Result: 
   Either detect that you should not send large file,
   or do not crash 

*Actual Result:
   After awhile, it crashed (reboot or freeze), pc shows "Timed out waiting for response" in ubuntu 12.04

Comment 1

[Walter Chen[:ypwalter][:wachen]]

not pretty reproducable. might be my issue, temp mark as resolved invalid until I know more info and get logcat

Comment 2

[Walter Chen[:ypwalter][:wachen]]

Attachment: log

still crashed, attached logcat

Comment 3

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 4

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

Attachment: patch 1: v1: fixed memory leak in Obex/OPP implementation

* Bluetooth module eats too much memory while receiving a file because there are memory leaks. This patch will fix it.

Comment 5

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 691284 [details] [diff] [review]
patch 1: v1: fixed memory leak in Obex/OPP implementation

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

Looks good, r=me. Thanks!

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1be998d20007

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2af0ee41e9da
https://hg.mozilla.org/releases/mozilla-b2g18/rev/826b3a823b0b

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 691284 [details] [diff] [review]
patch 1: v1: fixed memory leak in Obex/OPP implementation

>+++ b/dom/bluetooth/ObexBase.cpp
>@@ -118,6 +118,7 @@ ParseHeaders(const uint8_t* aHeaderStart,
>     uint8_t* content = new uint8_t[contentLength];
>     memcpy(content, ptr, contentLength);
>     aRetHandlerSet->AddHeader(new ObexHeader(headerId, contentLength, content));
>+    delete [] content;
> 
>     ptr += contentLength;
>   }

Two notes on this:
 (a) ideally, |content| should be a nsAutoArrayPtr<uint8_t>, so we don't have to worry about deleting it -- that'll automatically happen when it goes out of scope.
But...
 (b) Why do we have |content| there at all?  It looks like the "new ObexHeader()" constructor just memcpy's from the pointer that's passed to it, so it looks like we could just directly pass in |ptr| instead of doing an extra memcpy to the intermediate buffer "content".

Comment 9

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

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Comment on attachment 691284 [details] [diff] [review]
> patch 1: v1: fixed memory leak in Obex/OPP implementation
> 
> >+++ b/dom/bluetooth/ObexBase.cpp
> >@@ -118,6 +118,7 @@ ParseHeaders(const uint8_t* aHeaderStart,
> >     uint8_t* content = new uint8_t[contentLength];
> >     memcpy(content, ptr, contentLength);
> >     aRetHandlerSet->AddHeader(new ObexHeader(headerId, contentLength, content));
> >+    delete [] content;
> > 
> >     ptr += contentLength;
> >   }
> 
> Two notes on this:
>  (a) ideally, |content| should be a nsAutoArrayPtr<uint8_t>, so we don't
> have to worry about deleting it -- that'll automatically happen when it goes
> out of scope.
> But...
>  (b) Why do we have |content| there at all?  It looks like the "new
> ObexHeader()" constructor just memcpy's from the pointer that's passed to
> it, so it looks like we could just directly pass in |ptr| instead of doing
> an extra memcpy to the intermediate buffer "content".

Hi Daniel,

You're right. The buffer "content" is not necessary. I'm going to file a 
bug for this. Thanks for reminding.

Eric

Comment 10

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

follow-up: bug 823427

Comment 11

[Walter Chen[:ypwalter][:wachen]]

verified fixed in 2012/12/24 build