1187464 - crash in mozilla::layers::BasicCompositor::DrawQuad(mozilla::gfx::RectTyped<T> const&, mozilla::gfx::RectTyped<T> const&, mozilla::layers::EffectChain const&, float, mozilla::gfx::Matrix4x4 const&)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[u279076]

This bug was filed from the Socorro interface and is 
report bp-bdbfeb54-a138-4687-b7c5-c5eb82150724.
=============================================================
0 	XUL 	mozilla::layers::BasicCompositor::DrawQuad(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::EffectChain const&, float, mozilla::gfx::Matrix4x4 const&) 	gfx/layers/basic/BasicCompositor.cpp
1 	XUL 	mozilla::layers::ImageHost::Composite(mozilla::layers::EffectChain&, float, mozilla::gfx::Matrix4x4 const&, mozilla::gfx::Filter const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, nsIntRegion const*) 	gfx/layers/composite/ImageHost.cpp
2 	XUL 	mozilla::layers::CanvasLayerComposite::RenderLayer(nsIntRect const&) 	gfx/layers/composite/CanvasLayerComposite.cpp
3 	XUL 	void mozilla::layers::RenderLayers<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, mozilla::gfx::IntRectTyped<mozilla::RenderTargetPixel> const&) 	gfx/layers/composite/ContainerLayerComposite.cpp
4 	XUL 	void mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, nsIntRect const&) 	gfx/layers/composite/ContainerLayerComposite.cpp
5 	XUL 	void mozilla::layers::RenderLayers<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, mozilla::gfx::IntRectTyped<mozilla::RenderTargetPixel> const&) 	gfx/layers/composite/ContainerLayerComposite.cpp
6 	XUL 	void mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, nsIntRect const&) 	gfx/layers/composite/ContainerLayerComposite.cpp
7 	XUL 	mozilla::layers::LayerManagerComposite::Render() 	gfx/layers/composite/LayerManagerComposite.cpp
8 	XUL 	mozilla::layers::LayerManagerComposite::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/composite/LayerManagerComposite.cpp
9 	XUL 	mozilla::layers::CompositorParent::CompositeToTarget(mozilla::gfx::DrawTarget*, nsIntRect const*) 	gfx/layers/composite/LayerManagerComposite.cpp
10 	XUL 	mozilla::layers::CompositorVsyncObserver::Composite(mozilla::TimeStamp) 	gfx/layers/ipc/CompositorParent.cpp
11 	XUL 	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) 	ipc/chromium/src/base/message_loop.cc
12 	XUL 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc
13 	XUL 	base::MessagePumpDefault::Run(base::MessagePump::Delegate*) 	ipc/chromium/src/base/message_pump_default.cc
14 	XUL 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
15 	XUL 	base::Thread::ThreadMain() 	ipc/chromium/src/base/thread.cc
16 	XUL 	ThreadFunc 	ipc/chromium/src/base/platform_thread_posix.cc
17 	libsystem_pthread.dylib 	_pthread_body 	
18 	libsystem_pthread.dylib 	_pthread_start 	
19 	libsystem_pthread.dylib 	thread_start 	
20 	XUL 	base::strlcpy(char*, char const*, unsigned long) 	ipc/chromium/src/base/string_util.cc
21 		@0x10e1cbc6f 	
=============================================================
More reports: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Alayers%3A%3ABasicCompositor%3A%3ADrawQuad%28mozilla%3A%3Agfx%3A%3ARectTyped%3CT%3E+const%26%2C+mozilla%3A%3Agfx%3A%3ARectTyped%3CT%3E+const%26%2C+mozilla%3A%3Alayers%3A%3AEffectChain+const%26%2C+float%2C+mozilla%3A%3Agfx%3A%3AMatrix4x4+const%26%29

Note, I've filed this as a Mac OS X bug but there are reports of this on Windows. I suspect the Windows crashes deserve a different bug for the following reasons:

1. The crash reasons are different.
* Windows shows EXCEPTION_ACCESS_VIOLATION_READ
* Mac shows EXC_BAD_ACCESS/KERN_INVALID_ADDRESS

