Last Comment Bug 509839 - Crash [@ nsViewManager::CreateView][@ nsCSSFrameConstructor::AddFrameConstructionItemsInternal] on closing print preview with -moz-transform, position: fixed table displays
: Crash [@ nsViewManager::CreateView][@ nsCSSFrameConstructor::AddFrameConstruc...
Status: RESOLVED FIXED
[sg:critical?]
: crash, fixed1.9.0.20, testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla1.9.3a5
Assigned To: Mats Palmgren (:mats)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-11 15:35 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2013-05-18 18:15 PDT (History)
11 users (show)
roc: blocking1.9.2-
roc: wanted1.9.2-
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.4+
.4-fixed
.10+
.10-fixed


Attachments
testcase (447 bytes, text/html)
2009-08-11 15:35 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
valgrind's explanation for the crash (54.24 KB, text/plain; charset=UTF-8)
2009-08-17 10:37 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details
testcase2 (209 bytes, text/html)
2009-11-30 14:39 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
stack for creating the property (7.01 KB, text/plain)
2010-04-07 06:00 PDT, Mats Palmgren (:mats)
no flags Details
fix for trunk (3.70 KB, patch)
2010-04-07 07:08 PDT, Mats Palmgren (:mats)
fantasai.bugs: review+
Details | Diff | Splinter Review
fix for 1.9.2, 1.9.1 and 1.9.0 (3.53 KB, patch)
2010-04-07 07:10 PDT, Mats Palmgren (:mats)
fantasai.bugs: review+
christian: approval1.9.2.4+
christian: approval1.9.1.10+
dveditz: approval1.9.0.next+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2009-08-11 15:35:56 PDT
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...
Comment 1 georgi - hopefully not receiving bugspam 2009-08-12 04:38:58 PDT
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 [:mwargers] (not working for Mozilla) 2009-08-17 10:09:29 PDT
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-10 2009-08-17 10:26:12 PDT
###!!! 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-10 2009-08-17 10:37:01 PDT
Created attachment 394841 [details]
valgrind's explanation for the crash
Comment 5 georgi - hopefully not receiving bugspam 2009-08-18 00:56:36 PDT
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 2009-08-19 04:37:37 PDT
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 [:mwargers] (not working for Mozilla) 2009-08-19 10:19:07 PDT
Also worksforme in current trunk build.
Comment 8 David Baron :dbaron: ⌚️UTC-10 2009-08-19 23:13:45 PDT
(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 2009-08-20 02:33:34 PDT
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 [:mwargers] (not working for Mozilla) 2009-08-21 07:59:34 PDT
Actually, I still might have a testcase that crashes, I haven't minimized it yet.
Comment 11 David Baron :dbaron: ⌚️UTC-10 2009-09-10 05:37:32 PDT
I no longer see the valgrind warning that I saw with the original testcase.
Comment 12 georgi - hopefully not receiving bugspam 2009-09-10 06:08:00 PDT
no valgrind warning here too.
Comment 13 georgi - hopefully not receiving bugspam 2009-09-27 00:10:17 PDT
>> //#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 [:mwargers] (not working for Mozilla) 2009-11-30 14:39:56 PST
Created attachment 415257 [details]
testcase2
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-03-16 14:42:54 PDT
Mats, any chance you can work on this soon?
Comment 16 Mats Palmgren (:mats) 2010-04-06 12:25:19 PDT
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) 2010-04-06 14:03:36 PDT
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 (:mats) 2010-04-06 16:33:17 PDT
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) 2010-04-06 16:40:22 PDT
Ouch, that's unfortunate since I don't think we should fix 526394 on the branch!
Comment 20 Mats Palmgren (:mats) 2010-04-07 05:54:33 PDT
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 (:mats) 2010-04-07 06:00:08 PDT
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.
Comment 22 Mats Palmgren (:mats) 2010-04-07 07:08:50 PDT
Created attachment 437555 [details] [diff] [review]
fix for trunk
Comment 23 Mats Palmgren (:mats) 2010-04-07 07:10:05 PDT
Created attachment 437556 [details] [diff] [review]
fix for 1.9.2, 1.9.1 and 1.9.0
Comment 24 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-07 13:01:21 PDT
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 christian 2010-04-12 11:26:31 PDT
a=LegNeato for 1.9.2.4, 1.9.1.10 (approving before the request to move this along)
Comment 26 Mats Palmgren (:mats) 2010-04-12 11:40:16 PDT
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 28 Al Billings [:abillings] 2010-04-13 15:54:42 PDT
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] 2010-05-17 20:22:31 PDT
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 (:mats) 2010-05-26 15:52:43 PDT
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 (:mats) 2013-05-18 09:45:01 PDT
Crash tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0baa18cd25
Comment 32 Phil Ringnalda (:philor) 2013-05-18 18:15:13 PDT
https://hg.mozilla.org/mozilla-central/rev/4a0baa18cd25

Note You need to log in before you can comment on or make changes to this bug.