Last Comment Bug 589659 - [SeaMonkey 2.1, mochitest-browser-chrome] Lots of mozapps/extensions/test/ failures
: [SeaMonkey 2.1, mochitest-browser-chrome] Lots of mozapps/extensions/test/ fa...
Status: VERIFIED FIXED
[sm-perma][cc-orange]
: intermittent-failure
Product: SeaMonkey
Classification: Client Software
Component: Testing Infrastructure (show other bugs)
: Trunk
: All All
: -- major (vote)
: seamonkey2.1b1
Assigned To: Bruno 'Aqualon' Escherl
:
:
Mentors:
Depends on: SMAddonMgr
Blocks: SmTestFail 594342
  Show dependency treegraph
 
Reported: 2010-08-22 18:32 PDT by Robert Kaiser
Modified: 2012-11-26 02:55 PST (History)
4 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Minimal fix (2.71 KB, patch)
2010-09-01 10:06 PDT, Bruno 'Aqualon' Escherl
bugspam.Callek: feedback-
Details | Diff | Splinter Review
patch v2 [Checkin: Comment 22] (3.18 KB, patch)
2010-09-02 09:26 PDT, Bruno 'Aqualon' Escherl
bugspam.Callek: review+
neil: superreview+
bugspam.Callek: feedback+
Details | Diff | Splinter Review
test, v1: simplified case of extensions tests (2.59 KB, patch)
2010-09-05 06:12 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
test, v2: even simpler, and showing the problem better [Checkin: Comment 38] (1.63 KB, patch)
2010-09-05 06:29 PDT, Robert Kaiser
bugspam.Callek: review+
Details | Diff | Splinter Review
Revert to the earlier check. (638 bytes, patch)
2010-09-05 21:22 PDT, Justin Wood (:Callek)
neil: review-
kairo: feedback-
Details | Diff | Splinter Review
just drop the busy check on tab [Checkin: Comment 47] (809 bytes, patch)
2010-09-07 08:03 PDT, Justin Wood (:Callek)
neil: review+
bugzilla: feedback+
kairo: feedback+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-08-22 18:32:20 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1282522863.1282525084.30201.gz

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_backgroundupdate_menuitem.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_backgroundupdate_menuitem.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_backgroundupdate_menuitem.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557943.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557943.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557943.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562899.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562899.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562899.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562992.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562992.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562992.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug567127.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug567127.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug567127.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug567137.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug567137.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug567137.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug572561.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug572561.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug572561.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug577990.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug577990.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug577990.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_dragdrop.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_dragdrop.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_dragdrop.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_manualupdates.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_manualupdates.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_manualupdates.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_recentupdates.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_recentupdates.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_recentupdates.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_searching.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_searching.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_searching.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_sorting.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_sorting.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_sorting.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_uninstalling.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_uninstalling.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_uninstalling.js | Found a tab after previous test timed out: about:blank


We really should get those failures out of the test runs, it's hard to see something next to them.
Comment 1 Serge Gautherie (:sgautherie) 2010-08-22 20:19:40 PDT
Do all these tests actually time out and leave tabs over on their own,
or is all this caused by some of the previous test failures (in the log) ?

Fwiw, this was already happening on
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1280778070.1280780147.27410.gz
Linux comm-central-trunk debug test mochitest-other on 2010/08/02 12:41:10
(and maybe earlier... if someone wants to check further)
Comment 2 Bruno 'Aqualon' Escherl 2010-09-01 03:55:51 PDT
I've run the first test alone and got the following error in the error console:

Error: aBrowser.contentWindow is undefined
Source File: chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/head.js
Line: 163
Comment 3 Robert Kaiser 2010-09-01 05:37:44 PDT
(In reply to comment #2)
> Error: aBrowser.contentWindow is undefined

Hmm, I'd think every legitimate browser should have .contentWindow defined... Are we sure that aBrowser is what it should be?
Comment 4 neil@parkwaycc.co.uk 2010-09-01 07:56:33 PDT
No, it's not. The problem is that switchToTabHavingURI tries to use the return value from openUILink, which is a window, not a browser. It needs to do what Firefox does and just get the selected browser directly from the tabbrowser.

Although I'm not sure what's supposed to happen on the Mac with no windows open, or other cases when there is no browser for whatever reason.
Comment 5 Bruno 'Aqualon' Escherl 2010-09-01 10:06:40 PDT
Created attachment 471154 [details] [diff] [review]
Minimal fix

This is a minimal fix for the browser_backgroundupdate_menuitem.js test and most following tests in toolkit/mozapps/extensions/test/browser. I don't know if copying the further parts from bug 555767 could be useful for us. The next failing test with this patch is browser_searching.js

Neil: could you give me some feedback if that is enough to do here and set the review-flags accordingly?
Comment 6 Justin Wood (:Callek) 2010-09-01 10:46:03 PDT
Comment on attachment 471154 [details] [diff] [review]
Minimal fix

>   // No opened tab has that url.
>   if (aOpenNew) {
>-    let browser = openUILinkIn(aURI.spec, "tabfocused");
>+    openUILinkIn(aURI.spec, "tabfocused");
>     if (aCallback) {
>+      let browser = gBrowser.selectedBrowser;
>       browser.addEventListener("pageshow", function(event) {

Untested, but this is wrong.  Try opening the addonManager from MailNews/Chatzilla/DOMi etc. If openUILinkIn actually returns a window, you need to use that to get its browser instead, sure. But we can't just pretend that the _current_ window is always a browser window.
Comment 7 neil@parkwaycc.co.uk 2010-09-01 12:55:58 PDT
Comment on attachment 471154 [details] [diff] [review]
Minimal fix

>+ * Determines if a tab is "empty", usually used in the context of determining
>+ * if it's ok to close the tab.
Well, not close, at least, not yet ;-)

>+         !aTab.hasAttribute("busy");
Bah, Gavin must have forgotten about browser.webProgress.isLoadingDocument (given which the function could be rewritten isBrowserEmpty(aBrowser) and could in theory even be passed a tabbrowser to check the selected browser).

>+  // reuse the tab if it's empty
>+  if (isTabEmpty(gBrowser.selectedTab))
Need to use w.getBrowser() since w might not be the current window.

>+    openUILinkIn(aURI.spec, "tabfocused");
>     if (aCallback) {
>+      let browser = gBrowser.selectedBrowser;
As Callek pointed out, this doesn't work if we're opening a new window, but it occurs to me that the pageshow event loop will probably still work; you just need to save the window returned by openUILinkIn and pass its w.getBrowser().selectedBrowser to the callback function.
Comment 8 Robert Kaiser 2010-09-01 16:28:40 PDT
(In reply to comment #7)
> (given which the function could be rewritten isBrowserEmpty(aBrowser) and could
> in theory even be passed a tabbrowser to check the selected browser).

In theory, yes, but I'd prefer if we could keep the function name and options for add-on and code porting compatibility.
Ideally, it really should have been made a tabbrowser method, IMHO, but we should follow FF here, I guess.
Comment 9 Serge Gautherie (:sgautherie) 2010-09-02 00:55:59 PDT
(In reply to comment #8)
> we should follow FF here, I guess.

Agreed. Or could it be possible to update Firefox first?
Comment 10 Bruno 'Aqualon' Escherl 2010-09-02 09:26:06 PDT
Created attachment 471527 [details] [diff] [review]
patch v2
[Checkin: Comment 22]

Second version of the patch, incorporating the feedback comments.

The main problem is, that isTabEmpty() in navigator.js is not available if we have an open browser window and call openUILinkIn from a non-browser window (e.g. error console or mailnews). I solved this by creating an isBrowserEmpty() function in utilityOverlay.js and calling that function from isTabEmpty() in navigator.js to preserve the compatibility KaiRo mentioned in #c8.
Comment 11 Robert Kaiser 2010-09-02 11:08:52 PDT
(In reply to comment #10)
> The main problem is, that isTabEmpty() in navigator.js is not available if we
> have an open browser window and call openUILinkIn from a non-browser window
> (e.g. error console or mailnews). I solved this by creating an isBrowserEmpty()
> function in utilityOverlay.js and calling that function from isTabEmpty() in
> navigator.js to preserve the compatibility KaiRo mentioned in #c8.

I would not have bothered to go this far and instead would have put the isTabEmpty() function in utilityOverlay and left out isBrowserEmpty() completely. But then, let's leave that decision up to the reviewers. ;-)
Comment 12 Justin Wood (:Callek) 2010-09-02 11:16:32 PDT
Comment on attachment 471527 [details] [diff] [review]
patch v2
[Checkin: Comment 22]

This looks good to me, I'm unsure on the isBrowserEmpty impl though, and I won't have time until tomorrow night to investigate that.
Comment 13 neil@parkwaycc.co.uk 2010-09-02 13:15:39 PDT
(In reply to comment #11)
> I would not have bothered to go this far and instead would have put the
> isTabEmpty() function in utilityOverlay and left out isBrowserEmpty()
> completely. But then, let's leave that decision up to the reviewers. ;-)
Avoiding isTabEmpty saves us from having to get hold of the tab unnecessarily.
Comment 14 Ian Neal 2010-09-03 16:00:50 PDT
Comment on attachment 471527 [details] [diff] [review]
patch v2
[Checkin: Comment 22]

>diff --git a/suite/common/utilityOverlay.js b/suite/common/utilityOverlay.js
>@@ -1237,16 +1237,20 @@ function openUILinkIn(url, where, aAllow
> 
>   if (!w || where == "window") {
>     return window.openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", url,
>                              null, null, flags);
>   }
> 
>   var loadInBackground = getBoolPref("browser.tabs.loadInBackground", false);
> 
>+  // reuse the browser if its current tab is empty
>+  if (isBrowserEmpty(w.getBrowser()))
>+    where = "current";
>+
Are we sure that we always want to load the page into the current window and focus it?


>@@ -1370,31 +1374,39 @@ function switchToTabHavingURI(aURI, aOpe
>     if (browserWin.closed || browserWin == window)
>       continue;
>     if (switchIfURIInWindow(browserWin))
>       return true;
>   }
> 
>   // No opened tab has that url.
>   if (aOpenNew) {
>-    let browser = openUILinkIn(aURI.spec, "tabfocused");
>+    let browserWin = openUILinkIn(aURI.spec, "tabfocused");
>     if (aCallback) {
>-      browser.addEventListener("pageshow", function(event) {
>+      browserWin.addEventListener("pageshow", function(event) {
>         if (event.target.location.href != aURI.spec)
>           return;
>-        browser.removeEventListener("pageshow", arguments.callee, true);
>-        aCallback(browser);
>+        browserWin.removeEventListener("pageshow", arguments.callee, true);
>+        aCallback(browserWin.getBrowser().selectedBrowser);
Show we be setting/removing the event listener on browserWin or browserWin.getBrowser().selectedBrowser?
Comment 15 Justin Wood (:Callek) 2010-09-03 18:55:36 PDT
(In reply to comment #14)
> Are we sure that we always want to load the page into the current window and
> focus it?

Yes we are

> >@@ -1370,31 +1374,39 @@ function switchToTabHavingURI(aURI, aOpe
> Show we be setting/removing the event listener on browserWin or
> browserWin.getBrowser().selectedBrowser?

I thought about that, but I'm not certain.. Neil?
Comment 16 Ian Neal 2010-09-04 02:09:32 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > Are we sure that we always want to load the page into the current window and
> > focus it?
> 
> Yes we are
Really? This isn't just for when add-on manager is opened, it is for any time we ask a link to be opened when where <> "window" or there isn't a browser window open. It would ignore loadinbackground pref and/or any modifiers.
Comment 17 Justin Wood (:Callek) 2010-09-04 04:35:05 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Are we sure that we always want to load the page into the current window and
> > > focus it?
> > 
> > Yes we are
> Really? This isn't just for when add-on manager is opened, it is for any time
> we ask a link to be opened when where <> "window" or there isn't a browser
> window open. It would ignore loadinbackground pref and/or any modifiers.

Yes, really. Remember this is ONLY if there is a focused/current tab in the current window that is empty (such as about:blank). Otherwise all other prefs/norms are enforced.
Comment 18 Ian Neal 2010-09-04 05:41:01 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > > Are we sure that we always want to load the page into the current window and
> > > > focus it?
> > > 
> > > Yes we are
> > Really? This isn't just for when add-on manager is opened, it is for any time
> > we ask a link to be opened when where <> "window" or there isn't a browser
> > window open. It would ignore loadinbackground pref and/or any modifiers.
> 
> Yes, really. Remember this is ONLY if there is a focused/current tab in the
> current window that is empty (such as about:blank). Otherwise all other
> prefs/norms are enforced.

I'm probably missing where it checks to see if it is the focused/current tab in the current window - does getBrowser() get the current tab or getBrowser().selectedBrowser?

Is it any more efficient to check if where == "current" before calling isBrowserEmpty?
Comment 19 neil@parkwaycc.co.uk 2010-09-04 05:59:55 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > Are we sure that we always want to load the page into the current window and
> > focus it?
> Yes we are
Yes. Also, performing the isBlank check on getBrowser() works although maybe not quite as fast as performing it directly on getBrowser().selectedBrowser

> > >@@ -1370,31 +1374,39 @@ function switchToTabHavingURI(aURI, aOpe
> > Show we be setting/removing the event listener on browserWin or
> > browserWin.getBrowser().selectedBrowser?
> I thought about that, but I'm not certain.. Neil?
We have to do it on browserWin because in the new window case it hasn't loaded its browser yet.
Comment 20 neil@parkwaycc.co.uk 2010-09-04 14:38:46 PDT
(In reply to comment #18)
> Is it any more efficient to check if where == "current" before calling
> isBrowserEmpty?
Perhaps only if you were to completely rewrite it like this:
if (where == "current" || isBrowserEmpty(w.getBrowser()) {
  w.loadURI(url, aPostData, flags);
  w.content.focus();
} else {
  var browser = w.getBrowser();
  var tab = browser.addTab(...);
  if (where == "tabfocused" ||
      where == "tabshifted" ==
               getBoolPref("browser.tabs.loadInBackground", false)) {
    browser.selectedTab = tab;
    w.content.focus();
  }
}
return w;
Comment 21 Justin Wood (:Callek) 2010-09-04 15:08:16 PDT
(In reply to comment #20)
> (In reply to comment #18)
> > Is it any more efficient to check if where == "current" before calling
> > isBrowserEmpty?
PLEASE NO, that is horribly messy.

The reason we replace on a blank tab is that ctrl+t (or ctrl+w) is almost always followed directly by a user action for something he wants to do in that tab. Anything that invokes this will be the result of a user action, so if we have an empty tab as current, we want to replace that with whatever the function is trying to open.
Comment 22 Justin Wood (:Callek) 2010-09-04 20:32:16 PDT
http://hg.mozilla.org/comm-central/rev/751fccd282f6

Thanks!
Comment 23 Justin Wood (:Callek) 2010-09-04 22:06:05 PDT
For the record on first glance, on OSX this regressed stuff...

Total/FAIL/TODO
From: mochitest-browser-chrome: 2285/65/8
To:   mochitest-browser-chrome: 4371/80/8

But remember to look at the total number of tests run, so since we are running more tests, we are failing more (unexpected tab it seems).

Not sure where that is coming from yet.
Comment 24 Serge Gautherie (:sgautherie) 2010-09-05 00:24:44 PDT
(In reply to comment #23)

> For the record on first glance, on OSX this regressed stuff...

Confirmed: Linux too.
(Windows hasn't run yet)

> But remember to look at the total number of tests run, so since we are running
> more tests, we are failing more (unexpected tab it seems).
> 
> Not sure where that is coming from yet.

Afaict, the tests are no longer timing out,
nor reporting "Found a tab after previous test timed out: about:addons
" :-)

But they still report "Found an unexpected tab at the end of test run: about:blank",
and browser_searching.js is now reporting its check failures...

Reopening to fix the remaining/revealed issues.
Comment 25 Justin Wood (:Callek) 2010-09-05 05:06:51 PDT
The issue I think is related to an earlier test....

Also, no need to convolute this bug by doing multiple issues in it. Bugs are cheap.
Comment 26 Robert Kaiser 2010-09-05 05:24:40 PDT
Unfortunately, the reopen is correct, as the patch here doesn't work as expected. If I open a new window, set it to about:blank and then open add-ons manager from the menu, I get a second tab instead of it opening in the current one.

We probably should even add a test for specifically the functionality we've been adding here.
Comment 27 Justin Wood (:Callek) 2010-09-05 05:44:32 PDT
(In reply to comment #26)
> Unfortunately, the reopen is correct, as the patch here doesn't work as
> expected. If I open a new window, set it to about:blank and then open add-ons
> manager from the menu, I get a second tab instead of it opening in the current
> one.
> 
> We probably should even add a test for specifically the functionality we've
> been adding here.

err that means neil was wrong on irc about the isBrowserEmpty impl... will have to look into that. :/
Comment 28 Robert Kaiser 2010-09-05 06:09:48 PDT
Oh, actually, the behavior is opening a new tab when only one is open seem correct, but there is a different failure, and I have a simplified test to show it, will attach in a minute (we should open that one to our suite).
Comment 29 Robert Kaiser 2010-09-05 06:12:40 PDT
Created attachment 472257 [details] [diff] [review]
test, v1: simplified case of extensions tests

This is a simplified version of the extensions tests and shows the same problem:

mochitest-browser-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_isempty.js | We're back to the same number of tabs - Got 2, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_isempty.js | Found an unexpected tab at the end of test run: about:blank
Comment 30 Robert Kaiser 2010-09-05 06:29:28 PDT
Created attachment 472259 [details] [diff] [review]
test, v2: even simpler, and showing the problem better
[Checkin: Comment 38]

Actually, the test can be made even a lot simpler - and this shows the problem even better by checking isTabEmpty() as well:

mochitest-browser-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_isempty.js | Added tab is empty - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_isempty.js | We're still at the same number of tabs - Got 2, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_isempty.js | Found an unexpected tab at the end of test run: about:blank
Comment 31 Robert Kaiser 2010-09-05 14:05:54 PDT
(In reply to comment #7)
> >+         !aTab.hasAttribute("busy");
> Bah, Gavin must have forgotten about browser.webProgress.isLoadingDocument

Actually, seems like this is a clever trick as a newly added tab from .addTab() doesn't yet have that attribute set but browser.webProgress.isLoadingDocument is true.
And that is why we fail all those tests now, from what I can see.
Comment 32 Robert Kaiser 2010-09-05 14:17:03 PDT
After commenting out the aBrowser.webProgress.isLoadingDocument line, my test passes and the tabs handling errors on the extensions tests are gone - a number of others are left, but those should be dealt with in a different bug.
Comment 33 Robert Kaiser 2010-09-05 15:03:06 PDT
Neil proposed to move the b.stop() in tabbrowser's addTab() before the blank check it is in, and while that makes this case succeed, it makes other testes, like the alltabslistener one for example, fail, so I guess it's not the way to go either.
Comment 34 Justin Wood (:Callek) 2010-09-05 21:18:37 PDT
Comment on attachment 472259 [details] [diff] [review]
test, v2: even simpler, and showing the problem better
[Checkin: Comment 38]

fwiw I'm ok with this test landing, before its fix.
Comment 35 Justin Wood (:Callek) 2010-09-05 21:22:11 PDT
Created attachment 472324 [details] [diff] [review]
Revert to the earlier check.

Untested so far. But from code inspection, this is the only piece of this that is bad. And this should fix it (If our tabbrowser is missing some spots where it should set "busy" I think we can/should do that in a followup)
Comment 36 neil@parkwaycc.co.uk 2010-09-06 01:08:08 PDT
Comment on attachment 472324 [details] [diff] [review]
Revert to the earlier check.

But you could add this to the isTabEmpty function.
Comment 37 Robert Kaiser 2010-09-06 05:02:59 PDT
Comment on attachment 472324 [details] [diff] [review]
Revert to the earlier check.

aTab is not defined here.

Neil, would you be OK with going back to only doing isTabEmpty (we'll need to to that in utilityOverlay as the case we need is openUILinkIn)?

Alternatively, is there any reason we really need the "busy" check there? Can we ever run into all the other conditions when the tab is loading something else than about:blank?
Comment 38 Robert Kaiser 2010-09-06 06:34:48 PDT
Comment on attachment 472259 [details] [diff] [review]
test, v2: even simpler, and showing the problem better
[Checkin: Comment 38]

Pushed this new test as http://hg.mozilla.org/comm-central/rev/6c3e40bf5887
Comment 39 Bruno 'Aqualon' Escherl 2010-09-06 06:58:38 PDT
Perhaps we could also use something like !(aBrowser.docshell.busyFlags & Components.interfaces.nsIDocShell.BUSY_FLAGS_NONE)? But that's something Neil should decide.
Comment 40 neil@parkwaycc.co.uk 2010-09-06 16:30:05 PDT
Ah, so the problem is that open_manager depends on switchToTabHavingURI using the tab that it just added? Why does it bother adding a tab in that case?
Comment 41 neil@parkwaycc.co.uk 2010-09-06 16:34:02 PDT
Hmm, so when the addons manager switched to reusing the current tab, this made the Firefox tests fail because the current tab was also blank, so they had to open a new tab so that the addons manager would reuse that tab instead.
Comment 42 neil@parkwaycc.co.uk 2010-09-06 16:40:53 PDT
And the use of getAttribute("busy") dates all the way back to bug 343895 :-(

Maybe we can persuade toolkit to take a test "fix" of adding a .stop()?

I'm vaguely interested in how many tests rely on "busy".
Comment 43 Justin Wood (:Callek) 2010-09-07 08:03:02 PDT
Created attachment 472627 [details] [diff] [review]
just drop the busy check on tab
[Checkin: Comment 47]

Try this as a quick and dirty way, (avoiding .stop() for the moment) so we can try and get tests passing sooner.
Comment 44 Robert Kaiser 2010-09-07 09:20:46 PDT
Comment on attachment 472627 [details] [diff] [review]
just drop the busy check on tab
[Checkin: Comment 47]

f+ for the fact that I don't know why we need the check at all - but it would be of interest to me and probably us if/how we can come into the state of .currentURI being about:blank when we are not an empty tab. If there is no case where this happens, the check removed here is really unnecessary and we should backport this to FF.
Comment 45 Bruno 'Aqualon' Escherl 2010-09-07 10:29:50 PDT
Comment on attachment 472627 [details] [diff] [review]
just drop the busy check on tab
[Checkin: Comment 47]

I can't think either of a situation where this could be a problem at the moment, so just try it.
Comment 46 Justin Wood (:Callek) 2010-09-07 19:13:20 PDT
This seems to have fixed many many tests, as we suspected. Neil followup bug if needed here, I think... no need to keep this open indefinitely
Comment 47 Serge Gautherie (:sgautherie) 2010-09-08 05:35:59 PDT
Comment on attachment 472627 [details] [diff] [review]
just drop the busy check on tab
[Checkin: Comment 47]

http://hg.mozilla.org/comm-central/rev/1d94b3afbf36
Comment 48 Serge Gautherie (:sgautherie) 2010-09-08 05:59:44 PDT
Linux and MacOSX failures dropped from "85" to "45",
Windows ones from "95" to "75".

V.Fixed

I filed bug 594342 on the remaining failures.

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