Closed
Bug 1391154
Opened 7 years ago
Closed 6 years ago
Intermittent dom/base/test/test_intersectionobservers.html | supports CSS transitions and transforms [observe] - got +0, expected 1
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: intermittent-bug-filer, Assigned: mrbkap)
Details
(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:product])
Attachments
(1 file, 3 obsolete files)
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details |
Filed by: philringnalda [at] gmail.com https://treeherder.mozilla.org/logviewer.html#?job_id=123753301&repo=autoland https://queue.taskcluster.net/v1/task/St0c-QFLTgK_TZM3yH5yog/runs/0/artifacts/public/logs/live_backing.log
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 3•7 years ago
|
||
https://wiki.mozilla.org/Bugmasters#Intermittent_Test_Failure_Cleanup
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 6•7 years ago
|
||
https://wiki.mozilla.org/Bugmasters#Intermittent_Test_Failure_Cleanup
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → INCOMPLETE
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Comment hidden (Intermittent Failures Robot) |
Comment 8•6 years ago
|
||
https://wiki.mozilla.org/Bug_Triage#Intermittent_Test_Failure_Cleanup
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
Resolution: --- → INCOMPLETE
Comment hidden (Intermittent Failures Robot) |
Comment 10•6 years ago
|
||
Recent failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=164186195&repo=autoland&lineNumber=9414
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 13•6 years ago
|
||
There have been 38 failures in the past 7 days, on the following platforms: -linux x64 OPT - 12 failures -linux x64 PGO - 7 failures -linux32-stylo-disabled OPT - 3 failures -linux64-stylo-disabled OPT - 5 failures -macosx64-nightly OPT - 1 failures -macosx64-devedition OPT - 3 failures -OS X 10.10 OPT - 5 failures -windows 7 DEBUG - 1 failure -windows 10-64 DEBUG - 1 failure Recent failure log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=167775741&lineNumber=4945 Snippet: 13:56:20 INFO - TEST-PASS | dom/base/test/test_intersectionobservers.html | handles sub-root element scrolling [observe] 13:56:20 INFO - TEST-PASS | dom/base/test/test_intersectionobservers.html | handles sub-root element scrolling [observe] 13:56:20 INFO - TEST-PASS | dom/base/test/test_intersectionobservers.html | supports CSS transitions and transforms [observe] 13:56:20 INFO - Buffered messages finished 13:56:20 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_intersectionobservers.html | supports CSS transitions and transforms [observe] - got +0, expected 1 13:56:20 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:312:5 13:56:20 INFO - fn@dom/base/test/test_intersectionobservers.html:91:13 13:56:20 INFO - io<@dom/base/test/test_intersectionobservers.html:896:11 13:56:20 INFO - IntersectionCallback*@dom/base/test/test_intersectionobservers.html:890:14 13:56:20 INFO - next@dom/base/test/test_intersectionobservers.html:141:9 13:56:20 INFO - next/<@dom/base/test/test_intersectionobservers.html:143:11 13:56:20 INFO - io<@dom/base/test/test_intersectionobservers.html:873:11 13:56:20 INFO - IntersectionCallback*@dom/base/test/test_intersectionobservers.html:870:14 13:56:20 INFO - next@dom/base/test/test_intersectionobservers.html:141:9 13:56:20 INFO - next/<@dom/base/test/test_intersectionobservers.html:143:11 13:56:20 INFO - runSequence@dom/base/test/test_intersectionobservers.html:1124:15 13:56:20 INFO - runSequence/<@dom/base/test/test_intersectionobservers.html:1121:9 13:56:20 INFO - @dom/base/test/test_intersectionobservers.html:862:15 13:56:20 INFO - fn@dom/base/test/test_intersectionobservers.html:162:11 13:56:20 INFO - IntersectionCallback*@dom/base/test/test_intersectionobservers.html:807:14 13:56:20 INFO - next@dom/base/test/test_intersectionobservers.html:141:9 13:56:20 INFO - next/<@dom/base/test/test_intersectionobservers.html:143:11 13:56:20 INFO - runSequence@dom/base/test/test_intersectionobservers.html:1124:15 13:56:20 INFO - runSequence/<@dom/base/test/test_intersectionobservers.html:1121:9 13:56:20 INFO - @dom/base/test/test_intersectionobservers.html:794:15 13:56:20 INFO - fn@dom/base/test/test_intersectionobservers.html:162:11 13:56:20 INFO - IntersectionCallback*@dom/base/test/test_intersectionobservers.html:770:14 13:56:20 INFO - next@dom/base/test/test_intersectionobservers.html:141:9 13:56:20 INFO - next/<@dom/base/test/test_intersectionobservers.html:143:11 13:56:20 INFO - runSequence@dom/base/test/test_intersectionobservers.html:1124:15 13:56:20 INFO - runSequence/<@dom/base/test/test_intersectionobservers.html:1121:9 13:56:20 INFO - @dom/base/test/test_intersectionobservers.html:758:15 13:56:20 INFO - fn@dom/base/test/test_intersectionobservers.html:162:11 13:56:20 INFO - IntersectionCallback*@dom/base/test/test_intersectionobservers.html:692:14 13:56:20 INFO - next@dom/base/test/test_intersectionobservers.html:141:9 13:56:20 INFO - next/<@dom/base/test/test_intersectionobservers.html:143:11 13:56:20 INFO - runSequence@dom/base/test/test_intersectionobservers.html:1124:15 13:56:20 INFO - runSequence/<@dom/base/test/test_intersectionobservers.html:1121:9 13:56:20 INFO - io<@dom/base/test/test_intersectionobservers.html:677:15 13:56:20 INFO - IntersectionCallback*@dom/base/test/test_intersectionobservers.html:667:18 13:56:20 INFO - runSequence@dom/base/test/test_intersectionobservers.html:1120:7 13:56:20 INFO - runSequence/<@dom/base/test/test_intersectionobservers.html:1121:9 13:56:20 INFO - io<@dom/base/test/test_intersectionobservers.html:658:15 13:56:20 INFO - IntersectionCallback*@dom/base/test/test_intersectionobservers.html:650:18 13:56:20 INFO - runSequence@dom/base/test/test_intersectionobservers.html:1120:7 13:56:20 INFO - runSequence/<@dom/base/test/test_intersectionobservers.html:1121:9 13:56:20 INFO - io<@dom/base/test/test_intersectionobservers.html:641:15 13:56:20 INFO - IntersectionCallback*@dom/base/test/test_intersectionobservers.html:631:18 13:56:20 INFO - runSequence@dom/base/test/test_intersectionobservers.html:1120:7 13:56:20 INFO - TEST-PASS | dom/base/test/test_intersectionobservers.html | uses the viewport when no root is specified [observe] 13:56:21 INFO - TEST-OK | dom/base/test/test_intersectionobservers.html | took 3988ms Even though it has failed over 30 times/last week, the graph in orange factor shows a descending trend.
Flags: needinfo?(overholt)
Whiteboard: [stockwell needswork]
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 16•6 years ago
|
||
Who knows intersection observers these days?
Flags: needinfo?(overholt) → needinfo?(dholbert)
Comment 17•6 years ago
|
||
I think mrbkap knows them best... (I recall him being the reviewer on recent-ish bugs that I was CC'd on, at least.)
Flags: needinfo?(dholbert) → needinfo?(overholt)
Comment 18•6 years ago
|
||
He and mstange, I think. (I'm roughly familiar with the spec, but I don't really know our implementation at all.) Looks like mstange was a reviewer on many of the changes to this test, so maybe he'd be a good person to look at this? Just to get things started, the error is: INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_intersectionobservers.html | supports CSS transitions and transforms [observe] - got +0, expected 1 ...which I'm assuming comes from one of these lines: > it('supports CSS transitions and transforms', function(done) { > [...] > io = new IntersectionObserver(function(records) { > [...] > expect(records.length).to.be(1); > expect(records[0].intersectionRatio).to.be(1); https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/dom/base/test/test_intersectionobservers.html#895-896
Flags: needinfo?(overholt) → needinfo?(mstange)
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 21•6 years ago
|
||
:dholbert, :mstange any info on this fail? Should we go ahead and disable it on linux64, windows7-32 and osx-10 ?
Comment 22•6 years ago
|
||
Attachment #8964075 -
Flags: review?(jmaher)
Comment 23•6 years ago
|
||
(In reply to Narcis Beleuzu [:NarcisB] from comment #21) > :dholbert, :mstange any info on this fail? I don't know much here -- I'm really hoping mstange can look into it, since he's reviewed changes to this test and a decent amount of the related code... > Should we go ahead and disable it on linux64, windows7-32 and osx-10 ? That sounds pretty close to "all platforms". :-/ And this seems to be the main test for this feature. If it'd be possible to just skip this one sub-test (the code quoted in comment 18) on those platforms, e.g. using SpecialPowers mochitest API, that would likely be preferable to disabling the whole test...
Comment 24•6 years ago
|
||
Comment on attachment 8964075 [details] [diff] [review] Disable test on Win7 and Linux Review of attachment 8964075 [details] [diff] [review]: ----------------------------------------------------------------- while this patch does a good job of minimizing the effect if disabling, lets go with the suggestion of disabling the subtest, I am testing that on try server here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e722805e7fcb13f2da37a73b95d6af21e7848b42
Attachment #8964075 -
Flags: review?(jmaher) → review-
Comment 25•6 years ago
|
||
If it's possible to make the subtest-disabling be platform-specific (for the spammiest platforms), that would definitely be preferable, so that we still get *some* test coverage for this code... (I'm assuming we'd still get occasional random failures on the still-enabled platform, but it'd still be less-spammy-while-still-getting-coverage, for the time being, until we can get somebody to take a closer look at the (probably-real) bug here.)
Comment hidden (Intermittent Failures Robot) |
Comment 27•6 years ago
|
||
Hi! I tried to make a patch for disabling this subtest with the help of Joel. Daniel, could you please a look over it? Thank you. Here are my try pushes: https://tinyurl.com/y77u8mst - here I ran tests only on Linux and on QR I could see where it was skipped https://treeherder.mozilla.org/logviewer.html#?job_id=171961122&repo=try&lineNumber=1963 https://tinyurl.com/yc7rsutu - and this is with the other platforms, this runs pretty rarely but managed to make it fail one time.
Attachment #8964075 -
Attachment is obsolete: true
Flags: needinfo?(jmaher)
Flags: needinfo?(dholbert)
Attachment #8965203 -
Flags: review?(dholbert)
Comment 28•6 years ago
|
||
thanks for making the patch, lets see if :dholbert likes the approach.
Flags: needinfo?(jmaher)
Comment 29•6 years ago
|
||
Comment on attachment 8965203 [details] [diff] [review] disable_subtestbug1391154.patch Review of attachment 8965203 [details] [diff] [review]: ----------------------------------------------------------------- Nearly there, just a few nits: ::: dom/base/test/test_intersectionobservers.html @@ +881,4 @@ > }, 0); > }); > > + if (!WIN && !LINUX) { Please add a brief explanatory comment here -- e.g. // Unfortunately this subtest is extremely flaky on Windows and Linux, // so we skip it there for now. See Bug 1391154. Also, please indent the contents of this "if" block (by 2 more spaces per line) so that the nesting is easier to follow. @@ +903,5 @@ > callDelayed(function() { > targetEl1.style.transform = 'translateX(-40px) translateY(-40px)'; > }, 0); > + }) > + }; You're moving the semicolon here, but shouldn't do so... I think you need to leave the "});" line untouched -- aside from indenting it by 2 spaces -- and then just add a "}" on the line after it.
Attachment #8965203 -
Flags: review?(dholbert) → review-
Comment 30•6 years ago
|
||
Sorry for my silence here - I've seen the bug, but I have a really big backlog of things to do at the moment so I'm not expecting to be able to look into this any time soon.
Updated•6 years ago
|
Flags: needinfo?(dholbert)
Comment hidden (Intermittent Failures Robot) |
Comment 33•6 years ago
|
||
Thanks for the input Daniel. I've made the modifications you said. Could you please take a look and see if it's ok? Thanks.
Attachment #8965933 -
Flags: review?(dholbert)
Updated•6 years ago
|
Attachment #8965203 -
Attachment is obsolete: true
Comment hidden (Intermittent Failures Robot) |
Comment 35•6 years ago
|
||
Comment on attachment 8965933 [details] [diff] [review] disable_subtestbug1391154.patch Review of attachment 8965933 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/test/test_intersectionobservers.html @@ +882,5 @@ > }); > > + if (!WIN && !LINUX) { > + // Unfortunately this subtest is extremely flaky on Windows and Linux, > + // so we skip it there for now. See Bug 1391154. RE indentation: I was asking that you indent all the code inside this new "if" that you're adding. But in this patch version, you seem to have only intended its final 4 lines. To be extra-clear, typically code should look like: if (foo) { aaa; bbb; } ...not: if (foo) { aaa; bbb; } So: this still hasn't addressed that piece of feedback. Having said that, I'm also curious if mrbkap has anything promising from comment 32 -- we should check with him before landing a test-tweak here, because if he's about to fix this, we'd like to be able to detect that the fix worked.
Attachment #8965933 -
Flags: review?(dholbert) → review-
Assignee | ||
Updated•6 years ago
|
Attachment #8965933 -
Attachment is obsolete: true
Assignee | ||
Comment 36•6 years ago
|
||
As I went to fix this, I realized that the spec doesn't agree with test_intersectionobservers.html, Chrome's behavior, or our behavior. I'm going to file a bug to understand if we should all fix our code (and the test) or change the spec. This bug is caused by some overcomplicated code in the intersection observer implementation. In the case where the root element and the observed element only intersect at their edges, we ignore the thresholds passed into the observer's constructor and fire a notification when we otherwise wouldn't. This case is very timing dependent: if the refresh driver ticked when there was even a single pixel of overlap or just before the edges touched, we would not fire a notification. This is the behavior the test assumes, so I've made it consistent: we won't fire a notification until the intersectionRatio passes the smallest threshold.
Flags: needinfo?(mstange)
Comment hidden (mozreview-request) |
Comment 38•6 years ago
|
||
mozreview-review |
Comment on attachment 8966796 [details] Bug 1391154 - Handle edge cases around the borders of IntersectionObservers more consistently. https://reviewboard.mozilla.org/r/235476/#review241562 ::: dom/base/DOMIntersectionObserver.cpp:426 (Diff revision 1) > - if (intersectionRatio >= 1.0) { > - intersectionRatio = 1.0; > - threshold = (int32_t)mThresholds.Length(); > + // Let thresholdIndex be the index of the first entry in > + // observer.thresholds whose value is greater than intersectionRatio. > + threshold = mThresholds.IndexOfFirstElementGt(intersectionRatio); The comment says "thresholdIndex" but the variable is actually named "threshold". This is confusing. At first I thought this was a typo, but now I see you're quoting the spec (and the spec uses the term thresholdIndex) Please adjust to make that clearer. E.g. maybe: // Spec: "Let thresholdIndex be the ..." // ...
Attachment #8966796 -
Flags: review?(dholbert) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 40•6 years ago
|
||
I filed https://github.com/w3c/IntersectionObserver/issues/291 on the spec.
Comment 41•6 years ago
|
||
Pushed by mrbkap@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ee8433c44341 Handle edge cases around the borders of IntersectionObservers more consistently. r=dholbert
Comment 42•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ee8433c44341
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 43•6 years ago
|
||
Please request Beta approval on this when you get a chance.
status-firefox60:
--- → affected
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 44•6 years ago
|
||
I will once we have evidence of this fixing the intermittent.
Comment 45•6 years ago
|
||
OrangeFactor hasn't seen any reports since this landed. I think we're good :)
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 47•6 years ago
|
||
Comment on attachment 8966796 [details] Bug 1391154 - Handle edge cases around the borders of IntersectionObservers more consistently. Approval Request Comment [Feature/Bug causing the regression]: n/a [User impact if declined]: IntersectionObservers' behavior would not match Chrome's. [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]: No. [List of other uplifts needed for the feature/fix]: n/a [Is the change risky?]: Low risk. [Why is the change risky/not risky?]: There is some risk here, as we're changing the behavior of an interface. However, given that this change brings us in line with Chrome's behavior (and the previous behavior didn't even match the spec), no sites should depend on the old behavior. [String changes made/needed]: n/a
Flags: needinfo?(mrbkap)
Attachment #8966796 -
Flags: approval-mozilla-beta?
Comment 48•6 years ago
|
||
Comment on attachment 8966796 [details] Bug 1391154 - Handle edge cases around the borders of IntersectionObservers more consistently. Fixes a frequent orange and improves parity with Chrome. Approved for 60.0b13.
Attachment #8966796 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 49•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b8c46f489084
Flags: in-testsuite+
Whiteboard: [stockwell disable-recommended] → [stockwell fixed:product]
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•