265367 - [FIX]Frame construction tries to construct kids of leaf frames

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Testcase coming up.  In short, dynamic DOM changes confuse the frame constructor
and cause it to possibly lose frames, which can lead to crashes (a la bug 263846).

I propose to add an IsLeaf() method on nsIFrame which will return PR_TRUE,
override on container frames of various sorts to return PR_FALSE, and make use
of it in the frame constructor.  Seem reasonable?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

This asserts:

###!!! ASSERTION: not a container: 'PR_FALSE', file
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/base/src/nsFrame.cpp, line 597

which effectively leaks the new image frame past page destruction, which is
exactly how bug 263846 arises.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I guess I'll take the silence as tacit approval... ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note to self: there's some code in ContentInserted that checks for object (and
col, possibly) frames.  That can probably go away, with some other col frame
checking, when I do this.

Comment 4

[Bernd]

What we currently have in ContentAppended and ContentInsert is a protection
against children of object and col frames, a foreign frame protection below
colgroups and the double caption protection, which sounds pretty much like
refactoring.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Another interesting testcase

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

This fixes the testcases in this bug, the crash testcases in the bugs this bug
blocks, some of the textarea weirdness.  It doesn't regress bug 280618, I've
tested that all the form controls work right, and I've audited all the SVG,
XUL, and HTML frame classes involved to make sure that IsLeaf() will do what
|processChildren| used to do.  The only case where it didn't is filed as bug
293104 and I'll make sure that's fixed before I land this.

Comment 7

[Darin Fisher]

Any chance this bug might be fixed for 1.8 instead of 1.9?  Is the fix to risky
for 1.8?

Comment 8

[Boris Zbarsky [:bzbarsky]]

I haven't had the chance to do much testing with this patch (eg I haven't had a
chance to run the layout regression tests and worse yet I doubt those would
catch cases where this patch could fail -- documents that use a lot of
scripting, and not just HTML ones), and any change to the frame constructor has
a certain amount of regression potential...  If this is desperately needed for
1.8 someone would need to test it thoroughly first.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 182795 [details] [diff] [review]
Patch

roc, if you get to this before dbaron does, could you just r+sr?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 182795 [details] [diff] [review]
Patch

>Index: layout/generic/nsIFrame.h
>+  virtual PRBool IsLeaf() const { return PR_TRUE; }

No point having this inline.  Could you put the implementation in nsFrame.cpp?

>Index: layout/generic/nsObjectFrame.cpp

Tell biesi how to update his object rewrite patch when this lands.

>Index: layout/base/nsCSSFrameConstructor.cpp
>nsCSSFrameConstructor::ConstructMathMLFrame(nsFrameConstructorState& aState,
>+    // MathML frames are inline frames, so just process their kids

How about an NS_ASSERTION that IsLeaf returns false?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 182795 [details] [diff] [review]
Patch

Actually, never mind that last assertion comment.

r+sr=dbaron

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Could you put the implementation in nsFrame.cpp?

Will do.

> Tell biesi how to update his object rewrite patch when this lands.

Also will do.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: And even including the nsFrame.cpp change

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk two days ago.

Comment 16

[Brendan Eich [:brendan]]

See bug 306663.

/be

Comment 17

[Mike Schroepfer]

Will take fix on branch but will not hold b1 for this.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Er... if this is approved I can just land this today.  Should I be requesting
approval?  Or waiting till after b1?

Comment 19

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

Comment on attachment 192867 [details] [diff] [review]
And even including the nsFrame.cpp change

+ing based on
https://bugzilla.mozilla.org/show_bug.cgi?id=306663#c13
this is obviously what everyone wants

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.

Comment 21

[Jesse Ruderman]

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