Closed
Bug 737846
Opened 12 years ago
Closed 12 years ago
Ensure favicons can't add unwanted pages to history
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: mak, Assigned: mak)
Details
(Keywords: addon-compat, dev-doc-needed)
Attachments
(1 file, 2 obsolete files)
38.60 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
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•12 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•12 years ago
|
||
And just confirmed this through bugzilla landfill testing.
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #608297 -
Flags: feedback?
Assignee | ||
Updated•12 years ago
|
Attachment #608297 -
Flags: feedback? → feedback?(paolo.mozmail)
Assignee | ||
Comment 4•12 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•12 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•12 years ago
|
||
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•12 years ago
|
Keywords: addon-compat,
dev-doc-needed
Assignee | ||
Comment 7•12 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•12 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•12 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•12 years ago
|
Attachment #608554 -
Attachment is obsolete: true
Attachment #608554 -
Flags: review?(dietrich)
Assignee | ||
Updated•12 years ago
|
Summary: Ensure we don't add POST pages to history → Ensure favicons can't add unwanted pages to history
Assignee | ||
Comment 10•12 years ago
|
||
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 11•12 years ago
|
||
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•12 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•12 years ago
|
||
with comments addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/033512dd3678
Flags: in-testsuite+
Target Milestone: --- → mozilla14
Assignee | ||
Comment 14•12 years ago
|
||
fix a typo in a test (too many braces) https://hg.mozilla.org/integration/mozilla-inbound/rev/08227c5bef9b
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/033512dd3678 https://hg.mozilla.org/mozilla-central/rev/08227c5bef9b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•