1291296 - Crash in mozilla::layers::ShadowLayerForwarder::UseTextures

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-432a6aee-5fd3-4619-9097-8a6802160802.
=============================================================

New crash showing up on the Nightly radar: http://bit.ly/2ay3Aft. Appears to have started using 20160731030203. All Windows crashes with highest concentration of Windows 10.

Some of the URLs are videos.

ni on nsilva for possible ideas since he appears to have been working in that code.

Comment 1

[Nicolas Silva [:nical]]

Excellent. This is basically bug 1286437 with a more interesting stack trace.

Comment 2

[Nicolas Silva [:nical]]

Attachment: Support switching a shared buffer provider to a new and/or incompatible PCompositorBridge

Not asking review yet because I haven't verified that the patch fixes the issue. I have only been able to reproduce the crash on one laptop and only with an optimized build, I'll test the patch on that configuration tomorrow.

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8777397 [details] [diff] [review]
Support switching a shared buffer provider to a new and/or incompatible PCompositorBridge

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

Unfortunately I am only able to reproduce the bug with a download nightly build, not with my own builds. That said, this patch makes it impossible to run into the assertion of the crash (incompatible channels when updating a canvas), because it checks that the channels are compatible before sending and tries to do something sensible (copy the data into something compatible with the new channel) in case it they are not.

Comment 4

[Ethan Lin[:ethlin]]

(In reply to Nicolas Silva [:nical] from comment #3)
> Comment on attachment 8777397 [details] [diff] [review]
> Support switching a shared buffer provider to a new and/or incompatible
> PCompositorBridge
> 
> Review of attachment 8777397 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unfortunately I am only able to reproduce the bug with a download nightly
> build, not with my own builds. That said, this patch makes it impossible to
> run into the assertion of the crash (incompatible channels when updating a
> canvas), because it checks that the channels are compatible before sending
> and tries to do something sensible (copy the data into something compatible
> with the new channel) in case it they are not.

I can reproduce this bug with my own build by dragging a canvas tab to another window with e10s off. The behavior is similar to bug 1250954.

Comment 5

[Ethan Lin[:ethlin]]

Comment on attachment 8777397 [details] [diff] [review]
Support switching a shared buffer provider to a new and/or incompatible PCompositorBridge

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

::: gfx/layers/PersistentBufferProvider.cpp
@@ +147,5 @@
> +    // The forwarder should not change most of the time.
> +    return true;
> +  }
> +
> +  if (mFwd->AsTextureForwarder() == aFwd->AsTextureForwarder() ||

Should this be '!='?

Comment 6

[Nicolas Silva [:nical]]

(In reply to Ethan Lin[:ethlin] from comment #5)
> Should this be '!='?

oh my. Yes it should!

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 8777397 [details] [diff] [review]
Support switching a shared buffer provider to a new and/or incompatible PCompositorBridge

Cancelling the review, there is another adjustement I need t make to this patch regarding the activity tracker. I am also able to reproduce the bug with e10s disabled with my own builds now (thanks for the tip!).

Comment 8

[Nicolas Silva [:nical]]

Attachment: Support switching a shared buffer provider to a new and/or incompatible PCompositorBridge

I have been able to verify that this patch fixes the problem.

Comment 9

[Ethan Lin[:ethlin]]

Comment on attachment 8777779 [details] [diff] [review]
Support switching a shared buffer provider to a new and/or incompatible PCompositorBridge

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

::: gfx/layers/PersistentBufferProvider.cpp
@@ +170,5 @@
> +        BackendSelector::Canvas,
> +        TextureFlags::DEFAULT,
> +        TextureAllocationFlags::ALLOC_DEFAULT
> +      );
> +

I think we should have 'MOZ_ASSERT(newTexture)' here.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d59fd1ff4bec
Make it possible to change a PersistentBufferProvider's Forwarder without crashing. r=ethlin

Comment 11

[Marco Castelluccio [:marco]]

[Tracking Requested - why for this release]: #6 in the top crasher list for Firefox 50.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d59fd1ff4bec

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Nicolas, could you please request uplift to Aurora50? Thanks!

Comment 14

[Nicolas Silva [:nical]]

Comment on attachment 8777779 [details] [diff] [review]
Support switching a shared buffer provider to a new and/or incompatible PCompositorBridge

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Crashes
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: Low risk, has been baking on central for a while.
[String/UUID change made/needed]: None.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8777779 [details] [diff] [review]
Support switching a shared buffer provider to a new and/or incompatible PCompositorBridge

Crash fix, the data from Nightly showed this still happened on 08-16 (even with the fix) but I still think this is worth uplifting to Aurora50.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2bdd8158b056

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'mozilla::layers::ShadowLayerForwarder::UseTextures':
 - nightly (version 51): 46 crashes from 2016-08-01.
 - aurora  (version 50): 414 crashes from 2016-08-01.
 - beta    (version 49): 3 crashes from 2016-08-02.
 - release (version 48): 2 crashes from 2016-07-25.
 - esr     (version 45): 1 crash from 2016-05-02.

Crash volume on the last weeks (Week N is from 08-22 to 08-28):
            W. N-1  W. N-2  W. N-3
 - nightly      10       3      26
 - aurora      115     241      48
 - beta          2       0       0
 - release       0       0       2
 - esr           0       0       0

Affected platforms: Windows, Mac OS X

Crash rank on the last 7 days:
           Browser   Content     Plugin
 - nightly #72       #207
 - aurora  #32
 - beta    #13586
 - release
 - esr