1334583 - WebAnimation crash: Assertion failure: !IsNull() (Cannot compute with a null value) [@mozilla::EnableIf]

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase-webanimations-3.html

Tested with https://hg.mozilla.org/integration/mozilla-inbound/rev/54cecb685bca

#0 0x111b79e25 in mozilla::EnableIf<(IsSame<decltype(fp0(fp)), void>::value) && (IsSame<decltype(fp1(fp)), void>::value), void>::Type mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2, mozilla::EnableIf<IsSame<decltype(fp0(fp)), void>::value, void>::Type mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2>(mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2 const&)::'lambda'(mozilla::layers::Layer*)>(mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2 const&, mozilla::EnableIf<IsSame<decltype(fp0(fp)), void>::value, void>::Type mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2>(mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2 const&)::'lambda'(mozilla::layers::Layer*) const&) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x296fe25)
#1 0x111b79ca2 in mozilla::EnableIf<(IsSame<decltype(fp0(fp)), void>::value) && (IsSame<decltype(fp1(fp)), void>::value), void>::Type mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2, mozilla::EnableIf<IsSame<decltype(fp0(fp)), void>::value, void>::Type mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2>(mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2 const&)::'lambda'(mozilla::layers::Layer*)>(mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2 const&, mozilla::EnableIf<IsSame<decltype(fp0(fp)), void>::value, void>::Type mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2>(mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2 const&)::'lambda'(mozilla::layers::Layer*) const&) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x296fca2)
#2 0x111b79ca2 in mozilla::EnableIf<(IsSame<decltype(fp0(fp)), void>::value) && (IsSame<decltype(fp1(fp)), void>::value), void>::Type mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2, mozilla::EnableIf<IsSame<decltype(fp0(fp)), void>::value, void>::Type mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2>(mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2 const&)::'lambda'(mozilla::layers::Layer*)>(mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2 const&, mozilla::EnableIf<IsSame<decltype(fp0(fp)), void>::value, void>::Type mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2>(mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2 const&)::'lambda'(mozilla::layers::Layer*) const&) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x296fca2)
#3 0x111b41c2c in mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x2937c2c)
#4 0x111cf2e05 in mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x2ae8e05)
#5 0x111cf3659 in mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x2ae9659)
#6 0x1183e58dc in nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x91db8dc)
#7 0x117a4034c in nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x883634c)
#8 0x1178c83b4 in mozilla::PresShell::Paint(nsView*, nsRegion const&, unsigned int) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x86be3b4)
#9 0x116edf17a in nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x7cd517a)
#10 0x116ede41d in nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x7cd441d)
#11 0x116ee1cf0 in nsViewManager::ProcessPendingUpdates() (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x7cd7cf0)
#12 0x11782b9a1 in nsRefreshDriver::Tick(long long, mozilla::TimeStamp) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x86219a1)
#13 0x117838ac3 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x862eac3)
#14 0x1178385f7 in mozilla::RefreshDriverTimer::Tick(long long, mozilla::TimeStamp) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x862e5f7)
#15 0x11783a94c in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x863094c)
#16 0x11824e2c7 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x90442c7)
#17 0x110b79be7 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x196fbe7)
#18 0x1105cea37 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x13c4a37)
#19 0x1104d6d05 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x12ccd05)
#20 0x1104cee3c in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x12c4e3c)
#21 0x1104d36a7 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x12c96a7)
#22 0x1104d4d2e in mozilla::ipc::MessageChannel::MessageTask::Run() (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x12cad2e)
#23 0x10f500540 in nsThread::ProcessNextEvent(bool, bool*) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x2f6540)
#24 0x10f4f8810 in NS_ProcessPendingEvents(nsIThread*, unsigned int) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x2ee810)
#25 0x116f73d0f in nsBaseAppShell::NativeEventCallback() (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x7d69d0f)
#26 0x117083bb4 in nsAppShell::ProcessGeckoEvents(void*) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x7e79bb4)
#27 0x7fffcd058980 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0xa7980)
#28 0x7fffcd039a7c in __CFRunLoopDoSources0 (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x88a7c)
#29 0x7fffcd038f75 in __CFRunLoopRun (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x87f75)
#30 0x7fffcd038973 in CFRunLoopRunSpecific (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x87973)
#31 0x7fffcc5c4acb in RunCurrentEventLoopInMode (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox+0x30acb)
#32 0x7fffcc5c4900 in ReceiveNextEventCommon (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox+0x30900)
#33 0x7fffcc5c4735 in _BlockUntilNextEventMatchingListInModeWithFilter (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox+0x30735)
#34 0x7fffcab6aae3 in _DPSNextEvent (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x46ae3)
#35 0x7fffcb2e521e in -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x7c121e)
#36 0x11708210c in -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x7e7810c)
#37 0x7fffcab5f464 in -[NSApplication run] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x3b464)
#38 0x117085017 in nsAppShell::Run() (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x7e7b017)
#39 0x1196f3628 in XRE_RunAppShell() (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0xa4e9628)
#40 0x1104e0fba in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x12d6fba)
#41 0x110418207 in MessageLoop::RunInternal() (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x120e207)
#42 0x110417ecc in MessageLoop::Run() (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x120decc)
#43 0x1196f28f0 in XRE_InitChildProcess(int, char**, XREChildData const*) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0xa4e88f0)
#44 0x1024fe675 in content_process_main(mozilla::Bootstrap*, int, char**) (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container+0x100002675)
#45 0x1024fe945 in main (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container+0x100002945)
#46 0x1024fd343 in start (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container+0x100001343)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/XUL+0x296fe25) in mozilla::EnableIf<(IsSame<decltype(fp0(fp)), void>::value) && (IsSame<decltype(fp1(fp)), void>::value), void>::Type mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2, mozilla::EnableIf<IsSame<decltype(fp0(fp)), void>::value, void>::Type mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2>(mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2 const&)::'lambda'(mozilla::layers::Layer*)>(mozilla::layers::Layer*, mozilla::layers::Layer::StartPendingAnimations(mozilla::TimeStamp const&)::$_2 const&, mozilla::EnableIf<IsSame<decltype(fp0(fp)), void>::value, void>::Type mozilla::layer

