Last Comment Bug 530880 - Crashes [@ nsIFrame::GetStyleDisplay() ]
: Crashes [@ nsIFrame::GetStyleDisplay() ]
Status: RESOLVED FIXED
[sg:critical?]
: fixed1.9.0.18
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla1.9.3a1
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks: PoisonFrameCrash 531175
  Show dependency treegraph
 
Reported: 2009-11-24 12:39 PST by chris hofmann
Modified: 2010-06-20 10:06 PDT (History)
9 users (show)
mbeltzner: blocking1.9.2-
dveditz: blocking1.9.0.18+
dveditz: wanted1.9.0.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta5-fixed
.8+
.8-fixed


Attachments
wip (4.08 KB, patch)
2009-11-24 13:12 PST, Olli Pettay [:smaug]
no flags Details | Diff | Review
wip (2.63 KB, patch)
2009-11-24 13:15 PST, Olli Pettay [:smaug]
no flags Details | Diff | Review
wip (3.10 KB, patch)
2009-11-24 13:19 PST, Olli Pettay [:smaug]
bzbarsky: review+
roc: superreview+
mbeltzner: approval1.9.2+
Details | Diff | Review
1.9.0/1 (3.06 KB, patch)
2009-12-21 09:22 PST, Olli Pettay [:smaug]
dveditz: approval1.9.1.8+
dveditz: approval1.9.0.18+
Details | Diff | Review
for 1.9.0/1 (2.53 KB, patch)
2010-01-13 00:50 PST, Olli Pettay [:smaug]
no flags Details | Diff | Review

Description chris hofmann 2009-11-24 12:39:06 PST
new crash on trunk and firefox 3.6 due to frame poisoning

spin off from bug 526587

#12 rank in report from 2009 11 22
https://bug526587.bugzilla.mozilla.org/attachment.cgi?id=414317&t=ytKa49fedH
12. 92 0xfffffffff0dea817 Windows NT nsIFrame::GetStyleDisplay()

reports at

http://crash-stats.mozilla.com/report/index/244668c7-8d87-40de-b8c1-97bde2091123

rame  	Module  	Signature [Expand]  	Source
0 	xul.dll 	nsIFrame::GetStyleDisplay 	layout/style/nsStyleStructList.h:95
1 	xul.dll 	xul.dll@0x41cb0b 	
2 	xul.dll 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:6471
3 	xul.dll 	PresShell::HandlePositionedEvent 	layout/base/nsPresShell.cpp:6296
4 	xul.dll 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6160
5 	xul.dll 	nsViewManager::HandleEvent 	view/src/nsViewManager.cpp:1222
6 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:1201
7 	xul.dll 	HandleEvent 	view/src/nsView.cpp:167
8 	xul.dll 	nsWindow::DispatchEvent 	widget/src/windows/nsWindow.cpp:2885
9 	xul.dll 	nsWindow::DispatchWindowEvent 	widget/src/windows/nsWindow.cpp:2913
10 	xul.dll 	nsWindow::DispatchMouseEvent 	widget/src/windows/nsWindow.cpp:3288
11 	xul.dll 	ChildWindow::DispatchMouseEvent 	widget/src/windows/nsWindow.cpp:6959

sort on address for more reports

http://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=exact&query=&date=&range_value=1&range_unit=weeks&do_query=1&signature=nsIFrame::GetStyleDisplay%28%29
Comment 1 chris hofmann 2009-11-24 12:41:05 PST
not much to go on from user comments to produce STR, but if code inspection can turn up a quick fix it would be good to take that fix in 3.6b/rc/final
Comment 2 Olli Pettay [:smaug] 2009-11-24 13:11:54 PST
So I'm not quite sure yet, but perhaps this has something to do with
PresShell::NotifyDestroyingFrame and PresShell::ClearFrameRefs.
The first one is called always but the latter one only when 
frame has NS_FRAME_EXTERNAL_REFERENCE  or NS_FRAME_SELECTED_CONTENT bit.

Bug 67752 (ireflow) added mFramesToDirty.RemoveEntry(aFrame) to ClearFrameRefs,
but I'm not yet sure whether it adds the right bits to frames.

We could probably merge NotifyDestroyingFrame and ClearFrameRefs, and use the
frame bits to just optimize weakframe destroy handling.
Comment 3 Olli Pettay [:smaug] 2009-11-24 13:12:15 PST
Created attachment 414338 [details] [diff] [review]
wip
Comment 4 Olli Pettay [:smaug] 2009-11-24 13:14:25 PST
Oops that patch contains some unrelated htmlinput things
Comment 5 Olli Pettay [:smaug] 2009-11-24 13:15:07 PST
Created attachment 414339 [details] [diff] [review]
wip
Comment 6 Olli Pettay [:smaug] 2009-11-24 13:19:48 PST
Created attachment 414343 [details] [diff] [review]
wip

Perhaps it is even safer to clear mFramesToDirty
Comment 7 Olli Pettay [:smaug] 2009-11-24 13:52:25 PST
My analysis of mCurrentEventFrame handling is probably a bit wrong.
In practice ESM will have an nsWeakFrame pointing to mCurrentEventFrame, so
ClearFrameRefs will be called.

mFramesToDirty handling is something to fix.
And maybe mCurrentEventFrameStack handling too.

But still, I'm not at all sure the patch would fix this bug :/

Jst, could I get a minidump for this.
Maybe I could get something out of xul.dll@0x41cb0b
Comment 8 Boris Zbarsky [:bz] 2009-11-24 21:29:32 PST
Doh.  Good catch on mFramesToDirty!
Comment 9 Olli Pettay [:smaug] 2009-11-25 05:54:32 PST
Comment on attachment 414343 [details] [diff] [review]
wip

