389744 - Crash [@ nsPropertyTable::PropertyList::Equals] when zooming in a dd with overflow:scroll; position:fixed

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, when zooming in, I crash with current trunk build.
This regressed between 2007-06-25 and 2007-06-26:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-06-25+04&maxdate=2007-06-26+09&cvsroot=%2Fcvsroot
Regression from bug 384836? Or bug 370525?

Comment 1

[Martijn Wargers (dead)]

http://crash-stats.mozilla.com/report/index/18eec46b-3bc2-11dc-9141-001a4bd43ed6
0  	nsPropertyTable::PropertyList::Equals(unsigned short,nsIAtom *)
1 	nsPropertyTable::GetPropertyListFor(unsigned short,nsIAtom *)
2 	nsPropertyTable::GetPropertyInternal(nsPropertyOwner,unsigned short,nsIAtom *,int,unsigned int *)
3 	nsPropertyTable::GetProperty(nsPropertyOwner,nsIAtom *,unsigned int *)
4 	nsContainerFrame::GetOverflowFrames(nsPresContext *,int)
5 	nsContainerFrame::GetFirstChild(nsIAtom *)
6 	nsFrameManager::CaptureFrameState(nsIFrame *,nsILayoutHistoryState *)
7 	nsFrameManager::CaptureFrameState(nsIFrame *,nsILayoutHistoryState *)
8 	nsFrameManager::CaptureFrameState(nsIFrame *,nsILayoutHistoryState *)
9 	nsFrameManager::CaptureFrameState(nsIFrame *,nsILayoutHistoryState *)

Comment 2

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

Fantasai, this is probably yours

Comment 3

[fantasai]

Not mine. I tried backing out my changes and it still crashes.
I can tell you that it crashes on a ButtonBoxFrame(scrollbarbutton), though.

Capturing frame state for Canvas(html)(-1) frame 0x8e330bc
Checking child list ...done
Checking child list Overflow-list...done
Checking child list Overflow-list...done
Checking child list Fixed-list...done
Capturing frame state for TextBox(label)(-1)[value=] frame 0x8ee3dd8
Checking child list ...done
Capturing frame state for TextBox(label)(1)[value=…] frame 0xb3b8a574
Checking child list ...done
Capturing frame state for HTMLScroll(dd)(1) frame 0x92fa5b8
Checking child list ...done
Capturing frame state for ScrollbarFrame(scrollbar)(-1) frame 0x92fa7b4
Checking child list ...done
Capturing frame state for ButtonBoxFrame(scrollbarbutton)(-1) frame 0x92fa824
Checking child list ...done
Checking child list Overflow-list...
Program dist/bin/firefox-bin (pid = 22268) received signal 11.

Comment 4

[Eli Friedman]

Attachment: Patch

The issue here is that changing the text size is causing a reframe, which is an unusual case for this codepath.  I haven't quite figured out how changing the text size causes a reframe. However, this change is still correct by itself, and it fixes the crash.

Comment 5

[Eli Friedman]

I tracked down the cause of the reframe: nsLayoutUtils::GetBeforeFrame is broken for scrollframes because it assumes that the content insertion frame (in this case, the scrolled frame) is the first child of the frame that gets passed in, which is incorrect for scrollframes.  It doesn't find the existing generated content frame, and assumes that one needs to be constructed because the scrollframe has a generated content style.  Therefore, a scrollframe with a ::before style unconditionally requests a frame reconstruct every time its style gets reresolved.

CCing bz because he wrote nsLayoutUtils::GetBeforeFrame.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So... this reframe will happen in general for anything that doesn't support ::before but is styled with it.  We really need a better way to handle pseudo-elements.  I do agree that we should fix GetBeforeFrame, though...

It looks like simply calling GetFirstChildFrame on aFrame->GetContentInsertionFrame() instead of aFrame should do the trick, right?  That change would make a lot of sense to me.

Comment 8

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

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

r+sr+a1.9=dbaron, but please add a comment explaining that we can't destroy the old rule tree until after ProcessRestyledFrames because if there are any frame reconstructs in the change list, the frames to be reconstructed will still have pointers to the old rule tree.

Comment 9

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

I checked this in to the trunk for you since I wasn't sure whether you'd be able to before the freeze.  Sorry for taking so long to get to reviewing it...

Comment 10

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100104 Minefield/3.0a9pre