2. The crashes start with different Firefox versions
* Windows crashes started with Firefox 30
* Mac crashes started with Firefox 39

3. Volume is higher on Mac OS X than it is on Windows, especially when you consider user population.
* Windows has 43% of the crashes with 87% of the users
* Mac OS X has 57% of the crashes with 7% of the users

4. The URLs reported are quite different on Windows compared to Mac 
* Top URL on Windows is Facebook.com with 83% whereas only 2% of the crashes on Mac OS X are with Facebook.com
* Top URL on Mac is Google (Maps and Chrome) with 70% whereas these URLs don't show up at all for Windows users

5. The stacks are slightly different.
* Frame 2 on Windows is in mozilla::layers::ImageLayerComposite::RenderLayer(nsIntRect const&)
* Frame 2 on Mac is in mozilla::layers::CanvasLayerComposite::RenderLayer(nsIntRect const&)

I'll follow-up here with the bug number for the Windows crashes once filed.

Comment 1

[u279076]

[Tracking Requested - why for this release]: regression in Firefox 39 based on crash-stats.

Note, the Windows version of this crash is bug 1187466 but those crashes started in Firefox 30 (again based on crash-stats).

Comment 2

[u279076]

Note that this is the #6 topcrash for Mac-specific crashes accounting for 0.78%.

Comment 3

[Sotaro Ikeda [:sotaro]]

From the crash log, it might be possible that the following return nullptr.
  > TextureSourceBasic* source = texturedEffect->mTexture->AsSourceBasic();

When BasicCompositor::DrawQuad() is called, TexturedEffect and TextureSource need to be valid. One possibility is that TextureSource is not compatible to BasicCompositor.

Comment 4

[Sotaro Ikeda [:sotaro]]

BasicCompositor compatible TextureSource is created by BasicCompositor::CreateDataTextureSource(). The CreateDataTextureSource() is called in BufferTextureHost::Upload().

MacIOSurface case, MacIOSurfaceTextureHostBasic creates MacIOSurfaceTextureSourceBasic as BasicCompositor compatible TextureSource.

Comment 5

[Sotaro Ikeda [:sotaro]]

nical, do you know if there is a possibility that incompatible TextureSource is created when BasicCompositor is used?

Comment 6

[dylan]

fwiw, Firefox 39 is an automatic crash for me with Google Maps. about:crashes led me to this bug, as well as #1187466

Relevant crash reports that were all caused by simply visiting maps.google.com:
bp-c3e54866-ea09-4957-a7ed-67e9b2150809
	8/9/15	04:16
bp-c1226648-3fb9-4744-9ccf-ecfe42150805
	8/5/15	10:52
bp-0c374fdd-8529-45fc-b638-024522150731
	7/31/15	12:41
9DC11FE5-0B67-4FFE-89FD-5FC8BF66C8AF
	7/30/15	00:02
bp-a2f8421a-b154-4727-a349-c74ff2150726
	7/26/15	07:09
bp-bb4bd41c-8f0f-49f9-83a5-dca182150724
	7/24/15	16:15
bp-c6899c74-2b7e-4092-bb9e-d0bc52150721
	7/21/15	07:57
bp-f418002b-0184-4b45-8ced-a6ad22150716
	7/16/15	13:18
bp-1ca98412-5f50-49ab-ab2f-01c5d2150716
	7/16/15	13:18
bp-2705edea-2ac1-401e-9f3b-8917d2150716
	7/16/15	12:38

There would be far more crashes, but I've mostly remembered to not open Google Maps.

While I doubt I'm doing anything unique, let me know if you need me to try anything.

Comment 7

[u279076]

