Timeout exceeded for 'subject.scrollButton.hasAttribute('notifybgtab') == true' (testBackgroundTabScrolling.js)

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
MODULE: firefox/testTabbedBrowsing/testBackgroundTabScrolling.js
TEST:   testScrollBackgroundTabIntoView    
FAIL:   controller.waitForEval: Timeout exceeded for 'subject.scrollButton.hasAttribute('notifybgtab') == true'
BRANCH: default
OS:     Windows

We have to disable the test on Windows until bug 578162 has been fixed.
The notifybgtab should be there regardless of bug 578162.
(Assignee)

Comment 2

8 years ago
Looks like you are right and this is a timing issue in the test. I was finally able to reproduce it in my XP VM. I will investigate.
(Assignee)

Comment 3

8 years ago
Created attachment 502161 [details] [diff] [review]
Patch v1

This patch updates the test to not use the old waitForEval and assertJS members, which makes it much cleaner. For the real issue on this bug I'm not totally sure, what's going on. After the check for the scroll arrows the test has to wait for at least 100ms before I can open the last tab, which will cause a highlight on the scroll arrow.

Dao, is there an event we have to wait for? I really would like to get rid of this sleep call. For opening tabs we already register for the TabOpen event. So not sure what's really missing here.
Assignee: nobody → hskupin
Attachment #502161 - Flags: review?(gmealer)
Attachment #502161 - Flags: feedback?(dao)
(Assignee)

Updated

8 years ago
No longer depends on: 578162
Comment on attachment 502161 [details] [diff] [review]
Patch v1

> Dao, is there an event we have to wait for?

There's an overflow event dispatched to the tab strip, which might be useful here.
Attachment #502161 - Flags: feedback?(dao)
(Assignee)

Comment 5

8 years ago
Comment on attachment 502161 [details] [diff] [review]
Patch v1

Thanks. Lets see if that helps.
Attachment #502161 - Flags: review?(gmealer)
(Assignee)

Comment 6

8 years ago
(In reply to comment #4)
> There's an overflow event dispatched to the tab strip, which might be useful
> here.

Dao, are we talking about tabbrowser-tabs? If yes, the overflow event never gets dispatched for me:

function overflow(event) {
  tabstrip.removeEventListener("overflow", this);
  window.alert("overflow");
}

var tabstrip = window.document.getElementById("tabbrowser-tabs");
tabstrip.addEventListener("overflow", overflow, false);

Am I doing something wrong?
(Assignee)

Comment 7

8 years ago
Ok, got it. The failure was the missing 3rd parameter of the removeEventListener call.
(Assignee)

Comment 8

8 years ago
(In reply to comment #4)
> There's an overflow event dispatched to the tab strip, which might be useful
> here.

Ok, so using the event doesn't make any difference for this code path. Even with attaching a listener for the overflow event, opening a new background tab right after the scroll arrows have been shown, doesn't add the 'notifybgtab' attribute to the right arrow button. Only a sleep of about 100ms before the tab open action solved this problem.

For now I will add back the dependency for bug 578162 because the sleep is only necessary on Windows but no other platform. And bug 578162 is Windows only.
Depends on: 578162
(Assignee)

Comment 9

8 years ago
Created attachment 502664 [details] [diff] [review]
Patch v1.1 [checked-in]

Slightly updated patch
Attachment #502161 - Attachment is obsolete: true
Attachment #502664 - Flags: review?(gmealer)
Comment on attachment 502664 [details] [diff] [review]
Patch v1.1 [checked-in]

>-    // Wait until the new tab has been opened
>-    controller.waitForEval("subject.tabs.length == subject.count", gTimeout, 100,
>-                           {tabs: tabBrowser, count: ++count});
>-  } while ((scrollButtonDown.getNode().getAttribute("collapsed") == 'true') || count > 50)
>+    // Wait until the pages have been loaded, so they can be loaded from the cache
>+    var tab = controller.tabs.getTab(controller.tabs.length - 1);
>+    controller.waitForPageLoad(tab);

Not necessarily wrong, but I think you may have weakened the code a little bit here. 

Prior to your edit, it kept a running count of how many tabs should be there, and ensured that new tabs were opened by the middle-click. Post-edit, it just makes sure the right-most tab is fully loaded. If the click didn't fire for some reason or was too slow to pop the tab before you enter waitForPageLoad(), you'd simply move on because you'll be checking the wrong tab.

If you wanted to belt-and-suspenders, best bet might be to waitFor {controller.tabs.length === ++count} as before, then waitForPageLoad() once you've determined you have the right number of tabs.

Other than that, looks fine. r+, but please consider the comment above. r=me if you decide to make that change.
Attachment #502664 - Flags: review?(gmealer) → review+
(Assignee)

Comment 11

8 years ago
(In reply to comment #10)
> Prior to your edit, it kept a running count of how many tabs should be there,
> and ensured that new tabs were opened by the middle-click. Post-edit, it just
> makes sure the right-most tab is fully loaded. If the click didn't fire for
> some reason or was too slow to pop the tab before you enter waitForPageLoad(),
> you'd simply move on because you'll be checking the wrong tab.

We don't have to check the tab count anymore. It's part of the openInNewTab method from the tabs module. We are waiting for the 'TabOpen' event, which should be enough in most cases.

I don't think it is worth adding the tab count check to all of our tests. We have testNewTab which should take care of this check. I can update the mentioned test right after, if you agree on.
(Assignee)

Comment 12

8 years ago
Geo, please see my last comment.
Henrik, ah, OK. I hadn't realized openInNewTab did an end-check. Thanks for letting me know, and r+!
(Assignee)

Comment 14

8 years ago
Comment on attachment 502664 [details] [diff] [review]
Patch v1.1 [checked-in]

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/4be98fab891c

Lets keep this bug open for todays results. Also we should think about backporting it to the older branches, which would make the output more verbose.
Attachment #502664 - Attachment description: Patch v1.1 → Patch v1.1 [checked-in]
(Assignee)

Comment 15

8 years ago
Test failure has been gone in todays test-run. I will close this bug by leaving the sleep value in the test. We will have to revisit once bug 578162 has been fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.