Closed
Bug 453442
Opened 16 years ago
Closed 16 years ago
Secure favicon.ico request pop-ups certificate error dialog
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
People
(Reporter: mayhemer, Assigned: johnath)
References
(Blocks 1 open bug, )
Details
(Keywords: regression)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
johnath
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Reporter | |
Comment 2•16 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•16 years ago
|
||
This is pretty annoying - any chance that you have cycles to find a fix, Honza?
Assignee | ||
Comment 4•16 years ago
|
||
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•16 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 6•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
cc'ng Kairo in case this is a change SeaMonkey would like as well.
Assignee | ||
Comment 9•16 years ago
|
||
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 10•16 years ago
|
||
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•16 years ago
|
||
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 | ||
Comment 13•16 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/bd9c0bfb6d38
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 14•16 years ago
|
||
Why are you trying to use the site favicon instead of the page's link icon?
Comment 15•16 years ago
|
||
verisign.com does not specify a favicon, so we fall back to hostname/favicon.ico.
Comment 17•16 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.
Comment 18•16 years ago
|
||
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•16 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.
Description
•