594774 - Crash [@ nsDisplayBackground::Paint(nsDisplayListBuilder*, nsIRenderingContext*) ]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Scoobidiver (away)]

Build : Mozilla/5.0 (Windows NT 6.1; rv:2.0b6pre) Gecko/20100909 Firefox/4.0b6pre

Here is a bug that is very similar to bug 593511.
It is #6 top crasher for the last week.
It has a low daily crash rate until 20100904 build (1 crash/day). Since 20100905 build (D3D9 ON by default), the daily rate is about 40 crashes/day
2 comments say that they were on google finance when the crash occurred.

Signature	nsDisplayBackground::Paint(nsDisplayListBuilder*, nsIRenderingContext*)
UUID	eaec51a0-089f-477c-a6b0-6fe122100908
Time 	2010-09-08 10:58:29.651630
Uptime	72818
Last Crash	1574131 seconds (2.6 weeks) before submission
Install Age	72818 seconds (20.2 hours) since version was first installed.
Product	Firefox
Version	4.0b6pre
Build ID	20100907041859
Branch	2.0
OS	Windows NT
OS Version	6.1.7600
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 11
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0xfffffffff0de801b
User Comments	
App Notes 	AdapterVendorID: 10de, AdapterDeviceID: 0165
Processor Notes 	
EMCheckCompatibility	False
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsDisplayBackground::Paint 	layout/base/nsDisplayList.cpp:844
1 	xul.dll 	mozilla::FrameLayerBuilder::DrawThebesLayer 	layout/base/FrameLayerBuilder.cpp:1660
2 	xul.dll 	mozilla::layers::ThebesLayerD3D9::DrawRegion 	gfx/layers/d3d9/ThebesLayerD3D9.cpp:358
3 	xul.dll 	mozilla::layers::ThebesLayerD3D9::RenderLayer 	gfx/layers/d3d9/ThebesLayerD3D9.cpp:213
4 	xul.dll 	mozilla::layers::ContainerLayerD3D9::RenderLayer 	gfx/layers/d3d9/ContainerLayerD3D9.cpp:220
5 	xul.dll 	mozilla::layers::ContainerLayerD3D9::RenderLayer 	gfx/layers/d3d9/ContainerLayerD3D9.cpp:220
6 	xul.dll 	mozilla::layers::LayerManagerD3D9::Render 	gfx/layers/d3d9/LayerManagerD3D9.cpp:218
7 	xul.dll 	mozilla::layers::LayerManagerD3D9::EndTransaction 	gfx/layers/d3d9/LayerManagerD3D9.cpp:131
8 	xul.dll 	nsDisplayList::PaintForFrame 	layout/base/nsDisplayList.cpp:418
9 	xul.dll 	nsLayoutUtils::PaintFrame 	layout/base/nsLayoutUtils.cpp:1412
10 	xul.dll 	PresShell::Paint 	layout/base/nsPresShell.cpp:5934
11 	xul.dll 	nsViewManager::RenderViews 	view/src/nsViewManager.cpp:459
12 	xul.dll 	nsViewManager::Refresh 	view/src/nsViewManager.cpp:425
13 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:940
14 	xul.dll 	AttachedHandleEvent 	view/src/nsView.cpp:193
15 	xul.dll 	nsWindow::DispatchEvent 	widget/src/windows/nsWindow.cpp:3534
16 	xul.dll 	nsWindow::DispatchWindowEvent 	widget/src/windows/nsWindow.cpp:3565
17 		@0x22d0d7 	

More reports at :
http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&date=2010-09-09%2005%3A00%3A00&signature=nsDisplayBackground%3A%3APaint%28nsDisplayListBuilder*%2C%20nsIRenderingContext*%29&version=Firefox%3A4.0b6pre

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

It's reproducabale on given URL.
bp-909503d8-9fae-4cdf-972b-c0b642100909
Setting layers.accelerate-all;false and layers.accelerate-none;true helps.

Comment 2

[Jo Hermans]

