Closed Bug 1578125 Opened 3 months ago Closed 3 months ago

Perma Windows 7 debug Assertion failure: (aA != 9223372036854775807i64 || aB != (-9223372036854775807i64 - 1) when Gecko 70 merges to Beta on 2019-08-26

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- fixed
firefox71 --- verified

People

(Reporter: CosminS, Assigned: birtles)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

[Tracking Requested - why for this release]:

Central as beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=bc7396c6e978cb53a149e4cf6c6f6680d06a6bee&selectedJob=264489407&searchStr=windows%2C7%2Cdebug%2Cweb%2Cplatform%2Ctests%2Ctest-windows7-32%2Fdebug-web-platform-tests-e10s-8%2Cw%28wpt8%29

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=264489407&repo=try&lineNumber=10240

Last good sim: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=96b1ef962641a70f9b12a259f83d06e84485a5ac&searchStr=windows%2C7%2Cdebug%2Cweb%2Cplatform%2Ctests%2Ctest-windows7-32%2Fdebug-web-platform-tests-e10s

central rev: https://hg.mozilla.org/mozilla-central/rev/c75d6a0539eb4b2c7961f0c9782fcb364198c6b2

First bad sim: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=fd3891b30a2211e9487925dc6f264ec5aae2a685&searchStr=windows%2C7%2Cdebug%2Cweb%2Cplatform%2Ctests%2Ctest-windows7-32%2Fdebug-web-platform-tests-e10s&selectedJob=263673753

central rev: https://hg.mozilla.org/mozilla-central/rev/993cf82f6bbf09537f4cec8144cb69bfdf26bff3

Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c75d6a0539eb4b2c7961f0c9782fcb364198c6b2&tochange=993cf82f6bbf09537f4cec8144cb69bfdf26bff3

I assumed this to be fixed by this wpt update: https://hg.mozilla.org/try/rev/6fdb1282586def80e1a0d8c4c85f84e44a860b66 but wasn't and went on starred as an intermittent against bug 1271788.

Flags: needinfo?(james)
Flags: needinfo?(aryx.bugmail)
Summary: Perma Assertion failure: (aA != 9223372036854775807i64 || aB != (-9223372036854775807i64 - 1) when Gecko 70 merges to Beta on 2019-08-26 → Perma Windows 7 debug Assertion failure: (aA != 9223372036854775807i64 || aB != (-9223372036854775807i64 - 1) when Gecko 70 merges to Beta on 2019-08-26

From the wpt side I think we can mark the crash as expected, which might or might not be enough to turn the job green, depending on whether the failing assert independently triggers failure. If that doesn't work then we can disable the test.

But really we want a layout developer to look at this and fix the underlying issue.

Flags: needinfo?(james)

I have this bugmail sitting in my inbox to remind to look into this. Hopefully this week.

Just got similar errors when I push to try for offset-path:

PROCESS-CRASH | /css/motion/animation/offset-path-interpolation-001.html | application crashed [@ mozilla::TimingParams::Update()]
PID 5696 | Assertion failure: (aA != 9223372036854775807i64 || aB != (-9223372036854775807i64 - 1)) && (aA != (-9223372036854775807i64 - 1) || aB != 9223372036854775807i64) ('Infinity + -Infinity' and '-Infinity + Infinity' are undefined), at z:/build/build/src/obj-firefox/dist/include\mozilla/StickyTimeDuration.h:35
PROCESS-CRASH | /css/motion/animation/offset-path-interpolation-002.html | application crashed [@ mozilla::TimingParams::Update()]
PID 3924 | Assertion failure: (aA != 9223372036854775807i64 || aB != (-9223372036854775807i64 - 1)) && (aA != (-9223372036854775807i64 - 1) || aB != 9223372036854775807i64) ('Infinity + -Infinity' and '-Infinity + Infinity' are undefined), at z:/build/build/src/obj-firefox/dist/include\mozilla/StickyTimeDuration.h:35
PROCESS-CRASH | /css/motion/animation/offset-path-interpolation-003.html | application crashed [@ mozilla::TimingParams::Update()]
PID 6124 | Assertion failure: (aA != 9223372036854775807i64 || aB != (-9223372036854775807i64 - 1)) && (aA != (-9223372036854775807i64 - 1) || aB != 9223372036854775807i64) ('Infinity + -Infinity' and '-Infinity + Infinity' are undefined), at z:/build/build/src/obj-firefox/dist/include\mozilla/StickyTimeDuration.h:35
PROCESS-CRASH | /css/motion/animation/offset-path-interpolation-004.html | application crashed [@ mozilla::TimingParams::Update()]
PID 5324 | Assertion failure: (aA != 9223372036854775807i64 || aB != (-9223372036854775807i64 - 1)) && (aA != (-9223372036854775807i64 - 1) || aB != 9223372036854775807i64) ('Infinity + -Infinity' and '-Infinity + Infinity' are undefined), at z:/build/build/src/obj-firefox/dist/include\mozilla/StickyTimeDuration.h:35
PROCESS-CRASH | /css/motion/animation/offset-path-interpolation-005.html | application crashed [@ mozilla::TimingParams::Update()]
TEST-UNEXPECTED-CRASH | /css/motion/animation/offset-path-interpolation-005.html | expected OK
PID 5824 | Assertion failure: (aA != 9223372036854775807i64 || aB != (-9223372036854775807i64 - 1)) && (aA != (-9223372036854775807i64 - 1) || aB != 9223372036854775807i64) ('Infinity + -Infinity' and '-Infinity + Infinity' are undefined), at z:/build/build/src/obj-firefox/dist/include\mozilla/StickyTimeDuration.h:35
PROCESS-CRASH | /css/motion/animation/offset-rotate-interpolation.html | application crashed [@ mozilla::TimingParams::Update()]

Brian, can you take a look at this in the coming days, please?

Flags: needinfo?(brian)

Yes, it's on my list. I'm currently traveling but hope to have a chance to look at this on Friday.

Flags: needinfo?(brian)

Looking into this as part of bug 1576866 it seems that even if we fix the assertions, these tests will still fail. Instead we should fix the test suite.

(In reply to Brian Birtles (:birtles) from comment #9)

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

There are a few failures there on Windows (which don't reproduce locally for me on a debug Windows build). Interesting they all occur on the Web Animations interpolation test at 0.5. Looking at the code we have:

      var animation = target.animate(keyframes, {
        fill: 'forwards',
        duration: 1,
        easing: createEasing(at),
      });
      animation.pause();
      animation.currentTime = 0.5;

And createEasing has:

  function createEasing(y) {
    if (y == 0) {
      return 'steps(1, end)';
    }
    if (y == 1) {
      return 'steps(1, start)';
    }
    if (y == 0.5) {
      return 'steps(2, end)';
    }
    // Approximate using a bezier.
    var b = (8 * y - 1) / 6;
    return 'cubic-bezier(0, ' + b + ', 1, ' + b + ')';
  }

Judging by the nature of the test we are hitting the steps(2, end) case but then presumably there is some floating-point inaccuracy that means when we come to sample the timing function at 0.5 we land just before the cliff and end up sampling the lower value.

This seems like a pretty flaky way to write a test, so we should either:

  • use steps(3, jump-none) (not widely enough supported yet?), or
  • drop the special case for 0.5 (is it needed?), or
  • just use linear.

The latter seems the safest so I will just try that.

Yet again js-format / clang-format completely destroyed my repository when working with mercurial queues. And after disabling them hg qpush just freezes. You'd think I'd learn. 3 hours lost so far this month to those tools.

The border-image-slice-interpolation.html test still fails but precisely because it is testing at the 50% mark for a discrete animation. There's just some floating point precision issue there. It might be better to make the Web Animations one use a duration of 100s and seek to 50s rather than 1s and seek to 0.5s.

(In reply to Brian Birtles (:birtles) from comment #13)

It might be better to make the Web Animations one use a duration of 100s and seek to 50s rather than 1s and seek to 0.5s.

Actually, it uses a duration of 1ms seeks to 0.5ms.

The alternative would be to simply make the expectFlip function test at 0.501 instead of precisely at 0.5.

I'm currently running both through try but I'll stick to the former if it works.

So many unexpected passes taking so long to identify and re-annotate. Seems like something in our process let us down here.

This patch fixes a number of issues with the common interpolation testing
functions upstreamed from Blink.

Firstly this test file sets an animation duration of 2,000,000,000s and a delay
of -1,000,000,000s but that does not appear to be necessary since no time
elapses between when the animation is established and when the result is
checked (measure() is called immediately after interpolate() without any
interleaving call to requestAnimationFrame, for example).

Furthermore, this long time will cause some configurations of Firefox on
Windows to treat the time as an infinite time and end up producing discrete
animation. There is nothing in CSS values that requires supporting times of
this magnitude (~63 years) so it seems best not to make the tests rely on it
being supported (the tests are intended to cover interpolation after all, not
timing range) and instead to use a shorter time.

Similarly, for the Web Animations interpolation tests, this utility function
uses a duration of 1ms and seeks to 0.5ms. This can likewise lead to precision
issues, particularly when testing interpolation pairs that fall back to
discrete animation since in that case, the test will seek to precisely 0.5ms
and test that the result is at the top of the step that occurs at that time.
Floating-point error can cause this to fail so this patch changes the duration
and seek time to 100s and 50s respectively.

This patch also includes a few additional tweaks to this file including:

  • Replacing the "steps(2, end)" timing function with "linear" since this
    avoids floating-point errors at the 0.5 mark.
  • Adding translation for "float" to "cssFloat".
  • Triggering transitions by querying the property being transitioned.
    (In theory a UA could skip flushing all styles when accessing the 'left'
    property but in practice that is probably not Web-compatible.)
Pushed by bbirtles.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0626a86d78b1
Fix flakiness in common animation interpolation testing functions; r=hiro
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19197 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

There are so many unexpected passes as a result of this. I've spent a whole day just updating test annotations.

Flags: needinfo?(brian)
Upstream PR merged by moz-wptsync-bot
Pushed by bbirtles.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73d9659e7715
Fix flakiness in common animation interpolation testing functions; r=hiro
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/9aa5501ed68a
Update some more expectations for tests that now pass. r=me

(In reply to Brian Birtles (:birtles) from comment #21)

There are so many unexpected passes as a result of this. I've spent a whole day just updating test annotations.

FYI, you can point to a WPT log and do ./mach wpt-udpate <log> to update those annotations automatically.

Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/c36980d14017
More passing tests on win x86 and aarch64. r=me
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/3c1bb6a2bd10
Restore a line that shouldn't have been deleted.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/d45ca6b3be99
perspective-origin-interpolation.html also passes now.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/ff6f7d26bb5a
Also tentatively update font-variation-settings-interpolation.html expectations for aarch64

Thanks Emilio for all the follow ups! I had green try runs on Win 7 and Win 10 so I'm not sure what happened there unless something else landed in between.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)

(In reply to Brian Birtles (:birtles) from comment #21)

There are so many unexpected passes as a result of this. I've spent a whole day just updating test annotations.

FYI, you can point to a WPT log and do ./mach wpt-udpate <log> to update those annotations automatically.

Yes, I realised that half way through, but I think I'd need to be on Win 7 / x86 to do that (and I don't have much confidence in my local setup matching CI -- I often get unexpected failures locally due to pixel density differences).

Thanks again!

Yes, I realised that half way through, but I think I'd need to be on Win 7 / x86 to do that (and I don't have much confidence in my local setup matching CI -- I often get unexpected failures locally due to pixel density differences).

You can always do a try run of just the relevant tests to get the unexpected results for all platforms and update for those logs. I've got a little tool to help download all the logs [1]

We should probably also force the tests to run at single density on desktop like we do on android.

It looks like the upstream PR was merged before this reached central? Which seems like a bug…

[1] https://github.com/jgraham/fetchlogs

(In reply to James Graham [:jgraham] from comment #32)

Yes, I realised that half way through, but I think I'd need to be on Win 7 / x86 to do that (and I don't have much confidence in my local setup matching CI -- I often get unexpected failures locally due to pixel density differences).

You can always do a try run of just the relevant tests to get the unexpected results for all platforms and update for those logs. I've got a little tool to help download all the logs [1]

Yes, I ran the tests on try and went off the Win 7 logs there (and thought I had them all since it was all green). The trouble was more that half of the cases I needed to update the meta file by hg rm-ing the entire file, and the other half I had to trim the now-passing lines but leave some still-failing entries. In doing that I came across bug 1583012 which made me realise I probably should continue hand editing the files to avoid further corruption.

It looks like the upstream PR was merged before this reached central? Which seems like a bug…

Yes, I noticed that too. I'm not sure what triggered it although it might have been the backout push to autoland that did it.

Nevertheless, what landed in the wpt repository should be correct -- the subsequent pushes didn't touch that part.

Duplicate of this bug: 1581928

Brian, can you request uplift here to fix the failures on mozilla-beta?

Flags: needinfo?(brian)

(In reply to Andreea Pavel [:apavel] from comment #38)

Brian, can you request uplift here to fix the failures on mozilla-beta?

Yes, I'm working on it now. It might take me a day or so, however.

No problem. Thank you for giving us updates on this.

I've reworked all the patches to apply to beta. If they seem to work then I'll fold them up and apply for beta approval:

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

Flags: needinfo?(brian)
Attached patch Patch for beta (obsolete) — Splinter Review

Beta/Release Uplift Approval Request

  • User impact if declined: None, but tests in CI will fail.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It only affects tests.
  • String changes made/needed: No
Attachment #9097855 - Flags: approval-mozilla-beta?

I haven't been able to run the beta patch on CI (see comment 41 and comment 42) so if someone is able to do that before merging, that would be a good idea -- particularly running WPT on Windows 7.

Asking in #sheriffs for someone to take this on.

Looks like this needs the last patch from bug 1579062 too (to turn on dom.animations-api.implicit-keyframes.enabled everyone).

Attachment #9097855 - Attachment is obsolete: true
Attachment #9097855 - Flags: approval-mozilla-beta?

Comment on attachment 9098412 [details] [diff] [review]
Patch for beta v2

Beta/Release Uplift Approval Request

  • User impact if declined: None, but tests in CI will fail.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It only affects tests.
  • String changes made/needed: None.
Attachment #9098412 - Flags: approval-mozilla-beta?
Attachment #9094363 - Flags: approval-mozilla-beta?
Attachment #9094363 - Flags: approval-mozilla-beta?
Comment on attachment 9098412 [details] [diff] [review]
Patch for beta v2

Test fixes; let's uplift for beta 13.
Attachment #9098412 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.