Command: /srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container -appdir /srv/mozilla/mozilla-inbound/ff-x86_64-apple-darwin16.3.0-asan-debug/dist/NightlyDebug.app/Contents/Resources/browser -profile /var/folders/ph/3q0jpmfd0j55k72jc86s9x0c0000gn/T/tmp86g6l2kh 54756 org.mozilla.machname.1250006789 tab

==54760==ABORTING

Comment 1

[Hiroyuki Ikezoe (:hiro)]

The test case casues an underflow at TimeStamp::operator-= [1], as a result the calculated TimeStamp value is 0, then it is treated as null.

[1] https://hg.mozilla.org/mozilla-central/file/71224049c0b5/mozglue/misc/TimeStamp.h#l542

Leaving ni? to me, I will figure out how to avoid this situation.

Comment 2

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

Do you have any further details? What is the stack for the underflow? What value becomes null?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

The failure exactly happens here [1]

[1] https://hg.mozilla.org/mozilla-central/file/71224049c0b5/gfx/layers/Layers.cpp#l497

 anim.startTime() = aReadyTime - anim.holdTime() + anim.delay();

aReadyTime is        16758573864738
holdTime().mValue is 2147483748170632

The holdTime is calculated by 2147483647 (which is set as currentTime in the test case) * kNsPerMsd (1000000.0) in TimeStamp_posix.cpp.

I think we have a bug for this kind of problems caused by TimeDuration::FromMilliseconds, but couldn't find it now.
Though I found another bug that might be related to this. bug 1258882.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

I think we should use Maybe<> for mValue in TimeStamp to distinguish zero value from uninitialized value.  Or we should have a value that represents the very beginning.

Nathan, any suggestion?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> I think we should use Maybe<> for mValue in TimeStamp to distinguish zero
> value from uninitialized value.  Or we should have a value that represents
> the very beginning.
> 
> Nathan, any suggestion?

Using Maybe<> for the value seems like a large change.  Defining arithmetic methods with a Maybe<> value would probably be similar to the semantics for floating-point math: None would be like NaN, whereas Some(x) would be an ordinary floating-point number, and anything involving None should produce None as an output.  But that raises the question of whether you should have:

  TimeStamp operator-(const TimeDuration&);

or:

  Maybe<TimeStamp> operator-(const TimeDuration&);

That is, should TimeStamp carry around Maybe<> internally, and expect its users to check it, or should TimeStamp objects always be valid, and Maybe<> is used to denote uninitialized objects?  Because in the first variant, which is your proposal, something that we almost certainly do today:

  initialized TimeStamp via Now() - constructed TimeStamp = X - 0 = X

would be:

  initialized TimeStamp via Now() - constructed TimeStamp = Some(X) - None = None

and I would not like to try and chase all those bugs down.  The second variant would have the virtue of helping you track those places down, but making the conversion much more tedious.

The proposal in comment 4 is to use the above change to turn underflowing arithmetic into a None TimeStamp, assuming I understand the proposal correctly?  I'm not sure about that; I think catching places where we rely on the current underflowing behavior would be just as tedious.  The analogy drawn above with floating-point arithmetic also breaks down: floating-point has -Inf to represent underflowing calculations, but the proposal under consideration would turn everything into the equivalent of NaN, which doesn't seem right.

I'm not sure what to do here to prevent the problem in the future; removing TimeStamp::operator-(const TimeStamp&) and having more explicit methods like:

* SubtractUnderflowToZero(const TimeStamp&, const TimeStamp&)
* template<typename Handler> SubtractWithUnderflowHandler(const TimeStamp&, const TimeStamp&, Handler&&)
* some other awkwardly-named method that would make reading the code difficult.

might be better?

Comment 6

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

Instead of trying to address the bigger question of how underflowing TimeStamps should behave in general, I had a look at the specific error encountered in this bug.

The problem is that certain values of timing inputs mean we can end up with a start time that we can't represent as a TimeStamp on certain platforms due to the way we represent TimeStamp values on those platforms. Specifically, the start time of the animation can be too far in the past.

That's not a problem on the main thread because we represent start times there as TimeDurations which have a negative range.

Specifically, it's this calculation here where we run in to trouble:

  anim.startTime() = aReadyTime - anim.holdTime() + anim.delay();

In this case, aReadyTime is a TimeStamp and anim.holdTime is a TimeDuration. This subtraction can underflow the unsigned integer used to represent the TimeStamp. We successfully detect this case and return a null TimeStamp (which seems reasonable). The subsequent call to operator+ then complains about this (and fair enough too).

If we change that line to:

  anim.startTime() = aReadyTime - (anim.holdTime() + anim.delay());

we avoid the assertion but we still end up with a null startTime and we'll trigger an assertion later on (in AnimationHelper::SampleAnimationForEachNode, at least) because we have the following invariant:

  EITHER the startTime is non-null, OR the animation is marked as "not playing" and the hold time is set.

So ultimately the problem is that there are some startTimes we just can't represent as TimeStamp values. The question, then, is, do we need to? That is, do we need to support animations with start time really far in the past?

I think, unfortunately, we do. Consider the following situations:

a) An animation that started a long time in the past but has now finished and is filling forwards. This could come about
   if we have a very long sequence of animations where we seek to the very end which pushes the early animations past the
   TimeStamp zero time. Also, depending on some upcoming changes to scroll-driven timelines, this might become more common.
b) An animation that started a long time in the past and repeats forever.
c) A really really long-running animation (e.g. an animation that changes the background color over a 24 hour cycle where
   we do an initial seek to the current time of day -- this is a bit like (b) except that the zero time of the current
   iteration might also fall before the TimeStamp zero time).

These seem like reasonable and perhaps even common enough situations that we should do something sensible in these cases.

Some possible solutions:

1. Detect these cases on the main thread and force these animations to run on the main thread. That seems attractive but it
   is complicated by the fact that we have to force *all* animations for that property on that element to run on the main 
   thread so we can do compositing correctly. Do-able but a fair bit of code (including generating a new animation performance
   warning) and perhaps not particularly easy to test in a cross-platform way.

2. Instead of a startTime TimeStamp, pass a reference TimeStamp and a TimeDuration offset from that TimeStamp. Then when we
   calculate the elapsedDuration (see below), calculate the difference between the TimeStamps first then do the rest of the
   math using TimeDurations.

   For reference, this is how we calculate the elapsedDuration:

    TimeDuration elapsedDuration = animation.isNotPlaying()
      ? animation.holdTime()
      : (aTime - animation.startTime())
          .MultDouble(animation.playbackRate());

   So we'd be doing something like the following instead:

    TimeDuration elapsedDuration = animation.isNotPlaying()
      ? animation.holdTime()
      : (aTime - animation.timelineOrigin() - animation.startTime())
          .MultDouble(animation.playbackRate());

