Closed Bug 707663 Opened 9 years ago Closed 8 years ago

Timeout failure in /testTabbedBrowsing_PinAndUnpinAppTab/test1.js | Tab has scrolled into view

Categories

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

defect

Tracking

(firefox19 fixed, firefox20 fixed, firefox21 fixed, firefox22 fixed, firefox-esr10 wontfix, firefox-esr17 fixed)

RESOLVED FIXED
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
firefox21 --- fixed
firefox22 --- fixed
firefox-esr10 --- wontfix
firefox-esr17 --- fixed

People

(Reporter: mihaelav, Assigned: mario.garbi)

References

()

Details

(Whiteboard: [mozmill-test-failure][mozmill-endurance] s=130211 u=failure c=tabbedbrowsing p=1)

Attachments

(5 files, 3 obsolete files)

URL for the dashboard reports where the failure occurs:
Windows NT 6.1: http://mozmill-release.brasstacks.mozilla.com/#/endurance/report/2d28d4333da522cea0605939fb187d42
Mac OS X 10.6: http://mozmill-release.brasstacks.mozilla.com/#/endurance/report/2d28d4333da522cea0605939fb18794b
Linux Ubuntu 10.10: http://mozmill-release.brasstacks.mozilla.com/#/endurance/report/2d28d4333da522cea0605939fb188778

TEST: /testTabbedBrowsing_PinAndUnpinAppTab/test1.js::testPinAndUnpinAppTab
ERROR: TimeoutError("Tab has scrolled into view.")@resource://mozmill/modules/utils.js:429 waitFor((function () {return scrollButtonDown.getNode().hasAttribute("collapsed") || scrollButtonDown.getNode().disabled;}),"Tab has scrolled into view.",5000,100,(void 0))@resource://mozmill/modules/utils.js:467 ((function () {return scrollButtonDown.getNode().hasAttribute("collapsed") || scrollButtonDown.getNode().disabled;}),"Tab has scrolled into view.")@resource://mozmill/modules/controller.js:648 ()@resource://mozmill/modules/frame.js -> file:///c:/users/mozilla/appdata/local/temp/tmpikjrlu.mozmill-tests/tests/endurance/testTabbedBrowsing_PinAndUnpinAppTab/test1.js:83
WHEN: 2011-11-30
FIRST: 2011-11-30
FREQ: 1
BRANCHES: Nightly
As far as I can tell this hasn't occurred on more recent testruns.
Yeah, I can't find more than one occurrence. Let's make a habit of only opening failure bugs when they happen more frequently. My suggestion would be two days in a row.

Resolving WONTFIX for now. If it happens again more frequently, please reopen.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-endurance]
Resolution: WONTFIX → WORKSFORME
Issue is happening again on Ubuntu 10.10(since Dec 9) and Mac OS X 10.6(since Dec 10)

