1322382 - SEGV on unknown address [@ GetUnit]

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes a SEGV at an unknown high address in mozilla-central rev c2526f6786f0.

==23126==ERROR: AddressSanitizer: SEGV on unknown address 0x000297d74003 (pc 0x7f46b2617d07 bp 0x7ffe56745d60 sp 0x7ffe56745880 T0)
    #0 0x7f46b2617d06 in GetUnit /home/worker/workspace/build/src/layout/style/nsCSSValue.h:639:38
    #1 0x7f46b2617d06 in nsStyleTransformMatrix::ReadTransforms(nsCSSValueList const*, nsStyleContext*, nsPresContext*, mozilla::RuleNodeCacheConditions&, nsStyleTransformMatrix::TransformReferenceBox&, float, bool*) /home/worker/workspace/build/src/layout/style/nsStyleTransformMatrix.cpp:929
    #2 0x7f46b238121e in mozilla::StyleAnimationValue::GetScaleValue(nsIFrame const*) const /home/worker/workspace/build/src/layout/style/StyleAnimationValue.cpp:4732:25
    #3 0x7f46b27c2983 in UpdateMinMaxScale /home/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:524:18
    #4 0x7f46b27c2983 in GetMinAndMaxScaleForAnimationProperty /home/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:561
    #5 0x7f46b27c2983 in nsLayoutUtils::ComputeSuitableScaleForAnimation(nsIFrame const*, nsSize const&, nsSize const&) /home/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:591
    #6 0x7f46b2f9b753 in ChooseScaleAndSetTransform /home/worker/workspace/build/src/layout/painting/FrameLayerBuilder.cpp:5317:15
    #7 0x7f46b2f9b753 in mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits> const*, unsigned int) /home/worker/workspace/build/src/layout/painting/FrameLayerBuilder.cpp:5478
    #8 0x7f46b305bd9a in nsDisplayTransform::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:6384:38

Comment 1

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.log

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I have no idea how to fix this crash yet, but I leave a comment what's going on there.

In case of 'details' element, frame reconstruction is processed, as a result, EffectCompositor::UpdateEffectProperties() is called, then the base style is cleared.  After the clearing the base style, we have no luck with resolving the base style.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba3adcc389f1c91b855720a8e67bd9a31e509e5b

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1322382 - Add EffectCompositor::GetBaseStyle for nsIFrame.

Currently this function is used when an nsIFrame is generated by reframing. In
reframing, we call EffectCompositor::UpdateEffectProperties, as a result, we
have to clear the base style that we resolved once. So, for such cases, we need
to re-resolve the base style with the style context that is associated with the
nsIFrame.

Review commit: https://reviewboard.mozilla.org/r/97878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97878/

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8817653 [details]
Bug 1322382 - Add EffectCompositor::GetBaseStyle for nsIFrame.

https://reviewboard.mozilla.org/r/97878/#review98242

::: dom/animation/EffectCompositor.h:231
(Diff revision 1)
>                                            dom::Element& aElement,
>                                            CSSPseudoElementType aPseudoType);
>  
> +  // Returns the base style corresponding to |aFrame|.
> +  // This function should be called only after restyle process has done, i.e.
> +  // the |aFrame| has a resolved style context.

Drop the "the" here, i.e. s/the |aFrame|/|aFrame/

::: dom/animation/EffectCompositor.cpp:928
(Diff revision 1)
> +/* static */ StyleAnimationValue
> +EffectCompositor::GetBaseStyle(nsCSSPropertyID aProperty,
> +                               const nsIFrame* aFrame)

This should go immediately after the other GetBaseStyle.

::: dom/animation/EffectCompositor.cpp:938
(Diff revision 1)
> +  MOZ_ASSERT(pseudoElement,
> +             "The frame should have an associated element");

Should we assert pseudoElement && pseudoElement->mElement?

(Seeing as we dereference mElement below)

::: dom/animation/EffectSet.h:202
(Diff revision 1)
> +  // Don't use this function directly, use EffectCompositor::GetBaseStyle()
> +  // instead.

"This function is intended to be called by EffectCompositor::GetBaseStyle and should not be called directly."

?

::: layout/base/nsLayoutUtils.cpp:558
(Diff revision 1)
>        // We need to factor in the scale of the base style if the base style
>        // will be used on the compositor.
>        if (effect->NeedsBaseStyle(prop.mProperty)) {
> -        EffectSet* effects = EffectSet::GetEffectSet(aFrame);
>          StyleAnimationValue baseStyle =
> -          effects->GetBaseStyle(prop.mProperty);
> +          EffectCompositor::GetBaseStyle(eCSSProperty_transform, aFrame);

Isn't it better to pass prop.mProperty here?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8817653 [details]
Bug 1322382 - Add EffectCompositor::GetBaseStyle for nsIFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97878/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35209805acb4
Add EffectCompositor::GetBaseStyle for nsIFrame. r=birtles

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/35209805acb4

Comment 10

[Tyson Smith [:tsmith]]

This should likely get a sec rating.

Comment 11

[Daniel Veditz [:dveditz]]

Hiro: please request sec-approval on this bug retroactively so we can evaluate it for other supported branches
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 12

[Hiroyuki Ikezoe (:hiro)]

This is a regression by bug 1305325, I don't thinks it needs to be uplifted.
I am sorry for less information.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8817653 [details]
Bug 1322382 - Add EffectCompositor::GetBaseStyle for nsIFrame.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
A test case is included in the patch.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
A test case is included in the patch.

Which older supported branches are affected by this flaw?
Firefox 53.

If not all supported branches, which bug introduced the flaw?
Bug 1305325.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This bug does not need backports.

How likely is this patch to cause regressions; how much testing does it need?
It's been for a month after landing the patch.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8817653 [details]
Bug 1322382 - Add EffectCompositor::GetBaseStyle for nsIFrame.

sec-approval+ for trunk.