832611 - crash in mozilla::css::OverflowChangedTracker::Flush

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Scoobidiver (away)]

This bug tracks crashes not fixed by bug 822906. See bug 822906 comment 3 for the regression range.
Note that bug 815666 is tracked for 19.0 Beta.

Signature 	mozilla::css::OverflowChangedTracker::Flush() More Reports Search
UUID	644b2f56-374c-46e1-93f6-becda2130119
Date Processed	2013-01-19 06:06:24
Uptime	22
Last Crash	24 seconds before submission
Install Age	4.1 hours since version was first installed.
Install Time	2013-01-19 02:00:02
Product	Firefox
Version	21.0a1
Build ID	20130118030915
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 58 stepping 9
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0xfffffffff0de801b
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x679a, AdapterSubsysID: 3000174b, AdapterDriverVersion: 9.12.0.0
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
Processor Notes 	sp-processor01.phx1.mozilla.com_15787:2008
EMCheckCompatibility	True
Adapter Vendor ID	0x1002
Adapter Device ID	0x679a
Total Virtual Memory	4294836224
Available Virtual Memory	3728887808
System Memory Use Percentage	16
Available Page File	30868680704
Available Physical Memory	14310612992

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::css::OverflowChangedTracker::Flush 	layout/base/RestyleTracker.h:92
1 	xul.dll 	mozilla::css::RestyleTracker::DoProcessRestyles 	layout/base/RestyleTracker.cpp:248
2 	xul.dll 	nsCSSFrameConstructor::ProcessPendingRestyles 	layout/base/nsCSSFrameConstructor.cpp:12160
3 	xul.dll 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:3868
4 	xul.dll 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:3757
5 	xul.dll 	nsGfxScrollFrameInner::AsyncScrollPortEvent::Run 	layout/generic/nsGfxScrollFrame.cpp:2869
6 	xul.dll 	PresShell::WillPaint 	layout/base/nsPresShell.cpp:7148
7 	xul.dll 	nsViewManager::CallWillPaintOnObservers 	view/src/nsViewManager.cpp:1152
8 	xul.dll 	nsRefreshDriver::Tick 	layout/base/nsRefreshDriver.cpp:955
9 	xul.dll 	mozilla::RefreshDriverTimer::Tick 	layout/base/nsRefreshDriver.cpp:156
10 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:482
11 	winmm.dll 	timeGetTime 	
12 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:565
13 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
14 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:82
15 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:208
16 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:182
17 	xul.dll 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:163
18 	xul.dll 	nsAppShell::Run 	widget/windows/nsAppShell.cpp:232
19 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:288
20 	xul.dll 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3823
21 	xul.dll 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3890
22 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4093

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Acss%3A%3AOverflowChangedTracker%3A%3AFlush%28%29

Comment 1

[Robert Kaiser]

This is still topcrash #15 over the last 3 days of builds on 21.0a1, so this seems significant.

Comment 2

[Matt Woodrow (:mattwoodrow)]

This looks similar to bug 822906, my guess is that we're trying to access a deleted frame.

Is there any way to get a URL for one of the crash reports? My guess is that this is reproducible, I just need a testcase.

The previous fix removed any frames (and all descendants) when we got a nsChangeHint_ReconstructFrame hint.

Do we also need to walk through placeholders when looking for descendants, or something along those lines?

Is there anything else that happens during style changes that could trigger frame destruction?

Comment 3

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> The previous fix removed any frames (and all descendants) when we got a
> nsChangeHint_ReconstructFrame hint.
> 
> Do we also need to walk through placeholders when looking for descendants,
> or something along those lines?

Yes we do!

> Is there anything else that happens during style changes that could trigger
> frame destruction?

Reconstruction of a frame F can cause nsCSSFrameConstructor to reconstruct F's ancestors actually. :-( Sorry, I should have thought of that.

I think for sanity's sake we should hook up OverflowChangedTracker to nsFrame::Destroy (probably via nsCSSFrameConstructor::NotifyDestroyingFrame).

Comment 4

[Robert Kaiser]

Top URLs:

7 	http://www.bbc.co.uk/cbbc/games/
4 	http://www.bbc.co.uk/cbbc/games/by/type/webcamgames
3 	http://www.imovirtual.com/imoveis/casas/moradia-t2-para-arrendamento-cascais-parede/775906/
3 	about:blank
3 	http://www.imovirtual.com/imoveis/casas/moradia-t2-para-arrendamento-cascais-par
2 	http://www.bbc.co.uk/cbbc/games/by/type/creativegames
2 	http://www.bbc.co.uk/cbbc/games/by/type/musicgames
2 	http://22feet.in/
2 	http://www.mmo-champion.com/threads/998811-2-audio-playback-devices
2 	http://www.bbc.co.uk/cbbc/watch/by/show/the-dumping-ground
2 	http://www.imovirtual.com/imoveis/apartamentos/av-25-abril-optimo-t4-com-estacio

There are more with only 1 hit, if needed. As I try to make sure that the URLs are sane to post in public, I crashed when hovering over one of the numbers on 22feet.in - I don't have crash reporter here, but I guess it's likely that it's this crash.

Comment 5

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) from comment #4)
> I crashed when hovering over one of the numbers on 22feet.in 
I crash also when hovering the first top left image but I don't know if it's the same crash signature as crashes are no longer submitted: bp-c31b5351-2a84-4a64-b07c-103112130124.

Comment 6

[Robert Kaiser]

(In reply to Scoobidiver from comment #5)
> (In reply to Robert Kaiser (:[e-mail]) from comment #4)
> > I crashed when hovering over one of the numbers on 22feet.in 
> I crash also when hovering the first top left image but I don't know if it's
> the same crash signature as crashes are no longer submitted:
> bp-c31b5351-2a84-4a64-b07c-103112130124.

This one has been submitted. There's some problem apparently in that it doesn't show up on crash-stats right now, but that belongs in a different bug.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Use NotifyDestroyingFrame to remove frames from the tracker

Thanks for the URL's, I can reproduce the 22feet.in crash, and this fixes it.

Comment 8

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

Comment on attachment 706239 [details] [diff] [review]
Use NotifyDestroyingFrame to remove frames from the tracker

Review of attachment 706239 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSFrameConstructor.h
@@ +240,5 @@
>    // ProcessRestyledFrames call in a view update batch and a script blocker.
>    // This function does not call ProcessAttachedQueue() on the binding manager.
>    // If the caller wants that to happen synchronously, it needs to handle that
>    // itself.
> +  nsresult ProcessRestyledFrames(nsStyleChangeList& aRestyleArray);

I think you should keep passing aTracker in here instead of using Get/SetOverflowChangedTracker. You can set mOverflowChangedTracker internally, set it to null when the method returns, and assert it's null when this method is called.

That seems simpler and less fragile... among other things it means we don't have to store mFrameConstructor in OverflowChangedTracker.

Comment 9

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)

> I think you should keep passing aTracker in here instead of using
> Get/SetOverflowChangedTracker. You can set mOverflowChangedTracker
> internally, set it to null when the method returns, and assert it's null
> when this method is called.
> 
> That seems simpler and less fragile... among other things it means we don't
> have to store mFrameConstructor in OverflowChangedTracker.

That means that we need to require that no frames are deleted outside of ProcessRestyledFrames, but still within the scope of the OverflowChangedTracker.

I think this is true currently, but I don't see any easy way to assert this going forward and prevent new bugs. Tying the NotifyDestroyingFrame hook to the lifetime of the OverflowChangedTracker seemed easier.

I'll make another patch regardless in the interest of getting this landed asap :)

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Use NotifyDestroyingFrame to remove frames from the tracker - alternate

Comment 11

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/472f37a8f433

Comment 12

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> I think this is true currently, but I don't see any easy way to assert this
> going forward and prevent new bugs.

If this is a problem, another approach would be to have an OverflowChangedTracker permanently attached to the nsCSSFrameConstructor and notified of every frame destruction. Normally it would just be empty.

Comment 13

[Jesse Ruderman]

Attachment: a fuzz testcase

Doesn't crash on inbound, so I guess it's fixed by the patch in this bug.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/472f37a8f433

Comment 15

[Amiad]

Firefox crash in http://www.syntevo.com/smartgithg/index.html
Crash report: https://crash-stats.mozilla.com/report/index/bp-a854d2b0-c71b-4ca2-aea4-e055d2130129

Comment 16

[Scoobidiver (away)]

Amiad, please test with 21.0a1/20130129 or wait the fix lands in Aurora.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 706693 [details] [diff] [review]
Use NotifyDestroyingFrame to remove frames from the tracker - alternate

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 815666
User impact if declined: Reproducible crashes on some sites.
Testing completed (on m-c, etc.): Been on m-c for a few days, fixes known crashers.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None

Comment 18

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34e17e7cafd5

Comment 19

[Simona Badau, Desktop QA]

Firefox 20 beta 1 still crashes when loading http://22feet.in/ and hovering over one of the numbers:
https://crash-stats.mozilla.com/report/index/bp-dacd0b24-65eb-4905-b32e-fa5ae2130226

Also, in Socorro there are several crash reports for Firefox 20 (including on the beta branch), 21 and 22:

http://bit.ly/13f3aTY - for the signature [@ mozilla::css::OverflowChangedTracker::Flush()]

http://bit.ly/YvXK14 - for the signature [@ nsIFrame::Properties()]

Is this expected in any way?

Comment 20

[Scoobidiver (away)]

(In reply to Simona B [QA] from comment #19)
> Firefox 20 beta 1 still crashes when loading http://22feet.in/ and hovering
> over one of the numbers:
Not for me.