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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Layout
--
critical
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Mats Palmgren (vacation - back in August))

Tracking

(5 keywords)

unspecified
mozilla1.9.3a5
crash, fixed1.9.0.20, testcase, verified1.9.1, verified1.9.2
Points:
---
Bug Flags:
blocking1.9.2 -
wanted1.9.2 -
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 .4+, status1.9.2 .4-fixed, blocking1.9.1 .10+, status1.9.1 .10-fixed)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(6 attachments)

(Reporter)

Description

8 years ago
Created attachment 393908 [details]
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...
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
(Reporter)

Comment 2

8 years ago
I guess it might be security sensitive then. In Firefox3.5, it might be the propertyEquals crash thing, though.
Group: core-security
Flags: blocking1.9.2?
###!!! 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
Created attachment 394841 [details]
valgrind's explanation for the crash
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.
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)
(Reporter)

Comment 7

8 years ago
Also worksforme in current trunk build.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WORKSFORME
(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.
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]
(Reporter)

Comment 10

8 years ago
Actually, I still might have a testcase that crashes, I haven't minimized it yet.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
I no longer see the valgrind warning that I saw with the original testcase.
no valgrind warning here too.
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
>> //#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 ?
(Reporter)

Comment 14

8 years ago
Created attachment 415257 [details]
testcase2

Updated

8 years ago
Whiteboard: [sg:critical?]
Assignee: nobody → matspal
Mats, any chance you can work on this soon?
WFM, on Linux trunk. No warnings when run under Valgrind.
Crashes in my Linux 1.9.2 debug build.  I'm debugging this now.
Martijn, can you still reproduce on trunk?

If it's fixed on trunk Linux, we should look for a fix range.
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.
OS: Windows XP → All
Hardware: x86 → All
Version: Trunk → 1.9.2 Branch
Ouch, that's unfortunate since I don't think we should fix 526394 on the branch!
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
Created attachment 437541 [details]
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.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Flags: blocking1.9.0.19?
Version: 1.9.2 Branch → unspecified
Created attachment 437555 [details] [diff] [review]
fix for trunk
Attachment #437555 - Flags: review?(fantasai.bugs)
Created attachment 437556 [details] [diff] [review]
fix for 1.9.2, 1.9.1 and 1.9.0
Attachment #437556 - Flags: review?(fantasai.bugs)
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
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 :(
blocking1.9.1: ? → .10+
blocking1.9.2: ? → .4+

Updated

7 years ago
Attachment #437555 - Flags: review?(fantasai.bugs) → review+

Updated

7 years ago
Attachment #437556 - Flags: review?(fantasai.bugs) → review+

Comment 25

7 years ago
a=LegNeato for 1.9.2.4, 1.9.1.10 (approving before the request to move this along)
status1.9.1: --- → wanted
status1.9.2: --- → wanted

Updated

7 years ago
Attachment #437556 - Flags: approval1.9.2.4+
Attachment #437556 - Flags: approval1.9.1.10+
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...
Whiteboard: [sg:critical?][needs review] → [sg:critical?]
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.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago7 years ago
status1.9.1: wanted → .10-fixed
status1.9.2: wanted → .4-fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Attachment #437556 - Flags: approval1.9.0.19?
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).
Keywords: verified1.9.1, verified1.9.2
Attachment #437556 - Flags: approval1.9.0.19? → approval1.9.0.next+
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)
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
Flags: blocking1.9.0.19?
Keywords: fixed1.9.0.20
Group: core-security
Crash Signature: [@ nsViewManager::CreateView] [@ nsCSSFrameConstructor::AddFrameConstructionItemsInternal]
Crash tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0baa18cd25
Crash Signature: [@ nsViewManager::CreateView] [@ nsCSSFrameConstructor::AddFrameConstructionItemsInternal] → [@ nsViewManager::CreateView] [@ nsCSSFrameConstructor::AddFrameConstructionItemsInternal]
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/4a0baa18cd25
You need to log in before you can comment on or make changes to this bug.