308463 - remove nsPrescontext parameter from Destroy

Status: NEW

(Core :: Layout, defect)

Description

[Vidar Haarr (not reading bugmail)]

Initial patch in a minute.

Comment 1

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.1

Removes nsPresContext from Destroy methods, both nsIFrame and nsFrameList.

Comment 2

[Vidar Haarr (not reading bugmail)]

roc: Could you check in attachment 196028 [details] [diff] [review], please?

Comment 3

[Vidar Haarr (not reading bugmail)]

bz: Since roc is leaving for two weeks, and you're a layout man, could you help
me check in attachment 196028 [details] [diff] [review]?

Comment 4

[Vidar Haarr (not reading bugmail)]

Attachment: update for bitrot

Update for slight bitrot caused by fix from bug 306895.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So... This means that destroying a frame without initing it first will crash,
right?  It would be nice to document that very very clearly in nsIFrame.h.  Make
it very very clear that once you've allocated a frame you MUST init it before
you can deallocate it.

Also would be nice to know that you checked all nsIFrame::Destroy() calls and
made sure that they all happen on already-inited frames and that you've checked
that all nsIFrame::Init impls guarantee to set the style context (which is what
GetPresContext() uses).  Did you do that?

Comment 6

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

Probably a better idea to bulletproof nsFrame::Destroy by checking whether
mStyleContext is null.

Comment 7

[Boris Zbarsky [:bzbarsky]]

But then we never deallocate the memory to the arena...

Comment 8

[Boris Zbarsky [:bzbarsky]]

And more importantly, some of the subclass Destroy() impls need a prescontext.

Comment 9

[Vidar Haarr (not reading bugmail)]

(In reply to comment #5)
> So... This means that destroying a frame without initing it first will crash,
> right?

Yes. Even
<http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#638> needs a
pres context.

> Also would be nice to know that you checked all nsIFrame::Destroy() calls and
> made sure that they all happen on already-inited frames and that you've checked
> that all nsIFrame::Init impls guarantee to set the style context (which is what
> GetPresContext() uses).  Did you do that?

I've checked some, but I can't claim that I've checked all of them by far.

All nsIFrame::Init are certain, as far as I can see, to set the style context
unless something terrible occurs, like we're out of memory. Most frames call
their superclass::Init immediately and do their local magic afterwards. There
are exceptions, however, but I can't remember seeing any that force a return
before superclass::Init.

That said, I haven't really thought about it much since I made the initial
patch, so I could have another look. However, as for creating new frames, I'm
not sure how I can find all places where we do - any tips?

Anyway, you probably know this better than me. Are there specific cases where we
will fail that you know of?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Creating frames is done mostly in nsCSSFrameConstructor, but some frame impls do
it too... I'm starting to think that perhaps the operator new for a frame should
just take and set the style context.  That would make sure we can't screw this up.

And no, I don't know specific cases that would fail.  SVG has been known to
screw this up in the past (eg by not calling nsFrame::Init at all), but I think
we've fixed that, mostly...

Comment 11

[Vidar Haarr (not reading bugmail)]

(In reply to comment #10)
> Creating frames is done mostly in nsCSSFrameConstructor, but some frame impls do
> it too... I'm starting to think that perhaps the operator new for a frame should
> just take and set the style context.  That would make sure we can't screw this up.

Want me to try and make a patch for this, maybe in a seperate bug?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I don't know; I haven't thought the idea through enough.  Might be worth finding
out what roc and dbaron think about it.

Comment 13

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

It sounds reasonable to me.

Comment 14

[Vidar Haarr (not reading bugmail)]

So the new signature for the nsFrame::new operator would be:
|void* operator new(size_t sz, nsStyleContext* aStyleContext) CPP_THROW_NEW;|

and the new signature for the NS_New*Frame methods would be:
|nsIFrame* NS_New*Frame(nsStyleContext* aStyleContext [, ...]);|

Most NS_NewFrame's don't have any "custom"/additional arguments, but some do. The point is that they all take a PresShell now, but that can be obtained from nsStyleContext->PresContext()->PresShell(), so do we need to pass it around?

We could also probably change the nsIFrame::SetStyleContext signature from
|void SetStyleContext(nsPresContext* aPresContext, nsStyleContext* aContext);|
to
|void SetStyleContext(nsStyleContext* aContext);|

dbaron: Maybe you have some thoughts on the whole idea?

Comment 15

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

I think it's probably best to minimize the number of parameters, i.e., just pass the stylecontext to NS_New*Frame

Comment 16

[Boris Zbarsky [:bzbarsky]]

Note that if we make this change we should be a little careful with SVG, which does weird things on setting the style context (including in Init()).

Comment 17

[Vidar Haarr (not reading bugmail)]

X-ref bug 328926.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.