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

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
13 days ago

People

(Reporter: AndreeaMatei, Assigned: andrei)

Tracking

({regression})

Dependency tree / graph

Firefox Tracking Flags

(firefox24 fixed, firefox25 fixed)

Details

(Whiteboard: [mozmill-test-failure][sprint2013-39], )

Attachments

(3 attachments, 4 obsolete attachments)

Started yesterday to reproduce on Nightly, Linux only, all locales.
Whiteboard: [mozmill-test-failure]
We need more information here. Please take the usual steps to get this investigated soon. Is that a clear regression?
Looking into this one.
Assignee: nobody → andrei.eftimie
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][sprint2013-39]
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.
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.
Status: NEW → ASSIGNED
Posted patch Fix patch v1 (obsolete) — Splinter Review
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.
Attachment #768334 - Flags: feedback?(hskupin)
Attachment #768334 - Flags: feedback?(dave.hunt)
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 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.
Attachment #768334 - Flags: feedback?(hskupin)
Attachment #768334 - Flags: feedback?(dave.hunt)
Attachment #768334 - Flags: feedback-
It also reproduces in 2.0
I'll get a short testcase asap
Posted file simplified testcase (obsolete) —
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
Attachment #768903 - Flags: feedback?(hskupin)
Attachment #768903 - Flags: feedback?(dave.hunt)
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
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.
(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.
Happened again today on Linux 12.04 with Firefox 24.0a2:
http://mozmill-ci.blargon7.com/#/functional/report/a1b02004612785c13cf7c6bf1e472e04
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.
(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.
Posted patch Skip Patch v1 (obsolete) — Splinter Review
Attached is a skip patch.

Testrun: http://mozmill-crowd.blargon7.com/#/functional/report/a1b02004612785c13cf7c6bf1e53367d
Attachment #769672 - Flags: review?(andreea.matei)
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.
Attachment #769672 - Flags: review?(andreea.matei) → review-
Posted patch Skip Patch v2Splinter Review
Fixed nits.

Should be good to go.
Attachment #769672 - Attachment is obsolete: true
Attachment #769674 - Flags: review?(andreea.matei)
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)
Attachment #769674 - Flags: review?(andreea.matei) → review+
Whiteboard: [mozmill-test-failure][sprint2013-39] → [mozmill-test-failure][sprint2013-39][mozmill-test-skipped]
Posted file simplified testcase v2 (obsolete) —
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
Attachment #768903 - Attachment is obsolete: true
Attachment #768903 - Flags: feedback?(hskupin)
Attachment #768903 - Flags: feedback?(dave.hunt)
Flags: needinfo?(mano)
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.
Blocks: 834539
Flags: needinfo?(mak77)
Keywords: regression
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.
Posted file testcase v3
Thanks Marco for helping us figuring this out.

I've updated the testcase with some sleep statements, but it does not help.
Attachment #770242 - Attachment is obsolete: true
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.
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.
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.
Flags: needinfo?(mak77)
Flags: needinfo?(mano)
Depends on: 892454
The patch in bug 892454 should fix the problem, this test failure has been really useful, thank you!
Thanks Marco!

I'll look over the issue again once it reaches central.
(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.
The regression is fixed in central, please verify it satisfies the test.
Posted patch unskipSplinter Review
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?
Attachment #768334 - Attachment is obsolete: true
Attachment #777059 - Flags: review?(andreea.matei)
Flags: needinfo?(mak77)
I already asked for aurora approval, just matter of waiting for it.
Flags: needinfo?(mak77)
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)
Attachment #777059 - Flags: review?(andreea.matei) → review+
The fix is in Aurora too.
Awesome!
Thanks Marco
Andreea please backport the unskip patch to Aurora, it is passing now:
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d6164680a
Flags: needinfo?(andreea.matei)
Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/e40ce68c05ea (aurora)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(andreea.matei)
Resolution: --- → FIXED
Whiteboard: [mozmill-test-failure][sprint2013-39][mozmill-test-skipped] → [mozmill-test-failure][sprint2013-39]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.