Closed Bug 1156058 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Layers, defect)

All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox38.0.5 + fixed
firefox39 + fixed
firefox40 --- verified

People

(Reporter: Sylvestre, Assigned: jgilbert)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
Milan, Jeff, does it ring a bell?
Flags: needinfo?(milan)
Flags: needinfo?(jgilbert)
Skia font metrics; and it looks like Lollipop 5.1?  Adding a few more people on this, not sure this is Jeff's domain.
Flags: needinfo?(snorp)
Flags: needinfo?(milan)
Flags: needinfo?(jgilbert)
Flags: needinfo?(gwright)
Busted stack. Any way of getting STR?
Flags: needinfo?(gwright)
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
Flags: needinfo?(snorp) → needinfo?(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.
Assignee: nobody → jgilbert
(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.
Attached patch Null pointer check. r=jgilbert (obsolete) — Splinter Review
Null pointer check, wouldn't worry about try.
Flags: needinfo?(jgilbert)
Attachment #8596798 - Flags: review?(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.
Attachment #8596798 - Flags: review?(jgilbert) → review+
Use gfxCriticalError instead of the assert (as it asserts in debug anyway, but also leaves a trace in release build without crashing.)
Attachment #8596798 - Attachment is obsolete: true
Attachment #8597380 - Flags: review+
nullptr check, skipping try run.
Keywords: checkin-needed
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.
(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.
Ah, yeah. I'm not used to seeing that pattern.
https://hg.mozilla.org/mozilla-central/rev/b04704294a81
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
[Tracking Requested - why for this release]: Crash signature that is resolved on 40. Topcrash for Android.
Milan, can you fill an uplift request for 38.0.5b3? Thanks
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]:
Flags: needinfo?(milan)
Attachment #8597380 - Flags: approval-mozilla-beta?
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 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.
Attachment #8597380 - Flags: approval-mozilla-release?
Comment on attachment 8597380 [details] [diff] [review]
Null pointer check. Carry r=jgilbert

Top crash #1 on Android, taking it.
Attachment #8597380 - Flags: approval-mozilla-release?
Attachment #8597380 - Flags: approval-mozilla-release+
Attachment #8597380 - Flags: approval-mozilla-beta?
Attachment #8597380 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.