Intermittent browser_tabview_bug587503.js | In the end, the group was showing a dropSpace - Got false, expected true

RESOLVED FIXED in Firefox 17

Status

Firefox Graveyard
Panorama
P3
normal
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: philor, Assigned: andreshm)

Tracking

({intermittent-failure})

Trunk
Firefox 17
intermittent-failure
Dependency tree / graph

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1294886929.1294888492.26455.gz#err4
Build Log (Brief)
Rev3 WINNT 6.1 mozilla-central opt test mochitest-other on 2011/01/12 18:48:49
s: talos-r3-w7-043

TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug587503.js | Tab 5 is back and again the fifth tab.
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug587503.js | The group began by not showing a dropSpace
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug587503.js | In the end, the group was showing a dropSpace - Got false, expected true
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug587503.js | The group is empty again
Comment hidden (Treeherder Robot)
Blocks: 585689

Updated

6 years ago
Priority: -- → P3
Don't see this happens recently.  May be this has been fixed by other patch(es)?
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Resolution: FIXED → WORKSFORME
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Seems to happen very rarely, but it does.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
OS: Windows 7 → All
Hardware: x86 → All
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Does anybody have any idea why this happened?  I have a cycle collector patch that is triggering this about 1/4 the time, which is bizarre.
Comment hidden (Treeherder Robot)
(Assignee)

Comment 17

5 years ago
I'm able to reproduce this bug when moving focus out of the window while testing. 
I think is probably related to the timeouts in the test code. 

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug587503.js | Tab 5 is back and again the fifth tab. - Got 6, expected 4
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug587503.js | In the end, the group was showing a dropSpace - Got false, expected true
(Assignee)

Comment 18

5 years ago
Created attachment 640417 [details] [diff] [review]
Patch v1

I updated the test in order to use the common methods in the head file and to try to avoid the focus issues with timeouts. 
The tests should be checking the same things that in the old test.  

Also, if we include the TestRunner in the TabCandy tests, then we can improve it more.
Attachment #640417 - Flags: review?(ttaubert)
Comment on attachment 640417 [details] [diff] [review]
Patch v1

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

Thank you for refactoring this antique test! Looks good but there are still some issues we need to fix.

The test failed for me while running the whole test suites but not when running standalone. This is most definitely caused by tests that run before this one that move and resize groups. To achieve more consistency when having tests like this that need exact group and tab positions and sizes we usually open a new window, do our testing and close it afterwards. It's pretty easy as you'll see below.

::: browser/components/tabview/test/browser_tabview_bug587503.js
@@ +41,5 @@
> +
> +    children.forEach(function(child) {
> +      url = child.tab.linkedBrowser.currentURI.spec;
> +      indexes.push(url.replace("about:blank#", ''));
> +    });

You could do:

> return aGroup.getChildren().map(function (child) {
>   let url = child.tab.linkedBrowser.currentURI.spec;
>   return url.replace("about:blank#", "");
> }).join(",");

