885723 - Test failure "Autocomplete popup has been opened" in testSwitchToTab.js

Status: RESOLVED FIXED

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

Description

[Andreea Matei [:AndreeaMatei]]

Started yesterday to reproduce on Nightly, Linux only, all locales.

Comment 1

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

We need more information here. Please take the usual steps to get this investigated soon. Is that a clear regression?

Comment 2

[Andrei Eftimie]

Looking into this one.

Comment 3

[Andrei Eftimie]

Managed to reproduce this constantly.
Fails on OSX and Linux only if testSuggestBookmarks.js is run before testSwitchToTab.js
The test passes fine if run independently. We might not properly clean up before.

Will have a regression range shortly.

Comment 4

[Andrei Eftimie]

Relevant pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=734147446def

This references bug 834539
Looking into what these changes mean for us and why we are failing here.

Comment 5

[Andrei Eftimie]

Attachment: Fix patch v1

While this patch fixes our issue, it looks to me as a regression in Firefox. I can also manually clear the History (while the test runs, before we type in the requested string) to achieve the same result.

It seems that if we have previously visited the page, the Autocomplete Popup does not load.

I haven't been able to manually reproduce this.
I have alos tried reproducing it in the same test with no success.
Something Mozmill is doing between tests to "write" the History down.

I'll try to have a simplified testcase or manual steps to reproduce this before logging a new defect against Firefox.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Can you create a reduced test case for this? If it needs to be two files that need to be run in sequence then that's fine, but even better would be a single test. Also, can you replicate this with Mozmill 2.0?

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 768334 [details] [diff] [review]
Fix patch v1

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

While it may be suitable for us to remove history between tests, doing it in just this one place would not prevent the test from potentially failing if another test it run immediately before it. Let's establish first if this is a Firefox regression.

Comment 8

[Andrei Eftimie]

It also reproduces in 2.0
I'll get a short testcase asap

Comment 9

[Andrei Eftimie]

Attachment: simplified testcase

Here is a 'simplified' testcase.
I couldn't trim it down further.

# Basic steps we perform here:
1) open the 'mozilla_grants.html' document
2) bookmark it via the bookmark menu entry
3) clear history
4) restore default bookmarks
5) open the 'mozilla_grants.html' document
6) open a new tab
7) write the expected string to trigger the 'mozilla_grants.html' document

# Expected result:
We should see an entry with the 'mozilla_grants.html' as 'Switch to tab' open in the Autocomplete Popup

# Actual result:
The Autocomplete Popup does not open at all

