nsDirectionalFrame allocated on heap

RESOLVED FIXED

Status

()

Core
Layout: Text
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: roc, Assigned: mkaply)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

13 years ago
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

13 years ago
Created attachment 202085 [details] [diff] [review]
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.
Attachment #202085 - Flags: review?(roc)

Updated

13 years ago
Attachment #202085 - Flags: superreview+

Comment 4

13 years ago
the patch fixes the issue in bug 315193

Comment 5

13 years ago
*** Bug 315193 has been marked as a duplicate of this bug. ***
Attachment #202085 - Flags: review?(roc) → review+

Comment 6

13 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.