@@ +65,5 @@
> +    is(aCW.GroupItems.groupItems.length, 3, "Validate group count in test 3.");
> +
> +    // Test 4: Move the fifth tab back into the group, on the second row.
> +    // Wait for the tab to resize, as it does after we move it out of the group.
> +    window.setTimeout(function() {

This doesn't work locally for me because it's still unreliable. We can't expect the transition to have ended when our timeout gets called. As we use css transitions to animate the group resizing you can do the following:

> let groupContainer = tab.parent.container;
> groupContainer.addEventListener("transitionend", function onTransitionEnd() {
>   groupContainer.removeEventListener("transitionend", onTransitionEnd);
>   /* ... validate tab positions and finish ... */
> });

@@ +73,5 @@
> +    }, 0);
> +  }
> +
> +  function createGroup() {
> +    let cw = TabView.getContentWindow();

function createGroup(win) {
  registerCleanupFunction(function () win.close());
  let cw = win.TabView.getContentWindow();

@@ +87,5 @@
> +
> +    moveTabs(group, cw);
> +  }
> +
> +  showTabView(createGroup);

newWindowWithTabView(createGroup);
Attachment #640417 - Flags: review?(ttaubert) → feedback+
Assignee: nobody → andres
(Assignee)

Comment 20

5 years ago
Created attachment 641238 [details] [diff] [review]
Patch v2

Updated patch.
Attachment #640417 - Attachment is obsolete: true
Attachment #641238 - Flags: review?(ttaubert)
(Assignee)

Updated

5 years ago
Status: REOPENED → ASSIGNED
Comment hidden (Treeherder Robot)
Comment on attachment 641238 [details] [diff] [review]
Patch v2

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

Thank you! r=me with the little fix below.

::: browser/components/tabview/test/browser_tabview_bug587503.js
@@ +108,5 @@
> +function waitForTransition(aTab, aCallback) {
> +  let groupContainer = aTab.parent.container;
> +  groupContainer.addEventListener("transitionend", function onTransitionEnd() {
> +    groupContainer.removeEventListener("transitionend", onTransitionEnd);
> +    if (aCallback) {

We don't need to check for aCallback here as there's only one consumer that passes a callback - so the second argument is required.
Attachment #641238 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 23

5 years ago
Created attachment 646213 [details] [diff] [review]
Patch v3

Fixed issue.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3debd6feaa81
Attachment #641238 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
Try finished with some oranges: https://tbpl.mozilla.org/?tree=Try&rev=3debd6feaa81
Win opt shows this:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug587503.js | Validate tab positions in test 4. - Got 0,6,2,3,5,1,4, expected 0,6,2,3,4,5,1 

Looks like the test is flaky again :/
(Assignee)

Comment 26

5 years ago
Created attachment 646737 [details] [diff] [review]
Patch v4

I changed the last test to move the tab to the desired tab position instead of the group center. 

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=cf634618f129
Attachment #646213 - Attachment is obsolete: true
Attachment #646737 - Flags: review?(ttaubert)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 28

5 years ago
Try fail again on the last test. When moving the tab back to the group center. 

https://tbpl.mozilla.org/?tree=Try&rev=a97968e46315

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug587503.js | Validate tab positions in test 4. - Got 0,6,2,3,5,1,4, expected 0,6,2,3,4,5,1
Comment on attachment 646737 [details] [diff] [review]
Patch v4

Clearing review until you got that orange sorted out.
Attachment #646737 - Flags: review?(ttaubert)
Comment hidden (Treeherder Robot)
I think the solution here is to make "waitForTransition" look like this:

> function waitForTransition(aTab, aCallback) {
>   let groupContainer = aTab.parent.container;
>   groupContainer.addEventListener("transitionend", function onTransitionEnd(aEvent) {
>     if (aEvent.target == groupContainer) {
>       groupContainer.removeEventListener("transitionend", onTransitionEnd);
>       executeSoon(aCallback);
>     }
>   });
> }

I checked locally and the .resizer element does indeed send a transitionend event as well. That might be what causes the intermittent orange. Also we should probably use executeSoon() to make sure the transition has ended here and other listeners have done their work.
Try seems to agree:

https://tbpl.mozilla.org/?tree=Try&rev=a232603371b8
(Assignee)

Comment 33

5 years ago
Created attachment 647994 [details] [diff] [review]
Patch v5

Great! I just updated the patch.
Attachment #646737 - Attachment is obsolete: true
Attachment #647994 - Flags: review?(ttaubert)
Comment on attachment 647994 [details] [diff] [review]
Patch v5

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

Thanks!
Attachment #647994 - Flags: review?(ttaubert) → review+
Depends on: 780134
https://hg.mozilla.org/integration/fx-team/rev/84324c323762
Whiteboard: [orange] → [orange][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/84324c323762
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [orange][fixed-in-fx-team] → [orange]
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/releases/mozilla-beta/rev/325bc7718307
status-firefox15: --- → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/233563ef88bd
status-firefox16: --- → fixed
Keywords: intermittent-failure
Whiteboard: [orange]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.