## Notes:
- Removing either 3) or 4) or both makes the test pass
- Reversing the order of 3) with 4) makes the test pass
- Adding a long sleep between 3) and 4) still fails the test (to help with the asyncronicity of the changes introduced with bug 834539

Comment 10

[Andrei Eftimie]

Some additional notes:

- This testcase FAILS with any Nightly build newer than (and including) the 22nd of June and PASSES with any older build.
- I've added the testcase as a zip file for it to be stand-alone. I've bundled all the needed library modules and the data page within

Comment 11

[Andrei Eftimie]

So given the testcase, I'm not 100% it is a regression in Firefox.
We might need to update our code for places.removeAllHistory() and/or places.restoreDefaultBookmarks() to work fine with the new multithreaded implementation from bug 834539
But I am not sure...

Dave, Henrik I would like some input from you guys on how to proceed.

Should I file a bug against Firefox (Toolkit / Places), or should we first dig deeper into our code? I don't really understand at this point how big the scope of the changes from bug 834539 are and how it affects us in these 2 aforementioned places.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Andrei Eftimie from comment #11)
> So given the testcase, I'm not 100% it is a regression in Firefox.
> We might need to update our code for places.removeAllHistory() and/or
> places.restoreDefaultBookmarks() to work fine with the new multithreaded
> implementation from bug 834539
> But I am not sure...
> 
> Dave, Henrik I would like some input from you guys on how to proceed.
> 
> Should I file a bug against Firefox (Toolkit / Places), or should we first
> dig deeper into our code? I don't really understand at this point how big
> the scope of the changes from bug 834539 are and how it affects us in these
> 2 aforementioned places.

I would like to see the test case reduced further. Ideally there would be no data/libs referenced at all. You can use mozqa.com instead of a local resource if necessary.

Are you able to replicate this manually at all? If not, then I suspect you are correct and we will need to update our standard modules to reflect recent changes. For now, let's get this test skipped or a suitable workaround applied.

Comment 13

[mario garbi]

Happened again today on Linux 12.04 with Firefox 24.0a2:
http://mozmill-ci.blargon7.com/#/functional/report/a1b02004612785c13cf7c6bf1e472e04

Comment 14

[Andrei Eftimie]

From what I've seen, (see comment 9) this happens when we use our places library.

I'm not able to replicate it manually at all.
I was not able to further reduce this testcase.
I might try, but we need to use lib/places.js for this to occur, and that adds most overhead.

Will supply a skip patch shortly.
Then I'll look into places.js to see if I can find any correlation with the recent changes that led to our tests failing.

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Andrei Eftimie from comment #14)
> From what I've seen, (see comment 9) this happens when we use our places
> library.
> 
> I'm not able to replicate it manually at all.
> I was not able to further reduce this testcase.
> I might try, but we need to use lib/places.js for this to occur, and that
> adds most overhead.

A reduced test case would move any dependent functions from shared libraries into the test itself. That way you're removing all functions not used, and reducing the amount of code required to replicate the issue.

Comment 16

[Andrei Eftimie]

Attachment: Skip Patch v1

Attached is a skip patch.

Testrun: http://mozmill-crowd.blargon7.com/#/functional/report/a1b02004612785c13cf7c6bf1e53367d

Comment 17

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 769672 [details] [diff] [review]
Skip Patch v1

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

Please add a commit message. And I see reports for nightly as well, why are we only skipping on aurora?

::: tests/functional/testAwesomeBar/manifest.ini
@@ +6,5 @@
>  [testPasteLocationBar.js]
>  [testSuggestBookmarks.js]
>  [testSuggestHistory.js]
>  [testSwitchToTab.js]
> +disabled = Bug 885723 - Test failure "Autocomplete popup has been opened" in testSwitchToTab.js

We are not using the test name anymore, it's right above.

::: tests/functional/testAwesomeBar/testSwitchToTab.js
@@ +95,5 @@
> +
> +setupModule.__force_skip__ = "Bug 885723 - Test failure 'Autocomplete popup " +
> +                             "has been opened' in testSwitchToTab.js";
> +teardownModule.__force_skip__ = "Bug 885723 - Test failure 'Autocomplete popup " +
> +                                "has been opened' in testSwitchToTab.js";

Same here, we're already in the giving test.

Comment 18

[Andrei Eftimie]

Attachment: Skip Patch v2

Fixed nits.

Should be good to go.

Comment 19

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 769674 [details] [diff] [review]
Skip Patch v2

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

Thanks. Skipped:
http://hg.mozilla.org/qa/mozmill-tests/rev/152a09aee73e (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/7cc277e0591f (aurora)

Comment 20

[Andrei Eftimie]

Attachment: simplified testcase v2

Mano, it would be great if you could give us some guidance.
From landing bug 834539 we've had a failure in one of our automated tests.

Attached is a simplified testcase.

As a general flow we are: bookmarking a page, removing the history, importing the default bookmarks, opening the initial page and in a new tab writing a keyword in and waiting for the Autocomplete bar to open.

This passes in Firefox pre bug 834539 and fails in every build afterwards.

To run the testcase you need mozmill:
> pip install mozmill

Then
> mozmill --v
should output: mozmill 2.0rc4

To run the test simply point mozmill to the test and FF binary
> mozmill -t testcase.js -b %APPLICATION_PATH%

My question for you Mano is:
Can you point us where we would need to update to be compatible with the new changes?
Or if what we are doing should work, any idea where this might have regressed?

Thanks

Comment 21

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

It's clearly a regression in our tests given the changes on bug 834539. As what has been already identified we most likely handle some places interaction incorrectly. I'm also adding Marco for information given that he already helped us a couple of times.

Comment 22

[Marco Bonardo [:mak]]

I will look at it better, at first glance I don't see a so strict relationship between the failure and the regressing bug, that bug added a new API but so far it's unused and no behavior should have changed. Could be the code recompile have changed the thread scheduling though and you lack some waiting, in some point of the code.
I will look at it later, in the meanwhile, could you please try if adding a sleep after waitForPageLoad helps? You are waiting for the page load there, but not for the visit addition.

Comment 23

[Andrei Eftimie]

Attachment: testcase v3

Thanks Marco for helping us figuring this out.

I've updated the testcase with some sleep statements, but it does not help.

Comment 24

[Marco Bonardo [:mak]]

I've rewritten the test as a mochitest-browser test, and I can reproduce the problem, if I remove the importFromURL stuff then everything works as expected.
The special thing about this import is that the second parameter is true, that means it's an import defaults, that means it will remove all bookmarks before restoring new ones. So our bookmark is gone.
We also clear history, so at this point the page should not exist anymore nor in history, nor in bookmarks.
Then we try to add another visit in the same docshell. I suspect here we hit the history flooding protection and the visit is ignored. I still have to verify that, but mostly I'm still unsure about the relation with bug 834539. Btw, as soon as I'll have a new build I can do some more debugging.

Comment 25

[Marco Bonardo [:mak]]

ok, I retire my theory about the flooding protection, it's not this case.
Instead looks like the new visit ends up having frecency 0, pages with this frecency are excluded from the autocomplete.
This is wrong, it should not have frecency 0. Something in the patch may have caused this misreading.

Comment 26

[Marco Bonardo [:mak]]

ah indeed, FetchPageInfo was not reading frecency before, Mano added frecency to the statements... I will file a bug to investigate this apart, it's a regression in history.

Comment 27

[Marco Bonardo [:mak]]

The patch in bug 892454 should fix the problem, this test failure has been really useful, thank you!

Comment 28

[Andrei Eftimie]

Thanks Marco!

I'll look over the issue again once it reaches central.

Comment 29

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

(In reply to Marco Bonardo [:mak] from comment #27)
> The patch in bug 892454 should fix the problem, this test failure has been
> really useful, thank you!

Oh, wow. Thanks a lot also for your quick investigation. We wouldn't have been come so far. Looking forward to the fix being landed.

Comment 30

[Marco Bonardo [:mak]]

The regression is fixed in central, please verify it satisfies the test.

Comment 31

[Andrei Eftimie]

Attachment: unskip

We can safely reenable the test on Nightly. (Aurora will still fail ATM)
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947a75a9a

Marco, do you know if the fix will be backported to Aurora?

Comment 32

[Marco Bonardo [:mak]]

I already asked for aurora approval, just matter of waiting for it.

Comment 33

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 777059 [details] [diff] [review]
unskip

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

Re-enabled on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/3b58270adf17 (default)

Comment 34

[Marco Bonardo [:mak]]

The fix is in Aurora too.

Comment 35

[Andrei Eftimie]

Awesome!
Thanks Marco

Comment 36

[Andrei Eftimie]

Andreea please backport the unskip patch to Aurora, it is passing now:
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d6164680a

Comment 37

[Andreea Matei [:AndreeaMatei]]

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/e40ce68c05ea (aurora)