Secure favicon.ico request pop-ups certificate error dialog

RESOLVED FIXED

Status

()

Toolkit
Places
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: mayhemer, Assigned: johnath)

Tracking

(Blocks: 1 bug, {regression})

Trunk
regression
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.0.x ?
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
When requesting https:// page with incorrect certificate certificate error page and the certificate error pop-up dialog appears both.

I am testing with a secure site with invalid certificate running on localhost and with https://verisign.com. I get the correct page to add an exception. But in few seconds the dialog as described above appears too.

This is caused by https://verisign.com/favicon.ico request. When certificate error during this subsequent request is detected pipnss is trying to figure out if there were assigned external handler for cert errors. It fails because nsCOMPtr<nsIDocShellTreeItem> item(do_GetInterface(proxiedCallbacks)) at nsNSSSocketInfo::EnsureDocShellDependentStuffKnown fails. This getInterface proxies to nsHttpConnection::GetInterface, what is correct. It successfully delegates GetInterface to mTransaction->mSecurityCallbacks which is (in this case) nsInterfaceRequestorAgg, also correct. But, it has assigned just mFirst member that points to FaviconLoadListener. And this class doesn't provide nsIDocShellTreeItem interface. mSecond is null.

Therefor, pipnss believes there is not any external handler assigned (actually, there is none) -> dialog appears.

Doesn't appear in FF 3.0.1.
Dupe of bug 431712? Good to confirm it is the favicon load, though.
(Reporter)

Comment 2

10 years ago
(In reply to comment #1)
> Dupe of bug 431712? Good to confirm it is the favicon load, though.

It could be the same bug however, I can reproduce this in 100% cases (on my dev machine). The analyzes in description is the cause.
(Assignee)

Comment 3

10 years ago
This is pretty annoying - any chance that you have cycles to find a fix, Honza?
(Assignee)

Comment 4

10 years ago
Created attachment 343973 [details] [diff] [review]
Change shouldLoadFavicon to say no to about: pages

Honza - does this fix the issue for you?

With the patch, I no longer see the dialog box when I follow your STR (on Mac, but shouldn't matter...)
Attachment #343973 - Flags: review?(gavin.sharp)
(Reporter)

Comment 5

10 years ago
Oh, I believed someone else is already working on this at bug 431712. The patch fixes the problem also on win. I don't see any other way to fix this instead of bounding doc shell to the FaviconLoadListener, what is imho overkill.
Comment on attachment 343973 [details] [diff] [review]
Change shouldLoadFavicon to say no to about: pages

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>             return (aURI && this.mPrefs.getBoolPref("browser.chrome.site_icons") &&
>                     this.mPrefs.getBoolPref("browser.chrome.favicons") &&
>+                    !/^about:/.test(this.mCurrentBrowser.contentDocument.documentURI) &&

Could just use: this.contentDocument.documentURI, but in saying that I just realized that this won't work if this shouldLoadFavIcon invocation is for a background tab. Can solve that by having shouldLoadFavIcon take a browser rather than a URI, I think (all of the callers already just pass in browser.currentURI).
Attachment #343973 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 7

10 years ago
Created attachment 344085 [details] [diff] [review]
Add browser argument

Good point.

This adds the browser argument, and updates consumers in tabbrowser.xml, browser.js, and nsSidebar.js, which appears to be all the non-suite consumers.

http://mxr.mozilla.org/mozilla/search?string=shouldLoadFavicon
Assignee: nobody → johnath
Attachment #343973 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #344085 - Flags: review?(gavin.sharp)
(Assignee)

Comment 8

10 years ago
cc'ng Kairo in case this is a change SeaMonkey would like as well.
(Assignee)

Comment 9

10 years ago
Created attachment 344089 [details] [diff] [review]
Better approach - just use documentURIObject 

Gavin and I chatted about this and agree that a more elegant way to do this is to exploit the fact that on http/https pages, content.location == document.documentURI, but on error pages, they don't.  So by just changing the callers to pass in documentURIObject instead, the existing checks which only allow http/https schemes through should be enough.

To confirm, I session-restored a session with two error pages in the background, a normal (https) page in the background, and a normal (http) page in the foreground.  Both normal pages loaded their favicons, and neither error page caused the PSM dialog to appear.
Attachment #344085 - Attachment is obsolete: true
Attachment #344089 - Flags: review?(gavin.sharp)
Attachment #344085 - Flags: review?(gavin.sharp)
Comment on attachment 344089 [details] [diff] [review]
Better approach - just use documentURIObject 

Perhaps it's worth adding comments explaining why we're not passing currentURI, lest these callers be unknowingly "optimized" in the future?

It probably makes more sense long term to have shouldLoadFavIcon take an optional <browser> argument (defaulting to the tabbrowser's selectedBrowser) so that callers don't have to worry about the distinction. Could also turn that ("schemeIs" in aURI) check into an instanceof check while we're at it! Followup?
Attachment #344089 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 11

10 years ago
Created attachment 344145 [details] [diff] [review]
With comments

Carrying forward the flag, this patch just adds a comment to each caller referencing this bug.

Followup sounds fine for the other work - let's kill this nag dialog now.
Attachment #344089 - Attachment is obsolete: true
Attachment #344145 - Flags: review+
(Assignee)

Updated

10 years ago
Blocks: 461010
(Assignee)

Comment 12

10 years ago
Filed bug 461010 for the followup
(Assignee)

Comment 13

10 years ago
Pushed: http://hg.mozilla.org/mozilla-central/rev/bd9c0bfb6d38
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Blocks: 438831

Updated

10 years ago
Blocks: 461137

Comment 14

10 years ago
Why are you trying to use the site favicon instead of the page's link icon?
verisign.com does not specify a favicon, so we fall back to hostname/favicon.ico.
Is this worth fixing in a 3.0.x update?
Flags: wanted1.9.0.x?

Comment 17

10 years ago
(In reply to comment #16)
> Is this worth fixing in a 3.0.x update?

YES. This is going to block our Firefox 3 rollout (Yeah I know we're late) inside IBM because we have so many freaking self signed certs.
Johnathan: Can you work up a 1.9.0 patch for consideration?

(Also, any reason this isn't in a testsuite?)
Flags: in-testsuite?
(Reporter)

Comment 19

9 years ago
Seems this could still be present on trunk: bug 482660 comment 3 but it have to be confirmed yet.
You need to log in before you can comment on or make changes to this bug.