I got it too (bp-9c2f5f0b-36f8-42e0-9150-7eabe2100910), at the google finance page. I had set layers.accelerate-all;false (because of the black screen problem), but I didn't change layers.accelerate-none.

Comment 3

[Bas Schouten (:bas.schouten)]

I can reproduce this. Will investigate.

Comment 4

[Bas Schouten (:bas.schouten)]

Before the crash there's a long series of assertions:

###!!! ASSERTION: Want to fire mutation events, but it's not safe: '(aNode->IsNodeOfType(nsINode::eCONTENT) && static_cast<nsIContent*>(aNode)-> IsInNativeAnonymousSubtree()) || sScriptBlockerCount == sRemovableScriptBlockerCount', file c:/Users/Bas/Development/mozilla/firefox-debug-clean/content/base/src/../../../../mozilla-central-clean/content/base/src/nsContentUtils.cpp, line 3664

And then right before the crash:

JavaScript warning: http://www.google.com/finance/s/uNjjKDd8l3M/js/finance-opt.j
s?hl=en&gl=us, line 125: A long running script was terminated

Comment 5

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

I'll take this.

Comment 6

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

I examined the stack for the assertion. A windowless Flash object calls NPN_Evaluate during painting, running some page script "flashToXML" that apparently updates the DOM during painting. Something's obviously changed to make this crash more often than it used to, but fundamentally DOM modifications during painting have always been forbidden/deadly.

Bug 556487 will fix this for out-of-process plugins --- once we add Windows/Mac support.

Comment 7

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

But I'll see if I can come up with some kind of workaround so at least we don't crash.

Comment 8

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

Attachment: Workaround

This adds a global generation counter that increments every time nsCSSFrameConstructor sees a DOM update.

In FrameLayerBuilder, if we detect a DOM modification during painting, we bail out of our DrawThebesLayer callbacks ASAP.

This will not and cannot protect us against all crashes due to DOM changes during painting, but it definitely should help.

Comment 9

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

Attachment: test

Testcase. Crashes trunk, does not crash with the patch.

Comment 10

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

Comment on attachment 475369 [details] [diff] [review]
Workaround

