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)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

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

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:product])

Attachments

(1 file, 3 obsolete files)

Bug 1391154 - Handle edge cases around the borders of IntersectionObservers more consistently.
59 bytes, text/x-review-board-request
dholbert
: review+
RyanVM
: approval-mozilla-beta+
Details 
https://wiki.mozilla.org/Bugmasters#Intermittent_Test_Failure_Cleanup
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
https://wiki.mozilla.org/Bugmasters#Intermittent_Test_Failure_Cleanup
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → INCOMPLETE
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
https://wiki.mozilla.org/Bug_Triage#Intermittent_Test_Failure_Cleanup
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
Resolution: --- → INCOMPLETE
Recent failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=164186195&repo=autoland&lineNumber=9414
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
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]
Who knows intersection observers these days?
Flags: needinfo?(overholt) → needinfo?(dholbert)
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)
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)
:dholbert, :mstange any info on this fail?
Should we go ahead and disable it on linux64, windows7-32 and osx-10 ?
Attached patch Disable test on Win7 and Linux (obsolete) — — Splinter Review 

        Attachment #8964075 -
        Flags: review?(jmaher)

    
    

    
  
(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 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-

    
    

    
  
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.)

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        disable_subtestbug1391154.patch (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  
thanks for making the patch, lets see if :dholbert likes the approach.
Flags: needinfo?(jmaher)

    
    

    
  
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-

    
    

    
  
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.
Flags: needinfo?(dholbert)

    
    

    
      
  

    
    

    
  
I have a theory I'm testing out.
Assignee: nobody → mrbkap

    
    

    
  

      
      
      
      

        Attached patch
          
          
        disable_subtestbug1391154.patch (obsolete)
        — 
          — Splinter Review
      

    


  
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)

        Attachment #8965203 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
  
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-

        Attachment #8965933 -
        Attachment is obsolete: true

    
    

    
  
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 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+

    
    

    
      
  

    
    

    
  
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

    
    

    
  
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

    
    

    
  
Please request Beta approval on this when you get a chance.

          status-firefox60:
          --- → affected
Flags: needinfo?(mrbkap)

    
    

    
  
I will once we have evidence of this fixing the intermittent.

    
    

    
  
OrangeFactor hasn't seen any reports since this landed. I think we're good :)

    
    

    
      
  

    
    

    
  
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 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+

    
    

    
  
https://hg.mozilla.org/releases/mozilla-beta/rev/b8c46f489084

          status-firefox60:
          affected → fixed
Flags: in-testsuite+
Whiteboard: [stockwell disable-recommended] → [stockwell fixed:product]
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: