301119 - Need to allow chrome favicons for XUL error pages

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Currently checkLoadURI in tabbrowser.xml restricts loading of favicon for
chrome-URIs.

We should allow loading of favicons here because normal images doesn't have this
restriction. I noticed this bug while working on bug 280190.

http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/tabbrowser.xml#716
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsContentUtils.cpp#1821

Patch upcoming...

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Enable loading of favicons for xpfe

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Enable loading of favicons for toolkit

Comment 3

[Boris Zbarsky [:bzbarsky]]

Nromal images have this restriction because known functionality (eg some remote
XUL) would break with it.  The fact that they don't have this restriction does
NOT make me happy...

Is there an actual use case for chrome favicons?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #3)
> Is there an actual use case for chrome favicons?

As I mentioned before we want to use a favicon on the new netError.xhtml page. 

Comment 5

[Boris Zbarsky [:bzbarsky]]

How about only allowing loading of chrome favicons by a chrome page, then?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #5)
> How about only allowing loading of chrome favicons by a chrome page, then?

One possibility, but this wouldn't help us due to netError.xhtml doesn't have
chrome permissions since bug 292624 was fixed. Or how we could handle his? But
setting a restriction for loading chrome favicons by normal webpages is a good
idea. They don't need access.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Can't you get the actual URI loaded from the page's docshell and just look at
the scheme?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 189619 [details] [diff] [review]
Enable loading of favicons for xpfe

Per comments

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #7)
> Can't you get the actual URI loaded from the page's docshell and just look at
> the scheme?

Sorry, I don't really understand, what I should do. You are only interested to
know the current scheme of netError? After the checkin of bug 292624 the scheme
was changed to "moz-neterror:page?e=error&u=url&d=desc". 

Comment 10

[Boris Zbarsky [:bzbarsky]]

That's the URI that starts to be loaded.  I'm talking about the URI as actually
loaded...  biesi, do we have a way of getting at it in this case?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #10)
> That's the URI that starts to be loaded.  I'm talking about the URI as actually
> loaded...  biesi, do we have a way of getting at it in this case?

Ok, I've used DOMi to get the documentURI. I hope this is that one you wanna
have... 

It looks like that one:
about:neterror?e=dnsNotFound&u=http%3A//www.not.existent/&c=UTF-8&d=www.not.existent%20could%20not%20be%20found.%20Please%20check%20the%20name%20and%20try%20again.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

hmm, I guess docshell.currentURI really is the channel's originalURI.
document.documentURI maybe?

Comment 13

[Boris Zbarsky [:bzbarsky]]

For an about: load, document.documentURI will be the originalURI of the channel.
 The webnavigation's currentURI will generally be the channel's final URI, but
for error pages I believe we might force it to be the URI the user originally
started to load...

In any case, is there a reason to allow chrome favicons for anything but
"about:neterror" (as documentURI here)?

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #13)
> For an about: load, document.documentURI will be the originalURI of the channel.
>  The webnavigation's currentURI will generally be the channel's final URI, but
> for error pages I believe we might force it to be the URI the user originally
> started to load...

Ok, I tested the values for the above objects.

document.documentURI:          about:neterror?e=dnsNotFound...
getWebNavigation().currentURI: http://www.not.existent/

> In any case, is there a reason to allow chrome favicons for anything but
> "about:neterror" (as documentURI here)?

At this time I don't think so. Afaik netError.xhtml will be the only page using
a chrome referenced favicon.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I'd be in favor of restricting to that, then.

Comment 16

[Caleb]

*** Bug 302797 has been marked as a duplicate of this bug. ***

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 189620 [details] [diff] [review]
Enable loading of favicons for toolkit

equivalent to the one bz rejected

Comment 18

[Daniel Veditz [:dveditz]]

As summarized this is WFM: chrome pages *can* load chrome favicons and
checkLoadURI works fine.

Rather than a general bug I think this is specific to the about:neterror? page.
This should get fixed in tabbrowser rather than caps.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #18)
> As summarized this is WFM: chrome pages *can* load chrome favicons and
> checkLoadURI works fine.

Indeed a bad summary. Thanks for adjusting to the real issue.

> Rather than a general bug I think this is specific to the about:neterror? page.
> This should get fixed in tabbrowser rather than caps.

Yes, it seems only to be used for netError.xhtml. Personaly I havn't an idea
where to start now. 

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: allow favicons for error pages only

maybe something like this, but this is completely untested

(the reason for the chrome check is that I feared XSS attacks on the error page
leading to insertion of another <link rel="icon">, which I suspect does load
the favicon. I haven't verified that, but I thought better to be on the safe
side...)

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

(I don't intend to do anything with this patch, someone else will have to take
care of it)

Comment 22

[Daniel Veditz [:dveditz]]

Christian's patch is what I was suggesting (good call on checking that the
favicon is indeed chrome). If we think this is at all useful we could let all
about: URIs set a chrome favicon, but this is a conservative change to fix the
narrowly stated problem.

the Toolkit tabbrowser would need the same patch.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 191143 [details] [diff] [review]
allow favicons for error pages only

r=dveditz
I have no idea how important neterror favicons are to 1.8b4. This is not marked
blocking (or even nominated). We need bsmedberg's (or mconnor's) r= on this.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Ported patch for toolkit [Checkin: Comment 26 (& 27)]

