1604180 - Crash in [@ mozilla::gfx::DrawTargetD2D1::OptimizeSourceSurface]

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P1)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

This bug is for crash report bp-220effc3-85b5-4d4a-bc50-7fefe0191216.

Top 10 frames of crashing thread:

0 xul.dll mozilla::gfx::DrawTargetD2D1::OptimizeSourceSurface const gfx/2d/DrawTargetD2D1.cpp:2222
1 xul.dll static nsLayoutUtils::SurfaceFromElement layout/base/nsLayoutUtils.cpp:7614
2 xul.dll static nsLayoutUtils::SurfaceFromElement layout/base/nsLayoutUtils.cpp:7774
3 xul.dll mozilla::dom::CanvasRenderingContext2D::DrawImage dom/canvas/CanvasRenderingContext2D.cpp:4430
4 xul.dll mozilla::dom::CanvasRenderingContext2D_Binding::drawImage dom/bindings/CanvasRenderingContext2DBinding.cpp
5 xul.dll mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3151
6 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:549
7 xul.dll Interpret js/src/vm/Interpreter.cpp:3116
8 xul.dll js::RunScript js/src/vm/Interpreter.cpp:424
9 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:590

The earliest crash with this signature has bug id 20191209215019

Comment 1

[Kris Taeleman (:ktaeleman)]

Hey Bob, this looks related to the following changes that landed in that release:
mozilla-central: pushlog

908c5b3e65b491cfcd71acedc853f989191a3e22**Bob Owen — Bug 1598582 Part 2: Add event to record OptimizeSourceSurface. r=jrmuizel
** 593bc922d369bab405331fc9a5496d21303bdf2b**Bob Owen — Bug 1598582: Create a DataSourceSurface in RecordedSourceSurfaceCreation::PlayEvent. r=jrmuizel
** 604a9ee956073ad6494f37ba7e18a08c78b50cbfBob Owen — Bug 1572415: Convert clip paths when potentially changing canvas backend in CanvasRenderingContext2D. r=jrmuizel

Comment 2

[Bob Owen (:bobowen)]

I think this can only be happening when falling back from remote canvas 2D.
It is trying to optimize a SourceSurfaceRecording and if the remote playback has failed the we can't get the data.
I probably just need to add in a null check for the return from aSurface->GetDataSurface();

Comment 3

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

:bobowen, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 4

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

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 5

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #2)

I think this can only be happening when falling back from remote canvas 2D.
It is trying to optimize a SourceSurfaceRecording and if the remote playback has failed the we can't get the data.
I probably just need to add in a null check for the return from aSurface->GetDataSurface();

This conclusion was wrong, this appears to be happening when other (non-canvas) recorded surfaces are optimized and then passed to canvas code.
Bug 1598582, changed things to properly record the optimization (it previously returned the original surface), but this means the returned surface then returns null from GetDataSurface.

I think the simplest thing is to allow the optimized SourceSurfaceRecording to keep a reference to the original surface, so it can be used when GetDataSurface is called.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Bug 1604180: Use original surface for GetDataSurface after DrawTargetRecording::OptimizeSourceSurface. r=jrmuizel!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3be461cff92
Use original surface for GetDataSurface after DrawTargetRecording::OptimizeSourceSurface. r=jrmuizel

Comment 8

[Bob Owen (:bobowen)]

Comment on attachment 9120462 [details]
Bug 1604180: Use original surface for GetDataSurface after DrawTargetRecording::OptimizeSourceSurface. r=jrmuizel!

Beta/Release Uplift Approval Request

• User impact if declined: Fairly frequent content process crash (top 10 content crasher on beta).
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple change, which means the original surface that was optimized is used to get the data surface when dealing with DrawTargetRecording.
  This should be effectively the same as happened before the change that caused the crash.
• String changes made/needed: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9120462 [details]
Bug 1604180: Use original surface for GetDataSurface after DrawTargetRecording::OptimizeSourceSurface. r=jrmuizel!

Let's get this into 73.0b6 to see what affect it has on the Beta population. Thanks for the patch, Bob!

Comment 10

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5ba85903b676

Comment 11

[Natalia Csoregi [:nataliaCs]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

This was backed out from Beta73 for causing bug 1604800. It remains landed on m-c for 74+.