Last Comment Bug 464043 - change nsDeque to use malloc for mData
: change nsDeque to use malloc for mData
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla1.9.3a1
Assigned To: timeless
:
Mentors:
: 549546 (view as bug list)
Depends on:
Blocks: 457483
  Show dependency treegraph
 
Reported: 2008-11-10 08:31 PST by timeless
Modified: 2010-03-22 15:14 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2-fixed
.9-fixed


Attachments
stage one - don't crash (2.00 KB, patch)
2008-11-10 08:31 PST, timeless
no flags Details | Diff | Review
just the malloc changes (895 bytes, patch)
2009-11-11 09:21 PST, timeless
benjamin: review+
mbeltzner: approval1.9.2.2+
mbeltzner: approval1.9.1.9+
Details | Diff | Review

Description timeless 2008-11-10 08:31:32 PST
Created attachment 347291 [details] [diff] [review]
stage one - don't crash

bug 457483 comment 26 shows nsDeque fails under low memory.

I have other patches which would change Push(Front) to indicate failure to the callers. I'm working on it in a repo, I'd want to test that before I pushed it to a bug, but this is actually independent
Comment 1 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-11-10 12:03:05 PST
Comment on attachment 347291 [details] [diff] [review]
stage one - don't crash

>-  void** temp=new void*[theNewSize];
>+  void** temp=(void**)malloc(theNewSize * sizeof(void*));
This should use C++ casting, not C.

>+    PRInt32 capacity = mCapacity;
>+    if (GrowCapacity() == capacity) {
>+      NS_WARNING("No memory for element");
>+      return *this;
>+    }

>+    PRInt32 capacity = mCapacity;
>+    if (GrowCapacity() == capacity) {
>+      NS_WARNING("No memory for element");
>+      return *this;
>+    }
These are kind of useless if the caller doesn't know that the call has failed.
How is the other patch?
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-12-09 12:12:52 PST
Comment on attachment 347291 [details] [diff] [review]
stage one - don't crash

Waiting to get some answer to the question - clearing the review request for now.
Comment 3 timeless 2009-11-11 07:58:54 PST
smaug: waiting for answers to questions fails. dbaron did some stuff in bug  520661
Comment 4 timeless 2009-11-11 09:21:37 PST
Created attachment 411698 [details] [diff] [review]
just the malloc changes

smaug: please note that dbaron took in the warning bits that you complained about.

all that's left is the don't throw c++ exceptions bit. I have a real live customer who complained about that in bug 526843.
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-11-12 00:27:34 PST
Comment on attachment 411698 [details] [diff] [review]
just the malloc changes

Benjamin is an XPCOM peer.
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-11-12 00:27:54 PST
...but the patch looks ok to me :)
Comment 7 Phil Ringnalda (:philor) 2009-11-14 16:47:11 PST
http://hg.mozilla.org/mozilla-central/rev/586c7fa4491f
Comment 8 timeless 2010-03-01 22:20:00 PST
*** Bug 549546 has been marked as a duplicate of this bug. ***
Comment 9 Brendan Eich [:brendan] 2010-03-01 22:40:04 PST
Is it really the case that we have to stop using operator new to avoid C++ exceptions, which might nest into JS while the JS GC is running below the code that called new? I don't believe it.

There are lots of operator new uses in our code. nsDeque is hardly first, or last.

/be
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-03-01 22:48:53 PST
(In reply to comment #9)
> Is it really the case that we have to stop using operator new to avoid C++
> exceptions, which might nest into JS while the JS GC is running below the code
> that called new? I don't believe it.
> 

No.  After abort-on-OOM re-lands this week, std::bad_alloc will be gone forever.  (Initially it'll be replaced with abort(), which isn't much better on windows, but that's not the long-term solution.)
Comment 11 Bas Schouten (:bas.schouten) 2010-03-02 06:41:30 PST
Doesn't using non-throwing new make more sense than malloc(In reply to comment #9)
> Is it really the case that we have to stop using operator new to avoid C++
> exceptions, which might nest into JS while the JS GC is running below the code
> that called new? I don't believe it.
> 
> There are lots of operator new uses in our code. nsDeque is hardly first, or
> last.
Just as another idea on this, I guess using non-throwing new is also a possibility?
Comment 12 Brendan Eich [:brendan] 2010-03-02 09:49:48 PST
(In reply to comment #11)
> Doesn't using non-throwing new make more sense than malloc(In reply to comment
> #9)
> > Is it really the case that we have to stop using operator new to avoid C++
> > exceptions, which might nest into JS while the JS GC is running below the code
> > that called new? I don't believe it.
> > 
> > There are lots of operator new uses in our code. nsDeque is hardly first, or
> > last.
> Just as another idea on this, I guess using non-throwing new is also a
> possibility?

See bug 353144 and the brief newsgroup thread it spawned for back-story:

http://groups.google.com/group/mozilla.dev.platform/browse_frm/thread/650c48ab782d296/93fbbef5fc6525b5?lnk=gst&q=353144#93fbbef5fc6525b5

Chris Jones et al. have a better plan for mozilla-central. Timeless argues this particular patch is helpful on branches -- could be, drivers can decide.

What I hope we can avoid is trying to annotate all operator new uses in our codebase. The strong preference of developers is not to have yet another Mozilla-ism on top of standard (for some value of standard) C++.

/be
Comment 13 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-03 13:01:02 PST
Comment on attachment 411698 [details] [diff] [review]
just the malloc changes

a1919/1922=beltzner for this particular case, though I agree with Brendan's comments above.
Comment 15 [On PTO until 6/29] 2010-03-22 15:14:11 PDT
Is there anything for QA to do here? It doesn't look like it.

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