Last Comment Bug 802459 - OOM crash if attaching too large file for filelink
: OOM crash if attaching too large file for filelink
Status: RESOLVED FIXED
: crash
Product: Thunderbird
Classification: Client Software
Component: FileLink (show other bugs)
: 16 Branch
: x86 Windows 7
: -- critical (vote)
: Thunderbird 19.0
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-16 19:04 PDT by Makoto Kato [:m_kato]
Modified: 2012-10-31 00:01 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Patch v1 (3.22 KB, patch)
2012-10-25 11:57 PDT, Mike Conley (:mconley) - (Needinfo me!)
m_kato: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2012-10-16 19:04:53 PDT
I have fixed Gecko side by bug 801483 if nsIBufferedInputStream.init(<too large size) causes OOM crashes (ex. bp-f7294fd5-e5d4-4c5f-81ea-86b722121012).  So init(<too large size>) in Gecko 19 will throw NS_ERROR_OUT_OF_MEMORY instead of crash.

But, buffer length parameter of init should not use file size because this is buffer size for seeking.

the following code is incorrect usages of nsIBufferedInputStream.

/mail/components/cloudfile/nsUbuntuOne.js (View Hg log or Hg annotations)
    line 692 -- createInstance(Ci.nsIBufferedInputStream);
/mail/components/cloudfile/nsYouSendIt.js (View Hg log or Hg annotations)
    line 1074 -- .createInstance(Ci.nsIBufferedInputStream);
/mail/components/cloudfile/nsBox.js (View Hg log or Hg annotations)
    line 857 -- .createInstance(Ci.nsIBufferedInputStream);
Comment 1 Mike Conley (:mconley) - (Needinfo me!) 2012-10-18 13:12:52 PDT
(In reply to Makoto Kato from comment #0)
> I have fixed Gecko side by bug 801483 if nsIBufferedInputStream.init(<too
> large size) causes OOM crashes (ex.
> bp-f7294fd5-e5d4-4c5f-81ea-86b722121012).  So init(<too large size>) in
> Gecko 19 will throw NS_ERROR_OUT_OF_MEMORY instead of crash.
> 
> But, buffer length parameter of init should not use file size because this
> is buffer size for seeking.
> 
> the following code is incorrect usages of nsIBufferedInputStream.
> 
> /mail/components/cloudfile/nsUbuntuOne.js (View Hg log or Hg annotations)
>     line 692 -- createInstance(Ci.nsIBufferedInputStream);
> /mail/components/cloudfile/nsYouSendIt.js (View Hg log or Hg annotations)
>     line 1074 -- .createInstance(Ci.nsIBufferedInputStream);
> /mail/components/cloudfile/nsBox.js (View Hg log or Hg annotations)
>     line 857 -- .createInstance(Ci.nsIBufferedInputStream);

Hey Makoto,

Thanks for letting us know about this.

So what is an appropriate buffer size? I'm seeing 4096 being tossed around here: http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/test/unit/test_file_protocol.js#33

Would that be sufficient?

-Mike
Comment 2 Makoto Kato [:m_kato] 2012-10-24 22:22:33 PDT
(In reply to Mike Conley (:mconley) from comment #1)
> (In reply to Makoto Kato from comment #0)
> > I have fixed Gecko side by bug 801483 if nsIBufferedInputStream.init(<too
> > large size) causes OOM crashes (ex.
> > bp-f7294fd5-e5d4-4c5f-81ea-86b722121012).  So init(<too large size>) in
> > Gecko 19 will throw NS_ERROR_OUT_OF_MEMORY instead of crash.
> > 
> > But, buffer length parameter of init should not use file size because this
> > is buffer size for seeking.
> > 
> > the following code is incorrect usages of nsIBufferedInputStream.
> > 
> > /mail/components/cloudfile/nsUbuntuOne.js (View Hg log or Hg annotations)
> >     line 692 -- createInstance(Ci.nsIBufferedInputStream);
> > /mail/components/cloudfile/nsYouSendIt.js (View Hg log or Hg annotations)
> >     line 1074 -- .createInstance(Ci.nsIBufferedInputStream);
> > /mail/components/cloudfile/nsBox.js (View Hg log or Hg annotations)
> >     line 857 -- .createInstance(Ci.nsIBufferedInputStream);
> 
> Hey Makoto,
> 
> Thanks for letting us know about this.
> 
> So what is an appropriate buffer size? I'm seeing 4096 being tossed around
> here:
> http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/test/unit/
> test_file_protocol.js#33
> 
> Would that be sufficient?

Yes, sufficient. nsXMLHttpRequest uses same size, too.
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2012-10-25 11:57:43 PDT
Created attachment 675232 [details] [diff] [review]
Patch v1

Does this look alright?
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2012-10-26 07:22:34 PDT
Comment on attachment 675232 [details] [diff] [review]
Patch v1

This is a cheap win - do we want it for Aurora and Beta?
Comment 5 Mark Banner (:standard8) (afk until 26th July) 2012-10-29 02:17:52 PDT
Comment on attachment 675232 [details] [diff] [review]
Patch v1

Definitely. a=me.
Comment 6 Mark Banner (:standard8) (afk until 26th July) 2012-10-29 02:41:38 PDT
This was checked in last week:

https://hg.mozilla.org/comm-central/rev/70e854549186
Comment 8 Wayne Mery (:wsmwk, NI for questions) 2012-10-30 23:35:06 PDT
what might one expect to see in crash signature for this OOM crash?

Note You need to log in before you can comment on or make changes to this bug.