509839 - Crash [@ nsViewManager::CreateView][@ nsCSSFrameConstructor::AddFrameConstructionItemsInternal] on closing print preview with -moz-transform, position: fixed table displays

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build when closing the print preview window.

http://crash-stats.mozilla.com/report/index/6009c81f-304b-4c1d-90e9-a8d5a2090811?p=1
0  	kernel32.dll  	kernel32.dll@0x12afb  	
1 	mozcrt19.dll 	_CxxThrowException 	throw.cpp:159
2 	mozcrt19.dll 	operator new 	obj-firefox/memory/jemalloc/crtsrc/new.cpp:57
3 	xul.dll 	nsViewManager::CreateView 	view/src/nsViewManager.cpp:276
4 	xul.dll 	nsBoxFrame::CacheAttributes 	layout/xul/base/src/nsBoxFrame.cpp:287
5 	xul.dll 	nsMenuPopupFrame::Init 	layout/xul/base/src/nsMenuPopupFrame.cpp:147

http://crash-stats.mozilla.com/report/index/31a95af1-31e4-415d-8ea6-96d662090811?p=1
0  	kernel32.dll  	kernel32.dll@0x12afb  	
1 	mozcrt19.dll 	_CxxThrowException 	throw.cpp:159
2 	mozcrt19.dll 	operator new 	obj-firefox/memory/jemalloc/crtsrc/new.cpp:57
3 	xul.dll 	nsCSSFrameConstructor::AddFrameConstructionItemsInternal 	layout/base/nsCSSFrameConstructor.cpp:5338
4 	xul.dll 	nsCSSFrameConstructor::ProcessChildren 	layout/base/nsCSSFrameConstructor.cpp:9579
5 	xul.dll 	nsCSSFrameConstructor::ConstructFrameFromItemInternal 	layout/base/nsCSSFrameConstructor.cpp:3927
6 	xul.dll 	nsCSSFrameConstructor::ConstructFramesFromItem 	layout/base/nsCSSFrameConstructor.cpp:5531
7 	xul.dll 	nsCSSFrameConstructor::ProcessChildren 	layout/base/nsCSSFrameConstructor.cpp:9592
8 	xul.dll 	nsCSSFrameConstructor::ConstructFrameFromItemInternal 	layout/base/nsCSSFrameConstructor.cpp:3927
9 	xul.dll 	nsCSSFrameConstructor::ConstructFramesFromItem 	layout/base/nsCSSFrameConstructor.cpp:5531
10 	xul.dll 	nsCSSFrameConstructor::ProcessChildren 	layout/base/nsCSSFrameConstructor.cpp:9592
etc...

Comment 1

[georgi - hopefully not receiving bugspam]

this potentially may be "use after free" judging from the linux stack and 0xdd...
#4  <signal handler called>
#5  0x00007f47f3af4730 in nsPropertyTable::PropertyList::Equals (
    this=0xdddddddddddddddd, aCategory=0, aPropertyName=0x7f47f4d0ab90)
    at /opt/pub/firefox-35/src/content/base/src/nsPropertyTable.cpp:85
#6  0x00007f47f3af3fd8 in nsPropertyTable::GetPropertyListFor (
    this=<value optimized out>, aCategory=0, aPropertyName=0x7f47f4d0ab90)
    at /opt/pub/firefox-35/src/content/base/src/nsPropertyTable.cpp:295
#7  0x00007f47f3af427f in nsPropertyTable::GetPropertyInternal (
    this=0x7f47ea815b78, aObject={mObject = 0x7fff0f315f30}, aCategory=0, 
    aPropertyName=0x7f47f4d0ab90, aRemove=1, aResult=0x0)
    at /opt/pub/firefox-35/src/content/base/src/nsPropertyTable.cpp:190

(gdb) frame 5
#5  0x00007f47f3af4730 in nsPropertyTable::PropertyList::Equals (
    this=0xdddddddddddddddd, aCategory=0, aPropertyName=0x7f47f4d0ab90)
    at /opt/pub/firefox-35/src/content/base/src/nsPropertyTable.cpp:85
