1402547 - crash near null in [@ AddOrAccumulate]

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

==11230==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fc2ee9ac4f8 bp 0x7ffc4d203c20 sp 0x7ffc4d2039a0 T0)
==11230==The signal is caused by a READ memory access.
==11230==Hint: address points to the zero page.
    #0 0x7fc2ee9ac4f7 in AddOrAccumulate(nsSMILValue&, nsSMILValue const&, mozilla::dom::CompositeOperation, unsigned long) /src/dom/smil/nsSMILCSSValueType.cpp:365:30
    #1 0x7fc2ee9ac3bb in nsSMILCSSValueType::SandwichAdd(nsSMILValue&, nsSMILValue const&) const /src/dom/smil/nsSMILCSSValueType.cpp:422:10
    #2 0x7fc2ee9a2f77 in nsSMILAnimationFunction::ComposeResult(nsISMILAttr const&, nsSMILValue&) /src/dom/smil/nsSMILAnimationFunction.cpp:271:22
    #3 0x7fc2ee99f6ed in nsSMILCompositor::ComposeAttribute(bool&) /src/dom/smil/nsSMILCompositor.cpp:108:29
    #4 0x7fc2ee99cf9a in nsSMILAnimationController::DoSample(bool) /src/dom/smil/nsSMILAnimationController.cpp:455:17
    #5 0x7fc2ef6a6a5b in Resample /src/obj-firefox/dist/include/nsSMILAnimationController.h:74:21
    #6 0x7fc2ef6a6a5b in FlushResampleRequests /src/obj-firefox/dist/include/nsSMILAnimationController.h:90
    #7 0x7fc2ef6a6a5b in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /src/layout/base/PresShell.cpp:4160
    #8 0x7fc2eb5f420d in FlushPendingNotifications /src/obj-firefox/dist/include/nsIPresShell.h:557:5
    #9 0x7fc2eb5f420d in nsDocument::FlushPendingNotifications(mozilla::FlushType) /src/dom/base/nsDocument.cpp:8345
    #10 0x7fc2ea427f5b in nsDocLoader::DocLoaderIsEmpty(bool) /src/uriloader/base/nsDocLoader.cpp:703:14
    #11 0x7fc2ea42a1e5 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /src/uriloader/base/nsDocLoader.cpp:632:5
    #12 0x7fc2ea42ae4c in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /src/uriloader/base/nsDocLoader.cpp:488:14
    #13 0x7fc2e8c2e96d in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /src/netwerk/base/nsLoadGroup.cpp:629:28
    #14 0x7fc2eb5fa23d in nsDocument::DoUnblockOnload() /src/dom/base/nsDocument.cpp:9173:18
    #15 0x7fc2eb5f9e01 in nsDocument::UnblockOnload(bool) /src/dom/base/nsDocument.cpp:9095:9
    #16 0x7fc2eb5d3249 in nsDocument::DispatchContentLoadedEvents() /src/dom/base/nsDocument.cpp:5599:3
    #17 0x7fc2eb673e32 in applyImpl<nsDocument, void (nsDocument::*)()> /src/obj-firefox/dist/include/nsThreadUtils.h:1142:12
    #18 0x7fc2eb673e32 in apply<nsDocument, void (nsDocument::*)()> /src/obj-firefox/dist/include/nsThreadUtils.h:1148
    #19 0x7fc2eb673e32 in mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() /src/obj-firefox/dist/include/nsThreadUtils.h:1192
    #20 0x7fc2e8a80f8d in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14
    #21 0x7fc2e8a86cc8 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:521:10
    #22 0x7fc2e982b4c1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
    #23 0x7fc2e978d38b in RunInternal /src/ipc/chromium/src/base/message_loop.cc:326:10
    #24 0x7fc2e978d38b in RunHandler /src/ipc/chromium/src/base/message_loop.cc:319
    #25 0x7fc2e978d38b in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299
    #26 0x7fc2eef3b10f in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
    #27 0x7fc2f3095c81 in nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
    #28 0x7fc2f32767cb in XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4701:22
    #29 0x7fc2f32783c8 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4865:8
    #30 0x7fc2f32797fb in XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4960:21
    #31 0x4ebea3 in do_main /src/browser/app/nsBrowserApp.cpp:236:22
    #32 0x4ebea3 in main /src/browser/app/nsBrowserApp.cpp:309
    #33 0x7fc304160f44 in __libc_start_main /build/eglibc-SvCtMH/eglibc-2.19/csu/libc-start.c:287
    #34 0x41d9f8 in _start (firefox+0x41d9f8)

