Closed Bug 737846 Opened 12 years ago Closed 12 years ago

Ensure favicons can't add unwanted pages to history

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: mak, Assigned: mak)

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

This is explicitly excluded in the docshell, though just trying to submit a bug to bugzilla, I see the POST submit_bug.cgi page in history.  Should not be there.
So, the docshell part looks sane and working.  I think the problem relies in the favicons part that doesn't make any check.
So if the POST page has a favicon, we end up adding the page regardless.

We actually shouldn't allow the favicons service to add new pages, this was done in the past to accomplish some complex tasks (that I can't find anymore in the codebase), and for mixed APIs (like adding a favicon synchronously and a visit asynchronously).  I don't think it's worth supporting the latter case anymore, now that we have decent async APIs for both and we are deprecating the synchronous favicons APIs.  In case the page doesn't exist the async API will just silently fail, the sync one may probably throw.
And just confirmed this through bugzilla landfill testing.
Attached patch patch v1.0 (obsolete) — Splinter Review
Attachment #608297 - Flags: feedback?
Attachment #608297 - Flags: feedback? → feedback?(paolo.mozmail)
note the new chrome test is not really testing this, it's more generic, but I had already written it so it was a pity to throw it away.
Comment on attachment 608297 [details] [diff] [review]
patch v1.0

Review of attachment 608297 [details] [diff] [review]:
-----------------------------------------------------------------

Another step towards for code simplification. Great!

This is an API change, don't forget the bug keywords ;-)

::: toolkit/components/places/tests/favicons/test_expireAllFavicons.js
@@ +53,5 @@
> +        uri: TEST_PAGE_URI,
> +        visits: [{ transitionType: Ci.nsINavHistoryService.TRANSITION_TYPED,
> +                   visitDate: Date.now() * 1000
> +                 }]
> +      });

Since we have this pattern of adding a single visit in several places now, I think it's worth making it a head.js function.

Also, here we're not waiting on the updatePlaces callback (because the next call's current implementation waits for the visit addition to be completed on the asynchronous thread), however I think it's more correct to wait, both for future robustness, and also because tests are used as examples for writing other code.
Attachment #608297 - Flags: feedback?(paolo.mozmail)
Attached patch patch v1.1 (obsolete) — Splinter Review
addressed paolo's comments.
The test changes are just adding visits where now are needed, there's some indentation change due to callbacks.
Attachment #608297 - Attachment is obsolete: true
Attachment #608554 - Flags: review?(dietrich)
hm, I just recalled we have an addvisits util in inline tests, so it's worth to merge those 2 into a single addVisits for code reuse.
(In reply to Marco Bonardo [:mak] from comment #7)
> hm, I just recalled we have an addvisits util in inline tests, so it's worth
> to merge those 2 into a single addVisits for code reuse.

If you're still working on this bug, I'd suggest adding also a more specific
helper function that takes only a callback and a page URI, and adds a single
TRANSITION_TYPED visit with the current date to the page. This is the most
common case we have in these tests at present, and I don't see great value
in making it explicit before each call when we're testing something else
(the type and date of the visit is often irrelevant, and one can see the new
helper function's documentation, after all, if interested in the details).
(In reply to Paolo Amadini [:paolo] from comment #8)
> If you're still working on this bug, I'd suggest adding also a more specific
> helper function that takes only a callback and a page URI.

I'll make the util generic enough to cover these case.
Attachment #608554 - Attachment is obsolete: true
Attachment #608554 - Flags: review?(dietrich)
Summary: Ensure we don't add POST pages to history → Ensure favicons can't add unwanted pages to history
Attached patch patch v1.2Splinter Review
Ok, looks good enough to me. I make LINK the default transition since it's more common across the codebase, though adding a typed visit is not really much harder ({ uri: some_uri, transition: TRANSITION_TYPED })
Attachment #608988 - Flags: review?(dietrich)
Comment on attachment 608988 [details] [diff] [review]
patch v1.2

Review of attachment 608988 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, just a few questions below.

::: toolkit/components/places/nsIFaviconService.idl
@@ +50,5 @@
>     *
>     * 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 favicons must only be associated
> +   * with a visited web page, a bookmark, or a "place:" URI, trying to associate

s/URI,/URI./

::: toolkit/components/places/tests/chrome/test_history_post.xul
@@ +51,5 @@
> +        });
> +      });
> +    }
> +
> +    function waitForAsyncUpdates(aCallback, aScope, aArguments)

what's this for and where is it called?

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage.js
@@ -27,5 @@
>    let pageURI = NetUtil.newURI("http://example.com/normal");
>    waitForFaviconChanged(pageURI, FAVICON_URI,
>                          function test_normal_callback() {
> -    do_check_true(isUrlHidden(pageURI));
> -    do_check_eq(frecencyForUrl(pageURI), 0);

why removing these?

::: toolkit/components/places/tests/head_common.js
@@ +877,5 @@
> + * @param aPlaceInfo
> + *        Can be an nsIURI, in such a case a single LINK visit will be added.
> + *        Otherwise can be an object describing the visit to add, or an array
> + *        of these objects.  The object should be in the form:
> + *          { uri, transition, [optional] title, [optional] visitDate }

for clarity, please show a proper example here, and describe each type (eg: what is transition? is visitData millisecond or micro?)

transition is also optional right?
Attachment #608988 - Flags: review?(dietrich) → review+
(In reply to Dietrich Ayala (:dietrich) from comment #11)
> ::: toolkit/components/places/tests/chrome/test_history_post.xul
> @@ +51,5 @@
> > +        });
> > +      });
> > +    }
> > +
> > +    function waitForAsyncUpdates(aCallback, aScope, aArguments)
> 
> what's this for and where is it called?

oops I used it in a previous iteration, will remove.

> :::
> toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage.js
> @@ -27,5 @@
> >    let pageURI = NetUtil.newURI("http://example.com/normal");
> >    waitForFaviconChanged(pageURI, FAVICON_URI,
> >                          function test_normal_callback() {
> > -    do_check_true(isUrlHidden(pageURI));
> > -    do_check_eq(frecencyForUrl(pageURI), 0);
> 
> why removing these?

They are no more true, were checking how was set a page created by the favicon service (hidden and with 0 frecency), but since we don't allow them anymore, these are no more useful.

> ::: toolkit/components/places/tests/head_common.js
> @@ +877,5 @@
> > + * @param aPlaceInfo
> > + *        Can be an nsIURI, in such a case a single LINK visit will be added.
> > + *        Otherwise can be an object describing the visit to add, or an array
> > + *        of these objects.  The object should be in the form:
> > + *          { uri, transition, [optional] title, [optional] visitDate }
> 
> for clarity, please show a proper example here, and describe each type (eg:
> what is transition? is visitData millisecond or micro?)
> 
> transition is also optional right?

no, either pass in URI or pass in { URI, transition } at minimum
with comments addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/033512dd3678
Flags: in-testsuite+
Target Milestone: --- → mozilla14
fix a typo in a test (too many braces)
https://hg.mozilla.org/integration/mozilla-inbound/rev/08227c5bef9b
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: