464043 - change nsDeque to use malloc for mData

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[timeless]

Attachment: 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][[e-mail]]]

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][[e-mail]]]

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]

smaug: waiting for answers to questions fails. dbaron did some stuff in bug  520661

Comment 4

[timeless]

Attachment: 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][[e-mail]]]

Comment on attachment 411698 [details] [diff] [review]
just the malloc changes

Benjamin is an XPCOM peer.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

...but the patch looks ok to me :)

Comment 7

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/586c7fa4491f

Comment 9

[Brendan Eich [:brendan]]

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]

(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)]

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]]

(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]]

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 14

[timeless]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/26d330339592
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a0da93ff265d

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Is there anything for QA to do here? It doesn't look like it.