Crashes in nsSVGOuterSVGFrame::IsSVGTransformed, on youtube.com videos

RESOLVED FIXED in Firefox 49

Status

()

Core
SVG
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: dbaron, Assigned: hiro)

Tracking

({crash, topcrash})

unspecified
mozilla50
Unspecified
Windows 7
crash, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox48 unaffected, firefox49+ fixed, firefox50 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-0475c899-d535-4707-8778-413312160602.
=============================================================

Yesterday's nightly had a spike in crashes with signature nsSVGOuterSVGFrame::IsSVGTransformed.  There were 10 crashes from 8 different users (1 user submitted 3 crashes).  Most of the crashes had a URL field beginning with https://www.youtube.com/watch .
(Reporter)

Comment 1

2 years ago
list of crashes:
https://crash-stats.mozilla.com/signature/?product=Firefox&release_channel=nightly&date=%3E%3D2016-05-01&signature=nsSVGOuterSVGFrame%3A%3AIsSVGTransformed
(Reporter)

Comment 2

2 years ago
Regression window from June 1 to June 2 builds:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=111970c738234569c8c180319155327316335deb&tochange=34a8be4346a9231e472fc36b1d7c0531e0fbf7c5
(Reporter)

Comment 3

2 years ago
Maybe bug 1273042?
Flags: needinfo?(hiikezoe)
(Reporter)

Comment 4

2 years ago
Yeah, this is almost certainly a regression from https://hg.mozilla.org/integration/mozilla-inbound/rev/df16848a29e3
Blocks: 1273042
status-firefox48: --- → unaffected
status-firefox49: --- → affected
tracking-firefox49: --- → ?
(Assignee)

Comment 5

2 years ago
Ah, nsSVGOuterSVGFrame::IsSVGTransformed checks child list but the list is not yet set in nsFrame::Init().  We should skip KeyframeEffectReadOnly::ShouldBlockAsyncTransformAnimations.  Transform animations with preserved-3d or backface-visibility:hidden don't create stacking context.
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hiikezoe)
(Assignee)

Comment 6

2 years ago
Created attachment 8760105 [details]
Bug 1277991 - We don't need to check ShouldBlockAsyncTransformAnimations() when we just want to know whether the frame has transform animations or not.

Review commit: https://reviewboard.mozilla.org/r/57820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57820/
Attachment #8760105 - Flags: review?(bbirtles)
(Assignee)

Comment 7

2 years ago
I also write a test case with 'backface-visibility:hidden' but it does not create stacking context for some reason.  I will handle the case in a later bug.
(Assignee)

Comment 8

2 years ago
Comment on attachment 8760105 [details]
Bug 1277991 - We don't need to check ShouldBlockAsyncTransformAnimations() when we just want to know whether the frame has transform animations or not.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57820/diff/1-2/
(Assignee)

Comment 9

2 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> I also write a test case with 'backface-visibility:hidden' but it does not
> create stacking context for some reason.  I will handle the case in a later
> bug.

I am sorry, the test case was accidentally included in the previous patch.
(Assignee)

Comment 10

2 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> I also write a test case with 'backface-visibility:hidden' but it does not
> create stacking context for some reason.  I will handle the case in a later
> bug.

Bug 1278136.
https://reviewboard.mozilla.org/r/57820/#review54644

It's not obvious to me what this patch is doing. It's probably right, but any extra comments or explanation would really help in reviewing.

::: dom/animation/EffectCompositor.h:150
(Diff revision 2)
>    }
>  
>    static bool HasAnimationsForCompositor(const nsIFrame* aFrame,
>                                           nsCSSProperty aProperty);
>  
> +  static bool MayBeTransformed(const nsIFrame* aFrame);

This needs a comment.

(Also, it shouldn't go between HasAnimationsForCompositor and GetAnimationsForCompositor since they are a pair.)

::: dom/animation/EffectCompositor.cpp:70
(Diff revision 2)
>  //
>  // Returns true if there are eligible animations, false otherwise.
>  bool
>  FindAnimationsForCompositor(const nsIFrame* aFrame,
>                              nsCSSProperty aProperty,
> +                            Candidate aCandidate,

We need to update the comment for this function to explain what this parameter means.
(Assignee)

Comment 12

2 years ago
Comment on attachment 8760105 [details]
Bug 1277991 - We don't need to check ShouldBlockAsyncTransformAnimations() when we just want to know whether the frame has transform animations or not.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57820/diff/2-3/
(Assignee)

Comment 13

2 years ago
https://reviewboard.mozilla.org/r/57820/#review54644

> This needs a comment.
> 
> (Also, it shouldn't go between HasAnimationsForCompositor and GetAnimationsForCompositor since they are a pair.)

Moved after GetAnimationsForCompositor, and added a comment for the function.

> We need to update the comment for this function to explain what this parameter means.

Updated the comment for FindAnimationsForCompositor.
https://reviewboard.mozilla.org/r/57820/#review54652

::: dom/animation/EffectCompositor.h:154
(Diff revision 3)
> +  // Returns true if the frame has animations of transform regardless of
> +  // whether the transform can be run on the compositor or not.
> +  static bool MayBeTransformed(const nsIFrame* aFrame);
> +

Can't we just use nsLayoutUtils::HasCurrentAnimationOfProperty for this?
(Assignee)

Comment 15

2 years ago
Yes, we can, but just for now. Since we are going to create stacking context in delay phase in bug 1223658, if we use nsLayoutUtils::HasCurrentAnimationOfProperty here, we have to add nsLayoutUtils::HasPlayableAnimationOfPropertyOnCompositor (which involves adding HasMatchingPlayableAnimations or move IsCurrent check into each caller side) in that bug.  Do you prefer it?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> Yes, we can, but just for now. Since we are going to create stacking context
> in delay phase in bug 1223658, if we use
> nsLayoutUtils::HasCurrentAnimationOfProperty here, we have to add
> nsLayoutUtils::HasPlayableAnimationOfPropertyOnCompositor (which involves
> adding HasMatchingPlayableAnimations or move IsCurrent check into each
> caller side) in that bug.  Do you prefer it?

Animations in their delay phase are still current. Do you mean that if this lands first it will return true for animations in their delay phase, but it shouldn't?
(Assignee)

Comment 17

2 years ago
I made a mistake.  I was also thinking after phase.  Yes, we can use HasCurrentAnimationOfProperty here. I will revise the patch.
(Assignee)

Comment 18

2 years ago
Comment on attachment 8760105 [details]
Bug 1277991 - We don't need to check ShouldBlockAsyncTransformAnimations() when we just want to know whether the frame has transform animations or not.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57820/diff/3-4/
Attachment #8760105 - Flags: review?(bbirtles) → review+
Comment on attachment 8760105 [details]
Bug 1277991 - We don't need to check ShouldBlockAsyncTransformAnimations() when we just want to know whether the frame has transform animations or not.

https://reviewboard.mozilla.org/r/57820/#review54660

Comment 20

2 years ago
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02145adb5fc0
We don't need to check ShouldBlockAsyncTransformAnimations() when we just want to know whether the frame has transform animations or not. r=birtles
Crash Signature: [@ nsSVGOuterSVGFrame::IsSVGTransformed] → [@ nsSVGOuterSVGFrame::IsSVGTransformed] [@ nsSVGOuterSVGFrame::IsSVGTransformed const]
Firefox crashed having the same signature as the one from this bug with the following steps:

1. Open youtube.com and play a video
2. Click Settings
3. Click Quality button and switch between Auto mode and 144p mode more that 2 times


Expected results:

The video should play without crashing 


Actual results:

Firefox crashes: 
https://crash-stats.mozilla.com/report/index/5fe61d1b-f45f-475a-b77c-fb3dd2160607

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/02145adb5fc0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Comment 23

2 years ago
(In reply to roxana.leitan from comment #21)
> Firefox crashed having the same signature as the one from this bug with the
> following steps:
> 
> 1. Open youtube.com and play a video
> 2. Click Settings
> 3. Click Quality button and switch between Auto mode and 144p mode more that
> 2 times
> 
> 
> Expected results:
> 
> The video should play without crashing 

Thanks for checking.   This bug has been fixed now but nightly with this fix has not released yet.  Please try the next nightly.

Updated

2 years ago
Duplicate of this bug: 1278093
(Reporter)

Comment 25

2 years ago
Please request aurora approval.
Flags: needinfo?(hiikezoe)
(Assignee)

Comment 26

2 years ago
Comment on attachment 8760105 [details]
Bug 1277991 - We don't need to check ShouldBlockAsyncTransformAnimations() when we just want to know whether the frame has transform animations or not.

Approval Request Comment
[Feature/regressing bug #]: bug 1273042
[User impact if declined]: crashes on sites which has CSS animations/transitions on specific SVG elements
[Describe test coverage new/current, TreeHerder]: This patch includes test cases.
[Risks and why]: Low risk because it's one line fix and the function which is called on the new line is much more simple than the replaced function.
[String/UUID change made/needed]: None
Flags: needinfo?(hiikezoe)
Attachment #8760105 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 27

2 years ago
This is the #1 non-shutdown crash on Aurora.  It should be approved.
Comment on attachment 8760105 [details]
Bug 1277991 - We don't need to check ShouldBlockAsyncTransformAnimations() when we just want to know whether the frame has transform animations or not.

Shutdown hang fix, please uplift to aurora.
Attachment #8760105 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Roxana, would you mind verifying this fix on aurora, once it lands there? Thanks!
Flags: needinfo?(roxana.leitan)
Tracking 49+ for this top crash.
tracking-firefox49: ? → +

Comment 31

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/600b78459494
status-firefox49: affected → fixed
20160628004053 Mozilla/5.0 (Windows NT 6.1; rv:49.0) Gecko/20100101 Firefox/49.0

I can confirm the issue is fixed on latest Aurora 49.0a2 build ID:20160628004053 and latest Nightly build 20160628004053 when following the STR from Comment 21.
Flags: needinfo?(roxana.leitan)
Crash volume for signature 'nsSVGOuterSVGFrame::IsSVGTransformed':
 - nightly (version 50): 45 crashes from 2016-06-06.
 - aurora  (version 49): 1165 crashes from 2016-06-07.
 - beta    (version 48): 1 crash from 2016-06-06.
 - release (version 47): 3 crashes from 2016-05-31.
 - esr     (version 45): 0 crash from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          0          1          0          1         42
 - aurora          14         13         19        157        439        423         96
 - beta             0          0          0          0          0          1          0
 - release          0          0          0          0          0          1          1
 - esr              0          0          0          0          0          0          0

Affected platforms: Windows, Linux
status-firefox47: --- → affected

Updated

a year ago
Depends on: 1319378
(Reporter)

Comment 34

a year ago
For the record, the stack from the report in comment 0 was:

 0 	xul.dll 	nsSVGOuterSVGFrame::IsSVGTransformed(mozilla::gfx::Matrix*, mozilla::gfx::Matrix*) 	layout/svg/nsSVGOuterSVGFrame.h:109
1 	xul.dll 	mozilla::dom::KeyframeEffectReadOnly::CanAnimateTransformOnCompositor(nsIFrame const*, mozilla::AnimationPerformanceWarning::Type&) 	dom/animation/KeyframeEffect.cpp:1270
2 	xul.dll 	mozilla::dom::KeyframeEffectReadOnly::ShouldBlockAsyncTransformAnimations(nsIFrame const*, mozilla::AnimationPerformanceWarning::Type&) 	dom/animation/KeyframeEffect.cpp:1305
3 	xul.dll 	mozilla::FindAnimationsForCompositor(nsIFrame const*, nsCSSProperty, nsTArray<RefPtr<mozilla::dom::Animation> >*) 	dom/animation/EffectCompositor.cpp:114
4 	xul.dll 	nsFrame::Init(nsIContent*, nsContainerFrame*, nsIFrame*) 	layout/generic/nsFrame.cpp:558
5 	xul.dll 	nsSplittableFrame::Init(nsIContent*, nsContainerFrame*, nsIFrame*) 	layout/generic/nsSplittableFrame.cpp:22
6 	xul.dll 	nsContainerFrame::Init(nsIContent*, nsContainerFrame*, nsIFrame*) 	layout/generic/nsContainerFrame.cpp:53
7 	xul.dll 	nsSVGOuterSVGFrame::Init(nsIContent*, nsContainerFrame*, nsIFrame*) 	layout/svg/nsSVGOuterSVGFrame.cpp:115
8 	xul.dll 	nsCSSFrameConstructor::InitAndRestoreFrame(nsFrameConstructorState const&, nsIContent*, nsContainerFrame*, nsIFrame*, bool) 	layout/base/nsCSSFrameConstructor.cpp:4888
9 	xul.dll 	nsCSSFrameConstructor::ConstructFrameWithAnonymousChild(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsFrameItems&, nsContainerFrame* (*)(nsIPresShell*, nsStyleContext*), nsContainerFrame* (*)(nsIPresShell*, nsStyleContext*), nsICSSAnonBoxPseudo*, bool) 	layout/base/nsCSSFrameConstructor.cpp:5133
10 	xul.dll 	nsCSSFrameConstructor::ConstructOuterSVG(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&) 	layout/base/nsCSSFrameConstructor.cpp:5190
11 	xul.dll 	nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) 	layout/base/nsCSSFrameConstructor.cpp:3856
12 	xul.dll 	nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) 	layout/base/nsCSSFrameConstructor.cpp:6085
13 	xul.dll 	nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, nsFrameItems&) 	layout/base/nsCSSFrameConstructor.cpp:10502
14 	xul.dll 	nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) 	layout/base/nsCSSFrameConstructor.cpp:10703

and it was against revision:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34a8be4346a9
You need to log in before you can comment on or make changes to this bug.