628805 - Add tests for invalid referrerURI and invalid sessionId to test_async_history_api.js

Status: RESOLVED FIXED

Description

[:Margaret Leibovic]

Created attachment 506906 [details] [diff] [review]
patch

See bug 606966 comment 199.

Comment 1

[Shawn Wilsher :sdwilsh]

To be clear, these tests pass as-is with no code changes needed?

Comment 2

[:Margaret Leibovic]

Yes, these tests pass right now.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 506906 [details] [diff] [review]
patch

>+function test_add_visit_invalid_referrerURI()
Let's go with test_add_visit_invalid_referrerURI_ignored

>+  place.visits[0].referrerURI = NetUtil.newURI(TEST_DOMAIN +
>+                                               "test_add_visit_invalid_referrerURI_unvisistedURI");
You could save some sanity on this line length here by just doing NetUtil.newURI(place.uri.spec + "_unvisitedURI")

Also, you should add a check to make sure that place.visits[0].referrerURI is not visited.

>+  gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) {
>+    do_check_true(Components.isSuccessCode(aResultCode));
>+    let visit = aPlaceInfo.visits[0];
>+    // Check to make sure we do not insert an unvisited URI into the database.
>+    do_check_false(gGlobalHistory.isVisited(visit.referrerURI));
You should also check that the visit we just added in moz_historyvisits has a null from_visit.  This will require you to roll your own query.

>+  run_next_test();
You want this in the updatePlaces callback, otherwise more than one test will be running at a time.

>+function test_add_visit_invalid_sessionId()
Let's go with test_add_visit_invalid_sessionId_ignored

>+  let place = {
>+    uri: NetUtil.newURI(TEST_DOMAIN +
>+                        "test_add_visit_invalidSessionId"),
nit: uri doesn't match test name.  We could actually just use gRunningTest.name in most of our tests...

>+  place.visits[0].sessionId = -1;
You should also check for the case where a sessionId is not in the database.  You'll have to query the database to get the highest value, then add something like ten to it.

>+  gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) {
>+    do_check_true(Components.isSuccessCode(aResultCode));
>+    let visit = aPlaceInfo.visits[0];
We should make sure that the visit was added in the database though!

>+    // Check to make sure we do not persist bogus sessionId.
>+    do_check_neq(visit.sessionId, -1);
>+  });
>+
>+  run_next_test();
ditto about where this runs

>   test_add_visit_no_date_throws,
>   test_add_visit_no_transitionType_throws,
>   test_add_visit_invalid_transitionType_throws,
>+  test_add_visit_invalid_referrerURI,
>+  test_add_visit_invalid_sessionId,
>   test_non_addable_uri_errors,
Both of the tests you add don't throw, nor do they raise error events, so lets move it after the *_errors tests.

This is r+ with comments addressed, but I want to see the changes in case I missed something.

Comment 4

[Shawn Wilsher :sdwilsh]

We should also add a test about having a non-nsIURI object on the referrer, and it should be ignored as well.  We may have talked about that on irc, but I can't recall offhand.

Comment 5

[:Margaret Leibovic]

Created attachment 507696 [details] [diff] [review]
invalid_referrerURI, nonnsIURI_referrerURI, invalid_sessionId

Addressed review comments and added a test to address comment 4. These tests all pass.

Comment 6

[:Margaret Leibovic]

Created attachment 507699 [details] [diff] [review]
unstored_sessionId test

This test does not currently pass because we are storing the bogus sessionId in the database.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 507696 [details] [diff] [review]
invalid_referrerURI, nonnsIURI_referrerURI, invalid_sessionId

>+++ b/toolkit/components/places/tests/unit/test_async_history_api.js
>+function test_add_visit_invalid_referrerURI_ignored()
I know I suggested it, but mmmulani came up with a better name over in bug 628865.  Just drop the add_visit_ part and we'll be golden :)

>+  do_check_false(gGlobalHistory.isVisited(place.visits[0].referrerURI));
Also check that place.uri is not visited.

>+    // Check to make sure we do not insert an unvisited URI into the database.
I think what you mean to say is something about making sure we don't visit the referrer.

>+    // Check to make sure visit has a null from_visit in database.
We are actually checking that it is zero ;)

>+function test_add_visit_nonnsIURI_referrerURI_ignored()
ditto about dropping add_viist_, although you could combine this test into the one above it.

>+  let place = {
>+    uri: NetUtil.newURI(TEST_DOMAIN +
>+                        "test_add_visit_nonnsIURI_referrerURI_ignored"),
>+    visits: [
>+      new VisitInfo(),
>+    ],
>+  };
>+  place.visits[0].referrerURI = "NON_NSIURI";
let's make it an actually uri looking thing though please
Also, check that place.uri is not visited

>+    // Check to make sure visit has a null from_visit in database.
checking for zero, not null

>+function test_add_visit_invalid_sessionId_ignored()
>+{
>+  let place = {
>+    uri: NetUtil.newURI(TEST_DOMAIN +
>+                        "test_add_visit_invalid_sessionId_ignored"),
>+    visits: [
>+      new VisitInfo(),
>+    ],
>+  };
>+  place.visits[0].sessionId = -1;
check that place.uri is not visited

>+  gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) {
>+    do_check_true(Components.isSuccessCode(aResultCode));
>+    let uri = aPlaceInfo.uri;
>+    do_check_true(gGlobalHistory.isVisited(uri));
>+
>+    // Check to make sure we do not persist bogus sessionId with the visit.
>+    let visit = aPlaceInfo.visits[0];
>+    do_check_neq(visit.sessionId, -1);
Compare against place.visits[0].sessionId please

Also, check the database to make sure it's not -1 please

>   test_non_addable_uri_errors,
>+  test_add_visit_invalid_referrerURI_ignored,
>+  test_add_visit_nonnsIURI_referrerURI_ignored,
>+  test_add_visit_invalid_sessionId_ignored,
>   test_observer_topic_dispatched_when_complete,
Where you added these here is not the same place as where you added them with the actual test methods.  Keep the order the same please :)

r=sdwilsh

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 507699 [details] [diff] [review]
unstored_sessionId test

I'm going to have you patch this.  The code you need to change is here:
https://hg.mozilla.org/mozilla-central/annotate/1c2d53a2dcfb/toolkit/components/places/src/History.cpp#l746

In the if, add an or statement that does something like this:
(i && mPlaces[i].sessionId < mPlaces[0].sessionId)

That should make your test pass (and you may want to update the comment too).  I'll review this once you make that change.

Comment 9

[:Margaret Leibovic]

Created attachment 509194 [details] [diff] [review]
invalid_referrerURI, nonnsIURI_referrerURI, invalid_sessionId (updated for check-in)

Comment 10

[Mehdi Mulani [:mmm] (I don't check this)]

sorry, edited wrong bug.

Comment 11

[:Margaret Leibovic]

Created attachment 509873 [details] [diff] [review]
updated unstored_sessionId test w/ fix to make it pass

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 509873 [details] [diff] [review]
updated unstored_sessionId test w/ fix to make it pass

>+++ b/toolkit/components/places/src/History.cpp
>+      // If we are inserting a place into an empty mPlaces array, we need to
>+      // check to make sure we do not store a bogus session id that is higher
>+      // than the current maximum session id.
>+      if (i == 0) {
>+        PRInt64 newSessionId = navHistory->GetNewSessionID();
>+        if (mPlaces[0].sessionId > newSessionId)
>+          mPlaces[0].sessionId = newSessionId;
>+      }
nit: brace one line ifs

>       // Speculatively get a new session id for our visit if the current session
>       // id is non-valid.  While it is true that we will use the session id from
>       // the referrer if the visit was "recent" enough, we cannot call this
>       // method off of the main thread, so we have to consume an id now.
this comment needs to be updated too

>-      if (mPlaces[i].sessionId <= 0) {
>+      if (mPlaces[i].sessionId <= 0 ||
>+          (i > 0 && mPlaces[i].sessionId > mPlaces[0].sessionId)) {
I think we actually want >= here (equal to the new value should be ignored, and we should test for it instead of +10)

r=sdwilsh

Comment 13

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/175623f0113a
http://hg.mozilla.org/mozilla-central/rev/ef4d491f9ba0