Mac reports: 
- http://mozmill-release.brasstacks.mozilla.com/#/endurance/reports?branch=11.0&platform=Mac&from=2011-12-10&to=2011-12-12
Ubuntu reports:
- http://mozmill-release.brasstacks.mozilla.com/#/endurance/reports?branch=11.0&platform=Linux&from=2011-12-09&to=2011-12-12
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Can this be reproduce locally? In the meantime, we should probably disable this test. Are you okay with that Dave?
Reluctantly, yes. I believe there's an almost duplicate of this bug though where we've made the same suggestion. Given the increase in this issue in 11.0 I suspect we should be able to replicate this locally. Can someone try and report back? Can we disable the test in the default branch only?
I tried on release and you won't see the tabs anymore, only the apptabs.
Remus: Could you explain what you mean? What did you try? Does release mean latest Firefox release (currently 8)? What tabs do you no longer see?
I would tend to say same symptoms as for bug 709063. We should dynamically check if there is enough space for tabs/app tabs before executing the next entity iteration.
Assignee: nobody → remus.pop
Status: REOPENED → ASSIGNED
10 tabs should not be a problem. I have run this locally without being able to replicate the issue. I think the next step would be to attempt to replicate this on the qa box where it's occurring.
Well, the absolute next step is to get this test disabled on the branches it is failing. Remus, please submit a patch for this ASAP.
Strangely enough, this fails only on nightly and not on aurora.
Attachment #582778 - Flags: review?(vlad.mozbugs)
Attachment #582778 - Flags: review?(vlad.mozbugs) → review+
Not particularly strange, it just implies this is caused by a regression that's landed on the default branch. Has there been any further attempts to replicate this?
The faill is still reproducible on latest versions of Nightly: 12.0
What is the visual representation of this failure? Mihaela, can you reproduce this locally with any number of iterations/entities?
This is still failing on our daily test runs. Can we get an update on it. Are we able to reproduce this locally? Should we skip this test for now?
I ran endurance with 50 iterations and 10 entities but I haven't got any failures. I even ran with a small sized window.
This should be investigated in a qa box.
Comment on attachment 582778 [details] [diff] [review]
skip test [checked-in:default,aurora,beta]

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

Test has now been disabled.
Attachment #582778 - Attachment description: skip test (default) → skip test (default) [checked-in]
Whiteboard: [mozmill-test-failure][mozmill-endurance] → [mozmill-test-failure][mozmill-endurance][mozmill-test-skipped]
Duplicate of this bug: 709063
Comment on attachment 582778 [details] [diff] [review]
skip test [checked-in:default,aurora,beta]

Note, as per recent merges, this test is now disabled on Nightly, Aurora, and Beta.
Attachment #582778 - Attachment description: skip test (default) [checked-in] → skip test [checked-in:default,aurora,beta]
Alex, can you please continue the work from Remus?
Assignee: remus.pop → alex.lakatos
Attached patch debugPatch v0.1Splinter Review
With the attached debug patch, I can reproduce the issue 7 out of 10 times. Not really sure how to handle it though. Any suggestions that apply to endurance?
Attachment #644314 - Flags: feedback?(hskupin)
Attachment #644314 - Flags: feedback?(dave.hunt)
(In reply to Alex Lakatos[:AlexLakatos] from comment #21)
> Created attachment 644314 [details] [diff] [review]
> debugPatch v0.1
> 
> With the attached debug patch, I can reproduce the issue 7 out of 10 times.
> Not really sure how to handle it though. Any suggestions that apply to
> endurance?

It looks to me that the patch just forces 100 tabs to be opened. You can do the same with --entities=100 on the command line when running endurance tests. In this case you're probably more likely hitting bug 709063 where there's no room for additional tabs on the tab bar. As I understand it, we just need a more robust way of waiting for the latest new tab to be in view, however that does not explain why this fails when we run with just 10 app tabs...
Attachment #644314 - Flags: feedback?(hskupin)
Attachment #644314 - Flags: feedback?(dave.hunt)
Attachment #644314 - Flags: feedback-
This now also happens on ESR10. I would say we should check if a backport has been caused this "new" failure on that branch.

Dave can you please make sure that this test gets disabled on esr10? The skip test doesn't apply anymore on that branch.

http://mozmill-ci.blargon7.com/#/endurance/report/fac9ae8444a3cbcc1dcf9cbaff27358b
Status: ASSIGNED → NEW
Attached patch skip test [esr10] (obsolete) — Splinter Review
Skip test for ESR branch.
Assignee: alex.lakatos → dave.hunt
Status: NEW → ASSIGNED
Attachment #664006 - Flags: review?(hskupin)
Comment on attachment 664006 [details] [diff] [review]
skip test [esr10]

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

This patch misses to disable the test in the manifest file. Please make sure that a thing like that will not pass the internal review in the future.
Attachment #664006 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #25)
> that a thing like that will not pass the internal review in the future.

Ups. Haven't noticed that it was you Dave who uploaded the patch. Sorry.
Priority: -- → P2
My mistake. Thanks Henrik.
Attachment #664006 - Attachment is obsolete: true
Attachment #664206 - Flags: review?(hskupin)
Attachment #664206 - Flags: review?(hskupin) → review+
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/66578a87bd28 (esr10)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
It would be fixed if the test is working but we disabled it across branches. :)
Assignee: dave.hunt → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I need a vacation. :)
Status: REOPENED → NEW
Whiteboard: [mozmill-test-failure][mozmill-endurance][mozmill-test-skipped] → [mozmill-test-failure][mozmill-endurance][mozmill-test-skipped] s=130211 u=failure c=endurance p=1
Whiteboard: [mozmill-test-failure][mozmill-endurance][mozmill-test-skipped] s=130211 u=failure c=endurance p=1 → [mozmill-test-failure][mozmill-endurance][mozmill-test-skipped] s=130211 u=failure c=tabbedbrowsing p=1
Assignee: nobody → mario.garbi
I had a question about what the task is here. The problem here is (as stated before and in bug 709063) that we cannot fit unlimited pinned tabs in a browser and the limitation is relative to the instance size and if it's maximised the screen's resolution.
 Do we want to change the test so it somehow fixes this problem or set an entities limitation to the test?
