Closed Bug 317855 Opened 16 years ago Closed 15 years ago

[FIX]Crash on reload with evil xul testcase, using -moz-box, -moz-grid-group, float: left [@ nsIFrame::GetNextSibling]

Categories

(Core :: Layout, defect, P1)

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

References

Details

(4 keywords)

Crash Data

Attachments

(3 files)

See upcoming testcase.
Mozilla crashes when reloading that testcase.
Also happens in Mozilla1.7, so no (recent) regression.
Attached file testcase
Talkback ID: TB12271663H

nsFrame::Destroy  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsFrame.cpp, line 669]
nsContinuingTextFrame::Destroy  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsTextFrame.cpp, line 1005]
DestroyPropertyEnumerator  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsPropertyTable.cpp, line 305]
nsPropertyTable::PropertyList::Destroy  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsPropertyTable.cpp, line 315]
I get lots of the same asserts as in bug 317682....  Marking dependent on the same bugs.

That said, the issue is that the presshell destructor frees the frame arena, and then later the prescontext destructor clears the prescontext's property table.  Unfortunately, when the latter happens there is an overflow list hanging off of _something_ and we try to destroy those frames.  But the arena they were allocated from is already finished, so we're accessing memory we don't own, and we crash...
Depends on: 317278, 317682
And unfortunately, I see no way to clear/destroy the property table when the presshell is destroyed (that is, when the prescontext's presshell is set to null).

Bryner, what do you think about adding a Clear() method to property table and using it here?
Depends on: 316318
Crashes 1.5rc3 on OS/2
OS: Windows XP → All
Summary: Crash on reload with evil xul testcase, using -moz-box, -moz-grid-group, float: left → Crash on reload with evil xul testcase, using -moz-box, -moz-grid-group, float: left [@ nsIFrame::GetNextSibling]
(In reply to comment #3)
> Bryner, what do you think about adding a Clear() method to property table and
> using it here?
> 

Sounds fine to me.
Attached patch FixSplinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #204987 - Flags: superreview?(roc)
Attachment #204987 - Flags: review?(roc)
We should probably try to take this on the branch... :(
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
Priority: -- → P1
Summary: Crash on reload with evil xul testcase, using -moz-box, -moz-grid-group, float: left [@ nsIFrame::GetNextSibling] → [FIX]Crash on reload with evil xul testcase, using -moz-box, -moz-grid-group, float: left [@ nsIFrame::GetNextSibling]
Target Milestone: --- → mozilla1.9alpha
Attachment #204987 - Flags: superreview?(roc)
Attachment #204987 - Flags: superreview+
Attachment #204987 - Flags: review?(roc)
Attachment #204987 - Flags: review+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 204987 [details] [diff] [review]
Fix

We should take this on the branch -- without this, we make virtual calls on pointers to freed memory, which is potentially exploitable.
Attachment #204987 - Flags: approval1.8.0.1?
No crash loading and reloading testcase https://bugzilla.mozilla.org/attachment.cgi?id=204217&action=view with SeaMonkey trunk build 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051206 Mozilla/1.0

Verified FIXED on trunk
Status: RESOLVED → VERIFIED
Comment on attachment 204987 [details] [diff] [review]
Fix

Please land on 1.8 and 1.8.1 branches. Thanks!
Attachment #204987 - Flags: approval1.8.1+
Attachment #204987 - Flags: approval1.8.0.1?
Attachment #204987 - Flags: approval1.8.0.1+
Could someone please land this for me?  If not, I'll do it in early January...
Comment on attachment 204987 [details] [diff] [review]
Fix

MOZILLA_1_8_BRANCH:
mozilla/content/base/src/nsPropertyTable.cpp 	3.7.12.1
mozilla/content/base/src/nsPropertyTable.h 	1.7.18.1
mozilla/layout/base/nsPresShell.cpp 	3.852.2.12

MOZILLA_1_8_0_BRANCH:
mozilla/content/base/src/nsPropertyTable.cpp 	3.7.20.1
mozilla/content/base/src/nsPropertyTable.h 	1.7.26.1
mozilla/layout/base/nsPresShell.cpp 	3.852.2.11.2.1

the nsPropertyTable changes didn't apply quite as cleanly as i had hoped. i manually merged them later. i hope i got it right :).
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1+
verified fixed on the branch using the testcase in https://bugzilla.mozilla.org/show_bug.cgi?id=317855, and Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1. Repeatedly clicking on the testcase did not produce a crash.
Crash Signature: [@ nsIFrame::GetNextSibling]
You need to log in before you can comment on or make changes to this bug.