818128 - Failure in testAwesomeBar/testVisibleItemsMax.js | Number of visible rows should equal 6

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Alex Lakatos[:AlexLakatos]]

TEST: /testAwesomeBar/testVisibleItemsMax.js::testVisibleItemsMax
ERROR: Number of visible rows should equal 6
WHEN: 12-04-2012
FIRST: 12-02-2012
FREQ: 2

Comment 1

[Daniela Petrovici]

Did not happen since 12/04

Comment 2

[u279076]

Firefox 23.0 (23.0, zh-TW, 20130718163513) on Linux Ubuntu 12.10 (x86):
http://mozmill-release.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d611e12a2

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #2)
> Firefox 23.0 (23.0, zh-TW, 20130718163513) on Linux Ubuntu 12.10 (x86):
> http://mozmill-release.blargon7.com/#/functional/report/
> 180cf2548ef2865af3ae441d611e12a2

Hm, most likely we should file separate bugs in the future if the problem occurs only for localized builds of firefox or a special locale. This failure we haven't seen for a really long time so I think there might be different causes.

Given that the en-US failure never re-occurred lets more this bug to cover the zh-TW locale.

Comment 4

[u279076]

Firefox 23.0 (23.0, zh-TW, 20130722172257) - Linux Ubuntu 12.10 (x86):
> http://mozmill-release.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61a1babf

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Andreea, it would be great if someone from the SV team could have a quick check what's going on here. We face this issue each time we run tests with the zh-TW locale.

Comment 6

[Andrei Eftimie]

Looking into it.

Comment 7

[Andrei Eftimie]

Hight failure rate on Linux. (9 out of 10)
Fails across all branches except ESR17.

Comment 8

[Andrei Eftimie]

Attachment: zh-TW_autocomplete_taller.jpg

zh-TW locale returns 7 visible rows in 
> autoCompleteResultsList.getNode().getNumberOfVisibleRows()

I can see that the 7th item has about 10px more of visible area.
You can clearly see that in the attached picture wich contains en-US and zh-TW overlaid at 50% opacity.

This might be a genuine regression in Firefox if the intention is to only see 6 complete rows, not more.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks like we actually depend on bug 583683. So the test might have to be disabled or only run parts of the checks.

Comment 10

[Andrei Eftimie]

I'm not sure 583683 is to blame.
The setting is still set at 6 items.

en-US the parent element has a height of 246px (41 * 6)
zh-TW (on OSX is still 246) the parent element reports has a total height of 282px (47 * 6)

Each item in the list is 5px taller on Linux zh-TW (might be glyph rendering takes up more vertical space or something similar). 

Manually I can't make the autocomplete popup show me the extra item.
It is consistent at 282px (47 * 6) without space underneath.

I'll try to reproduce manually (make sure I have the same pages visited in the same order) or have a small testcase, this looks like some rendering issue in FF to me.

Comment 11

[Andrei Eftimie]

Attachment: testcase.js

I've attached a simplified testcase.
Run it with mozmill as usual:
> mozmill -t testcase.js -b %path/to/firefox/binary%

I've left some dump statements for the following:
1) the height of all Autocomplete items
2) cumulative height of the first 6 items
3) total height of the Autocomplete window

2) and 3) should be equal
They are with a en-US build
They differ with a zh-TW build

This looks like a regression in Firefox.

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

Happened today on Ubuntu 12.10 (x86) with Beta zh-TW
http://mozmill-release.blargon7.com/#/functional/report/b7ef1fb3d9703aeaf2c46e07d28385e5

Comment 13

[u279076]

This failure occurs with other locales too:
> http://mozmill-release.blargon7.com/#/functional/failure?branch=23.0&platform=All&from=2013-08-14&to=2013-08-15&test=%2FtestAwesomeBar%2FtestVisibleItemsMax.js&func=testVisibleItemsMax.js%3A%3AtestVisibleItemsMax

Comment 14

[Andrei Eftimie]

Thanks for the link Anthony.
I would have expected to see this fail on languages with a non-latin script.

I have however not been able to reproduce this on OSX.
Will check these out.

This looks to have regressed somewhere between Firefox 18 and Firefox 19, I'll have some compiling to do.

Comment 15

[Tracy Walker [:tracy]]

defintiely not Zh-TW only.  This failure appears on many of the locales now that we're running all locales at least once per functional pass against betas.

Comment 16

[Andrei Eftimie]

I have been able to reproduce the failure on OSX.
While zh-TW works good on my machine, or fails in exactly the same way.

Windows does not seem affected, only *nix systems.

Comment 17

[Andrei Eftimie]

Affected languages that we are aware ATM:
ak 
ar 
as 
ja 
mai
ml 
mr 
or 
ta 
ta-LK
te 
zh-TW

This is clearly not a localisation issue per-se.
Possibly a glyph rendering, line-height calculation, flex-box height issue.

Comment 18

[mario garbi]

Happened again today on Linux Ubuntu 13.04 (x86) with Firefox 24.0 (24.0, ja, 20130902131354):
http://mozmill-release.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1c3b9b1

Comment 19

[mario garbi]

Happened again on Ubuntu 12.04 and 13.04 with Beta 'ja' locale. 

http://mozmill-release.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace6744558a450
http://mozmill-release.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace67445589a04

Comment 20

[Tracy Walker [:tracy]]

