Closed Bug 814341 Opened 9 years ago Closed 9 years ago

[BLUETOOTH] Sending large file would crash the whole system

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

VERIFIED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: wachen, Assigned: echou)

Details

(Keywords: crash, reproducible)

Attachments

(2 files)

*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
not pretty reproducable. might be my issue, temp mark as resolved invalid until I know more info and get logcat
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
Attached file log
still crashed, attached logcat
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
blocking-basecamp: --- → ?
Keywords: crash
Assignee: nobody → echou
blocking-basecamp: ? → +
Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.
Priority: -- → P1
Keywords: reproducible
Target Milestone: --- → B2G C3 (12dec-1jan)
* Bluetooth module eats too much memory while receiving a file because there are memory leaks. This patch will fix it.
Attachment #691284 - Flags: review?(gyeh)
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!
Attachment #691284 - Flags: review?(gyeh) → review+
https://hg.mozilla.org/mozilla-central/rev/1be998d20007
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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".
(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
follow-up: bug 823427
verified fixed in 2012/12/24 build
Status: RESOLVED → VERIFIED
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
You need to log in before you can comment on or make changes to this bug.