Ensure favicons can't add unwanted pages to history

RESOLVED FIXED in mozilla14

Status

()

Toolkit
Places
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({addon-compat, dev-doc-needed})

Trunk
mozilla14
addon-compat, dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
And just confirmed this through bugzilla landfill testing.
(Assignee)

Comment 3

5 years ago
Created attachment 608297 [details] [diff] [review]
patch v1.0
Attachment #608297 - Flags: feedback?
(Assignee)

Updated

5 years ago
Attachment #608297 - Flags: feedback? → feedback?(paolo.mozmail)
(Assignee)

Comment 4

5 years ago
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 5

5 years ago
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)
(Assignee)

Comment 6

5 years ago
Created attachment 608554 [details] [diff] [review]
patch v1.1

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)
(Assignee)

Updated

5 years ago
Keywords: addon-compat, dev-doc-needed
(Assignee)

Comment 7

5 years ago
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.

Comment 8

5 years ago
(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).
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #608554 - Attachment is obsolete: true
Attachment #608554 - Flags: review?(dietrich)
(Assignee)

Updated

5 years ago
Summary: Ensure we don't add POST pages to history → Ensure favicons can't add unwanted pages to history
(Assignee)

Comment 10

5 years ago
Created attachment 608988 [details] [diff] [review]
patch v1.2

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+
(Assignee)

Comment 12

5 years ago
(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
(Assignee)

Comment 13

5 years ago
with comments addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/033512dd3678
Flags: in-testsuite+
Target Milestone: --- → mozilla14
(Assignee)

Comment 14

5 years ago
fix a typo in a test (too many braces)
https://hg.mozilla.org/integration/mozilla-inbound/rev/08227c5bef9b
https://hg.mozilla.org/mozilla-central/rev/033512dd3678
https://hg.mozilla.org/mozilla-central/rev/08227c5bef9b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.