3. Try to detect underflow when we set the startTime and set a suitable alternative startTime. e.g.

   For (a) above, just set holdTime to the end of the active interval, and mark the animation as not playing
   (and let startTime be null).

   For (b) above, work out a suitable startTime corresponding to the the first iteration of the animation that starts
   after the TimeStamp zero time. By also adjusting the iterationStart value, we could ensure we get the correct results
   even for animations with an iterationComposite mode other than 'replace'.

   For (c) above we would likewise need to calculate a suitable iterationStart but this time it would probably need to include
   a fractional component.

At this stage I lean towards (2) simply because it's the most testable. (3) is attractive since it's a very isolated change, but that code will only be hit on certain platforms for unusual content so it's likely to produce bugs that go unnoticed or unreported for a long time and are hard to reproduce. The code added in (2), on the other hand, will be executed all the time.


Note that with all these solutions we might still have problems from TimeDurations overflowing/underflowing, but those problems should appear on both the main thread and compositor so we can deal with them elsewhere when they arise.

Comment 7

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

Attachment: Reduced crash test

Comment 8

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

Attachment: WIP patch

Comment 9

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

Attachment: Bug 1334583 - Pass a separate timeOrigin and startTime for compositor animations;

By passing the startTime as a TimeDuration we are able to represent times in the
distant past (and with the same range as we can represent on the main thread so
that if we do encounter range errors in future, they should not differ between
the main thread and the compositor).

This patch includes a crashtest. I have verified that, without the code changes
included in this patch, this crashtest fails on debug builds on OSX.

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

Comment 10

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

Try run for this and all dependent bugs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7811ba617d470b17cfb9736a36818b252fee8dd5&selectedJob=95846740

(From which it looks like someone imported a bunch of csswg tests for writing modes and grid and hasn't marked the expected failures yet.)

Comment 11

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

Comment on attachment 8863626 [details]
Bug 1334583 - Pass a separate timeOrigin and startTime for compositor animations;

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863626 [details]
Bug 1334583 - Pass a separate timeOrigin and startTime for compositor animations;

https://reviewboard.mozilla.org/r/135416/#review138360

::: gfx/layers/ipc/LayersMessages.ipdlh:214
(Diff revision 2)
>    TimeDuration delay;
>    TimeDuration endDelay;
>    // The value of the animation's current time at the moment it was sent to the
>    // compositor.  This value will be used for below cases:
>    // 1) Animations that are play-pending. Initially these animations will have a
> -  //    null |startTime|. Once the animation is read to start (i.e. painting has
> +  //    zero |startTime|. Once the animation is ready to start (i.e. painting

IIUC, the original comment is still valud, now it's null_t though.

Comment 13

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

Comment on attachment 8863626 [details]
Bug 1334583 - Pass a separate timeOrigin and startTime for compositor animations;

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/095c3fdfc662
Pass a separate timeOrigin and startTime for compositor animations; r=hiro

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/095c3fdfc662

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Bug 1336274 hits on Beta too, so I guess that "disabled" was a bit premature on my part :). Is this something we could safely land on Beta54, Brian?

Comment 18

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

I think it's probably pretty safe to land. Will request uplift.

Comment 19

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

Attachment: Patch for beta

Approval Request Comment
[Feature/Bug causing the regression]: Unknown. Something changed in the Android test environment causing this to fail more frequently.
[User impact if declined]: dom/animation/test/chrome/test_restyles.html fails frequently on Android.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:  Yes.
[List of other uplifts needed for the feature/fix]: None (some minor dependencies are rolled into this patch).
[Is the change risky?]: Not overly.
[Why is the change risky/not risky?]: It has baked on Nightly, is covered by quite a few different tests, and is a fairly minor change (to how start times for compositor run animations are represented / calculated).
[String changes made/needed]: None.

I had to rebase this for beta so hopefully this try run can tell us if I missed anything:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5b7b2ea538bc2889e9bee08ec9bb36ca62ab3a4

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8867020 [details] [diff] [review]
Patch for beta

Fix a regression. Beta54+. Should be in 54 beta 8.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/87a47b9dff16