(In reply to mario garbi from comment #31)
> I had a question about what the task is here. The problem here is (as stated
> before and in bug 709063) that we cannot fit unlimited pinned tabs in a
> browser and the limitation is relative to the instance size and if it's
> maximised the screen's resolution.
>  Do we want to change the test so it somehow fixes this problem or set an
> entities limitation to the test?

That's not quite accurate. We can open the tabs, but once a certain threshold is reached (dependant most likely on size of the browser window) we do not wait long enough for the target tab (the one we want to pin) to scroll into view. Ideally, we will make this test more robust by waiting for an appropriate event. We already limit the number of tabs via the command line (10 in our CI testruns), and I don't think we want to reduce this.
We should disable tab scrolling for the endurance tests so that the tab will be visible right away. We might want to have two methods in the tab module to pin and unpin a tab.
 When saying that we should disable tab scrolling do you refer to setting the browser.tabs.tabMinWidth from about:config to 0? That is the only way I know how to disable the scroll buttons and it's obsolete/removed since 2010. As it is today we can set that preference from userChrome.css, file that is located in the install folder of Firefox or by using an addon. 
 After dissecting contextMenu.select into it's basic lines and observing them run I noticed that we fail the test when the controller.rightClick simply doesn't work and we get menu.state = "showing" and then suddently = "closed" instead of "open" as it should be, as if something stopped our drop down action.
 I should had be more clear in the initial comment, we can disable toolkit.scrollbox.smoothScroll so that we jump to the tab we want or set the value of toolkit.scrollbox.clickToScroll.scrollDelay to 0 so we have no delay... They both should help with the speed I think. Is this what you referred to?
That sounds fine to me and would correlate to my idea from the last comment.
Comment on attachment 716998 [details] [diff] [review]
patch v1.0

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

::: lib/tabs.js
@@ +374,5 @@
>  
>    /**
> +   * Pin the selected Tab
> +   *
> +   * @param {aTab} tab

I think "ElemBase" is the type here.

@@ +385,5 @@
> +
> +  /**
> +   * Unpin the selected Tab
> +   *
> +   * @param {aTab} tab

Also "ElemBase".

::: tests/endurance/testTabbedBrowsing_PinAndUnpinAppTab/test1.js
@@ +17,1 @@
>  function setupModule(module) {

Lets change to aModule here and in teardown(), since we're at it.

@@ +43,5 @@
>      // Open tabs
>      enduranceManager.loop(function () {
>          if (enduranceManager.currentEntity > 1) {
>            tabBrowser.openTab();
>          }

Here are 4 spaces as indentation, can you please edit this as well?

@@ +61,3 @@
>        }, "Tab has scrolled into view.");
>  
>        currentTab = tabBrowser.getTab(tabBrowser.length-1);

Could we use lastTabIndex here as well? It's getting updated with each iteration so I think it's safe.

@@ +77,5 @@
>      // Close all tabs
>      tabBrowser.closeAllTabs();
>    });
>  }
>  

Please remove this line since all test were cleaned of that.
Attachment #716998 - Flags: review?(andreea.matei) → review-
Could you also verify that this patch works with a large number of tabs (50+ entities)
 As long as we can't change the minWidth of pinned tabs we will be limited by the window size. After a certain number of tabs are pinned (depending on window size) the other tabs are placed in a container between the scroll arrows and this container's width is dynamically changed by the pinned tabs so we get a 0 width (we cannot click on the tabs any more). This can be reproduced manually and I'm not really sure how or if we can change it as long as pinned tabs act as they do now (they stay into view and don't change width dynamically).
 On my system with a maximized window it works with 50 entities, but if I resize the window it will fail.
 To add to the above comment and explain my patch, my preferences make the tab scrolling faster and reduce the chance of time-out, they don't fix the max pinned tabs issue as this is a firefox limitation. As I said since FF 4.0 they removed the browser.tabs.tabMinWidth from about:config and placed it into userChrome.css.
So I would be tempted to de-duplicate bug 709063. That would allow us to land this change and hopefully improve the stability of the test in our CI, but will leave a bug open for when we have 'too many' tabs open.
Status: NEW → ASSIGNED
Comment on attachment 718373 [details] [diff] [review]
patch v1.1

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

I tested on Linux and OS X, for Linux works with 25 tabs, on OS X with 27. After that fails.

Dave, can't we maximize the window for this test only as we did at that plugin visibility issue?

::: tests/endurance/testTabbedBrowsing_PinAndUnpinAppTab/test1.js
@@ +15,5 @@
> +const SCROLL_SMOOTH = "toolkit.scrollbox.smoothScroll";
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.tabBrowser = new tabs.tabBrowser(controller);

aModule.controller here as well.

@@ +16,5 @@
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.tabBrowser = new tabs.tabBrowser(controller);
> +  aModule.enduranceManager = new endurance.EnduranceManager(controller);

And here.

@@ +46,1 @@
>            tabBrowser.openTab();

This should also be 2 spaces from the parent (if).
Attachment #718373 - Flags: review?(andreea.matei) → review-
(In reply to Andreea Matei [:AndreeaMatei] from comment #44)
> Comment on attachment 718373 [details] [diff] [review]
> patch v1.1
> 
> Review of attachment 718373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I tested on Linux and OS X, for Linux works with 25 tabs, on OS X with 27.
> After that fails.
> 
> Dave, can't we maximize the window for this test only as we did at that
> plugin visibility issue?

Running with 'lots' of tabs is covered by bug 709063. Our CI uses 10 tabs, however if that causes issues then we should probably fix that bug too or like you say, try maximising the window.
 With this patch we should be good with 15 entities, after 20 we start having problems when there are too many pinned tabs.
Comment on attachment 722772 [details] [diff] [review]
patch v1.2

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/22f0acbac43b (default)

I have modified the commit message to reflect the changes done, please try to use more relevant descriptions.
Attachment #722772 - Flags: review?(andreea.matei) → review+
Attachment #724322 - Flags: review?(andreea.matei)
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/3c7672d6b0aa (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/9d44e1e69ca1 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/1e3f2e8a2493 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/8aac695c3d71 (esr17)

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-test-failure][mozmill-endurance][mozmill-test-skipped] s=130211 u=failure c=tabbedbrowsing p=1 → [mozmill-test-failure][mozmill-endurance] s=130211 u=failure c=tabbedbrowsing p=1
Attachment #724322 - Flags: review?(andreea.matei) → review+
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.