916602 - Assertion adding IsNull() timestamp in FrameAnimator::GetCurrentImgFrameEndTime()

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Attachment: timestamp.assert

Recent inbound pull (yesterday); debug build on Linux x64 F17:

Hit an assertion when leaving a github page; it was animating an icon for a file I'd visited and then hit back to get to the github page again.  I then hit back again and it asserted.

See attached callstack

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

I visited rtcpeerconnection.js (last file)

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: similar stack, doing online checkin on united.com

I hit what looks like the same thing on Friday; was waiting to get back home before filing, but Randell beat me to it.

Randell's stack is better since his looks like debug and mine is debug+opt, and I also got confused about which process it was and didn't manage to catch it in gdb, so I have only the console output stack.

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Any idea what's up here?

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I hit this reliably going to https://crash-stats.mozilla.com/topcrasher/products/Firefox/versions/25.0a2?days=7

Comment 5

[Seth Fowler [:seth] [:s2h]]

Another FrameAnimator regression. =\

I'll investigate this.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

poking again about this; I think you said there was something that probably ought to be backed out until this was fixed.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Yes. I'll leave the needinfo request open until I deal with this. Basically I just need to make a build with that patch backed out to confirm that my hunch about the cause is correct.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

After locally reverting https://hg.mozilla.org/mozilla-central/rev/810c464191d4 I don't see this anymore.

Comment 9

[Simon Lindholm]

Attachment: possible fix?

My, ImageLib is complicated. I suspect something like the attached patch might fix it, by being more careful about setting animation time when mAnimating = true, but I'm nowhere near certain. I didn't compile or test it; the only machine I have that is powerful enough to compile Firefox takes ~8 hours to do so.

Anyway, if additions to null timestamps assert, isn't the statement in GetCurrentImgFrameEndTime that does `return TimeStamp() + TimeDuration::FromMilliseconds(static_cast<double>(UINT64_MAX));` also quite wrong?

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 807850 [details] [diff] [review]
possible fix?

It's generally good to make review or feedback requests when attaching patches.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 807850 [details] [diff] [review]
possible fix?

Review of attachment 807850 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, Simon! I apologize for not noticing the patch you posted.

I've gone ahead and reviewed this patch as normal, but in the interest of getting this fixed ASAP I'm going to go ahead and making the changes myself so I can verify that this patch fixes the issue.

::: image/src/RasterImage.cpp
@@ +1434,5 @@
>  
>    EnsureAnimExists();
>  
>    imgFrame* currentFrame = GetCurrentImgFrame();
> +  if (currentFrame && currentFrame->GetTimeout() < 0) { // -1 means display this frame forever

I'd suggest moving this comment to the line above and maybe slightly rewording. This ends up being a pretty long line otherwise.

@@ +1482,2 @@
>      StopAnimation();
> +    mAnimating = false;

With this change, every caller of StopAnimation also executes "mAnimating = false". It should just move inside StopAnimation. (Which makes more sense anyway.)

Comment 12

[Seth Fowler [:seth] [:s2h]]

(In reply to Simon Lindholm from comment #9)
> Anyway, if additions to null timestamps assert, isn't the statement in
> GetCurrentImgFrameEndTime that does `return TimeStamp() +
> TimeDuration::FromMilliseconds(static_cast<double>(UINT64_MAX));` also quite
> wrong?

Absolutely. I've included a fix for this in my modified version of your patch.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Avoid null timestamp assertion in FrameAnimator::GetCurrentImgFrameEndTime.

OK, this looks good. (Still needs a try run, though.) Here's the updated version of the patch with review comments addressed.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Avoid null timestamp assertion when creating sentinel in FrameAnimator.

Here's an additional patch that addresses the issue identified by Simon in comment 9.

Comment 15

[Seth Fowler [:seth] [:s2h]]

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=9db2bebae756

Comment 16

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Avoid null timestamp assertion when creating sentinel in FrameAnimator.

Ack, this patch had the wrong committer. Fixed.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Try looks good.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Timothy.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d2fe9d9a7bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d477f84d533

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d2fe9d9a7bf
https://hg.mozilla.org/mozilla-central/rev/0d477f84d533