Comment 1

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: ca8440c0eeb360d1eaa6b5bbad829bd1646adb35
INFO: First bad revision: 42be15d3ed1b3075f56e298dc692bdb88f70d3a0
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ca8440c0eeb360d1eaa6b5bbad829bd1646adb35&tochange=42be15d3ed1b3075f56e298dc692bdb88f70d3a0

Comment 2

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

In a debug build the following assertion is failing:

  MOZ_ASSERT(destWrapper || valueToAddWrapper,
             "need at least one fully-initialized value");

I only have a debug opt build handy so I'm just guessing here, but I suspect the issue is that with the way we fetch base values after bug 1315874, we no longer flush styles and end up with an empty dest value when we didn't used to.

Furthermore, since the animation doesn't have a 'dur' attribute, the duration will be indefinite, so we'll never move past the  initial zero value of the by animation. As a result, we'll end trying to add two empty values together.

I'm not sure yet what the best fix is here, but if getting a resolved base style means flushing styles here (something which I think we were trying to avoid in bug 1171966), we might just decide to special case the "two empty values" case and return the appropriate zero value there (since I believe "empty" corresponds to "zero" in general).

I'll need to get a proper debug build going to be sure and I'm quite busy the next 3 days with MDN roadshow but assigning to me for now.

Comment 4

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

So I finally got 5 minutes to look into this, but that's all. I'll look into it properly on Monday.

A few things I noticed, however, are:

* This might only reproduces with calcMode='discrete' because in that case we *don't* call Interpolate which
  is where we'd normally convert the "empty" by-animation zero value into a proper zero value.
