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

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

See upcoming testcase.
Mozilla crashes when reloading that testcase.
Also happens in Mozilla1.7, so no (recent) regression.

Comment 1

[Martijn Wargers (dead)]

Attachment: 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]

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Felix Miata]

Crashes 1.5rc3 on OS/2

Comment 5

[Adam Guthrie]

Attachment: stacktrace from testcase

Comment 6

[Brian Ryner (not reading)]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 8

[Boris Zbarsky [:bzbarsky]]

We should probably try to take this on the branch... :(

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 12

[Mike Schroepfer]

Comment on attachment 204987 [details] [diff] [review]
Fix

Please land on 1.8 and 1.8.1 branches. Thanks!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Could someone please land this for me?  If not, I'll do it in early January...

Comment 14

[timeless]

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

Comment 15

[Marcia Knous [:marcia]]

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.