556684 - [SeaMonkey] browser-chrome: "browser_bug550565.js | gBrowser.getIcon is not a function" since landing

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Comment 1

[Justin Lebar (not reading bugmail)]

Huh...that's strange.  Do you have any information about the hang?  If not, I guess I'll go learn to build SeaMonkey and see what's up with this.

Comment 2

[Serge Gautherie (:sgautherie)]

Ah, sorry, I missed to copy the log:

{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1270172947.1270175974.32673.gz&fulltext=1
Linux comm-central-trunk debug test mochitest-other on 2010/04/01 18:49:07

Running chrome://mochikit/content/browser/docshell/test/browser/browser_bug550565.js...
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /builds/slave/comm-central-trunk-linux-debug/build/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 5511
WARNING: NS_ENSURE_TRUE(mMutable) failed: file /builds/slave/comm-central-trunk-linux-debug/build/mozilla/netwerk/base/src/nsSimpleURI.cpp, line 224
JavaScript error: chrome://mochikit/content/browser/docshell/test/browser/browser_bug550565.js, line 11: gBrowser.getIcon is not a function
WARNING: NS_ENSURE_TRUE(mMutable) failed: file /builds/slave/comm-central-trunk-linux-debug/build/mozilla/netwerk/base/src/nsSimpleURI.cpp, line 224
TEST-INFO | chrome://mochikit/content/browser/docshell/test/browser/browser_bug550565.js | Console message: [JavaScript Error: "gBrowser.getIcon is not a function" {file: "chrome://mochikit/content/browser/docshell/test/browser/browser_bug550565.js" line: 11}]
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/docshell/test/browser/browser_bug550565.js | Timed out
}

Comment 3

[Justin Lebar (not reading bugmail)]

This is the key:

[JavaScript Error: "gBrowser.getIcon is not a function" {file:
"chrome://mochikit/content/browser/docshell/test/browser/browser_bug550565.js"
line: 11}]

I call gBrowser.getIcon(tab) to get the favicon for |tab|.

Comment 4

[Dão Gottwald [:dao]]

Not a Firefox bug.

Comment 5

[Robert Kaiser]

Dão:
No, it's actually a docshell bug, but if Firefox-spcific tests are interoduced into docshell by a Firefox bug, then the bustage caused by that in other products belongs to the same component, IMHO.

It's wrong for a docshell test to use Firefox-specific functionality unless that test is only included for Firefox. Possibly the test belongs into browser/ though.

As a side note, once SeaMonkey supports places bookmarks, we can work on bug 554908, and my WIP patch for that introduces this function for SeaMonkey as well.

Comment 6

[Dão Gottwald [:dao]]

Every single browser chrome test uses Firefox-specific functionality.

Comment 7

[Dão Gottwald [:dao]]

So, since you're actually working on fixing this in SeaMonkey, I don't see the point of tracking this in Firefox. This would only spam people that aren't really interested in it.

Comment 8

[Robert Kaiser]

1) IT'S WRONG TO HAVE THIS BUG IN SEAMONKEY!!!!!

2) NO browser-chrome test that is not in browser/ uses Firefox-specific functionality unless it's excluded from being installed in any other BROWSER. Just because a test suite has "browser" in its name doesn't mean it's excludive to Firefox. You know, that's not the only browser in the world, and not even the only one in the Mozilla project, contrary to what you seem to believe.

3) I'm working on adding this Firefox-specific method some time far in the future, possibly a few months out. Having that test fail until then is not an option.

Bug 550565 has caused this failure and whoever broke things there HAS to fix it, possibly by making this test be either in browser/ or not be installed for any other application but Firefox, but preferably by using something that is not specific to a certain implementation of tabbrowser.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> 1) IT'S WRONG TO HAVE THIS BUG IN SEAMONKEY!!!!!

SeaMonkey fails a test (which should be reason enough to track this in SeaMonkey) because its tabbrowser.xml doesn't implement the same API as Firefox's tabbrowser.xml. We're not going to fix the failure in Firefox's tabbrowser.xml, which this component is about. I'm resolving this now accordingly. If you think a docshell browser chrome test must not use this API, move it to docshell.

