Last Comment Bug 663838 - Intermittent TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Test timed out
: Intermittent TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/brows...
Status: VERIFIED FIXED
, [inbound]
: intermittent-failure
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: x86 Linux
: -- normal
: Firefox 7
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 438871 660175 665844
  Show dependency treegraph
 
Reported: 2011-06-13 08:40 PDT by Matt Brubeck (:mbrubeck)
Modified: 2016-04-12 14:00 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot (427.85 KB, image/png)
2011-06-13 08:45 PDT, Matt Brubeck (:mbrubeck)
no flags Details
v1 (4.29 KB, patch)
2011-06-17 00:30 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Review
v2 (4.33 KB, patch)
2011-06-20 04:59 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Review
v3 (4.31 KB, patch)
2011-06-20 06:05 PDT, Raymond Lee [:raymondlee]
ian: review-
Details | Diff | Review
v4 (4.38 KB, patch)
2011-06-22 00:05 PDT, Raymond Lee [:raymondlee]
ian: review-
Details | Diff | Review
v5 (4.41 KB, patch)
2011-06-23 14:01 PDT, Raymond Lee [:raymondlee]
ian: review+
Details | Diff | Review
Patch for checkin (4.76 KB, patch)
2011-06-25 08:39 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Review

Description Matt Brubeck (:mbrubeck) 2011-06-13 08:40:59 PDT
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) 2011-06-13 08:45:07 PDT
Created attachment 538914 [details]
screenshot
Comment 2 Treeherder Robot 2011-06-14 10:51:47 PDT
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 Treeherder Robot 2011-06-16 11:54:08 PDT
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] 2011-06-17 00:30:52 PDT
Created attachment 540004 [details] [diff] [review]
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] 2011-06-17 05:49:45 PDT
(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] 2011-06-17 06:25:51 PDT
(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] 2011-06-17 06:43:05 PDT
(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] 2011-06-20 04:59:49 PDT
Created attachment 540435 [details] [diff] [review]
v2

Push to try and wait for results http://tbpl.mozilla.org/?tree=Try&rev=a53cd7620dbf
Comment 9 Tim Taubert [:ttaubert] 2011-06-20 05:12:25 PDT
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] 2011-06-20 06:05:51 PDT
Created attachment 540444 [details] [diff] [review]
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] 2011-06-20 09:55:40 PDT
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] 2011-06-20 10:49:59 PDT
(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] 2011-06-20 12:04:59 PDT
(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] 2011-06-21 00:54:56 PDT
Adding intermittent test failures to new meta bug.

(bugspam)
Comment 15 Raymond Lee [:raymondlee] 2011-06-22 00:05:50 PDT
Created attachment 540978 [details] [diff] [review]
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] 2011-06-22 10:23:39 PDT
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] 2011-06-23 14:01:28 PDT
Created attachment 541492 [details] [diff] [review]
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] 2011-06-24 10:00:17 PDT
Comment on attachment 541492 [details] [diff] [review]
v5

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

Beautiful, thank you!
Comment 19 Raymond Lee [:raymondlee] 2011-06-25 08:39:48 PDT
Created attachment 541934 [details] [diff] [review]
Patch for checkin
Comment 21 Marco Bonardo [::mak] 2011-06-28 02:27:04 PDT
http://hg.mozilla.org/mozilla-central/rev/33fa6743bde1
Comment 22 Treeherder Robot 2011-07-26 09:02:22 PDT
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] 2011-08-31 02:47:54 PDT
Verified fixed based on results in:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1314760632.1314761843.20879.gz

Note You need to log in before you can comment on or make changes to this bug.