Closed Bug 1277991 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Platform:
Unspecified
Windows 7
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox47 --- affected
firefox48 --- unaffected
firefox49 + fixed
firefox50 --- fixed

People

(Reporter: dbaron, Assigned: hiro)

References

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(1 file)

Bug 1277991 - We don't need to check ShouldBlockAsyncTransformAnimations() when we just want to know whether the frame has transform animations or not.
58 bytes, text/x-review-board-request
birtles
: review+
lizzard
: approval-mozilla-aurora+
Details 
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 .
Maybe bug 1273042?
Flags: needinfo?(hiikezoe)
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)
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.
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/
(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.
(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.
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/
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?
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?
I made a mistake.  I was also thinking after phase.  Yes, we can use HasCurrentAnimationOfProperty here. I will revise the patch.
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
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
https://hg.mozilla.org/mozilla-central/rev/02145adb5fc0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(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.
Please request aurora approval.
Flags: needinfo?(hiikezoe)
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?
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: ? → +
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
Depends on: 1319378
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.

Attachment

General

Creator:
Created:
Updated:
Size: