315321 - nsDirectionalFrame allocated on heap

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

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

nsDirectionalFrame is allocated on the heap. The obvious change to allocate it on the presshell causes crashes (see bug 315127). We need to figure out why.

Comment 1

[Boris Zbarsky [:bzbarsky]]

If nothing else, stuff like:

310     if (nsLayoutAtoms::directionalFrame == frameType) {
311       delete frame;
312       ++lineOffset;
313     }

in nsBidiPresUtils would do it, would it not?  That is, this should be calling frame->Destroy(), not deleting it....

Similar code appears at

780     if (nsLayoutAtoms::directionalFrame == frame->GetType()) {
781       delete frame;
782       ++aOffset;
783     }

in the same file.

Comment 2

[shutdown]

http://lxr.mozilla.org/seamonkey/source/layout/base/nsBidiPresUtils.cpp#254
254   nsIFrame*                frame = nsnull;
311       delete frame;

The type to be deleted here is |nsIFrame*|.
Since nsIFrame has no virtual destructor, the compiler
resolves the functions (dtor and operator delete) statically.
The compiler will not use |nsFrame::operator delete()| here.

Comment 3

[Marc Liddell]

Attachment: Call frame->Destroy() instead of delete

I caused this crash originally so here's my attempt at a patch.

This uses frame->Destroy as per comment #1. The testcase in bug 315127 no longer crashes and browsing around mozilla.org.il and ynet.co.il seem fine.

I wasn't sure who should be reviewing this.

Comment 4

[Bernd]

the patch fixes the issue in bug 315193

Comment 5

[Bernd]

*** Bug 315193 has been marked as a duplicate of this bug. ***

Comment 6

[Bernd]

patch checked in, Marc there is no automatic checkin when a patch gets reviewed. The system currently relies that people checkin themselfes or ask for checkin.