Closed Bug 614643 Opened 10 years ago Closed 10 years ago

Intermittent failure in browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | g should have scrolled vertically (or any other letter) (or horizontally)

Categories

(Toolkit :: XUL Widgets, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla5
Tracking Status
status2.0 --- ?

People

(Reporter: jdm, Assigned: ehsan)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290628411.1290629834.8192.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central opt test mochitest-other on 2010/11/24 11:53:31

s: talos-r3-leopard-051
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | g should have scrolled vertically
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | g should have scrolled horizontally
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290619777.1290621332.1944.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central opt test mochitest-other on 2010/11/24 09:29:37

s: talos-r3-leopard-022
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | d should have scrolled vertically
Blocks: 295977
This is almost certainly a regression from bug 588435, since it started to happen a few days after that bug was landed.

Looking at http://hg.mozilla.org/mozilla-central/rev/3b157fcde708, there's something which bothers me there:

    1.76 +      doc.getElementById("forceredraw").style.left = ev.timeStamp % 100;

There is no guarantee that |ev.timeStamp % 100| is not equal to the current value of |doc.getElementById("forceredraw").style.left|.  I did some local tests, and I confirmed that if that happens on the first iteration, we get a timeout, and if that happens on the second iteration, we get a failure similar to this bug.

I have a patch which makes sure that the repaint happens in a more solid way.
Blocks: 588435
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #519316 - Flags: review?(neil)
Comment on attachment 519316 [details] [diff] [review]
Patch (v1)

Sorry, I didn't look at the test closely enough, otherwise I might have noticed the curly quotes in the comments for instance :s
Attachment #519316 - Flags: review?(neil) → review+
http://hg.mozilla.org/mozilla-central/rev/bd942ed342bb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
Happened again, reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Note that mozilla-2.0 has been branched off.
Target Milestone: mozilla2.0 → ---
When writing these tests, I had to skip a few frames (see http://hg.mozilla.org/mozilla-central/file/bd942ed342bb/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js#l32 ) before checking if the actions had any effect. So AfterPaint fires before the logic in BeforePaint actually took place. Or that AfterPaint does not guarantee that BeforePaint was even fired.

Increasing the number of skipped frames may help as an intermediate solution.

But the problem itself lies somewhere else, maybe someone can enlighten me:
These are the steps that should (but do not apparently) occur in order:

- We synthesize the mouse click/move (this is async I believe? maybe the problem source is right there?)
-- Thus the autoscroll handler becomes active and does his own requestAnimationFrame/BeforePaint magic
- We add our AfterPaint listener in the test, hoping that the above BeforePaint magic already happened
- The test rolls its own requestAnimationFrame magic to also test the cases where there should be no magic done by the autoscroll handler.
(- The test skips 3 frames as mentioned above)

So how can we guarantee that the autoscroll handlers BeforePaint actually does his job before the tests AfterPaint checks for the result, especially without waiting 3 or more frames for each test (which adds up to longer test runtime)?!?
Attached patch Patch (v2)Splinter Review
OK, I investigated this test on IRC with Arpad more closely.

It seems that generally, the assumption that each beforepaint event is followed by an afterpaint event is incorrect.  These two events can fire in varying degrees, seemingly randomly.

A more robust approach would be to just use the MozBeforePaint event.  The first time that this event fires we should ignore it, since that's when the browser starts autoscroll.  We should handle it at the second time, and perform the test then.
Attachment #519316 - Attachment is obsolete: true
Attachment #520375 - Flags: review?(arpad.borsos)
Comment on attachment 520375 [details] [diff] [review]
Patch (v2)

I guess you have tested it to make sure it works? (I haven’t)
Attachment #520375 - Flags: review?(arpad.borsos) → review+
(In reply to comment #260)
> Comment on attachment 520375 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=520375
> Patch (v2)
> 
> I guess you have tested it to make sure it works? (I haven’t)

Yes, I have!
Depends on: post2.0
Whiteboard: [orange] → [orange][post-2.0]
http://hg.mozilla.org/projects/cedar/rev/b141be707734
Whiteboard: [orange][post-2.0] → [orange][post-2.0][fixed-in-cedar]
http://hg.mozilla.org/mozilla-central/rev/8f5bcdfe4e0e
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
No longer depends on: post2.0
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [orange][post-2.0][fixed-in-cedar] → [orange]
Target Milestone: --- → mozilla2.2
status2.0: --- → ?
Version: unspecified → Trunk