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

VERIFIED FIXED in seamonkey2.1b1

Status

SeaMonkey
Testing Infrastructure
--
major
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: Robert Kaiser, Assigned: Bruno 'Aqualon' Escherl)

Tracking

(Blocks: 1 bug, {intermittent-failure})

Trunk
seamonkey2.1b1
intermittent-failure
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sm-perma][cc-orange])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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.
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)
(Assignee)

Comment 2

7 years ago
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
(Reporter)

Comment 3

7 years ago
(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

7 years ago
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.
(Assignee)

Comment 5

7 years ago
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?
Assignee: nobody → aqualon
Status: NEW → ASSIGNED
Attachment #471154 - Flags: feedback?(neil)
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.
Attachment #471154 - Flags: feedback?(neil) → feedback-

Comment 7

7 years ago
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.
(Reporter)

Comment 8

7 years ago
(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.
(In reply to comment #8)
> we should follow FF here, I guess.

Agreed. Or could it be possible to update Firefox first?
(Assignee)

Comment 10

7 years ago
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.
Attachment #471154 - Attachment is obsolete: true
Attachment #471527 - Flags: superreview?(neil)
Attachment #471527 - Flags: review?(bugspam.Callek)
(Reporter)

Comment 11

7 years ago
(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 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.
Attachment #471527 - Flags: feedback+

Updated

7 years ago
Attachment #471527 - Flags: superreview?(neil) → superreview+
(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

7 years ago
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?
(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

7 years ago
(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.
(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

7 years ago
(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?
(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.
(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;
(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.

Updated

7 years ago
Attachment #471527 - Flags: review?(bugspam.Callek) → review+
http://hg.mozilla.org/comm-central/rev/751fccd282f6

Thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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.
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #471527 - Attachment description: patch v2 → patch v2 [Checkin: Comment 22]
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.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 26

7 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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. :/
(Reporter)

Comment 28

7 years ago
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).
(Reporter)

Comment 29

7 years ago
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
Attachment #472257 - Flags: review?(bugspam.Callek)
(Reporter)

Comment 30

7 years ago
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
Attachment #472257 - Attachment is obsolete: true
Attachment #472259 - Flags: review?(bugspam.Callek)
Attachment #472257 - Flags: review?(bugspam.Callek)
(Reporter)

Comment 31

7 years ago
(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.
(Reporter)

Comment 32

7 years ago
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.
(Reporter)

Comment 33

7 years ago
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 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.
Attachment #472259 - Flags: review?(bugspam.Callek) → review+
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)
Attachment #472324 - Flags: review?(neil)
Attachment #472324 - Flags: feedback?(kairo)

Updated

7 years ago
Attachment #472324 - Flags: feedback?(aqualon)
Comment on attachment 472324 [details] [diff] [review]
Revert to the earlier check.

But you could add this to the isTabEmpty function.
Attachment #472324 - Flags: review?(neil) → review-
(Reporter)

Comment 37

7 years ago
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?
Attachment #472324 - Flags: feedback?(kairo) → feedback-
(Reporter)

Comment 38

7 years ago
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
Attachment #472259 - Attachment description: test, v2: even simpler, and showing the problem better → test, v2: even simpler, and showing the problem better (checked in)
(Assignee)

Comment 39

7 years ago
Perhaps we could also use something like !(aBrowser.docshell.busyFlags & Components.interfaces.nsIDocShell.BUSY_FLAGS_NONE)? But that's something Neil should decide.
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?
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.
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".
Attachment #472259 - Attachment description: test, v2: even simpler, and showing the problem better (checked in) → test, v2: even simpler, and showing the problem better [Checkin: Comment 38]
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.
Attachment #472324 - Attachment is obsolete: true
Attachment #472627 - Flags: review?(neil)
Attachment #472627 - Flags: feedback?(aqualon)
Attachment #472324 - Flags: feedback?(aqualon)

Updated

7 years ago
Attachment #472627 - Flags: feedback?(kairo)

Updated

7 years ago
Attachment #472627 - Flags: review?(neil) → review+
(Reporter)

Comment 44

7 years ago
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.
Attachment #472627 - Flags: feedback?(kairo) → feedback+
(Assignee)

Comment 45

7 years ago
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.
Attachment #472627 - Flags: feedback?(aqualon) → feedback+
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
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
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
Attachment #472627 - Attachment description: just drop the busy check on tab. → just drop the busy check on tab [Checkin: Comment 47]
Blocks: 594342
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.
Severity: normal → major
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Target Milestone: --- → seamonkey2.1b1
Keywords: intermittent-failure
Whiteboard: [sm-perma][orange] → [sm-perma]

Updated

5 years ago
Whiteboard: [sm-perma] → [sm-perma][cc-orange]
You need to log in before you can comment on or make changes to this bug.