352335 - unchecked allocs in nsFrameSetFrame and uselessly checked placement new's

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[timeless]

i didn't use klocwork or coverity to find this. i don't care if they could have told me about any of these either :).

Comment 1

[Mats Palmgren (inactive)]

The checks after "placement new" aren't useless, in most cases they can fail
just like a "normal new". E.g. the frame allocation ends up here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsFrame.cpp&rev=3.673&root=/cvsroot&mark=455-465#452

Although there can be cases where a buffer have been pre-allocated,
but IMO callers shouldn't pretend they have knowledge of such cases.
It certainly makes auditing easier if one always assume it can fail.

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Here's a fix for the other cases...

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #1)
> The checks after "placement new" aren't useless, in most cases they can fail
> just like a "normal new". E.g. the frame allocation ends up here:
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsFrame.cpp&rev=3.673&root=/cvsroot&mark=455-465#452

That one's not placement new. That's a custom operator new that takes an argument.

Comment 4

[Mats Palmgren (inactive)]

(In reply to comment #3)
> That one's not placement new. That's a custom operator new that takes an
> argument.

Sure, but it's the one the compiler invokes when using "placement new"
syntax. The "placement new" term is widely used when discussing both the
call syntax and the method that implements it (comp.lang.c++.moderated,
comp.std.c++). The ISO C++ Standard itself uses "placement new expression"
and "placement deallocation function" for example.

Sorry, but I will continue to use "placement new", the term 
"custom operator new that takes an argument" is clunky and I suspect
should I use it people will probably just say "oh, you mean placement new?".

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can't we just fix all these to use nsAutoArrayPtr?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I mean we'd still need the checks, but we wouldn't need these manual delete[]s.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

(In reply to comment #5)
> Can't we just fix all these to use nsAutoArrayPtr?

Indeed much better, thanks.

I also found two more unchecked 'new' calls - during first reflow we
create nsHTMLFramesetBorderFrames between the child frames, this one is
a bit tricky since Reflow() will be called again with missing frames.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsFrameSetFrame.cpp&rev=3.185&mark=1065,1069,1097,1101#1048

I chose to just do a null check and skip the border (leaving a gap
instead). I tested this by forcing 1 of 4 border frame allocations
to fail and it seems to be robust. Lookups of these frames goes
through mHorBorders/mVerBorders and are always null checked AFAICT.
We could do something more fancy, like trying to allocate the missing
border frames in every reflow, but I'd rather see bug 3655 resolved before
rewriting this code to much.

FWIW, I have reviewed all memory handling in this file and it seems
correct with respect to OOM and leaks now.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -w)

Comment 9

[Mats Palmgren (inactive)]

Checked in to trunk at 2006-10-07 01:53 PDT.

->FIXED