approximately 50% of all failures in the runs for beta have been this bug.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

P1! Lets get this fixed to make QA happy!

Comment 22

[Andrei Eftimie]

We have filed bug 901946 as this is most probably a Firefox Regression.

THe only solution to manage these failures right now would be to skip the test.
We only fail on certain 'exotic' locales, we could implement a blacklist of locales we want the test skipped... or skip it across all locales (this may be to invasive).

On fixing the real issue:
- the regression was introduced between Firefox 18 and Firefox 19
- I have gotten closer to building a localised Firefox (using compare-locale, thanks :pike, I have a list of changes I need to do for the l10n repo, but it is time-consuming, and there is lots of ground to be covered)
- in theory we have a pushlog for mozilla-release: http://hg.mozilla.org/releases/mozilla-release/pushloghtml?fromchange=8efe34fa2289&tochange=20238b786063 but the webserver fails to deliver anything (probably to big)

To summmarize:
1) I'll provide a blacklist based skip-patch
2) I'll continue trying to narrow down the origin of the regression (it would be great to have older localised builds of FF)

Comment 23

[Cosmin Malutan, [:cosmin-malutan]]

Happened again on Mac with beta 'ko' locale.

http://mozmill-release.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace67445776386

Comment 24

[Andrei Eftimie]

Well scratch the mozilla-release pushlog link, I don't think we can find any useful information there :(

Comment 25

[mario garbi]

Attachment: skip.patch

Skip patch for Beta and Release

Comment 26

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 801646 [details] [diff] [review]
skip.patch

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

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/92af909ac97f (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/f9463c6441de (release)

Thanks.

Comment 27

[Andrei Eftimie]

Why has this been skipped across all locales and only on Beta and Release?
This fails across all branches on specific locales.

I've explained in comment 22 that we would need a blacklist of locales for which the test is skipped, I've worked today on compiling that list.

We haven't seen failures on Nightly and Aurora because we didn't run this test against them.
We will soonish, and this will fail on the same locales.

I also don't understand why this had to be rushed before the merge (since we have prior established the branches are closed until the actual merge). Couldn't it have waited until tomorrow?

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No, this was the most concerning point of QA which we discussed during the meeting today. So please read the meeting notes about it. We only skipped the test on those branches to stop failures for ondemand tests. We still have to work our a solution for all brnaches, but only keep beta and release skipped.

Comment 29

[Andrei Eftimie]

Actually ESR17 is not affected as the regression was introduced in Firefox 19

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

This test showed up as a failure email to mozmill-ci but with 0 failures. The report shows 3 tests were skipped including this one: http://mozmill-release.blargon7.com/#/functional/report/a3b8d1e9bea5a0fc05cd41dbf639a2a4  

I'm not sure it's necessary to comment here on the skipped test but here goes.

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

That is, in a failure email from functional tests for Firefox 31.0 RC.

Comment 32

[Andreea Matei [:AndreeaMatei]]

That means the testrun did not end properly, most likely a known issue we're facing with WSAEINVAL. Daniel is the duty this week and will retrigger such testruns.

Comment 33

[Andreea Matei [:AndreeaMatei]]

Here we're also blocked by bug 901946 where I don't see a fix in the near future. Can we find a fix, like instead of checking 6 rows to check the line height x 6? Since that differs for some locales. For a fact we know we should have 6 rows and some locales have different heights for those rows. 
Something like it was done in bug 804968:
https://hg.mozilla.org/mozilla-central/rev/0fc382c36b4c#l1.39

The assert for 5 or 6 rows doesn't seem that reliable to me as we could miss a regression this way.

Comment 34

[Andrei Eftimie]

Attachment: fix.patch

Let's test for the actual algorithm that sets the height for the AutoComplete toolbar, which is first row height * maxrows [1] 

This patch does just that. Checking this still leaves us asserting that the number of visible rows is maxrows.

[1] http://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#1065-1073

Comment 35

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8476589 [details] [diff] [review]
fix.patch

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

Just a small nit.

::: firefox/tests/functional/testAwesomeBar/testVisibleItemsMax.js
@@ +69,5 @@
>    var maxRows = locationBar.urlbar.getNode().getAttribute("maxrows");
> +
> +  // Bug 901946
> +  // Because the height is not compted dynamically but is row1 height * maxrows
> +  // getNumberOfVisibleRows() returns a different number then max for certain

nit: than

Comment 36

[Andrei Eftimie]

Attachment: fix2.patch

Actually had another typo to fix.

Comment 37

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8477391 [details] [diff] [review]
fix2.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/e05203fa117d (default)

For backporting you'll need to also unskip the test.

Comment 38

[Andreea Matei [:AndreeaMatei]]

Attachment: backport.patch

Backported to aurora, there the test was enabled.
http://hg.mozilla.org/qa/mozmill-tests/rev/6b20de7dabbe (aurora)

From beta down, I'm adding the patch.

Comment 39

[Andrei Eftimie]

Comment on attachment 8481254 [details] [diff] [review]
backport.patch

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

Works great.
https://hg.mozilla.org/qa/mozmill-tests/rev/af566e58a984 (mozilla-beta)

Comment 40

[Andrei Eftimie]

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/4bcdf37d9466 (mozilla-release)
https://hg.mozilla.org/qa/mozmill-tests/rev/f885cdd997bf (mozilla-esr31)

We should be good here.