Firefox 48 on Windows 10 hangs on an SVG file with hardware acceleration enabled

RESOLVED FIXED in Firefox 50

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: zoltan, Assigned: bas.schouten, NeedInfo)

Tracking

(Depends on 1 bug, {hang, regression})

48 Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox-esr45 unaffected, firefox50 fixed, firefox51 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 attachments)

Posted image poli-brand-logo.svg
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160804004004

Steps to reproduce:

A web page I developed caused Firefox 48 on Windows 10 to hang. I removed lines of code until I arrived at a specific SVG file that caused it. Loading the SVG file by itself also caused the hang. Firefox 48 on Mac OS X renders the SVG. With hardware acceleration off, the SVG renders. It happened with this specific SVG (attached), other SVG files loaded fine.


Actual results:

Firefox didn't load the page, displayed (Not responding) in the window title, kept using CPU.


Expected results:

The SVG should have rendered.
sounds similar issue to bug 1293134.
Component: Untriaged → SVG
zoltan, can you paste your graphics section from about:supports into this bug?
Flags: needinfo?(mstange)
I can reproduce this on Windows 10 with Direct2D 1.1. I don't need the about:supports information any more.
Summary: Firefox 48 on Windows 10 crashes on an SVG file with hardware acceleration enabled → Firefox 48 on Windows 10 hangs on an SVG file with hardware acceleration enabled
Component: SVG → Graphics
Duplicate of this bug: 1293134
So this just does a lot of waiting in Direct2D, for multiple minutes. Bas, can you take a look?

Before bug 1238496, we were doing blend modes using BasicLayers always, which would have used D2D. Then after bug 1238496, we were doing blend modes on the compositor always. Then bug 1248913 made us do blend modes on the compositor only when necessary. There are no active layers being blended in the testcase in this bug, so we do the blending using BasicLayers with Direct2D.
My guess is that something changed in how we render blend modes with Direct2D between the landings of bug 1238496 and bug 1248913, i.e. between 2016-02-03 and 2016-04-07.
Flags: needinfo?(bas)
(In reply to Markus Stange [:mstange] from comment #6)
> So this just does a lot of waiting in Direct2D, for multiple minutes. Bas,
> can you take a look?
> 
> Before bug 1238496, we were doing blend modes using BasicLayers always,
> which would have used D2D. Then after bug 1238496, we were doing blend modes
> on the compositor always. Then bug 1248913 made us do blend modes on the
> compositor only when necessary. There are no active layers being blended in
> the testcase in this bug, so we do the blending using BasicLayers with
> Direct2D.
> My guess is that something changed in how we render blend modes with
> Direct2D between the landings of bug 1238496 and bug 1248913, i.e. between
> 2016-02-03 and 2016-04-07.

Nothing really happened in there except the removal of D2D 1.0, but that didn't really affect how we do blending. I'll have a look.
Flags: needinfo?(bas)
(In reply to Markus Stange [:mstange] from comment #6)
> So this just does a lot of waiting in Direct2D, for multiple minutes. Bas,
> can you take a look?
> 
> Before bug 1238496, we were doing blend modes using BasicLayers always,
> which would have used D2D. Then after bug 1238496, we were doing blend modes
> on the compositor always. Then bug 1248913 made us do blend modes on the
> compositor only when necessary. There are no active layers being blended in
> the testcase in this bug, so we do the blending using BasicLayers with
> Direct2D.
> My guess is that something changed in how we render blend modes with
> Direct2D between the landings of bug 1238496 and bug 1248913, i.e. between
> 2016-02-03 and 2016-04-07.

This is related to the PushGroup/PopGroup work that we did. But this landed in November 2015 already.
Comment on attachment 8781007 [details]
Bug 1293586: Don't use command lists for an effect when that command list already has an effect with a command list used inside of it.

https://reviewboard.mozilla.org/r/71510/#review69170

I'm not the best person to review this code, but the logic seems sound.

::: gfx/2d/DrawTargetD2D1.cpp:1098
(Diff revision 1)
>  #ifdef _DEBUG
>    options.debugLevel = D2D1_DEBUG_LEVEL_WARNING;
>  #else
>    options.debugLevel = D2D1_DEBUG_LEVEL_NONE;
>  #endif
> -  //options.debugLevel = D2D1_DEBUG_LEVEL_INFORMATION;
> +  options.debugLevel = D2D1_DEBUG_LEVEL_INFORMATION;

Left over from debugging?
Attachment #8781007 - Flags: review?(mstange) → review+
Whiteboard: [gfx-noted]
Posted file Minimal testcase
Assignee: nobody → bas
Status: NEW → ASSIGNED
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f0e64326ce2b
Don't use command lists for an effect when that command list already has an effect with a command list used inside of it. r=mstange
https://hg.mozilla.org/mozilla-central/rev/f0e64326ce2b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Bas please consider uplift to 50 and 49.
Flags: needinfo?(bas)
Comment on attachment 8781007 [details]
Bug 1293586: Don't use command lists for an effect when that command list already has an effect with a command list used inside of it.

Approval Request Comment
[Feature/regressing bug #]: 1220629
[User impact if declined]: Hang when certain blending features are used.
[Describe test coverage new/current, TreeHerder]: Nightly test coverage
[Risks and why]: Fairly low, some small internal changes theoretically leading to slightly higher memory usage and small performance hit in some specific usecases.
[String/UUID change made/needed]: None

I think this patch is too risky to uplift to beta in this stage of the cycle.
Flags: needinfo?(bas)
Attachment #8781007 - Flags: approval-mozilla-aurora?
Thanks for your careful consideration Bas. Marking wontfix for 49.
Am not sure how to read "status: resolved fixed // wontfix" on this one. Does it mean that the instance I reported at http://cs.sru.edu/~ddailey/svg/modifier3RBlendSnapMode.svg won't be fixed?
Hello Zoltan, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(zoltan)
(In reply to david.dailey from comment #23)
> Am not sure how to read "status: resolved fixed // wontfix" on this one.
> Does it mean that the instance I reported at
> http://cs.sru.edu/~ddailey/svg/modifier3RBlendSnapMode.svg won't be fixed?

Hello David, this issue will not be backported to Fx48 (current release version) and neither will it be fixed in Fx49 (next release version, supposed to go live in ~10 days). However, this issue should be fixed in Firefox version 50 and on wards. Hope that helps!
Comment on attachment 8781007 [details]
Bug 1293586: Don't use command lists for an effect when that command list already has an effect with a command list used inside of it.

Fixed a hang in specific scenarios, Aurora50+
Attachment #8781007 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Ritu Kothari (:ritu) from comment #25)
> (In reply to david.dailey from comment #23)
> > Am not sure how to read "status: resolved fixed // wontfix" on this one.
> > Does it mean that the instance I reported at
> > http://cs.sru.edu/~ddailey/svg/modifier3RBlendSnapMode.svg won't be fixed?
> 
> Hello David, this issue will not be backported to Fx48 (current release
> version) and neither will it be fixed in Fx49 (next release version,
> supposed to go live in ~10 days). However, this issue should be fixed in
> Firefox version 50 and on wards. Hope that helps!

Absolutely! Thanks!
See Also: → 1300338
Depends on: 1332558
See Also: → 1392453
You need to log in before you can comment on or make changes to this bug.