Closed Bug 1049786 Opened 5 years ago Closed 5 years ago

Intermittent browser_bug295977_autoscroll_overflow.js | Window for m should have scrolled vertically

Categories

(Toolkit :: XUL Widgets, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 --- fixed
firefox-esr24 --- unaffected
firefox-esr31 --- unaffected

People

(Reporter: RyanVM, Assigned: martijn.martijn)

References

()

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 3 obsolete files)

https://tbpl.mozilla.org/php/getParsedLog.php?id=45343567&tree=Mozilla-Inbound

Windows XP 32-bit mozilla-inbound opt test mochitest-browser-chrome-3 on 2014-08-06 09:49:48 PDT for push 931943302ba5
slave: t-xp32-ix-078

09:56:52     INFO -  254 INFO timestamp=1407344212708 firstTimestamp=1407344212708 timeCompensation=0
09:56:52     INFO -  255 INFO timestamp=1407344212716 firstTimestamp=1407344212708 timeCompensation=0.4
09:56:52     INFO -  256 INFO timestamp=1407344212732 firstTimestamp=1407344212708 timeCompensation=1.2
09:56:52     INFO -  257 INFO TEST-PASS | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | j should have scrolled vertically - j should have scrolled vertically
09:56:52     INFO -  258 INFO TEST-PASS | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | j should not have scrolled horizontally - j should not have scrolled horizontally
09:56:52     INFO -  259 INFO Console message: [JavaScript Error: "chrome://browser/content/browser.xul : Unable to run script because scripts are blocked internally."]
09:56:52     INFO -  260 INFO must wait for load
09:56:52     INFO -  261 INFO Console message: [JavaScript Warning: "The character encoding of a framed document was not declared. The document may appear different if viewed without the document framing it." {file: "data:text/html,<div%20style='border:%205px%20solid%20blue;%20height:%20200%;%20width:%20200%;'></div>" line: 0}]
09:56:52     INFO -  262 INFO timestamp=1407344212796 firstTimestamp=1407344212796 timeCompensation=0
09:56:52     INFO -  263 INFO timestamp=1407344212827 firstTimestamp=1407344212796 timeCompensation=1.55
09:56:52     INFO -  264 INFO TEST-PASS | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | k should have scrolled vertically - k should have scrolled vertically
09:56:52     INFO -  265 INFO TEST-PASS | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | k should have scrolled horizontally - k should have scrolled horizontally
09:56:52     INFO -  266 INFO timestamp=1407344212835 firstTimestamp=1407344212835 timeCompensation=0
09:56:52     INFO -  267 INFO timestamp=1407344212848 firstTimestamp=1407344212835 timeCompensation=0.65
09:56:52     INFO -  268 INFO timestamp=1407344212867 firstTimestamp=1407344212835 timeCompensation=1.6
09:56:52     INFO -  269 INFO TEST-PASS | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | Window for k should not have scrolled vertically - Window for k should not have scrolled vertically
09:56:52     INFO -  270 INFO TEST-PASS | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | Window for k should not have scrolled horizontally - Window for k should not have scrolled horizontally
09:56:52     INFO -  271 INFO timestamp=1407344212882 firstTimestamp=1407344212882 timeCompensation=0
09:56:52     INFO -  272 INFO timestamp=1407344212899 firstTimestamp=1407344212882 timeCompensation=0.85
09:56:52     INFO -  273 INFO timestamp=1407344212916 firstTimestamp=1407344212882 timeCompensation=1.7
09:56:52     INFO -  274 INFO TEST-PASS | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | l should not have scrolled vertically - l should not have scrolled vertically
09:56:52     INFO -  275 INFO TEST-PASS | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | l should not have scrolled horizontally - l should not have scrolled horizontally
09:56:52     INFO -  276 INFO timestamp=1407344212915 firstTimestamp=1407344212915 timeCompensation=0
09:56:52     INFO -  277 INFO timestamp=1407344212951 firstTimestamp=1407344212915 timeCompensation=1.8
09:56:52     INFO -  278 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | Window for m should have scrolled vertically
09:56:52     INFO -  Stack trace:
09:56:52     INFO -  chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js:checkScroll:130
09:56:52     INFO -  null:null:0 - Window for m should have scrolled vertically
09:56:52     INFO -  Stack trace:
09:56:52     INFO -  chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js:checkScroll:130
09:56:52     INFO -  null:null:0
09:56:52     INFO -  TEST-INFO | expected PASS
09:56:53     INFO -  279 INFO TEST-OK | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | took 1409ms
Shat I noticed when this failure happens, I only see 2 timestamps of this kind beforehand:
9:56:52     INFO -  276 INFO timestamp=1407344212915 firstTimestamp=1407344212915 timeCompensation=0
09:56:52     INFO -  277 INFO timestamp=1407344212951 firstTimestamp=1407344212915 timeCompensation=1.8

With the passes, there are at least 3 and locally, I get sometimes 4 of these timestamp logs.

