506349 - Crash [@ nsContainerFrame::SyncFrameViewProperties] on reload with overflow:scroll, float, select, li display: table-cell and textZoom

Status: RESOLVED DUPLICATE of bug 396367

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase (uses enhanced privileges)

See testcase, which crashes current trunk build on reload.
This seems to have regressed between 2009-06-28 and 2009-06-29:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-06-28+04%3A00%3A00&enddate=2009-06-29+07%3A00%3A00
I'm guessing bug 500556.

http://crash-stats.mozilla.com/report/index/2cefdc18-e2d2-424a-bc41-02a592090724?p=1
0  	xul.dll  	nsContainerFrame::SyncFrameViewProperties  	 layout/generic/nsContainerFrame.cpp:638
1 	xul.dll 	UpdateViewsForTree 	layout/base/nsCSSFrameConstructor.cpp:7429
2 	xul.dll 	UpdateViewsForTree 	layout/base/nsCSSFrameConstructor.cpp:7452
3 	xul.dll 	UpdateViewsForTree 	layout/base/nsCSSFrameConstructor.cpp:7452
4 	xul.dll 	UpdateViewsForTree 	layout/base/nsCSSFrameConstructor.cpp:7452
5 	xul.dll 	DoApplyRenderingChangeToTree 	layout/base/nsCSSFrameConstructor.cpp:7475
6 	xul.dll 	UpdateViewsForTree 	layout/base/nsCSSFrameConstructor.cpp:7449
7 	xul.dll 	UpdateViewsForTree 	layout/base/nsCSSFrameConstructor.cpp:7452
8 	xul.dll 	UpdateViewsForTree 	layout/base/nsCSSFrameConstructor.cpp:7452
9 	xul.dll 	UpdateViewsForTree 	layout/base/nsCSSFrameConstructor.cpp:7452
10 	xul.dll 	UpdateViewsForTree 	layout/base/nsCSSFrameConstructor.cpp:7452
11 	xul.dll 	DoApplyRenderingChangeToTree 	layout/base/nsCSSFrameConstructor.cpp:7475
12 	xul.dll 	nsCSSFrameConstructor::ProcessRestyledFrames 	layout/base/nsCSSFrameConstructor.cpp:7777
13 	xul.dll 	xul.dll@0x3d7c67 	
14 	xul.dll 	nsPresContext::RebuildAllStyleData 	layout/base/nsPresContext.cpp:1501
15 	xul.dll 	xul.dll@0x3d7fcb 	
16 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
17 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2710

Comment 1

[Mats Palmgren (inactive)]

Attachment: various stacks + frame dump

The flush request from nsContentUtils::GenerateStateKey() leads to nested
frame construction, which destroys some frames:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#2014

This attachment contains stacks for nested frame construction and the ~nsView
call for the view that we later try to use and crash, and also the frame tree.

Comment 2

[Mats Palmgren (inactive)]

Attachment: wip

Perhaps we should flush content notifications up front in
DocumentViewerImpl::SetTextZoom() -- or is that just wallpapering?

Comment 3

[Boris Zbarsky [:bzbarsky]]

For now, I think it's the right thing.  We shouldn't be entering restyle processing with an unflushed sink.  It can easily lead to totally bogus behavior (e.g. double-construction of frames).

Comment 4

[Mats Palmgren (inactive)]

It's a "regression" from bug 482578, I cloned changeset 3820403125c2
(containing the fix) which crash, then backed out that fix which
made it not crash.  I'm guessing the fix in bug 482578 blocks a
content notification leaving the sink dirty for the SetTextZoom
call.  I'll see if I can dig up some more details, and see how
noisy the new assertions are...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Other way around; it allowed content flushes even during frame construction...

Comment 6

[Mats Palmgren (inactive)]

Ah, right.  Hmm, that seems like it would lead to even more cases of
nested frame construction though... :-(

Comment 7

[Mats Palmgren (inactive)]

Attachment: wallpaper

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 390996 [details] [diff] [review]
wallpaper

Looks good.  The non-wallpaper fix is switching to the HTML5 parser and getting rid of content sink flushes.  :)

Comment 9

[Mats Palmgren (inactive)]

That'll be great for trunk, but it's not going to happen on branches
I would presume?  Will adding asserts help with prompting more wallpapers
like this for branch?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Adding asserts where?

Comment 11

[Mats Palmgren (inactive)]

Changing the "recurring into frame construction" warning to an assert
for example.  It'll be a little noisy but maybe we can mute some of
the cases we think are safe.

Comment 12

[Boris Zbarsky [:bzbarsky]]

On branch that would fire all the time; once for every text input, for example.

Comment 13

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/3b6268073ecb

-> FIXED

Comment 14

[Mats Palmgren (inactive)]

Backed out since it caused a test to fail:
31054 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/navigation/test_bug430624.html | Check we're contentEditable after reload - got "contentEditable", expected "Still contentEditable"

Comment 15

[Timothy Nikkel (:tnikkel)]

I think this might be the same issue as bug 396367. Indeed, the patch in that bug fixes the crash for me.

Comment 16

[Timothy Nikkel (:tnikkel)]

And the patch in bug 396367 doesn't fail test_bug430624.html on my box (the patch in this bug does fail on my box).

Comment 17

[Mats Palmgren (inactive)]

Yes, your patch looks better.

Comment 18

[Johnny Stenback (:jst)]

Duping per above comments.