* The error actually shows up when we composite the second animation onto the first since in this case we *do*
  successfully resolve a base value (but we should probably harden this code so that it works even when we
  don't have a base value).

So in effect we do:

1. Get base value: gets a height value of some sort but I haven't checked exactly what yet
2. Run first animation function:
 2a. Interpolate produces "empty" zero value (the by-animation's "effective from value") since we are doing
     discrete animation.
 2b. SandwichAdd tries to add "empty" zero value with valid base value
 2c. FinalizeServoAnimationValues tries to promote "empty" value to a zero value but fails
 2d. Servo_AnimationValues_GetZeroValue returns null but I haven't dug into why yet. It probably depends on
     what we've stored in the base value.
 2e. SandwichAdd fails so we end up just setting the result to the "empty" zero value (the usual "failed to
     add" fallback behavior)
3. Run the second animation function:
 3a. Interpolate produces "empty" zero value
 3b. SandwichAdd tries to extract the values to work with only has two "empty" values and then it's all
     downhill from there.

So there seem to be multiple problems here:

- Firstly, 2d (and possibly 1 as well). We probably shouldn't be getting a null result here.
- Secondly, 2a. I wonder if we should really be returning an "empty" value when doing discrete interpolation.
- Thirdly, 3b. I wonder if we could do something more sensible when we are given two "empty" values. e.g. if
  we pass in the by-animation to-value as well, we could probably use it to get a valid zero value. That might
  complicate things too much though.

Comment 5

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

(In reply to Brian Birtles (:birtles) from comment #4)
>  2d. Servo_AnimationValues_GetZeroValue returns null but I haven't dug into
> why yet. It probably depends on
>      what we've stored in the base value.

It turns out that in this case we're getting 'auto' for the base value and we can't turn that into a zero value because we have:

  impl ToAnimatedZero for LengthOrPercentageOrAuto {
      #[inline]
      fn to_animated_zero(&self) -> Result<Self, ()> {
          match *self {
              LengthOrPercentageOrAuto::Length(_) |
              LengthOrPercentageOrAuto::Percentage(_) |
              LengthOrPercentageOrAuto::Calc(_) => {
                  Ok(LengthOrPercentageOrAuto::Length(Length::new(0.)))
              },
              LengthOrPercentageOrAuto::Auto => Err(()),
          }
      }
  }

> - Firstly, 2d (and possibly 1 as well). We probably shouldn't be getting a
> null result here.

Actually, maybe this is ok. For the above case I'm not sure there is a suitable zero value here except perhaps Auto itself?

> - Secondly, 2a. I wonder if we should really be returning an "empty" value
> when doing discrete interpolation.

Based on the next point, this might just be ok.

> - Thirdly, 3b. I wonder if we could do something more sensible when we are
> given two "empty" values. e.g. if
>   we pass in the by-animation to-value as well, we could probably use it to
> get a valid zero value. That might
>   complicate things too much though.

Given that nsSMILCSSProperty::SetAnimValue seems quite capable of handling these empty values (it just calls nsSMILCSSValueType::ValueToString which, if the value is empty, will return early leaving the passed-in empty string untouched) I suspect we can fix this by just making AddOrAccumulate (and probably Interpolate too, although I'm not sure how we'll test that) return an empty value when the two inputs are empty.

Comment 6

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

Attachment: Bug 1402547 - Handle addition/interpolate of nsSMILCSSValueType objects when both objects are empty;

In some rare cases we can end up with both arguments passed to
nsSMILCSSValueType::SandwichAdd being "empty", that is, having the type
nsSMILCSSValueType but a null pointer.

This can happen, for example, when we have a two by-animations and linear
interpolation causing us to pass the "empty" from-value up as the underlying
value from the first by-animation to the second by-animation (which it will try
to add with its empty from-value).

In this case the result of adding "empty" to "empty" should just be "empty"
which we achieve by just doing an early return (since the fallback behavior for
failed addition is to just use the second argument as the result; note that if
we return true though the result would also be the same).

We don't currently have a test case that produces this for interpolate but it
makes sense that trying to interpolate "empty" to "empty" should likewise fail
and just produce "empty". In this case failing will make us fall back to
discrete animation and just use the "empty" values as-is. Indicating failure,
however, has the additional effect of making us use the special keyTimes
behavior defined for discrete animation.

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

Comment 7

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

Attachment: Bug 1402547 - Add crashtest for multiple empty SMIL values;

I have verified that this fails without the code changes from the previous patch
in this series, and passes with them.

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

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8914170 [details]
Bug 1402547 - Handle addition/interpolate of nsSMILCSSValueType objects when both objects are empty;

https://reviewboard.mozilla.org/r/185506/#review190412

This patch makes sense to me. One thing I am wondering we should add reftest or mochitest for the fallback case.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8914171 [details]
Bug 1402547 - Add crashtest for multiple empty SMIL values;

https://reviewboard.mozilla.org/r/185508/#review190414

Comment 10

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

I went to write a mochitest for the fallback case but I'm not entirely sure what the behavior should be for this case:

  <svg id=svg>
  <animate id="anim-a" calcMode="discrete" attributeName="height" by="10"
    dur="1s" fill="freeze"/>
  <animate id="anim-b" calcMode="discrete" attributeName="height" by="10"
    dur="1s" fill="freeze"/>

The underlying height of <svg> is 'auto' which produces a computed value of '150px'.

After 1s, the animated height will be '20px'. However, during the first 1s what should it be? 'auto' or '0px'? I believe there are valid arguments for both although I lean towards saying '0px' (i.e. treat it as non-additive for the whole time).

Chrome's behavior here makes no sense to me. It uses 150px during the start of the animation but at the end I get 770px (or 460px if I use just one animation). It's like it has taken the default width of 300px, then added 10px to that, then added that to the auto value? Nevertheless, I think that so long as we say we can't add 10px to auto, we should neither try to add "empty" to auto in this case.

Making this work, however, requires some significant changes. Specifically, I think we want to do the empty-value fixup that we normally do when interpolating, even in the discrete case. It might make more sense to handle that as a separate bug even.

Comment 11

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

(In reply to Brian Birtles (:birtles) from comment #10)
> Making this work, however, requires some significant changes. Specifically,
> I think we want to do the empty-value fixup that we normally do when
> interpolating, even in the discrete case. It might make more sense to handle
> that as a separate bug even.

Filed bug 1404803 for this.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39239b51da8d
Handle addition/interpolate of nsSMILCSSValueType objects when both objects are empty; r=hiro
https://hg.mozilla.org/integration/autoland/rev/bae812b6b31b
Add crashtest for multiple empty SMIL values; r=hiro

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/39239b51da8d
https://hg.mozilla.org/mozilla-central/rev/bae812b6b31b

Comment 14

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

Comment on attachment 8914170 [details]
Bug 1402547 - Handle addition/interpolate of nsSMILCSSValueType objects when both objects are empty;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1315874
[User impact if declined]: Content process crash with certain animated SVG content (rare)
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No. It has just landed in m-c 4 hours ago so I don't believe it has been included in any nightly builds yet.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None, although attachment 8914171 [details] contains a crashtest that could also be uplifted if desired.
[Is the change risky?]: No
[Why is the change risky/not risky?]: It simply replaces three debug-build assertions with corresponding early returns.
[String changes made/needed]: None

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8914170 [details]
Bug 1402547 - Handle addition/interpolate of nsSMILCSSValueType objects when both objects are empty;

Philipp helped on this one. I am told [@ AddOrAccumulate] signature is not on seen on crash stats, given the lack of severity here, recommend this fix ride the 58 train.