Ok, as far as I see, we need this.

There are cases when mCurrentEventFrame is set, but without the flags, which would cause ClearFrameRefs to be called. (Those flags are set for example when ESM takes the reference.)

One such case is calling first PresShell::HandleDOMEventWithTarget then call PresShell::GetCurrentEventFrame().


One of the minidumps does show a case where PresShell::HandleDOMEventWithTarget is called, and then the event listener does something which spins the event loop and new  event patch is dispatched and bad things happen.

I'm still not 100% the patch will fix this crash.

And frame must be removed from mFramesToDirty.

http://mxr-test.konigsberg.mozilla.org/bonsai/cvsblame.cgi?file=layout/generic/nsFrame.cpp&xrev=70f1501c3088&tree=mozilla-central&mark=441-446#417 

I could file a followup to merge ClearFrameRefs and NotifyDestroyingFrame on trunk.
Comment 10 Olli Pettay [:smaug] 2009-11-25 06:02:04 PST
And note, mCurrentEventFrameStack.Length() is usually 0 when destroying frames.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-11-25 13:11:04 PST
Comment on attachment 414343 [details] [diff] [review]
wip

I assume mFramesToDirty will also be usually empty, since we only use it during the unwind for an interrupt?
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-11-25 13:12:02 PST
Ah, but it's a hashtable so that lookup is O(1) anyway.
Comment 13 Olli Pettay [:smaug] 2009-11-25 13:21:54 PST
Comment on attachment 414343 [details] [diff] [review]
wip

This is not blocking 1.9.2, so can't land even to trunk.
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-25 13:29:29 PST
a192=beltzner
Comment 15 Olli Pettay [:smaug] 2009-11-26 02:14:36 PST
http://hg.mozilla.org/mozilla-central/rev/77136b3d68fc

Will push to 1.9.2 once trunk tests have run.
Comment 17 Olli Pettay [:smaug] 2009-12-03 08:15:13 PST
I think we want this to 1.9.1.x
Comment 18 Daniel Veditz [:dveditz] 2009-12-14 14:37:56 PST
Is this needed on the 1.9.0 branch as well?
Comment 19 Olli Pettay [:smaug] 2009-12-18 11:20:00 PST
I believe yes, we need this on 1.9.0 too.
Comment 20 Olli Pettay [:smaug] 2009-12-21 09:22:23 PST
Created attachment 418666 [details] [diff] [review]
1.9.0/1

This applies to 1.9.0.x and 1.9.1.x. The main difference to original patch is that
1.9.2/trunk have mFramesToDirty.RemoveEntry(aFrame) line, which doesn't exists on older branches.
Comment 21 Daniel Veditz [:dveditz] 2009-12-21 14:59:32 PST
Comment on attachment 418666 [details] [diff] [review]
1.9.0/1

Approved for 1.9.1.8 and 1.9.0.18, a=dveditz for release-drivers
Comment 22 Olli Pettay [:smaug] 2010-01-13 00:50:38 PST
Created attachment 421400 [details] [diff] [review]
for 1.9.0/1

Uh, somehow I had uploaded a wrong patch for branches.
This one doesn't have any 1.9.2/trunk stuff.
Comment 24 Henrik Skupin (:whimboo) 2010-01-28 07:56:19 PST
This crash is not fixed on 1.9.2. There are still a dozen of crashes with the identical stack:

https://crash-stats.mozilla.com/query/query?product=Firefox&version=Firefox%3A3.6&date=&range_value=1&range_unit=weeks&query_search=signature&query_type=exact&query=nsIFrame%3A%3AGetStyleDisplay%28%29&build_id=&do_query=1

Crashes on 1.9.0 have a different stack. I wasn't able to find a bug which covers those crashes. Shall I file a new one? I believe those are different:

3.0.17: https://crash-stats.mozilla.com/report/index/5ab9f42b-880b-4b74-84b1-4688e2100126

And all those crashes are Windows only, not OS X.
Comment 25 Olli Pettay [:smaug] 2010-01-28 11:38:35 PST
Henrik, could you open a new bug.
The crash has dropped from #12 to somewhere significantly lower, I assume.
So a crash causing the stack trace seems to be fixed, but perhaps there is still
something else.
Comment 26 Henrik Skupin (:whimboo) 2010-01-28 12:05:07 PST
For Firefox 3.6 we have bug 542833 now. I don't think it's worth filing a bug for 3.0.x yet, there are only 5 crashes for 3.0.17.
Comment 27 Al Billings [:abillings] 2010-02-01 16:32:01 PST
How did 1.9.0.17 crashes go down when this wasn't fixed in 1.9.0.17, Henrik?
Comment 28 Henrik Skupin (:whimboo) 2010-02-01 17:57:40 PST
(In reply to comment #27)
> How did 1.9.0.17 crashes go down when this wasn't fixed in 1.9.0.17, Henrik?

See comment 24. Crashes on 1.9.0 which I have mentioned have another stack. So those are not related to this particular bug.
Comment 29 Al Billings [:abillings] 2010-02-10 17:37:40 PST
I assume that there is no particular way to induce this bug on 1.9.1 or 1.9.0 for the purposes of verification (especially with comment 24 and 27)?
Comment 30 Henrik Skupin (:whimboo) 2010-02-11 02:03:04 PST
Al, we should wait for the release and check crashstats later. If the number drops drastically on both branches we are good.

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