663838 - Intermittent TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Test timed out

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Matt Brubeck (:mbrubeck)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1307972575.1307974430.21626.gz

Rev3 Fedora 12 mozilla-central opt test mochitest-other on 2011/06/13 06:42:55 

INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_group.js | finished in 1314ms
TEST-INFO | checking window state
TEST-INFO | before wait for focus -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XrayWrapper [object Window]]) about:blank desired window: ([object ChromeWindow]) chrome://browser/content/browser.xul child window: ([object XrayWrapper [object Window]]) about:blank docshell visible: true
TEST-INFO | already focused
TEST-INFO | maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XrayWrapper [object Window]]) about:blank desired window: ([object ChromeWindow]) chrome://browser/content/browser.xul child window: ([object XrayWrapper [object Window]]) about:blank docshell visible: true
TEST-START | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View starts hidden
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | The tab view iframe exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Test timed out
XScreenSaver state: Disabled
User input has been idle for 5476 seconds
before 569344, after 548864, break 08719000
args: ['/home/cltbld/talos-slave/test/build/bin/screentopng']
[screenshot cut for length]
INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | finished in 30004ms
TEST-INFO | checking window state
TEST-INFO | before wait for focus -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XrayWrapper [object Window]]) about:blank desired window: ([object ChromeWindow]) chrome://browser/content/browser.xul child window: ([object XrayWrapper [object Window]]) about:blank docshell visible: true
TEST-INFO | already focused
TEST-INFO | maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XrayWrapper [object Window]]) about:blank desired window: ([object ChromeWindow]) chrome://browser/content/browser.xul child window: ([object XrayWrapper [object Window]]) about:blank docshell visible: true
TEST-START | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_multiwindow_search.js

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: screenshot

Comment 2

[Legacy TBPL/Treeherder Robot]

mbrubeck%mozilla.com
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1308072230.1308073490.11869.gz
Rev3 Fedora 12 mozilla-inbound opt test mochitest-other on 2011/06/14 10:23:50

s: talos-r3-fed-034
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Test timed out

Comment 3

[Legacy TBPL/Treeherder Robot]

jwein
http://tinderbox.mozilla.org/showlog.cgi?log=Try/1308181911.1308183386.10025.gz
Rev3 Fedora 12 try opt test mochitest-other on 2011/06/15 16:51:51

s: talos-r3-fed-029
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Test timed out

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v1

I am not sure what is causing the strange overlay box in the screenshot in comment 1.  I've modified the code to open the test in a new window and refactored the code.

Tim: any suggestions?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #4)
> I am not sure what is causing the strange overlay box in the screenshot in
> comment 1.  I've modified the code to open the test in a new window and
> refactored the code.

I guess that's just some artifacts from Xvfb on the linux build machines.

> Tim: any suggestions?

1.) We should refactor the test to not use setTimeout() with an interval bigger than zero. Otherwise we will be marked as "flaky" in the near future (bug 649012).

2.) The test times out in the waitForSwitch() loop, so we should find out why it fails there. I have no idea, yet.

3.) Why do we even have such a complicated approach here? showTabView() seems reliable and would be much easier (I don't know the history of the test and why we do that...)

Comment 6

[Raymond Lee [:raymondlee]]

(In reply to comment #5)
> (In reply to comment #4)
> > I am not sure what is causing the strange overlay box in the screenshot in
> > comment 1.  I've modified the code to open the test in a new window and
> > refactored the code.
> 
> I guess that's just some artifacts from Xvfb on the linux build machines.
> 
> > Tim: any suggestions?
> 
> 1.) We should refactor the test to not use setTimeout() with an interval
> bigger than zero. Otherwise we will be marked as "flaky" in the near future
> (bug 649012).
> 
> 2.) The test times out in the waitForSwitch() loop, so we should find out
> why it fails there. I have no idea, yet.
> 
> 3.) Why do we even have such a complicated approach here? showTabView()
> seems reliable and would be much easier (I don't know the history of the
> test and why we do that...)

See bug 594909 comment 56 was the bug which added this approach.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #6)
> > 3.) Why do we even have such a complicated approach here? showTabView()
> > seems reliable and would be much easier (I don't know the history of the
> > test and why we do that...)
> 
> See bug 594909 comment 56 was the bug which added this approach.

So, let's just add an event listener like this:

>  let onSelect = function (event) {
>    if (deck != event.target)
>      return;
>
>    let iframe = document.getElementById("tab-view");
>    if (!iframe || deck.selectedPanel != iframe)
>      return;
>
>    deck.removeEventListener("select", onSelect, false);
>
>    // do stuff here...
>  };
>
>  let deck = document.getElementById("tab-view-deck");
>  deck.addEventListener("select", onSelect, false);

This should of course be added in function "test" before TabView.toggle() is called. So we can get rid of all those flaky timeouts :)

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v2

Push to try and wait for results http://tbpl.mozilla.org/?tree=Try&rev=a53cd7620dbf

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 540435 [details] [diff] [review]
v2

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

Looks good!

::: browser/base/content/test/tabview/browser_tabview_launch.js
@@ +16,5 @@
> +      if (deck != event.target)
> +        return;
> +
> +      let iframe = win.document.getElementById("tab-view");
> +      if (!iframe || deck.selectedPanel != iframe)

nit: I know that line is from me but we don't need the "!iframe" check.

