From 00c5a4d4d9ccbbfa1232be9b574e4d9d96501f74 Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Tue, 27 Mar 2012 15:28:14 +0200 Subject: [PATCH] Bug 737846 - Ensure favicons service doesn't add unwanted pages to history. r=dietrich --- .../components/places/AsyncFaviconHelpers.cpp | 34 ++------- .../components/places/mozIAsyncFavicons.idl | 4 +- .../components/places/nsFaviconService.cpp | 5 +- .../components/places/nsIFaviconService.idl | 7 +- .../places/tests/chrome/Makefile.in | 3 +- .../places/tests/chrome/history_post.sjs | 6 ++ .../tests/chrome/test_favicon_annotations.xul | 45 ++++++++--- .../places/tests/chrome/test_history_post.xul | 67 +++++++++++++++++ .../tests/favicons/test_expireAllFavicons.js | 43 ++++------- .../places/tests/favicons/test_favicons.js | 9 +++ .../favicons/test_favicons_conversions.js | 36 ++++----- .../test_setAndFetchFaviconForPage.js | 7 +- ...test_setAndFetchFaviconForPage_failures.js | 75 ++++++++++++------- .../components/places/tests/head_common.js | 58 ++++++++++++++ .../places/tests/inline/head_autocomplete.js | 23 ------ .../inline/test_autocomplete_functional.js | 75 ++++--------------- .../places/tests/inline/test_typed.js | 26 ++----- 17 files changed, 300 insertions(+), 223 deletions(-) create mode 100644 toolkit/components/places/tests/chrome/history_post.sjs create mode 100644 toolkit/components/places/tests/chrome/test_history_post.xul diff --git a/toolkit/components/places/AsyncFaviconHelpers.cpp b/toolkit/components/places/AsyncFaviconHelpers.cpp index 6b78f327b3c5..a371370ddd37 100644 --- a/toolkit/components/places/AsyncFaviconHelpers.cpp +++ b/toolkit/components/places/AsyncFaviconHelpers.cpp @@ -794,37 +794,15 @@ AsyncAssociateIconToPage::Run() NS_ENSURE_SUCCESS(rv, rv); } - // If the page does not have an id, try to insert a new one. + // If the page does not have an id, don't try to insert a new one, cause we + // don't know where the page comes from. Not doing so we may end adding + // a page that otherwise we'd explicitly ignore, like a POST or an error page. if (mPage.id == 0) { - nsCOMPtr stmt = mDB->GetStatement( - "INSERT INTO moz_places (url, rev_host, hidden, favicon_id, frecency, guid) " - "VALUES (:page_url, :rev_host, 1, :favicon_id, 0, GENERATE_GUID()) " - ); - NS_ENSURE_STATE(stmt); - mozStorageStatementScoper scoper(stmt); - rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), mPage.spec); - NS_ENSURE_SUCCESS(rv, rv); - // The rev_host can be null. - if (mPage.revHost.IsEmpty()) { - rv = stmt->BindNullByName(NS_LITERAL_CSTRING("rev_host")); - } - else { - rv = stmt->BindStringByName(NS_LITERAL_CSTRING("rev_host"), mPage.revHost); - } - NS_ENSURE_SUCCESS(rv, rv); - rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("favicon_id"), mIcon.id); - NS_ENSURE_SUCCESS(rv, rv); - rv = stmt->Execute(); - NS_ENSURE_SUCCESS(rv, rv); - - // Get the new id and GUID. - rv = FetchPageInfo(mDB, mPage); - NS_ENSURE_SUCCESS(rv, rv); - - mIcon.status |= ICON_STATUS_ASSOCIATED; + return NS_OK; } + // Otherwise just associate the icon to the page, if needed. - else if (mPage.iconId != mIcon.id) { + if (mPage.iconId != mIcon.id) { nsCOMPtr stmt; if (mPage.id) { stmt = mDB->GetStatement( diff --git a/toolkit/components/places/mozIAsyncFavicons.idl b/toolkit/components/places/mozIAsyncFavicons.idl index bec46bc575d7..063b82d6dc22 100644 --- a/toolkit/components/places/mozIAsyncFavicons.idl +++ b/toolkit/components/places/mozIAsyncFavicons.idl @@ -60,8 +60,8 @@ interface mozIAsyncFavicons : nsISupports * cache we won't do anything unless aForceReload is true, in which case * we'll try to reload the favicon. * - * This function will only save favicons for "good" URIs, as defined by what - * gets added to history or is a bookmark. For "bad" URIs, this function + * This function will only save favicons for pages that are already stored in + * the database, like visited pages or bookmarks. For any other URIs, it * will succeed but do nothing. This function will also ignore the error * page favicon URI (see FAVICON_ERRORPAGE_URL below). * diff --git a/toolkit/components/places/nsFaviconService.cpp b/toolkit/components/places/nsFaviconService.cpp index b7c9229f387b..faab6ab3d8d3 100644 --- a/toolkit/components/places/nsFaviconService.cpp +++ b/toolkit/components/places/nsFaviconService.cpp @@ -320,8 +320,11 @@ nsFaviconService::SetFaviconUrlForPage(nsIURI* aPageURI, nsIURI* aFaviconURI) // Now, link our icon entry with the page. PRInt64 pageId; nsCAutoString guid; - rv = history->GetOrCreateIdForPage(aPageURI, &pageId, guid); + rv = history->GetIdForPage(aPageURI, &pageId, guid); NS_ENSURE_SUCCESS(rv, rv); + if (!pageId) { + return NS_ERROR_NOT_AVAILABLE; + } nsCOMPtr stmt = mDB->GetStatement( "UPDATE moz_places SET favicon_id = :icon_id WHERE id = :page_id" diff --git a/toolkit/components/places/nsIFaviconService.idl b/toolkit/components/places/nsIFaviconService.idl index d0bf066b2334..8eb0a70f3f77 100644 --- a/toolkit/components/places/nsIFaviconService.idl +++ b/toolkit/components/places/nsIFaviconService.idl @@ -50,9 +50,9 @@ interface nsIFaviconService : nsISupports * * Will create an entry linking the favicon URI to the page, regardless * of whether we have data for that icon. You can populate it later with - * SetFaviconData. However, remember that any favicons not associated with a - * visited web page, a bookmark, or a "place:" URI, will be removed during - * expiration runs. + * SetFaviconData. However, remember that favicons must only be associated + * with a visited web page, a bookmark, or a "place:" URI. Trying to + * associate the icon to any other page will throw. * * This will send out history pageChanged notification if the new favicon has * any data and it's different from the old associated favicon. This means @@ -63,6 +63,7 @@ interface nsIFaviconService : nsISupports * URI of the page whose favicon is being set. * @param aFaviconURI * URI of the favicon to associate with the page. + * @throws NS_ERROR_NOT_AVAILABLE if aPageURI doesn't exist in the database. */ void setFaviconUrlForPage(in nsIURI aPageURI, in nsIURI aFaviconURI); diff --git a/toolkit/components/places/tests/chrome/Makefile.in b/toolkit/components/places/tests/chrome/Makefile.in index d29f83e5f082..1756361fc7cf 100644 --- a/toolkit/components/places/tests/chrome/Makefile.in +++ b/toolkit/components/places/tests/chrome/Makefile.in @@ -61,6 +61,8 @@ _CHROME_FILES = \ test_favicon_annotations.xul \ test_303567.xul \ test_381357.xul \ + test_history_post.xul \ + history_post.sjs \ test_reloadLivemarks.xul \ $(NULL) @@ -69,4 +71,3 @@ libs:: $(_HTTP_FILES) libs:: $(_CHROME_FILES) $(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir) - diff --git a/toolkit/components/places/tests/chrome/history_post.sjs b/toolkit/components/places/tests/chrome/history_post.sjs new file mode 100644 index 000000000000..3c86aad7bc77 --- /dev/null +++ b/toolkit/components/places/tests/chrome/history_post.sjs @@ -0,0 +1,6 @@ +function handleRequest(request, response) +{ + response.setStatusLine("1.0", 200, "OK"); + response.setHeader("Content-Type", "text/plain; charset=utf-8", false); + response.write("Ciao"); +} diff --git a/toolkit/components/places/tests/chrome/test_favicon_annotations.xul b/toolkit/components/places/tests/chrome/test_favicon_annotations.xul index 8db5686bd0d0..bad9a1f9d2c9 100644 --- a/toolkit/components/places/tests/chrome/test_favicon_annotations.xul +++ b/toolkit/components/places/tests/chrome/test_favicon_annotations.xul @@ -135,6 +135,7 @@ function loadNextTest() function test() { + SimpleTest.waitForExplicitFinish(); let db = Cc["@mozilla.org/browser/nav-history-service;1"]. getService(Ci.nsPIPlacesDatabase). DBConnection; @@ -148,19 +149,39 @@ function test() return ios.newURI(aSpec, null, null); }; - // Set the favicon data. Note that the "moz-anno:" protocol requires the - // favicon to be stored in the database, but the replaceFaviconDataFromDataURL - // function will not save the favicon unless it is associated with a page. - // Thus, we must associate the icon with a page explicitly in order for it to - // be visible through the protocol. - fs.replaceFaviconDataFromDataURL(uri(testURIs[1]), expectedURIs[1], - (Date.now() + 60 * 60 * 24 * 1000) * 1000); - fs.setAndFetchFaviconForPage(uri("http://example.com/favicon_annotations"), - uri(testURIs[1]), true); + let pageURI = uri("http://example.com/favicon_annotations"); + let history = Cc["@mozilla.org/browser/history;1"] + .getService(Ci.mozIAsyncHistory); + history.updatePlaces( + { + uri: pageURI, + visits: [{ transitionType: Ci.nsINavHistoryService.TRANSITION_TYPED, + visitDate: Date.now() * 1000 + }], + }, + { + handleError: function UP_handleError() { + ok(false, "Unexpected error in adding visit."); + }, + handleResult: function () {}, + handleCompletion: function UP_handleCompletion() { + // Set the favicon data. Note that the "moz-anno:" protocol requires + // the favicon to be stored in the database, but the + // replaceFaviconDataFromDataURL function will not save the favicon + // unless it is associated with a page. Thus, we must associate the + // icon with a page explicitly in order for it to be visible through + // the protocol. + fs.replaceFaviconDataFromDataURL(uri(testURIs[1]), expectedURIs[1], + (Date.now() + 60 * 60 * 24 * 1000) * 1000); + fs.setAndFetchFaviconForPage(pageURI, uri(testURIs[1]), true); + + // And start our test process. + loadNextTest(); + } + } + }); + - // And start our test process. - loadNextTest(); - SimpleTest.waitForExplicitFinish(); } ]]> diff --git a/toolkit/components/places/tests/chrome/test_history_post.xul b/toolkit/components/places/tests/chrome/test_history_post.xul new file mode 100644 index 000000000000..95c7c0374693 --- /dev/null +++ b/toolkit/components/places/tests/chrome/test_history_post.xul @@ -0,0 +1,67 @@ + + + + + + + + + +