So I could add a check that checkScroll is called 3 times at least, before continuing with the test.
Attached patch 1049786.diff (obsolete) — Splinter Review
Like this, no idea if that would fix this intermittent failure for sure, though.
Attachment #8468729 - Flags: review?(adw)
Otoh, there might no 3 mozRequestAnimationFrame things be called in this instance,
Assignee: nobody → martijn.martijn
Comment on attachment 8468729 [details] [diff] [review]
1049786.diff

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

I'm pretty sure isn't right and won't truly fix it.  The right fix should probably be based on the amount of time that's passed since checkScroll was first called.  But the test is already doing that...

I don't have time to look at this in depth right now, but you could try changing the 1 that timeCompensation is compared against to something higher, like 5, which would make us wait 100ms's worth of frames instead of 20ms's to check the scroll: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js#119

Then push your patch to try with a fresh tree and retrigger a ton of tests.  If you still get oranges, try changing the value to 50, which would make us wait for 1s's worth of frames and ought to be more than enough.  If there are still oranges, then there's probably some other problem.
Attachment #8468729 - Flags: review?(adw) → review-
Attached patch 1049786.diff (obsolete) — Splinter Review
(In reply to Drew Willcoxon :adw from comment #5)
> Then push your patch to try with a fresh tree and retrigger a ton of tests. 
> If you still get oranges, try changing the value to 50, which would make us
> wait for 1s's worth of frames and ought to be more than enough.  If there
> are still oranges, then there's probably some other problem.

Well, it's already failing intermittently. Increasing the timeout should certainly not make matters worse, so why not check it in and see if it fixes oranges on Windows XP?
Attachment #8468729 - Attachment is obsolete: true
Attachment #8469751 - Flags: review?(adw)
I retriggered the tryserver mochitest-browser-chrome-3 on windows XP 32-bit 15 times, they are all green.
Comment on attachment 8469751 [details] [diff] [review]
1049786.diff

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

(In reply to Martijn Wargers [:mwargers] (QA) from comment #6)
> Well, it's already failing intermittently. Increasing the timeout should
> certainly not make matters worse, so why not check it in and see if it fixes
> oranges on Windows XP?

I think that's what I was suggesting.
Attachment #8469751 - Flags: review?(adw) → review+
Status: NEW → ASSIGNED
(In reply to Drew Willcoxon :adw from comment #11)
> I think that's what I was suggesting.

Er, I missed the "check it in" part of your comment, sorry.
Comment on attachment 8469751 [details] [diff] [review]
1049786.diff

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

::: toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js
@@ +76,5 @@
>        // Try to wait until enough time has passed to allow the scroll to happen.
>        // autoscrollLoop incrementally scrolls during each animation frame, but
>        // due to how its calculations work, when a frame is very close to the
>        // previous frame, no scrolling may actually occur during that frame.
>        // After 20ms's worth of frames, timeCompensation will be 1, making it

Oh, could you please update this to "... 100ms's worth of frames, timeCompensation will be 5..."
> Oh, could you please update this to "... 100ms's worth of frames,
> timeCompensation will be 5...

Done, thanks.
Let's hope this will fix the intermittent failure.
Attachment #8469751 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8470181 - Attachment description: 1049786.diff → 1049786.diff (for check-in)
https://hg.mozilla.org/mozilla-central/rev/85b280e74482

Leaving this open for now because I'm seeing some Linux bc3 timeouts that are making me nervous. We may want to back this out if they keep up.
Yeah, those in bug 943567 look strange. Not sure if they have anything to do with my changing this test (which is apparently very fragile). I'll comment in that bug.
Blocks: 251903
Attached patch 1049786_part2.diff (obsolete) — Splinter Review
Ok, what might be happening with the latest timeouts in bug 943567 is that perhaps the active test window was closed, because 1 (or both) of the tabs that should have opened by middle-click were not opening.
I noticed that when closing the test window on endTest did result into what I noticed in bug 943567, comment 38.
This should make the test more robust anyway.
Attachment #8470562 - Flags: review?(adw)
Blocks: 1051667
Comment on attachment 8470562 [details] [diff] [review]
1049786_part2.diff

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

::: toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js
@@ +206,4 @@
>  
> +      // remove 2 tabs that were opened by middle-click on links
> +      while (gBrowser.visibleTabs.length > 1) {
> +        gBrowser.removeTab(gBrowser.tabs[gBrowser.visibleTabs.length - 1]);

Shouldn't this be gBrowser.visibleTabs[gBrowser.visibleTabs.length - 1]?
Attachment #8470562 - Flags: review?(adw) → review+
(In reply to Drew Willcoxon :adw from comment #19)
> Comment on attachment 8470562 [details] [diff] [review]
> 1049786_part2.diff
> 
> Review of attachment 8470562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js
> @@ +206,4 @@
> >  
> > +      // remove 2 tabs that were opened by middle-click on links
> > +      while (gBrowser.visibleTabs.length > 1) {
> > +        gBrowser.removeTab(gBrowser.tabs[gBrowser.visibleTabs.length - 1]);
> 
> Shouldn't this be gBrowser.visibleTabs[gBrowser.visibleTabs.length - 1]?

Yes, it does!
Attachment #8470562 - Attachment is obsolete: true
Attachment #8470181 - Attachment description: 1049786.diff (for check-in) → 1049786.diff (checked in)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4d2bfef488bf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.