85          return mCategory == aCategory && mName == aPropertyName;
(gdb) x/i $rip
0x7f47f3af4730 <_ZN15nsPropertyTable12PropertyList6EqualsEtP7nsIAtom+2>:       cmp    %si,0x48(%rdi)
(gdb) p/x $rdi
$1 = 0xdddddddddddddddd

Comment 2

[Martijn Wargers (dead)]

I guess it might be security sensitive then. In Firefox3.5, it might be the propertyEquals crash thing, though.

Comment 3

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

###!!! ASSERTION: Shouldn't be incomplete if availableHeight is UNCONSTRAINED.: 'aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE', file /home/dbaron/builds/mozilla-central/mozilla/layout/generic/nsBlockFrame.cpp, line 1401

seems very likely to be related

Comment 4

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

Attachment: valgrind's explanation for the crash

Comment 5

[georgi - hopefully not receiving bugspam]

print preview got involved in few similar memory corruptions when dealing with XBL in the past.

is it possible that print preview has a fundamental bug that breaks invariants or is it just triggering an existing bug?

> ###!!! ASSERTION: Shouldn't be incomplete if availableHeight is UNCONSTRAINED

i see this assertion (or something worded very similarly) relatively often when fuzzing, so if this is the root cause there may be other bugs.

Comment 6

[georgi - hopefully not receiving bugspam]

i ran valgrind before my first comment here and i didn't see dbaron's error - valgrind just reported wild pointer once.
using valgrind 3.4.1 with options --tool=memcheck --trace-children=yes

the testcase doesn't crash on today trunk and valgrind is silent.

am i missing some advanced usage of valgrind? (i may have a build with strange malloc/new which valgrind misses for some reason)

Comment 7

[Martijn Wargers (dead)]

Also worksforme in current trunk build.

Comment 8

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

(In reply to comment #6)
> i ran valgrind before my first comment here and i didn't see dbaron's error -
> valgrind just reported wild pointer once.
> using valgrind 3.4.1 with options --tool=memcheck --trace-children=yes
> 
> the testcase doesn't crash on today trunk and valgrind is silent.
> 
> am i missing some advanced usage of valgrind? (i may have a build with strange
> malloc/new which valgrind misses for some reason)

You're not missing advanced usage of valgrind; just advanced debugging of Gecko code.  In particular (I usually mention it when I post valgrind output done this way, but forgot to here), when dealing with use-after-free problems on layout data structures, it's best to uncomment the line:

//#define DEBUG_TRACEMALLOC_PRESARENA 1

in layout/base/nsPresArena.cpp (which until a few days ago was in nsPresShell.cpp, with s/PRESARENA/FRAMEARENA/ ).  This disables pool allocation and recycling of such structures and makes each one use malloc/free separately, so that they are tracked by valgrind.

Comment 9

[georgi - hopefully not receiving bugspam]

thanks dbaron, that was valuable info.

since the defines are changing, i am tempted to file bugs about:
1. buildconfig option that does the right #define
2. realistic testcases about use after and overflow that are detected by valgrind in standard build
3. since you seem to be using custom allocator, it better put heap canaries to check some integrity of the heap (libc does a decent job) [if not already done]

Comment 10

[Martijn Wargers (dead)]

Actually, I still might have a testcase that crashes, I haven't minimized it yet.

Comment 11

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

I no longer see the valgrind warning that I saw with the original testcase.

Comment 12

[georgi - hopefully not receiving bugspam]

no valgrind warning here too.

Comment 13

[georgi - hopefully not receiving bugspam]

>> //#define DEBUG_TRACEMALLOC_PRESARENA 1

this used to work but breaks today build:

/opt/pub/firefox-central/src/layout/base/nsPresArena.cpp:206: error: expected ‘;’ before ‘(’ token

this code is:
void* Allocate(PRUnit32 /* unused */, size_t aSize)

i am not sure this is valid c++, file a bug ?

Comment 14

[Martijn Wargers (dead)]

Attachment: testcase2

Comment 15

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

Mats, any chance you can work on this soon?

Comment 16

[Mats Palmgren (inactive)]

WFM, on Linux trunk. No warnings when run under Valgrind.
Crashes in my Linux 1.9.2 debug build.  I'm debugging this now.

Comment 17

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

Martijn, can you still reproduce on trunk?

If it's fixed on trunk Linux, we should look for a fix range.

Comment 18

[Mats Palmgren (inactive)]

It was fixed on trunk in the range 2010-01-11-03 -- 2010-01-12-03:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=120667a01fd2&tochange=a43e2f7eda8f
most likely by bug 526394.

Comment 19

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

Ouch, that's unfortunate since I don't think we should fix 526394 on the branch!

Comment 20

[Mats Palmgren (inactive)]

I found the problem, here's the crash stack on 1.9.2:
#0  ?? ()
#1  nsFrame::Destroy
#2  nsBlockFrame::Destroy
#3  nsFrameList::DestroyFrames
#4  nsFrameList::Destroy
#5  DestroyPropertyEnumerator
#6  PL_DHashTableEnumerate
#7  nsPropertyTable::DeleteAllProperties
#8  PresShell::Destroy
...

the property we're deleting is ExcessOverflowContainersProperty,
so we're destroying a frame after the frame tree has been destroyed
by the frame manager.
(funny, we did discuss this very problem just recently in another bug)

The reason we missed this frame list during frame tree destruction is
that its removal is conditional on
IsFrameOfType(nsIFrame::eCanContainOverflowContainers):
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp#277
and we did add this property on a frame that doesn't have that bit:
nsPageContentFrame

Comment 21

[Mats Palmgren (inactive)]

Attachment: stack for creating the property

This is the stack and frame tree when we create the property.  (The assertion
at the top is one I added to catch this)

BTW, the bug also occurs on trunk, but it doesn't crash there.

Comment 22

[Mats Palmgren (inactive)]

Attachment: fix for trunk

Comment 23

[Mats Palmgren (inactive)]

Attachment: fix for 1.9.2, 1.9.1 and 1.9.0

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Fantasai, can you hurry the review? Marking blocking for now, but if this misses the code freeze on Monday the 12th then we might have to wait for 3.6.5 :(

Comment 25

[LegNeato]

a=LegNeato for 1.9.2.4, 1.9.1.10 (approving before the request to move this along)

Comment 26

[Mats Palmgren (inactive)]

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/5df7dac32b28
(without the crashtests for now)
Will push directly to branches as soon as the tree goes green...

Comment 27

[Mats Palmgren (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/49b8af2bf97f
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0cd1e5cc5e8d
Holding crashtests, just in case.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.2 using testcase 2 (which crashes 1.9.2.3) with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4pre) Gecko/20100413 Namoroka/3.6.4pre (.NET CLR 3.5.30729). 

Verified for 1.9.1 using same testcase and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.10pre) Gecko/20100413 Shiretoko/3.5.10pre (.NET CLR 3.5.30729).

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 437556 [details] [diff] [review]
fix for 1.9.2, 1.9.1 and 1.9.0

Approved for 1.9.0.20, a=dveditz (but please don't land the crash-tests until after the releases)

Comment 30

[Mats Palmgren (inactive)]

Checking in layout/generic/nsContainerFrame.cpp;
/cvsroot/mozilla/layout/generic/nsContainerFrame.cpp,v  <--  nsContainerFrame.cpp
new revision: 1.316; previous revision: 1.315
done
Checking in layout/generic/nsPageContentFrame.h;
/cvsroot/mozilla/layout/generic/nsPageContentFrame.h,v  <--  nsPageContentFrame.h
new revision: 1.18; previous revision: 1.17
done

Comment 31

[Mats Palmgren (inactive)]

Crash tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0baa18cd25

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4a0baa18cd25