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&)

RESOLVED FIXED in Firefox 47

Status

()

Core
Graphics: Layers
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: ashughes, Assigned: milan)

Tracking

({crash, regression})

Trunk
mozilla47
Unspecified
All
crash, regression
Points:
---

Firefox Tracking Flags

(firefox39- wontfix, firefox40 affected, firefox41 affected, firefox42 affected, firefox47 fixed)

Details

(crash signature)

Attachments

(3 attachments, 7 obsolete attachments)

2.82 KB, patch
milan
: review+
Details | Diff | Splinter Review
15.72 KB, patch
milan
: review+
Details | Diff | Splinter Review
4.18 KB, patch
mstange
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
[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).
tracking-firefox39: --- → ?
Keywords: regression
(Reporter)

Comment 2

2 years ago
Note that this is the #6 topcrash for Mac-specific crashes accounting for 0.78%.
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.
BasicCompositor compatible TextureSource is created by BasicCompositor::CreateDataTextureSource(). The CreateDataTextureSource() is called in BufferTextureHost::Upload().

MacIOSurface case, MacIOSurfaceTextureHostBasic creates MacIOSurfaceTextureSourceBasic as BasicCompositor compatible TextureSource.
nical, do you know if there is a possibility that incompatible TextureSource is created when BasicCompositor is used?
Flags: needinfo?(nical.bugzilla)

Comment 6

2 years ago
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.
(Reporter)

Comment 7

2 years ago
(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

2 years ago
(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.
(Reporter)

Comment 9

2 years ago
(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
(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
status-firefox39: affected → wontfix
tracking-firefox39: ? → -
FYI https://www.reddit.com/r/firefox/comments/3ibgpa/firefox_crashing_using_google_maps/cug18r2

Updated

2 years ago
Summary: 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&) → [Mac OS X] 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&)

Comment 12

2 years ago
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

Updated

2 years ago
Duplicate of this bug: 1205850

Updated

2 years ago
Crash Signature: [@ mozilla::layers::BasicCompositor::DrawQuad(mozilla::gfx::RectTyped<T> const&, mozilla::gfx::RectTyped<T> const&, mozilla::layers::EffectChain const&, float, mozilla::gfx::Matrix4x4 const&)] → [@ mozilla::layers::BasicCompositor::DrawQuad(mozilla::gfx::RectTyped<T> const&, mozilla::gfx::RectTyped<T> const&, mozilla::layers::EffectChain const&, float, mozilla::gfx::Matrix4x4 const&)] [@ mozilla::layers::BasicCompositor::DrawQuad]

Updated

2 years ago
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 14

2 years ago
Created attachment 8711112 [details]
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/
Attachment #8711112 - Flags: review?(mstange)
(Assignee)

Comment 15

2 years ago
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.
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 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
Attachment #8711112 - Flags: review?(mstange) → review+
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.
(Assignee)

Comment 19

2 years ago
(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.
Assignee: nobody → milan
(Assignee)

Comment 20

2 years ago
Created attachment 8711781 [details] [diff] [review]
Part 2. RTTI name on TextureSource class hierarchy and use it.  r=botond
Attachment #8711781 - Flags: review?(botond)
(Assignee)

Comment 21

2 years ago
Created attachment 8711783 [details] [diff] [review]
Part 3. More nullptr checks, including GetSurface. r=mstange
Attachment #8711783 - Flags: review?(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.
Attachment #8711783 - Flags: review?(mstange) → review-
(Assignee)

Comment 23

2 years ago
Created attachment 8711796 [details] [diff] [review]
Part 3. More nullptr checks, including GetSurface. r=mstange

Thanks for the quick review!
Attachment #8711783 - Attachment is obsolete: true
Attachment #8711796 - Flags: review?(mstange)
Attachment #8711796 - Flags: review?(mstange) → review+

Updated

2 years ago
Attachment #8711781 - Flags: review?(botond) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 24

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=109dc924a795
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
Flags: needinfo?(milan)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 26

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

Updated

2 years ago
See Also: → bug 1187466
(Assignee)

Comment 27

2 years ago
Created attachment 8714932 [details] [diff] [review]
Part 1. We can get the wrong texture source type, so deal with it. Carry r=mstange
Attachment #8711112 - Attachment is obsolete: true
Flags: needinfo?(milan)
(Assignee)

Comment 28

2 years ago
Created attachment 8714933 [details] [diff] [review]
Part 2. RTTI name on TextureSource class hierarchy and use it.  Carry r=botond
Attachment #8711781 - Attachment is obsolete: true
Attachment #8714933 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8714932 - Attachment description: Bug 1187464: Part 1. We can get the wrong texture source type, so deal with it. Carry r=mstange → Part 1. We can get the wrong texture source type, so deal with it. Carry r=mstange
Attachment #8714932 - Flags: review+
(Assignee)

Comment 29

2 years ago
Created attachment 8714934 [details] [diff] [review]
Part 3. More nullptr checks, including GetSurface. Carry r=mstange
Attachment #8711796 - Attachment is obsolete: true
Attachment #8714934 - Flags: review+
(Assignee)

Comment 30

2 years ago
Not sure why it didn't apply the first time, but here are the patches freshly rebased.
Keywords: checkin-needed

Comment 31

2 years ago
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
Keywords: checkin-needed
sorry had to back this out for reftest failures and problems like https://treeherder.mozilla.org/logviewer.html#?job_id=20974329&repo=mozilla-inbound
Flags: needinfo?(milan)

Comment 33

2 years ago
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
(Assignee)

Comment 34

2 years ago
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.
Attachment #8714934 - Flags: review+ → review-
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.
(Assignee)

Comment 36

2 years ago
Created attachment 8717101 [details] [diff] [review]
Part 1. We can get the wrong texture source type, so deal with it. Carry r=mstange
Attachment #8714932 - Attachment is obsolete: true
Flags: needinfo?(milan)
Attachment #8717101 - Flags: review+
(Assignee)

Comment 37

2 years ago
Created attachment 8717103 [details] [diff] [review]
Part 2. RTTI name on TextureSource class hierarchy and use it.  Carry r=botond
Attachment #8714933 - Attachment is obsolete: true
Attachment #8717103 - Flags: review+
(Assignee)

Comment 38

2 years ago
Created attachment 8717106 [details] [diff] [review]
Part 3. More nullptr checks, including GetSurface. Carry r=mstange
Attachment #8714934 - Attachment is obsolete: true
Attachment #8717106 - Flags: review+
(Assignee)

Comment 39

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe0d87bbc2a
(Assignee)

Comment 40

2 years ago
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.
Attachment #8717106 - Flags: review+ → review?(mstange)
Attachment #8717106 - Flags: review?(mstange) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 41

2 years ago
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
Keywords: checkin-needed

Comment 42

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/acb9cdf6473e
https://hg.mozilla.org/mozilla-central/rev/688a04c23fef
https://hg.mozilla.org/mozilla-central/rev/213814f2dccc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Updated

a year ago
OS: Mac OS X → All
Summary: [Mac OS X] 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&) → 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&)
(Assignee)

Updated

a year ago
Blocks: 1298058
You need to log in before you can comment on or make changes to this bug.