483382 - browser_480148.js causes browser_420786.js to fail

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Serge Gautherie (:sgautherie)]

{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1237021262.1237026184.29623.gz
Linux mozilla-central unit test on 2009/03/14 02:01:02

chrome://mochikit/content/browser/browser/components/shell/test/browser_420786.js
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/shell/test/browser_420786.js | Timed out
}

Comment 1

[Sylvain Pasche]

See bug 420786 comment 28. We could try using a document with an image instead of about:logo.

Comment 2

[Serge Gautherie (:sgautherie)]

Green : 7c183de433d1
Orange: d1776607d429

Regression timeframe:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-03-14+01%3A13%3A00&enddate=2009-03-14+01%3A51%3A23

Comment 3

[Dão Gottwald [:dao]]

I disabled the test for now:
http://hg.mozilla.org/mozilla-central/rev/a582dbde02ec

Sylvain, any idea how any of these landings could have caused the test to fail constantly?
http://hg.mozilla.org/mozilla-central/rev/d1776607d429
http://hg.mozilla.org/mozilla-central/rev/78c8e1b22296
http://hg.mozilla.org/mozilla-central/rev/e144dd6cd801
http://hg.mozilla.org/mozilla-central/rev/3c49ef2a9f76

Comment 4

[Sylvain Pasche]

I see nothing obvious in those. In particular, there's nothing loading about:logo that would prevent it from sending a load event as described in bug 420786 comment 28.

I can try to run the test locally to see if it fails.

Comment 5

[Sylvain Pasche]

I can reproduce locally, it fails when getting the shell service.

Components.classes["@mozilla.org/browser/shell-service;1"].             getService(Components.interfaces.nsIShellService);

Error: uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: javascript:%20Components.classes["@mozilla.org/browser/shell-service;1"].%20%20%20%20%20%20%20%20%20%20%20%20%20getService(Components.interfaces.nsIShellService); :: <TOP_LEVEL> :: line 1"  data: no]

Comment 6

[Sylvain Pasche]

Ignore my previous comment, there was something wrong on my system.

The issue is about changeset 78c8e1b22296 (bug 480148). If I back it out, the test passes.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

<syp> gavin: when commenting the session restore test, the shell test doesn't fail any more, so looks like it's only the test interfering.
<syp> gavin: should finish() be called on the main browser window? If I call it from an executeSoon it doesn't interfere with the other test.

It's not clear to me why the executeSoon helped, new tests are already run from an executeSoon invocation I believe.

Comment 8

[Dão Gottwald [:dao]]

I re-enabled browser_420786.js and disabled browser_480148.js.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Do we want to re-enable browser_480148.js and just wrap the finish with executeSoon()? I would like to get bug 480148 landed on 1.9.1 and if the fix for this is as simple as executeSoon, then that should be in the patch that lands in 1.9.1 instead of landing a disabled test.

Comment 10

[Dão Gottwald [:dao]]

It would be better if we understood what's going on... Gavin is right, finish() invokes the next test in executeSoon(): http://hg.mozilla.org/mozilla-central/annotate/6da5efbfef2c/testing/mochitest/browser-test.js#l163

Comment 11

[Sylvain Pasche]

One thing that crossed my mind but that I didn't test last time: the session restore test opens a new window and calls finish() in the context of that new window.  Maybe it should call it on the opener instead (opener.finish())? If the test context is not "synced" on every windows things could get messed up.

By the way, reproducing this implies running all of the browser chrome test on Linux (the session restore and shell tests are on different directories). The shortcut I found was to copy the Linux shell test in the session restore directory and give it a name that comes after the session restore test. Then you can use the --test-path and you don't need to run all of the tests.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I've run into a similar problem in my tests for bug 394759, where the first test runs fine, the 2nd runs fine if executed alone, but I run into problems when the 2nd one is executed immediately after the first. When I wrap finish() in the first test with executeSoon(), it fixed the problem there too.

Perhaps it's like you say Sylvain and it's the fact of where finish is called. I'll investigate, but looking at the other SessionStore tests, finish is usually called in the scope of some event listener and not from the immediate scope of test()

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #11)
> One thing that crossed my mind but that I didn't test last time: the session
> restore test opens a new window and calls finish() in the context of that new
> window.

The scope doesn't change. Note that it's finish(), not this.window.finish() or something like that. finish wouldn't even exist in a new window.

Comment 14

[Dão Gottwald [:dao]]

This is a bug in browser_420786.js's use of FUEL...

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/6628da386550

Comment 16

[Sylvain Pasche]

Out of curiosity, do you know what the underlying issue was? If it was an issue in FUEL, maybe it should be fixed to prevent such issue in the future?

Comment 17

[Dão Gottwald [:dao]]

Application.activeWindow didn't point to the browser window that the test was running in, but to a closing browser window from the previous test.

Comment 18

[Sylvain Pasche]

oh, ok. Thanks for the explanation.

Comment 19

[Dão Gottwald [:dao]]

I landed this on 1.9.2. It's not going to land on 1.9.1.