Closed Bug 453442 Opened 12 years ago Closed 12 years ago

Secure favicon.ico request pop-ups certificate error dialog

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mayhemer, Assigned: johnath)

References

(Blocks 1 open bug, )

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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.
(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.
This is pretty annoying - any chance that you have cycles to find a fix, Honza?
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)
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-
Attached patch Add browser argument (obsolete) — Splinter Review
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)
cc'ng Kairo in case this is a change SeaMonkey would like as well.
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+
Attached patch With commentsSplinter Review
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+
Blocks: 461010
Filed bug 461010 for the followup
Pushed: http://hg.mozilla.org/mozilla-central/rev/bd9c0bfb6d38
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 438831
Blocks: 461137
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?
(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?
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.