1156058 - crash in mozilla::layers::CanvasClientSharedSurface::Update(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::layers::ClientCanvasLayer*)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

I could reopen bug 1126918 but I guess this is likely to be a new bug.

A user complained on twitter ( https://twitter.com/iMilnb/status/589726237305757696 ) this morning about this bug. 
Here are the two crashes report:
bp-381511f3-49da-446a-ad46-116d72150418
bp-350ffee0-4951-4ea6-a2cc-8a7782150419

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Milan, Jeff, does it ring a bell?

Comment 2

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

Skia font metrics; and it looks like Lollipop 5.1?  Adding a few more people on this, not sure this is Jeff's domain.

Comment 3

[George Wright (:gw280) (needinfo me!)]

Busted stack. Any way of getting STR?

Comment 4

[Matt Woodrow (:mattwoodrow)]

Busted stack is annoying, but this is clearly in CanvasClientSharedSurface.

The crash is at UniquePtr::get() with |this| == 0x8.

In ShSurfHandle, mSurf is at 0x8 so calling ShSurfHandle::Surf() with |this| == 0x0 would cause this crash.

[1] calls mFront->Surf() on a ShSurfHandle, and we don't null check if previously if CloneSurface failed.

Jeff, want to fix this?

[1] http://hg.mozilla.org/releases/mozilla-release/annotate/9aecbdea0799/gfx/layers/client/CanvasClient.cpp#l366

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Busted stack is annoying, but this is clearly in CanvasClientSharedSurface.
> 
> The crash is at UniquePtr::get() with |this| == 0x8.
> 
> In ShSurfHandle, mSurf is at 0x8 so calling ShSurfHandle::Surf() with |this|
> == 0x0 would cause this crash.
> 
> [1] calls mFront->Surf() on a ShSurfHandle, and we don't null check if
> previously if CloneSurface failed.
> 
> Jeff, want to fix this?
> 
> [1]
> http://hg.mozilla.org/releases/mozilla-release/annotate/9aecbdea0799/gfx/
> layers/client/CanvasClient.cpp#l366

Sure.

Comment 6

[Robert Kaiser]

(In reply to George Wright (:gw280) from comment #3)
> Busted stack. Any way of getting STR?

Usually it can be helpful to load a minidump into a debugger and trying to get a stack from there. Or ask :dmajor to do that and try to get a cleaner stack if you really need it.

Comment 7

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

Attachment: Null pointer check. r=jgilbert

Null pointer check, wouldn't worry about try.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8596798 [details] [diff] [review]
Null pointer check. r=jgilbert

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

::: gfx/layers/client/CanvasClient.cpp
@@ +362,1 @@
>    MOZ_ASSERT(mFront);

Assert before the return. This is a bad case we want to know about.

Comment 9

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

Attachment: Null pointer check. Carry r=jgilbert

Use gfxCriticalError instead of the assert (as it asserts in debug anyway, but also leaves a trace in release build without crashing.)

Comment 10

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

nullptr check, skipping try run.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b04704294a81

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8597380 [details] [diff] [review]
Null pointer check. Carry r=jgilbert

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

::: gfx/layers/client/CanvasClient.cpp
@@ -374,5 @@
>      auto layersBackend = shadowForwarder->GetCompositorBackendType();
>  
>      newTex = TexClientFromReadback(surf, forwarder, flags, layersBackend);
>    }
> -  MOZ_ASSERT(newTex);

Please don't remove this.

Comment 13

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

(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> Comment on attachment 8597380 [details] [diff] [review]
> Null pointer check. Carry r=jgilbert
> 
> Review of attachment 8597380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/CanvasClient.cpp
> @@ -374,5 @@
> >      auto layersBackend = shadowForwarder->GetCompositorBackendType();
> >  
> >      newTex = TexClientFromReadback(surf, forwarder, flags, layersBackend);
> >    }
> > -  MOZ_ASSERT(newTex);
> 
> Please don't remove this.

It's redundant, right? It's followed by

if (!newTex) {
   gfxCriticalError...
}

and gfxCriticalError asserts.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Ah, yeah. I'm not used to seeing that pattern.

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: Crash signature that is resolved on 40. Topcrash for Android.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Milan, can you fill an uplift request for 38.0.5b3? Thanks

Comment 18

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

Comment on attachment 8597380 [details] [diff] [review]
Null pointer check. Carry r=jgilbert

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: This was one of the top Android crashers.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Null pointer check, low risk.
[String/UUID change made/needed]:

Comment 19

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Though there aren't many crashes with this signature for 39 (only 2 in the last month) we don't have a very big user base for aurora here. So it seems best to uplift this to 39 as well.

Comment 20

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8597380 [details] [diff] [review]
Null pointer check. Carry r=jgilbert

Setting a flag for mozilla-release for 38.0.5, which is I think Milan's intention in comment 18.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8597380 [details] [diff] [review]
Null pointer check. Carry r=jgilbert

Top crash #1 on Android, taking it.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e2654650356d

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/013da2859c88