552359 - Tinderbox orange: layout/reftests/forms/placeholder/placeholder-7.html placeholder-8.html placeholder-9.html

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Mats Palmgren (inactive)]

Attachment: Log with data: URIs

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268611926.1268613545.12202.gz
OS X 10.5.2 mozilla-central opt test reftest on 2010/03/14 17:12:06

See attach error log snippet for test image vs. reference image.

Comment 1

[Mats Palmgren (inactive)]

Looking at the test images, the <input> have the placeholder text, so it
seems to not have focus when the snapshot was taken?
I don't see how my caret cleanup (bug 544852) could have broken this
but I could be missing something...

Comment 2

[Markus Stange [:mstange]]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1269511430.1269512875.31049.gz
OS X 10.5.2 mozilla-central opt test reftest on 2010/03/25 03:03:50  
s: moz2-darwin9-slave17

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1269516844.1269518375.8836.gz
OS X 10.5.2 mozilla-central opt test reftest on 2010/03/25 04:34:04
s: moz2-darwin9-slave18

Comment 4

[(no longer active)]

This orange boils down to the fact that the input text boxes are not actually focused.  I think the issue will be solved by making sure that the reftest window is in foreground while reftests are running.  I'm still investigating a solution.

Comment 5

[(no longer active)]

Attachment: WIP

So, this patch does two things.  First, it uses the onfocus event handler on input elements to make sure to run the test only when they're focused.  It also modifies the reftest platform to make sure that the window is raised and focused before running each test.

This same code is used for browser chrome tests, but it doesn't work in the reftest framework.  I couldn't figure out why, asking for Neil's feedback who knows a lot more about our focus code than me.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm.  I rather liked the idea that reftests can work even when the window isn't focused.  I wonder if it's worth converting these to mochitests in order to preserve that.  Thoughts?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Caret reftests depend on having focus too.

What I would really like to do is have a way for tests to *pretend* a browser window has OS-level focus when it really doesn't.

Comment 8

[(no longer active)]

This particular problem, I think, shows an actual problem in the focus code.  Neil said that he's willing to debug this.

To debug this, run the reftests like this:

/path/to/firefox -reftest layout/reftests/forms/placeholder/reftest.list

so that the Firefox window is initially in the background.  Running tests that way makes them fail 100% of the time for me.

Comment 9

[Jeff Walden [:Waldo]]

Replacing the focus manager with a mock (controllable by preference) might be an interesting possibility, although preserving ability to dispatch and respond to OS-level events seems tricky.

Comment 10

[(no longer active)]

Attachment: Patch (v1)

So, Neil said that the main reason behind the problem is that the XUL browser element inside the chrome document is not focused.  Calling gBrowser.focus() seems to solve the problem.

So, I've fixed that problem in this patch.  I've also modified the placeholder reftests to actually depends on the focus event.  This means that they would still fail if the window is not focused, but they would time out this time, which is a lot better (at least, it's not misleading as a rendering problem.)

Also, note that the Tinderbox machines use |make reftest| which uses automation.py which passes -foreground to firefox, and it makes sure that the window is initially raised on Mac.

I think that the attached patch is enough for this bug.

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/e75d28fbaa0b

Comment 12

[Mats Palmgren (inactive)]

FYI, on Linux, you can use Xephyr to run tests on its own X server to avoid
focus problems.  As a bonus, it also makes it easier to limit CPU resources
using 'nice' for the tests.

Comment 13

[Mounir Lamouri (:mounir)]

I still have a random orange for placeholder-7.html. On 10 tries, I got it failing 7 times.
I simple workaround is to change disableReftestWait(); to setTimeout(disableReftestWait, 5);.

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch for placeholder-7.html

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Adding timeouts generally doesn't fix orange, it just makes it harder to reproduce.

We need to understand why this test fails without the timeout and then we can fix it properly.

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 439691 [details] [diff] [review]
Patch for placeholder-7.html

Canceling review per previous comment.
Someone else can reproduce this random failure ?

Comment 17

[(no longer active)]

Mounir, how did you run the tests?  Did you use |make reftest| or |firefox -reftest ...|?  Also, which platform did you try this on?

Comment 18

[Mounir Lamouri (:mounir)]

(In reply to comment #17)
> Mounir, how did you run the tests?  Did you use |make reftest| or |firefox
> -reftest ...|?  Also, which platform did you try this on?

I used |firefox - reftest| on a x86 platform.

Comment 19

[(no longer active)]

(In reply to comment #18)
> (In reply to comment #17)
> > Mounir, how did you run the tests?  Did you use |make reftest| or |firefox
> > -reftest ...|?  Also, which platform did you try this on?
> 
> I used |firefox - reftest| on a x86 platform.

I meant, which OS?  ;-)

Comment 20

[Mounir Lamouri (:mounir)]

Oups, why did I forget that? :)
I'm using GNU/Linux.

Comment 21

[(no longer active)]

So, when you run the test, does the window appear in the foreground or in the background?  Also, is it focued or not?  What happens when you use make reftest instead?  Also, do you get a reftest failure, or a timeout?

Here's why I'm asking these questions.  I found out that on Mac, if you use firefox -reftest, the Firefox window will appear in the background, so the text boxes are not focused, and that obviously messes up the placeholder tests, because they rely on the reftest window being focused.  Using make reftest solves this, because it runs Firefox in the foreground (and make reftest is what Tinderbox machines use).

On Linux, however, this should _not_ be an issue, AFAIK.

Comment 22

[Mounir Lamouri (:mounir)]

It shows in foreground so the issue is not linked with an unfocused input otherwise, |setTimeout| wouldn't fix it. It sounds much more like a repaint which is done after the reftest check.
There is a way to launch |make reftest| without doing _ALL_ reftests ? There is no 'reftest' target inside layout/ directory.

Btw, it also concerns placeholder-8.html.

Comment 23

[(no longer active)]

(In reply to comment #22)
> It shows in foreground so the issue is not linked with an unfocused input
> otherwise, |setTimeout| wouldn't fix it. It sounds much more like a repaint
> which is done after the reftest check.

Then I think this should be another bug, but let's make sure.

> There is a way to launch |make reftest| without doing _ALL_ reftests ? There is
> no 'reftest' target inside layout/ directory.

Yes:

make -C /path/to/objdir reftest TEST_PATH=layout/reftests/forms/placeholder/reftest.list

> Btw, it also concerns placeholder-8.html.

If you get the same failure with make reftest as well, please file a new bug and attach a reftest log of the failure.

Comment 24

[(no longer active)]

Let me be more clear here.  Unless we have a log besides the one attached in comment 0 which shows that this failure happens when the window actually has the focus here, I think this is a focus related issue, and this bug is FIXED now.

Comment 25

[Mounir Lamouri (:mounir)]

Attachment: Log for placeholder-7.html randomness

Comment 26

[(no longer active)]

(In reply to comment #25)
> Created an attachment (id=444802) [details]
> Log for placeholder-7.html randomness

Hmm, based on this log, this is a different bug.  Please file it separately.

Also, it would be great if you could debug this locally (because I haven't been able to reproduce this).  You could set a breakpoint in nsCaret::DrawCaret, and see the code path which invokes it for the reference file, and see what happens differently in the original test file when this fails randomly.

Marking this bug as FIXED again.

Comment 27

[Mounir Lamouri (:mounir)]

bug 565594 opened. Thansk Ehsan :)