Closed
Bug 265367
Opened 20 years ago
Closed 19 years ago
[FIX]Frame construction tries to construct kids of leaf frames
Categories
(Core :: Layout, defect, P1)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: fixed1.8, Whiteboard: [sg:fix])
Attachments
(3 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
roc
:
approval1.8b4+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•20 years ago
|
||
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.
Assignee | ||
Comment 2•20 years ago
|
||
I guess I'll take the silence as tacit approval... ;)
Assignee: nobody → bzbarsky
Target Milestone: --- → mozilla1.8alpha6
Assignee | ||
Comment 3•20 years ago
|
||
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.
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.
Assignee | ||
Comment 5•19 years ago
|
||
Assignee | ||
Comment 6•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #182795 -
Flags: superreview?(dbaron)
Attachment #182795 -
Flags: review?(dbaron)
Assignee | ||
Updated•19 years ago
|
Priority: -- → P1
Summary: Frame construction tries to construct kids of leaf frames → [FIX]Frame construction tries to construct kids of leaf frames
Target Milestone: mozilla1.8alpha6 → mozilla1.9alpha
Comment 7•19 years ago
|
||
Any chance this bug might be fixed for 1.8 instead of 1.9? Is the fix to risky for 1.8?
Assignee | ||
Comment 8•19 years ago
|
||
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.
Assignee | ||
Comment 9•19 years ago
|
||
Comment on attachment 182795 [details] [diff] [review] Patch roc, if you get to this before dbaron does, could you just r+sr?
Attachment #182795 -
Flags: review?(dbaron) → review?(roc)
Comment 10•19 years ago
|
||
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•19 years ago
|
||
Comment on attachment 182795 [details] [diff] [review] Patch Actually, never mind that last assertion comment. r+sr=dbaron
Attachment #182795 -
Flags: superreview?(dbaron)
Attachment #182795 -
Flags: superreview+
Attachment #182795 -
Flags: review?(roc)
Attachment #182795 -
Flags: review+
Assignee | ||
Comment 12•19 years ago
|
||
> 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.
Assignee | ||
Comment 13•19 years ago
|
||
Attachment #182795 -
Attachment is obsolete: true
Assignee | ||
Comment 14•19 years ago
|
||
Attachment #192866 -
Attachment is obsolete: true
Assignee | ||
Comment 15•19 years ago
|
||
Fixed on trunk two days ago.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 17•19 years ago
|
||
Will take fix on branch but will not hold b1 for this.
Flags: blocking1.8b4? → blocking1.8b5+
Assignee | ||
Comment 18•19 years ago
|
||
Er... if this is approved I can just land this today. Should I be requesting approval? Or waiting till after b1?
Assignee | ||
Updated•19 years ago
|
Attachment #192867 -
Flags: approval1.8b4?
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
Attachment #192867 -
Flags: approval1.8b4? → approval1.8b4+
Comment 21•19 years ago
|
||
*** Bug 306782 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
OS: Linux → All
Hardware: PC → All
Whiteboard: [sg:fix]
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•