Comment 25

[[e-mail]]

Comment on attachment 191143 [details] [diff] [review]
allow favicons for error pages only

Such a favicon will get pushed all the way up the UI to the URLbar and the
bookmarks. Pages without their own icon will not get reset when the page loads
succesfully. Maybe it's possible to fake a favicon like we do for image
documents.

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.102; previous revision: 1.101
done
Checking in xpfe/global/resources/content/bindings/tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <-- 
tabbrowser.xml
new revision: 1.125; previous revision: 1.124
done

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

I backed out the xpfe patch because of comment 25, which may make this patch not
needed at least for xpfe.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #25)
> (From update of attachment 191143 [details] [diff] [review] [edit])
> Such a favicon will get pushed all the way up the UI to the URLbar and the
> bookmarks. Pages without their own icon will not get reset when the page loads
> succesfully. Maybe it's possible to fake a favicon like we do for image
> documents.

I can't recognize this problem with my current cvs build of Firefox. Domains
without a favicon aren't updated with the netError warning icon. Also the
bookmark isn't updated. It still shows me the blank favicon after a complete reload.

What I have done:
- Put a bookmark without favicon to the PTB 
- Load this page
- Shut down the network
- Reload the page after clearing the cache
- Tab and location bar show the warning icon but not the bookmark
- Click bookmark to refresh it - still blank icon
- Reconnect network
- Reload page - tab and location bar shows blank favicon

Comment 29

[[e-mail]]

(In reply to comment #28)
>(In reply to comment #25)
>>Such a favicon will get pushed to the URLbar and bookmarks.
>I can't recognize this problem with my current cvs build of Firefox.
No, it turns out that Firefox only update the bookmarks icon when a page loads
(error pages don't load, they just show). So as long as you don't patch xpfe's
tabbrowser.xml then we'll be fine.                                             
                                                             

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

note that the xpfe part of bug 229737 is now checked in, so seamonkey shouldn't
change any favicons when error pages are loaded.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #30)
> note that the xpfe part of bug 229737 is now checked in, so seamonkey shouldn't
> change any favicons when error pages are loaded.

So we want to fix this bug or are favicons for xul error pages out of scope for now?

Comment 32

[[e-mail]]

(In reply to comment #31)
>So we want to fix this bug
The patch still applies, so I guess biesi can check it in (it's his patch)...

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

can someone else take care of checking this in for me, if it's still needed?

Comment 34

[Reed Loden [:reed]]

(In reply to comment #33)
> can someone else take care of checking this in for me, if it's still needed?

You mean backing it out, as per bug 430433?

Comment 35

[Philip Chee]

> You mean backing it out, as per bug 430433?
No Biesi means that the patch needs to be landed.

Comment 36

[Philip Chee]

>> You mean backing it out, as per bug 430433?
>No Biesi means that the patch needs to be landed.
But in /suite/ and not in /xpfe/

Comment 37

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

The patch needs to updated to apply to the suite tabbrowser, and should probably included about:blocked like the current Firefox code in browser.js does. It should probably also be tested and maybe even reviewed again after someone's done that.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So which patches have to be updated? Both or only the one for suite?

Reopening bug until the remaining work is done and an updated patch is checked in.

Comment 39

[Robert Kaiser]

The suite patch needs to be updated for the tabbrowser.xml living in suite/ - this is probably the cause of bug 431955.

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Updated patch for SeaMonkey

This patch updates the suite tabbrowser.xml in the same way how Firefox handles it.

Comment 41

[Robert Kaiser]

Comment on attachment 319128 [details] [diff] [review]
Updated patch for SeaMonkey

I don't really understand tabbrowser code, over to Neil who should actually know it.

Comment 42

[[e-mail]]

Comment on attachment 319128 [details] [diff] [review]
Updated patch for SeaMonkey

>+              const aboutBlocked = /^about:blocked\?/;
We don't (yet?) have about:blocked, so I don't think we should test for it, it will only confuse people. r=me with that fixed.

>+                const secMan =
>+                Components.classes["@mozilla.org/scriptsecuritymanager;1"]
>+                          .getService(nsIScriptSecurityManager);
Nit: need to reindent all three lines, not just the first ;-)

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Updated patch for SeaMonkey v2 [Checkin: Comment 46]

Fixed comments and taken over r+.

Robert, do we need sr for this module?

Comment 44

[Robert Kaiser]

Given that Neil as our "UI tsar" has done the review, I guess it's ok to go in this way.

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so I need someone to check this in. Thanks.

Comment 46

[Robert Kaiser]

Checking in suite/browser/tabbrowser.xml;
/cvsroot/mozilla/suite/browser/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.196; previous revision: 1.195
done

Is this fixed now? If so, leaving to Henrik to mark it as such :)

Comment 47

[Robert Kaiser]

Comment on attachment 322405 [details] [diff] [review]
Updated patch for SeaMonkey v2
[Checkin: Comment 46]

>+              if (!(aboutNeterr.test(targetDoc.documentURI) ||
>+                  !uri.schemeIs("chrome")) {

Grrr, the first of those lines has a ( too much, killed the one after the ! as a bustage fix, thanks to ajschult for pointing this out.

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry for the bustage. But with the follow-up fix this bug should be resolved now. Marking accordingly.