>+PRBool
>+FrameLayerBuilder::CheckDOMModified()
>+{
>+  if (mInitialDOMGeneration == nsCSSFrameConstructor::GetDOMGeneration())
>+    return PR_FALSE;
>+  if (mDetectedDOMModification)
>+    return PR_TRUE;
>+  mDetectedDOMModification = PR_TRUE;

Two problems here:
 * you fall off the end without returning
 * I think you meant to check if mDetectedDOMModification before the generation check (in case of counter wraparound); otherwise I don't see the point

So I think you meant:

if (mDetectedDOMModification)
  return PR_TRUE;
if (mInitialDOMGeneration == nsCSSFrameConstructor::GetDOMGeneration())
  return PR_FALSE;
NS_WARNING("Detected DOM modification during paint, bailing out!");
mDetectedDOMModification = PR_TRUE
return PR_TRUE;

or perhaps even:

if (mInitialDOMGeneration != nsCSSFrameConstructor::GetDOMGeneration())
{
  if (!mDetectedDOMModification) {
    NS_WARNING("Detected DOM modification during paint, bailing out!");
  }
  mDetectedDOMModification = PR_TRUE;
}
return mDetectedDOMModification;
r=dbaron with something like that

Comment 11

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

(In reply to comment #10)
> >+PRBool
> >+FrameLayerBuilder::CheckDOMModified()
> >+{
> >+  if (mInitialDOMGeneration == nsCSSFrameConstructor::GetDOMGeneration())
> >+    return PR_FALSE;
> >+  if (mDetectedDOMModification)
> >+    return PR_TRUE;
> >+  mDetectedDOMModification = PR_TRUE;
> 
> Two problems here:
>  * you fall off the end without returning
>  * I think you meant to check if mDetectedDOMModification before the generation
> check (in case of counter wraparound); otherwise I don't see the point

To not completely spam the console with NS_WARNINGS, and also so that we can add invalidation or some other mitigation here if we decide to.

Comment 12

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

Attachment: workaround v2

Comment 13

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

Comment on attachment 475383 [details] [diff] [review]
workaround v2

ok, I suppose.  Though it seems like the member could be debug-only in that case, but I guess it doesn't matter.

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a08e5d5a3256
http://hg.mozilla.org/mozilla-central/rev/25b165f66af3

Comment 15

[(no longer active)]

This seemed to cause reftest failures:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284566888.1284567334.16081.gz

and crashtest assertions:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284567249.1284567696.18115.gz&fulltext=1

Backed out:

http://hg.mozilla.org/mozilla-central/rev/63136f3a12bf

Comment 16

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

The assertions are caused by the test I added which mutates the DOM during painting. Those assertions are expected and should just be marked as such.

The reftest failures are because this patch broke SVG images (yay tests!). For SVG images we actually can do reflows etc during painting. This patch has to handle that. I'll have to think a bit to figure out the best way to do that.

Comment 17

[Giacomo Magnini]

Probably need to move to beta7 to not get out of the radar.

Comment 18

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

Attachment: workaround v3

This makes the DOM generation counter per-RootPresContext. Fixes the SVG-image test failures.

Comment 19

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

Attachment: test v2

Add "asserts(4)" to the crashtest.list; that's how many assertions I get on Mac. If we get other assertion counts on other platforms, we should just loosen the asserts() clause to cover them.

Comment 20

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

Comment on attachment 475759 [details] [diff] [review]
workaround v3

r=dbaron

Comment 21

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

And I'd recommend pushing to try tonight to check the assertion counts.

Comment 22

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

Yes, indeed.

Comment 23

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

The Windows crashtest run yielded:

REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/moz2_slave/tryserver-win32-debug-unittest-crashtest/build/reftest/tests/modules/plugin/test/crashtests/522512-1.html | timed out waiting for reftest-wait to be removed (after onload fired)
REFTEST TEST-UNEXPECTED-PASS | file:///e:/builds/moz2_slave/tryserver-win32-debug-unittest-crashtest/build/reftest/tests/modules/plugin/test/crashtests/522512-1.html | assertion count 0 is less than expected 4 assertions

There were also a bunch of other failures in that try push; I haven't investigated whether they were due to this patch or others, or due to pushing on top of a bad mozilla-central revision, or just all known intermittent (although a bunch of them do look known).

Comment 24

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

I belive the other are in fact known oranges.

Comment 25

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

Attachment: disable test on Windows

I need to look into why the Windows test failed --- I have no idea. But I think we could land this with the test skipped on Windows.

Comment 26

[Daniel Holbert [:dholbert]]

Landed patch:
  http://hg.mozilla.org/mozilla-central/rev/3c4af3fe446c
and tests:
  http://hg.mozilla.org/mozilla-central/rev/f1c23a9153cb

Comment 27

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

Scoobidiver or anyone else, can you verify that the test URL no longer causes a crash on Windows?

Comment 28

[Scoobidiver (away)]

I disabled HW acceleration : no crash on the test URL.
Moreover, no crash in b7pre/20100918 build :
http://crash-stats.mozilla.com/query/query?build_id=20100918041706&do_query=1
It is fixed.

Comment 29

[Michelle Sintov]

(In reply to comment #6)
> I examined the stack for the assertion. A windowless Flash object calls
> NPN_Evaluate during painting, running some page script "flashToXML" that
> apparently updates the DOM during painting. Something's obviously changed to
> make this crash more often than it used to, but fundamentally DOM modifications
> during painting have always been forbidden/deadly.

Is there anything the user needs to do to get NPN_Evaluate to be called during the paint event? Do you need to interact with the URL (http://www.google.com/finance?q=NYSE:RIG) at all? I ask because I am hoping this will help me see where the Flash Player is calling out to JavaScript while processing the paint event so I can plug this hole. Thanks.

Comment 30

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

You don't have to do anything special.

Note that on Firefox 4 trunk, on Windows, with out-of-process plugins enabled (the default), this will no longer assert because windowless plugin painting is asynchronous now. If you disable OOPP you should still be able to see this.