Intermittent layout/style/test/test_transitions.html | descendant test, property text-indent: computed value 150px should be between 90.720620px and 90.981408px at time between 1s and 1s.

RESOLVED INCOMPLETE

Status

()

P5
normal
RESOLVED INCOMPLETE
2 years ago
a year ago

People

(Reporter: intermittent-bug-filer, Assigned: dbaron)

Tracking

({bulk-close-intermittents})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [stockwell unknown])

Attachments

(2 attachments)

Comment 1

2 years ago
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Comment hidden (offtopic)

Updated

2 years ago
See Also: → bug 1159532
Comment hidden (offtopic)
Comment hidden (offtopic)
Depends on: 1159532
See Also: bug 1159532
Was the spike in reports of this bug just accidental mis-starring of bug 1159532 as this bug, since the two bugs are both suggested for that bug's failure?
No longer depends on: 1159532
See Also: → bug 1159532
Comment hidden (offtopic)
OK, I went through OrangeFactor and clicked on each log in the past week (ugh!).  (That covers builds from 8 Feb 2017, 11:35 UTC to 15 Feb 2017, 11:56 UTC.)

Every single occurrence starred for this bug in the past week was mis-starred.  All but one of them were bug 1159532.  (https://treeherder.mozilla.org/logviewer.html#?repo=autoland&job_id=76275138&lineNumber=2463 was unrelated.)
Given that the actual failure for which this bug was filed only seems to have occurred once, I'm going to blame it on cosmic rays and mark this as WORKSFORME, particularly since it's gathered a large amount of unrelated data.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
Actually, there was a second real occurrence in:
https://treeherder.mozilla.org/logviewer.html#?repo=autoland&job_id=4380882&lineNumber=1867

which was:
Job 	Android 4.3 API15+ opt Mochitest Mochitest M(20)
Machine 	tst-emulator64-spot-1292
Start 	Mon Oct 3, 13:30:42
End 	Mon Oct 3, 14:29:39
Revision 	29fc20c94897948013efcd0f52607630056d3339
13:39:27     INFO -  148 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions.html | descendant test, property text-indent: computed value 150px should be between 90.720620px and 90.981408px at time between 1s and 1s.

(The second star associated with this bug is too old to retrieve the log, so I can't check that one.)
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
But at this point I've clicked every log associated with this bug in orangefactor; the two oldest were no longer present (but presumably the first is canonical), and the one in the previous comment (the third) was the only one other than the first actually associated with this bug.  All the logs older than 8 Feb (i.e., that I didn't examine in comment 7) other than those first 4 were bug 1159532.

The fourth was:
https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=41083888&lineNumber=1944 was an entirely separate failure.
Status: REOPENED → NEW
Component: Layout → CSS Parsing and Computation
Priority: P3 → P5
It's also worth noting that in both occurrence #1 and occurrence #3 (i.e., the two real occurrences), the test failure was in the fourth of the descendant tests, i.e., the one that does:

    { parent_transition: "4s text-indent",
      child_transition: "1s text-indent" },

Following https://hg.mozilla.org/mozilla-central/rev/d2bfe90297da the error message for that will now say:
"descendant test #4, property text indent: computed value 150px should be between ..."
Oh, and, actually, I think this isn't too hard to fix.  The reason it happened is that it hit the case of the 2s timer from:

add_future_call(2, check_descendant_tests);

actually firing at *exactly* 1s.  This presumably leads to the boundary testing in check_descendant_tests() not matching our actual code.  (In other words, test_transitions.html in general allows for the timers to fire pretty much whenever, and has an expectation of what the result should be.  The goal is that they usually fire close to the correct times so that we usually test what the tests were written to test, but the test is supposed to know the correct result for any time.

In particular, the boundary condition (>) at:

            // Override the parent's transition with the child's as long
            // as the child transition is still running.
            if (property != "letter-spacing" && duration + delay > time) {
               ...
            }
            if (property != "text-indent" && duration + delay > time) {
               ...

are probably incorrect, and we should be using >= on both lines instead... at least based on the results the current code produces.  (Which behavior is correct is another question...)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8837870 [details]
Bug 1285461 - Fix check_descendant_tests() to correctly handle being called exactly at the time the child transition ends when the parent transition is still running.

https://reviewboard.mozilla.org/r/112864/#review114406

::: layout/style/test/test_transitions.html:688
(Diff revision 1)
> -            if (property != "letter-spacing" && duration + delay > time) {
> +            // It's not clear to me that this *should* be >= rather than >,
> +            // but our code appears to behave as though it's >=.

If the delay is 1s and the duration is 4s, then after exactly 5s the transition should be finished by virtue of the endpoint exclusive timing model so this seems like the expected behavior to me.
Attachment #8837870 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #14)
> If the delay is 1s and the duration is 4s, then after exactly 5s the
> transition should be finished by virtue of the endpoint exclusive timing
> model so this seems like the expected behavior to me.

But the behavior being observed here is actually the opposite; it's *inclusive* of the endpoint.
Flags: needinfo?(bbirtles)
Oh, so it is. In that case, I'm not quite sure what is going on. I'll have to look into the test more deeply but I wonder if there's not some rounding taking place that's producing this behavior (since we're using a layout property to measure time from my recollection of the test).

Comment 17

2 years ago
mozreview-review
Comment on attachment 8837870 [details]
Bug 1285461 - Fix check_descendant_tests() to correctly handle being called exactly at the time the child transition ends when the parent transition is still running.

https://reviewboard.mozilla.org/r/112864/#review114784

Clearing review because this appears to be a rounding issue and making this >= will cause the test to fail if the time is exactly 1s.
Attachment #8837870 - Flags: review+
Created attachment 8838381 [details]
Reduced version of test case the reproduces the problem

In this test case, we manually set the currentTime to 999.99 on all the transitions. As a result, the time as measured from the reference elements' textIndent is 125px which we calculate as 1s and hence the failure occurs.

While changing the condition "duration + delay > time" to >= fixes this case, if we set the currentTime to 1000 with that fix in place, the test will fail (because we'll decide the child transition should still be running, when in fact it is not).

I don't have any brilliant ideas for fixing this other than adding some tolerance around the time values. It appears the test was originally supposed to do that but in all my tests the "early" and "late" references always have the same values so they don't give us a range. Are there platforms or circumstances where they differ?

If not, perhaps we just need one reference element and then to add some tolerance either side of the time value.
Flags: needinfo?(bbirtles)
Comment hidden (offtopic)
The recent surge appears to be all mis-filings of bug 1159532 (and the recent ones are mostly from the graphics branch, presumably because the fix was merged there later than other branches).
Comment hidden (offtopic)
Nope, this is not fixed.  But this bug has only ever happened twice; bug 1159532 was fixed, and that stopped the frequent missstarring of it as this bug.
Whiteboard: [stockwell fixed]
thanks for clarifying David!
Whiteboard: [stockwell unknown]
If the timing is supposed to be endpoint-exclusive, then I think there's a real bug here, because I think this failure is showing that our timing is endpoint-inclusive.
The bug here is in the way the test uses the reference element's textIndent to calculate time (see comment 18).
Ah, the problem is that the use of textIndent means we're rounding a time that's actually slightly smaller than 1s to 1s.  OK.

I guess we could also use multiple reference elements that end at different times to determine where the exact time is relative to certain exact points we care about.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years agoa year ago
Keywords: bulk-close-intermittents
Resolution: --- → INCOMPLETE
Reopening and removing intermittent-failure tag since this is diagnosed and addressible.
Status: RESOLVED → REOPENED
Keywords: bulk-close-intermittents, intermittent-failure
Resolution: INCOMPLETE → ---
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Keywords: bulk-close-intermittents
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.