The default bug view has changed. See this FAQ.

change nsDeque to use malloc for mData

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
XPCOM
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

Trunk
mozilla1.9.3a1
x86
Windows XP
Points:
---

Firefox Tracking Flags

(status1.9.2 .2-fixed, status1.9.1 .9-fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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
Attachment #347291 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

9 years ago
No longer blocks: 457483
(Assignee)

Updated

9 years ago
Blocks: 457483
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 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.
Attachment #347291 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 3

8 years ago
smaug: waiting for answers to questions fails. dbaron did some stuff in bug  520661
(Assignee)

Comment 4

8 years ago
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.
Attachment #347291 - Attachment is obsolete: true
Attachment #411698 - Flags: review?(Olli.Pettay)
Comment on attachment 411698 [details] [diff] [review]
just the malloc changes

Benjamin is an XPCOM peer.
Attachment #411698 - Flags: review?(Olli.Pettay) → review?(benjamin)
...but the patch looks ok to me :)
Attachment #411698 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/586c7fa4491f
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Updated

7 years ago
Duplicate of this bug: 549546
(Assignee)

Updated

7 years ago
Attachment #411698 - Flags: approval1.9.2.2?
Attachment #411698 - Flags: approval1.9.1.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.

/be
(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.)
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?
(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 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.
Attachment #411698 - Flags: approval1.9.2.2?
Attachment #411698 - Flags: approval1.9.2.2+
Attachment #411698 - Flags: approval1.9.1.9?
Attachment #411698 - Flags: approval1.9.1.9+
(Assignee)

Comment 14

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/26d330339592
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a0da93ff265d
status1.9.1: --- → .9-fixed
status1.9.2: --- → .2-fixed
Is there anything for QA to do here? It doesn't look like it.
You need to log in before you can comment on or make changes to this bug.