589659 - [SeaMonkey 2.1, mochitest-browser-chrome] Lots of mozapps/extensions/test/ failures

Status: VERIFIED FIXED

(SeaMonkey :: Testing Infrastructure, defect)

Description

[Robert Kaiser]

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)]

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]

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]

(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

[[e-mail]]

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]

Attachment: 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)]

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

[[e-mail]]

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]

(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)]

(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]

Attachment: 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]

(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)]

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

[[e-mail]]

(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]

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)]

(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]

(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)]

(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]

(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

[[e-mail]]

(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

[[e-mail]]

(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)]

(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)]

http://hg.mozilla.org/comm-central/rev/751fccd282f6

Thanks!

Comment 23

[Justin Wood (:Callek)]

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)]

(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)]

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]

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)]

(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]

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]

Attachment: 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]

Attachment: 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]

(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]

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]

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)]

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)]

Attachment: 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

[[e-mail]]

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]

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]

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]

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

[[e-mail]]

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

[[e-mail]]

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

[[e-mail]]

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)]

Attachment: 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]

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]

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)]

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)]

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)]

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.