@@ +22,5 @@
> +
> +      deck.removeEventListener("select", onSelect, true);
> +
> +      whenTabViewIsShown(function() {
> +        waitForFocus(function() {

Please just wrap the EventUtils.synthesizeKey() call in waitForFocus().

@@ +35,5 @@
> +                finish();
> +              }, win);
> +              EventUtils.synthesizeKey("E", { accelKey: true, shiftKey: true }, win);
> +            }, win);
> +            win.document.getElementById("menu_tabview").doCommand();

Can't we just use this to hide the tabview and show it again with ctrl+shift+e? This would save some lines and make that test a bit cleaner.

@@ +48,3 @@
>  
>    registerCleanupFunction(function () {
> +    if (newWin)

nit: this check is unnecessary.

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3

(In reply to comment #9)
> Comment on attachment 540435 [details] [diff] [review] [review]
> v2
> 
> Review of attachment 540435 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: browser/base/content/test/tabview/browser_tabview_launch.js
> @@ +16,5 @@
> > +      if (deck != event.target)
> > +        return;
> > +
> > +      let iframe = win.document.getElementById("tab-view");
> > +      if (!iframe || deck.selectedPanel != iframe)
> 
> nit: I know that line is from me but we don't need the "!iframe" check.

Removed

> 
> @@ +22,5 @@
> > +
> > +      deck.removeEventListener("select", onSelect, true);
> > +
> > +      whenTabViewIsShown(function() {
> > +        waitForFocus(function() {
> 
> Please just wrap the EventUtils.synthesizeKey() call in waitForFocus().
> 

I've removed it but it turned out that the test would time out.  I have replaced it with executeSoon() and it works again.

> @@ +35,5 @@
> > +                finish();
> > +              }, win);
> > +              EventUtils.synthesizeKey("E", { accelKey: true, shiftKey: true }, win);
> > +            }, win);
> > +            win.document.getElementById("menu_tabview").doCommand();
> 
> Can't we just use this to hide the tabview and show it again with
> ctrl+shift+e? This would save some lines and make that test a bit cleaner.
> 

Changed to ctrl+shift+e

> @@ +48,3 @@
> >  
> >    registerCleanupFunction(function () {
> > +    if (newWin)
> 
> nit: this check is unnecessary.

Removed

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 540444 [details] [diff] [review]
v3

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

* Why the onSelect thing, instead of just whenTabViewIsShown?
* The old patch tested both the key combo and the menu item, while the new patch tests only the key combo; should test the menu item as well
* For both the key combo and menu item, test both launching into and leaving from tabView (the original tested only launching into, but since we have to exit anyway, might as well exit in the fashion you entered... you're already doing this with the key combo; might as well do the same for menu item)

Comment 12

[Raymond Lee [:raymondlee]]

(In reply to comment #11)
> Comment on attachment 540444 [details] [diff] [review] [review]
> v3
> 
> Review of attachment 540444 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> * Why the onSelect thing, instead of just whenTabViewIsShown?

We used it to replace the waitForSwitch() which was added in bug 594909 comment 56.  There was another Intermittent.  I guess we can try without the onSelect thing and use the whenTabViewIsShown instead.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #11)
> * Why the onSelect thing, instead of just whenTabViewIsShown?

I thought that was the whole crux of the patch. Testing whether the tabview is _really_ shown (the iframe is the selectedPanel), although we can be quite confident about that :)

> * The old patch tested both the key combo and the menu item, while the new
> patch tests only the key combo; should test the menu item as well

Yeah, I actually meant that both should be tested. Sorry, if I was a bit unclear about that.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Adding intermittent test failures to new meta bug.

(bugspam)

Comment 15

[Raymond Lee [:raymondlee]]

Attachment: v4

(In reply to comment #13)
> (In reply to comment #11)
> > * Why the onSelect thing, instead of just whenTabViewIsShown?
> 
> I thought that was the whole crux of the patch. Testing whether the tabview
> is _really_ shown (the iframe is the selectedPanel), although we can be
> quite confident about that :)

Yes, I am keeping this part.

> 
> > * The old patch tested both the key combo and the menu item, while the new
> > patch tests only the key combo; should test the menu item as well
> 
> Yeah, I actually meant that both should be tested. Sorry, if I was a bit
> unclear about that.

I put the menu item test back in and simplified the part.

Comment 16

[Ian Gilman [:iangilman]]

Comment on attachment 540978 [details] [diff] [review]
v4

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

Now you're exiting with the command and entering with the key. My suggestion was to do two rounds: enter and exit with the command, and enter and exit with the key. That way you get even test coverage. Sorry if I was unclear about that.

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: v5

(In reply to comment #16)
> Comment on attachment 540978 [details] [diff] [review] [review]
> v4
> 
> Review of attachment 540978 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Now you're exiting with the command and entering with the key. My suggestion
> was to do two rounds: enter and exit with the command, and enter and exit
> with the key. That way you get even test coverage. Sorry if I was unclear
> about that.

Done.

Pushed to try and waiting for results
http://tbpl.mozilla.org/?tree=Try&rev=d52569b535fd

Comment 18

[Ian Gilman [:iangilman]]

Comment on attachment 541492 [details] [diff] [review]
v5

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

Beautiful, thank you!

Comment 19

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/33fa6743bde1

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/33fa6743bde1

Comment 22

[Legacy TBPL/Treeherder Robot]

mbrubeck%mozilla.com
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Beta/1311638979.1311640384.2475.gz
Rev3 Fedora 12x64 mozilla-beta opt test mochitest-other on 2011/07/25 17:09:39

s: talos-r3-fed64-005
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Test timed out

Comment 23

[Virgil Dicu [:virgil] [QA]]

Verified fixed based on results in:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1314760632.1314761843.20879.gz