> 2) NO browser-chrome test that is not in browser/ uses Firefox-specific
> functionality unless it's excluded from being installed in any other BROWSER.
> Just because a test suite has "browser" in its name doesn't mean it's excludive
> to Firefox. You know, that's not the only browser in the world, and not even
> the only one in the Mozilla project, contrary to what you seem to believe.

Right, any other browser is free to implement the (tab)browser API that browser chrome tests depend on.

Comment 10

[Robert Kaiser]

OK, I request backout of the whole patch or the test then,a s Firefox devs seem to play stupid.

Comment 11

[Robert Kaiser]

Oh, and reopen, of course, as this sucker is far from fixed, just the people who made it happen deny any responsibility for their bustage.

Comment 12

[Robert Kaiser]

So, the real problem here is that a test in docshell/ tests a fix browser/ - IMHO, this is really a Firefox bug, but it has been exiled to docshell because the failure from Firefox people was to put the test in core. Fun times.

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #12)
> So, the real problem here is that a test in docshell/ tests a fix browser/ -
> IMHO, this is really a Firefox bug, but it has been exiled to docshell because
> the failure from Firefox people was to put the test in core. Fun times.

It tests a core feature, history.pushState.

Comment 14

[Robert Kaiser]

(In reply to comment #13)
> It tests a core feature, history.pushState.

No, it tests how Firefox - and only Firefox - uses a core feature. Heck, the fix it was checked in with and which it is supposed to test was even confined to a FIREFOX-SPECIFIC file!

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> (In reply to comment #13)
> > It tests a core feature, history.pushState.
> 
> No, it tests how Firefox - and only Firefox - uses a core feature.

Right, using an API you're supposed to implement anyway when running browser chrome tests.
And any browser implementing that API should pass the test; failing it would indicate that there's actually a bug (e.g. that SeaMonkey needs the same fix as Firefox).

Comment 16

[Robert Kaiser]

(In reply to comment #15)
> Right, using an API you're supposed to implement anyway when running browser
> chrome tests.

Can you show me where that wild claim is documented and where that API and what everything in it is supposed to do is documented? Note: CODE IS NO DOCUMENTATION.

Comment 17

[Dão Gottwald [:dao]]

It's quite obviously the way browser chrome tests work. Just because it's not documented doesn't mean you could run them without the tabbrowser API. As for tabbrowser API documentation, that's on MDC, but it's certainly not complete. I've just added dev-doc-needed to bug 525708.

Comment 18

[Robert Kaiser]

(In reply to comment #17)
> It's quite obviously the way browser chrome tests work. Just because it's not
> documented doesn't mean you could run them without the tabbrowser API.

"the tabbrowser API"? Bleh. Now there is only one tabbrowser in the world and only one true API for it?

Now I know why so many people hate the "arrogant attitude of Firefox devs". I tried to calm them down many times, but it seemd that it's true.

Comment 19

[Dão Gottwald [:dao]]

No, there's not only one tabbrowser API, but browser chrome tests in mozilla-central naturally use the one that exists in mozilla-central. You can criticize the browser chrome test framework for this approach, but blaming a single test doesn't seem to make much sense.

Comment 20

[Bill Gianopoulos [:WG9s]]

Well.  I don;t think the issue here has much to do with what APIs browsers should or should not support.  The problem here is that a test that should really be not be executed for other than Firefox is in the docshell directory so is being executed for all Mozilla products (perhaps some that are not even browsers) that use docshell.  One would think this is obviously NOT correct.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> Well.  I don;t think the issue here has much to do with what APIs browsers
> should or should not support.  The problem here is that a test that should
> really be not be executed for other than Firefox

I don't quite agree with this, see comment 15 and bug 550565 comment 18. If you accept that a browser running the test should implement the getIcon method, you should also expect this browser to pass the test.

> so is being executed for all Mozilla products (perhaps some that are not even
> browsers) that use docshell.

Those that aren't browsers shouldn't run browser chrome tests.

Comment 22

[Bill Gianopoulos [:WG9s]]

But this seems to be a docshell test and NOT a browser test.  If it is a browser test it should be in browser/tests and NOT docshell/tests.

Comment 23

[Bill Gianopoulos [:WG9s]]

Trying one more time to bring sanity to this discussion.  the issue here is that a browser only test has been made part of the docshell testsuite instead of being part of the browser testsuite where it obviously belongs.  Had it been part of the correct test suite in the first place, there would be no issue here.

Comment 24

[Bill Gianopoulos [:WG9s]]

Trying one more time to restate this for clarity.  The testsuite for the Gecko back-end should in no way be dependent on the front-end application being Firefox.  If there is any such dependency then the default condition should be that any such dependent tests not be run if there are Firefox only tests int he back-end testsuite, then it should be up to the Firefox test procedure to specifically invoke them and not up to every other potential Gecko application to specifically not run them.  This is the only sensible approach to a back-end test suite.

So, I think Firefox only or even browser only tests, do not belong in a non browser only test-suite unless running such tests is not the default action for running the test-suite.

That might not be how things are today, but that seems to be the only thing that makes any sense here.  Otherwise when the next new web application comes along (whatever that may be) we will be way behind implementing it because it will never be able to apps our test suite because we have all kinds of tests running under entirely the wrong conditions.

Comment 25

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

I don't really understand the opposition to fixing this bug in SeaMonkey - consolidating APIs between Firefox/SeaMonkey and sharing test coverage for bug 550565 seems like a net win for both projects.

On the other hand, there isn't much of a win for Firefox to keep the test in a core directory, so I don't really see any reason to oppose moving it to /browser (in practice, SeaMonkey and Firefox are the only Gecko consumers of the browser chrome test suite). So let's just do that - who wants to patch?

Comment 26

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

(I do understand the opposition to the way the SeaMonkey bug fix was requested - it ideally shouldn't be presented in the form of an orange tinderbox with no prior notice. If I've just mistaken that opposition for opposition to the fix itself, then that's great - let's also get a bug on file for porting bug 525708 to SeaMonkey, and once that's fixed we can undo the move.)

Comment 27

[Robert Kaiser]

(In reply to comment #25)
> I don't really understand the opposition to fixing this bug in SeaMonkey

Well, as you said, it came down the wrong way, and then, we are not even using the faviconservice in tabbrowser yet, so it's unclear how much that affects us overall.

That said, there is some interest in supporting the Firefox tabbrowser API in SeaMonkey (esp. for add-on compat), it's just that it's probably quite some work to do, and there's also interest in switching to using faviconservice - again, that's some work to do, and all that is not trivial and I'd prefer to get our tests green before having done that.

The bug 554908 I mentioned before might have a WIP patch that looks trivial right now, but only makes sense once places bookmarks have landed, and when one looks into bug 498596, it's quite easy to see that this is far from trivial...

Comment 28

[Dão Gottwald [:dao]]

It looks like getIcon could be implemented right now, returning browser.mIconURL just like in the WIP patch in bug 554908...

Comment 29

[Justin Lebar (not reading bugmail)]

Attachment: Patch

Robert, does this look better to you?

Comment 30

[Robert Kaiser]

Comment on attachment 436939 [details] [diff] [review]
Patch

yes, thanks.

Comment 31

[Justin Lebar (not reading bugmail)]

Comment on attachment 436939 [details] [diff] [review]
Patch

Dão, does this look good to you as well?

Comment 32

[Justin Lebar (not reading bugmail)]

Attachment: Patch for checkin [Checkin: Comment 41]

Comment 33

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> It looks like getIcon could be implemented right now, returning
> browser.mIconURL just like in the WIP patch in bug 554908...

KaiRo,
while the current patch seems fully right both theoretically and practically,
I nontheless feel like splitting out and landing just getIcon() from your own patch (as it happens to already exist! :-)) that Dão cited would be (even) better in the long run (from a SeaMonkey p-o-v).
Don't you agree?

Comment 34

[Robert Kaiser]

(In reply to comment #33)
> KaiRo,
> while the current patch seems fully right both theoretically and practically,
> I nontheless feel like splitting out and landing just getIcon() from your own
> patch (as it happens to already exist! :-)) that Dão cited would be (even)
> better in the long run (from a SeaMonkey p-o-v).
> Don't you agree?

No, a method without any users is not a good idea, and it's not that clear what we need there, I don't even know if the SeaMonkey tabbrowser even stores mIconURL. And then, even if we find reasons for doing it, it should not be in this bug.

Comment 35

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

The method would have a user - the test. And your tabbrowser doesn't store mIconURL directly, but it does set this.mBrowser.mIconURL (just like ours), so the implementation of getIcon in attachment 436817 [details] [diff] [review] looks fine as-is.

I don't understand why you wouldn't want to make that change in this bug.

Comment 36

[Robert Kaiser]

(In reply to comment #35)
> I don't understand why you wouldn't want to make that change in this bug.

Someone else would need to do it. I don't know any of that code, and don't want to touch tabbrowser (any of them) where I don't have to. If Neil or someone else takes it up, have fun and feel free to do it, I neither own tabbrowser nor understand its code.

Comment 37

[[e-mail]]

(In reply to comment #6)
> Every single browser chrome test uses Firefox-specific functionality.
Your test was the first test inside Gecko/toolkit that used Firefox-specific functionality. Other toolkit tests only use a limited set of functions:

gBrowser.addEventListener - not even browser-specific

gBrowser.loadURI
gBrowser.contentDocument - <browser> API since 2001/05/01*

gBrowser.contentWindow - <browser> API since 2001/09/01

gBrowser.addTab
gBrowser.removeTab
gBrowser.selectedTab
gBrowser.removeCurrentTab - <tabbrowser> API since 2001/09/27*

gBrowser.getBrowserForTab - <tabbrowser> API since 2001/10/25

gBrowser.selectedBrowser - <tabbrowser> API since 2002/04/01

So in theory all of these tests could run (presumably failing) on Netscape ;-)

*CVS 1.1 of the file so might have existed elsewhere (can't tell from bugzilla)

Comment 38

[Dão Gottwald [:dao]]

Well, no, other tests use Fuel which isn't that old (but has been ported to SeaMonkey). Anyway, the fact that most tests use an ancient part of the tabbrowser API doesn't mean that this is the agreed subset that browser chrome tests may use.

Comment 39

[Robert Kaiser]

What we probably should do is have some documentation of what can be expected to be there in such tests, and/or a good documentation of what the tabbrowser APIs provide - in various supported Firefox and SeaMonkey versions, as that's what add-ons devs also need to look into.

Comment 40

[Serge Gautherie (:sgautherie)]

(In reply to comment #36)
> Someone else would need to do it.

I'll try: see bug 554908 patch Bv1.

With that patch, the first getIcon() check passes :-)

Then
12 tab.linkedBrowser.contentWindow.history.pushState("page2", "page2", "page2");
fails due to
{
Error: uncaught exception: [Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"  location: "chrome://mochikit/content/browser/docshell/test/browser/browser_bug550565.js Line: 12"]
}
which causes the second getIcon() check to report failure.

Yet, it's already much better to get a "simple" failure than a timeout!

Comment 41

[Serge Gautherie (:sgautherie)]

Comment on attachment 436948 [details] [diff] [review]
Patch for checkin
[Checkin: Comment 41]


http://hg.mozilla.org/mozilla-central/rev/fa7794d7ce9a

Comment 42

[Serge Gautherie (:sgautherie)]

(In reply to comment #26)
> let's also get a bug on file for porting bug 525708
> to SeaMonkey, and once that's fixed we can undo the move.

Now, anyone is welcome to look into that...

Comment 43

[Serge Gautherie (:sgautherie)]

V.Fixed per SeaMonkey tinderboxes.