(In reply to dylan from comment #6)
> fwiw, Firefox 39 is an automatic crash for me with Google Maps.

What specifically are you doing on Google Maps leading up to this crash?
Do you still see this bug if you downgrade to Firefox 38?

Comment 8

[dylan]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #7)
> (In reply to dylan from comment #6)
> > fwiw, Firefox 39 is an automatic crash for me with Google Maps.
> 
> What specifically are you doing on Google Maps leading up to this crash?
> Do you still see this bug if you downgrade to Firefox 38?

Simply visiting maps.google.com. I've not tried a downgrade. I know it's not a universal problem as my other nearly identically configured Mac with Firefox 39.0.5 does not crash in this manner.

Comment 9

[u279076]

(In reply to dylan from comment #8)
> Simply visiting maps.google.com. I've not tried a downgrade. I know it's not
> a universal problem as my other nearly identically configured Mac with
> Firefox 39.0.5 does not crash in this manner.

Does this reproduce on the same system if you run with a new Firefox profile?
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

Comment 10

[Andrew Truong [:feer56] [:andrewtruong] [:atruong]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #9)
> (In reply to dylan from comment #8)
> > Simply visiting maps.google.com. I've not tried a downgrade. I know it's not
> > a universal problem as my other nearly identically configured Mac with
> > Firefox 39.0.5 does not crash in this manner.
> 
> Does this reproduce on the same system if you run with a new Firefox profile?
> https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-
> firefox-profiles

I have a user experiencing similar symptoms with this type of crash. I'll have them try a clean reinstall.. https://support.mozilla.org/en-US/questions/1077887

Comment 11

[Gian-Carlo Pascutto [:gcp]]

FYI https://www.reddit.com/r/firefox/comments/3ibgpa/firefox_crashing_using_google_maps/cug18r2

Comment 12

[Loic]

Issue reported on the French support board: https://forums.mozfr.org/viewtopic.php?p=801324#p801324

CR: https://crash-stats.mozilla.com/report/index/bp-afdcf1d4-e12f-49b3-bf22-3ccda2150914

Accélération graphique
Fenêtres avec accélération graphique 0/1 Basic (OMTC)
ID du périphérique 0x 126
ID du vendeur 0x8086
Prise en charge matérielle pour le décodage H264 false
Rendu WebGL ATI Technologies Inc. -- AMD Radeon HD 6750M OpenGL Engine
windowLayerManagerRemote true
Zoom/Panoramique asynchrones aucun
AzureCanvasBackend skia
AzureContentBackend quartz
AzureFallbackCanvasBackend none
AzureSkiaAccelerated 0

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: MozReview Request: Bug 1187464: Part 1. We can get the wrong texture source type, so deal with it. r?mstange

Review commit: https://reviewboard.mozilla.org/r/31981/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31981/

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

This is just to stop the crash, and skip the effect.  I'm assuming we somehow get accelerated filter, and can't deal with it?  Markus to review mostly because he'd know what to do to properly fix the problem, rather than just crash avoidance.  This will still crash in nightly+aurora, but at least we'll confirm why. Won't crash in beta/release.

Comment 16

[Markus Stange [:mstange]]

Oh no, so many people on BasicCompositor on OS X before it was ready... :( 
It looks like Mac people started getting BasicCompositor with 39 or a bit earlier, and then bug 1180688 put a stop to that in Firefox 42, but the patch that was uplifted to 40 and 41 only disabled BasicCompositor in the cases where people would have got an empty window.
As for the crash... source being null is one option, source->GetAsSurface(dest) being null is another option that could be happening. I doubt bug 1241665 will have fixed this - in the broken state that was fixed by bug 1241665, both source and the surface would have been non-null, but the surface would have had bad data.

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8711112 [details]
MozReview Request: Bug 1187464: Part 1. We can get the wrong texture source type, so deal with it. r?mstange

https://reviewboard.mozilla.org/r/31981/#review28897

Comment 18

[Markus Stange [:mstange]]

Hmm, it looks like we should null-check the result of source->GetAsSurface(dest) anyway. For example X11DataTextureSourceBasic::GetSurface(DrawTarget* aTarget) can explicitly return null.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Markus Stange [:mstange] from comment #18)
> Hmm, it looks like we should null-check the result of
> source->GetAsSurface(dest) anyway. For example
> X11DataTextureSourceBasic::GetSurface(DrawTarget* aTarget) can explicitly
> return null.

Good idea.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2. RTTI name on TextureSource class hierarchy and use it. r=botond

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 3. More nullptr checks, including GetSurface. r=mstange

Comment 22

[Markus Stange [:mstange]]

Comment on attachment 8711783 [details] [diff] [review]
Part 3. More nullptr checks, including GetSurface. r=mstange

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

The rest looks good.

::: gfx/layers/basic/BasicCompositor.cpp
@@ +417,5 @@
>    }
>  
>    switch (aEffectChain.mPrimaryEffect->mType) {
>      case EffectTypes::SOLID_COLOR: {
> +      if (sourceMask) {

This check is wrong. We want to be able to draw color layers even if they don't have a mask layer. FillRectWithMask handles the case when sourceMask is null.

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 3. More nullptr checks, including GetSurface. r=mstange

Thanks for the quick review!

Comment 24

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=109dc924a795

Comment 25

[Carsten Book [:Tomcat]]

this failed to apply:

renamed 1187464 -> 1187464.3.p2
applying 1187464.3.p2
patching file gfx/layers/basic/BasicCompositor.cpp
Hunk #3 FAILED at 441
1 out of 3 hunks FAILED -- saving rejects to file gfx/layers/basic/BasicCompositor.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1187464.3.p2

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

Must be something in inbound right now, I'll rebase as it gets through central.

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1. We can get the wrong texture source type, so deal with it. Carry r=mstange

Comment 28

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2. RTTI name on TextureSource class hierarchy and use it. Carry r=botond

Comment 29

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 3. More nullptr checks, including GetSurface. Carry r=mstange

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

Not sure why it didn't apply the first time, but here are the patches freshly rebased.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63e2a056311a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cee3cdcce99
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64710d45b28

Comment 32

[Carsten Book [:Tomcat]]

sorry had to back this out for reftest failures and problems like https://treeherder.mozilla.org/logviewer.html#?job_id=20974329&repo=mozilla-inbound

Comment 33

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dffbf73c3c68
https://hg.mozilla.org/integration/mozilla-inbound/rev/97f319b10c21
https://hg.mozilla.org/integration/mozilla-inbound/rev/52a2a349ff2a

Comment 34

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8714934 [details] [diff] [review]
Part 3. More nullptr checks, including GetSurface. Carry r=mstange

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

::: gfx/layers/basic/BasicCompositor.cpp
@@ +176,5 @@
>                               SourceSurface *aMask,
>                               const Matrix* aMaskTransform)
>  {
> +  if (!aSource || !aMask) {
> +    gfxWarning() << "DrawSurfaceWithTextureCoords problem " << gfx::hexa(aSource) << " and " << gfx::hexa(aMask);

Looks like this check is wrong as well - we should allow null aMask.

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

this has some nice perf wins (mostly on winxp):
http://alertmanager.allizom.org:8080/alerts.html?rev=b64710d45b2852b6f082411e6a066497aad50182&showAll=1&testIndex=0&platIndex=0

one regression about linxu64 tp5 xres, not sure why it jumps up 80%, but it still shows a win when looking at a longer history- we used to have much noisier xres data and higher data, this 80% regression only puts it halfway back to what it used to be.

Comment 36

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1. We can get the wrong texture source type, so deal with it. Carry r=mstange

Comment 37

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2. RTTI name on TextureSource class hierarchy and use it. Carry r=botond

Comment 38

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 3. More nullptr checks, including GetSurface. Carry r=mstange

Comment 39

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe0d87bbc2a

Comment 40

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8717106 [details] [diff] [review]
Part 3. More nullptr checks, including GetSurface. Carry r=mstange

Better to review this change, since it is different than the previous version.

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/acb9cdf6473e
https://hg.mozilla.org/integration/mozilla-inbound/rev/688a04c23fef
https://hg.mozilla.org/integration/mozilla-inbound/rev/213814f2dccc

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/acb9cdf6473e
https://hg.mozilla.org/mozilla-central/rev/688a04c23fef
https://hg.mozilla.org/mozilla-central/rev/213814f2dccc