739217 - Replace codebase usage of synchronous isVisited with asynchronous isURIVisited

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

we have mozIAsyncHistory::isURIVisited that should be preferred over the synchronous isVisited.  We should investigate replacing calls across the codebase, retaining just a test for isVisited until we stop implementing nsiGlobalHistory2.

Comment 1

[Andres Hernandez [:andreshm]]

I have been looking at this bug. I saw that most of the use of 'isVisited' is on test files. Except for the HistoryEngine module. 

I started updating the tests to use isURIVisited. But while running tests, I found that I'm getting different values with the following code: 

asyncHistory.isURIVisited(place.uri, function(aURI, aIsVisited) {
  do_check_true(aIsVisited); // false
  do_check_true(gGlobalHistory.isVisited(place.uri)); // true
});

Any reason why is not the same value? 
Just to confirm, I should update also when using Places.bhistory, which inherits from nsIGlobalHistory2, right?

Comment 2

[Marco Bonardo [:mak]]

(In reply to Andres Hernandez [:andreshm] from comment #1)
> I have been looking at this bug. I saw that most of the use of 'isVisited'
> is on test files. Except for the HistoryEngine module. 
> 
> I started updating the tests to use isURIVisited. But while running tests, I
> found that I'm getting different values with the following code: 
> 
> asyncHistory.isURIVisited(place.uri, function(aURI, aIsVisited) {
>   do_check_true(aIsVisited); // false
>   do_check_true(gGlobalHistory.isVisited(place.uri)); // true
> });
> 
> Any reason why is not the same value? 

it should be the same, yes, may depend on something else happening in the test itself, like an async clear history not being wait for, or similar. MAy even be a bug, do you have a reduced test showing that?

Comment 3

[Andres Hernandez [:andreshm]]

Attachment: Reduced test

This is the reduced test, as it shows in one case the value is false in the other one is true, and it should be true.

Comment 4

[Marco Bonardo [:mak]]

thanks for the test case, looking into it.

Comment 5

[Marco Bonardo [:mak]]

So, basically the problem is that isURIVisited can happen concurrently to updatePlaces. in this case we are inside handleResult, likely the main database connection has an open transaction yet (it's inserting the visit), while the second database connection is trying to resolve the isURIVisited request.
Due to concurrency the second database connection just queries the previous checkpoint, that doesn't yet contain the latest addition. So it doesn't see the new visit.

I solved the problem by changing the expectHandleResult helper to

function expectHandleResult(handleResultFunc)
{
  return {
    _places: [],
    handleError: function handleError(aResultCode, aPlacesInfo) {
      do_throw("Unexpected error: " + aResultCode);
    },
    handleResult: function(aPlace) {
      this._places.push(aPlace);
    },
    handleCompletion: function() {
      for (let place of this._places) {
        handleResultFunc(place);
      }
    }
  };
}

This makes so that we run the check on handleCompletion, when the transaction has been committed and the checkpoint is the latest.
This may be enough for the test here, but please file a bug specific for this issue, since we may want to delay handleResult calls to avoid the concurrency problem, but I'm not sure how hard that may end up being.

Comment 6

[Andres Hernandez [:andreshm]]

Attachment: Reduced test 2

This is another case, when async 'isURIVisited' returns different than sync 'isVisited'. I think may be related to be in private browsing mode.

Comment 7

[Andres Hernandez [:andreshm]]

Attachment: Current progress

This is the current progress patch. 
There are still 4 calls to 'isVisited', one described in the previous comment #6. The other 3 cases, are very similar. The problem is that these calls, need a lot of refactoring to make the test async. 
I kept the test_isVisited unchanged, as suggested in comment #1.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 657441 [details] [diff] [review]
Reduced test 2

Ah, this is a really good find!

So the fact is that in old PB mode we always return false to isVisited due to if (IsHistoryDisabled()) check at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#3420
We don't do such a check in isURIVisited, considered those visits already exist...

I'm not sure what's the expected behavior regarding PB, nor why we were doing the check originally, it's not that we are adding new visits, we are just returning what was there before PB mode started (And is still there)... Maybe we didn't want to reduce the sense of privacy by using previous history?

Asking feedback to Ehsan to figure the proper behavior.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 657443 [details] [diff] [review]
Current progress

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

Sorry, I have an hard time reviewing this patch, it has too much content...
What I may suggest it to split each single test into its patch, this way we can review and handle common/simple cases and slowly move through the list of the complex cases.
Also, in all the cases where we just do a synchronous addVisit and check for the visit just for sanity, we can avoid that, since addVisit would throw already, we were too picky in the past. In this case where you would just remove the checks, a unified patch may be fine.
Note you can easily split the hg patch into multiple with just some text copy/paste.

Some comments on the first part, that may apply to the whole changes:

::: browser/base/content/test/browser_sanitize-timespans.js
@@ +2,5 @@
>  var now_uSec = Date.now() * 1000;
>  
>  const dm = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
>  const bhist = Cc["@mozilla.org/browser/global-history;2"].getService(Ci.nsIBrowserHistory);
> +var asyncHistory = Cc["@mozilla.org/browser/history;1"].getService(Ci.mozIAsyncHistory);

please just use PlacesUtils.asyncHistory where needed

@@ +377,5 @@
> +          });
> +        });
> +      });
> +    });
> +  });

Hm, we should do something better here, too much bracing.
A first possibility could be to use promises and yield... otherwise just copy the waitForAsyncUpdates (note bug 775495 is going to change that to promiseAsyncUpdates) and do something like
isURIVisited(...)
isURIVisited(...)
isURIVisited(...)
isURIVisited(...)
waitAsyncUpdates(continue_to_next_function)

so there should be less indentation

due to the complexity in changing this test and reviewing changes to it, I suggest moving it to a separate patch.

::: browser/base/content/test/browser_sanitizeDialog.js
@@ +878,5 @@
> +  PlacesUtils.asyncHistory.isURIVisited(pURI, function(aVisitedURI, aIsVisited) {
> +    ok(aIsVisited,
> +      "Sanity check: history visit " + pURI.spec +
> +      " should exist after creating it");
> +  });

actually you can kill this check, if addVisit fails it throws, so no reason to check further
I'd say to remove all the isVisited checks after synchronous addVisit or add_visit, this should largely simplify the patch

::: browser/base/content/test/browser_sanitizeDialog_treeView.js
@@ +478,5 @@
> +  PlacesUtils.asyncHistory.isURIVisited(pURI, function(aVisitedURI, aIsVisited) {
> +    ok(aIsVisited,
> +      "Sanity check: history visit " + pURI.spec +
> +      " should exist after creating it");
> +  });

ditto

::: browser/components/places/tests/browser/browser_410196_paste_into_tags.js
@@ +70,5 @@
>      let visitId = add_visit(testURI1);
>      ok(visitId > 0, "A visit was added to the history");
> +    PlacesUtils.asyncHistory.isURIVisited(testURI1, function(aURI, aIsVisited) {
> +      ok(aIsVisited, MOZURISPEC + " is a visited url.");
> +    });

ditto

::: browser/components/places/tests/browser/browser_bookmarksProperties.js
@@ +429,1 @@
>    },

ditto

::: browser/components/places/tests/browser/browser_library_infoBox.js
@@ +24,4 @@
>      PlacesUtils.history.addVisit(PlacesUtils._uri(TEST_URI), Date.now() * 1000,
>                                   null, PlacesUtils.history.TRANSITION_TYPED,
>                                   false, 0);
> +    PlacesUtils.asyncHistory.isURIVisited(PlacesUtils._uri(TEST_URI), function(aURI, aIsVisited) {

here as well, so you don't need to reindent everything

::: browser/components/places/tests/browser/browser_library_left_pane_commands.js
@@ +21,5 @@
>      // Add a visit.
>      PlacesUtils.history.addVisit(PlacesUtils._uri(TEST_URI), Date.now() * 1000,
>                                   null, PlacesUtils.history.TRANSITION_TYPED,
>                                   false, 0);
> +    PlacesUtils.asyncHistory.isURIVisited(PlacesUtils._uri(TEST_URI), function(aURI, aIsVisited) {

ditto

::: browser/components/places/tests/browser/browser_sidebarpanels_click.js
@@ +56,5 @@
>                                     PlacesUtils.history.TRANSITION_TYPED,
>                                     false, 0);
> +      PlacesUtils.asyncHistory.isURIVisited(uri, function(aURI, aIsVisited) {
> +        ok(aIsVisited, "Item is visited");
> +      });

ditto

::: browser/components/privatebrowsing/test/unit/do_test_removeDataFromDomain.js
@@ +63,3 @@
>                                 Ci.nsINavHistoryService.TRANSITION_LINK, false,
>                                 0);
> +    check_visited(aURI, true, aCallback);

Hm at this point would be really nice to also remove the call to addVisit and instead use updatePlaces... then it would be really async!

@@ +83,5 @@
> +    checker(aIsVisited);
> +    if (aCallback) {
> +      aCallback();
> +    }
> +  });

unfortunately this is not ok...
- the callback should be mandatory
- the tests should be changed to use the common add_test run_next_text, currently they run in a strict synchronous loop. so what you do here is having a check that may run after the test already finished...

::: services/sync/tests/unit/test_corrupt_keys.js
@@ +213,5 @@
> +            });
> +          });
> +        });
> +      });
> +    });

another case we need to do something to limit bracing

Comment 10

[(no longer active)]

(In reply to Marco Bonardo [:mak] from comment #8)
> Comment on attachment 657441 [details] [diff] [review]
> Reduced test 2
> 
> Ah, this is a really good find!
> 
> So the fact is that in old PB mode we always return false to isVisited due
> to if (IsHistoryDisabled()) check at
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> nsNavHistory.cpp#3420
> We don't do such a check in isURIVisited, considered those visits already
> exist...
> 
> I'm not sure what's the expected behavior regarding PB, nor why we were
> doing the check originally, it's not that we are adding new visits, we are
> just returning what was there before PB mode started (And is still there)...
> Maybe we didn't want to reduce the sense of privacy by using previous
> history?

In PB mode, we don't want to mark the links you have visited before as visited.  We have a test which ensures this today: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/mochitest/test_bug_461710.html?force=1

It seems like a mistake for isURIVisited to not honor the IsHistoryDisabled() check.  At the very least it should honor the PB mode.

Comment 11

[(no longer active)]

Comment on attachment 657441 [details] [diff] [review]
Reduced test 2

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

::: toolkit/components/places/tests/unit/test_248970.js
@@ +202,4 @@
>      visited_URIs.forEach(function (visited_uri) {
> +      PlacesUtils.asyncHistory.isURIVisited(uri(visited_uri), function(aURI, aIsVisited) {
> +        do_print("Async: " + aIsVisited + " Sync: " + PlacesUtils.bhistory.isVisited(uri(visited_uri)));
> +        do_check_false(aIsVisited);

You also need to test whether isVisited returns false here.

Comment 12

[Marco Bonardo [:mak]]

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> In PB mode, we don't want to mark the links you have visited before as
> visited.  We have a test which ensures this today:

Strange, I can confirm in PB mode we don't mark as visited, but I can't find the condition check in code, do you remember where it is off-hand? otherwise will just debug it.

> It seems like a mistake for isURIVisited to not honor the
> IsHistoryDisabled() check.  At the very least it should honor the PB mode.

This API is not what we use to mark links (the underlying code it is using is though), it may be used from add-ons though, so basically we expect to lie to those requests as well?

Comment 13

[Josh Matthews [:jdm]]

(In reply to Marco Bonardo [:mak] from comment #12)
> (In reply to Ehsan Akhgari [:ehsan] from comment #10)
> > In PB mode, we don't want to mark the links you have visited before as
> > visited.  We have a test which ensures this today:
> 
> Strange, I can confirm in PB mode we don't mark as visited, but I can't find
> the condition check in code, do you remember where it is off-hand? otherwise
> will just debug it.

I think the patch in bug 722853 shows the code you're interested in.

Comment 14

[Marco Bonardo [:mak]]

(In reply to Josh Matthews [:jdm] from comment #13)
> in bug 722853 shows the code you're interested in.

Ah cool, that is about what I was suspecting (a filter at the style level).
So, basically, this means the current situation is that history just ignores PB, all history APIs say the truth regarding visited/unvisited (clearly regarding past data, new visits can't be added), but link coloring is disabled.
Is this wrong? If so, what's the use-case to have APIs lying?

Comment 15

[Marco Bonardo [:mak]]

We do not notify about visited/unvisited links since bug 722853 basically added a layout filter.

Instead APIs don't lie in PB mode, that means tests will need a fix.

Thank you everyone for help figuring out the new story :)

Comment 16

[Andres Hernandez [:andreshm]]

Attachment: Part 1 - Remove checks after addVisit

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 702529 [details] [diff] [review]
Part 1 - Remove checks after addVisit

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

thanks

Comment 18

[Andres Hernandez [:andreshm]]

Attachment: Part 2 - Docshell changes

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 703002 [details] [diff] [review]
Part 2 - Docshell changes

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

test_bug94514.html depends on Places implementation ("uri-visit-saved" is a Places notification), so it should also be moved (along with its support file bug94514-postpage.html) to
C:\mozilla\mozilla-inbound\toolkit\components\places\tests\mochitest

test_nsIDownloadHistory.js instead should stay as it is, it is testing the base implementation (http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDownloadHistory.cpp)
and when Places will stop implementing nsIGlobalHistory2 this test will just bail out in run_test

Comment 20

[Andres Hernandez [:andreshm]]

Attachment: Part 2 - Docshell changes

Comment 21

[Andres Hernandez [:andreshm]]

Attachment: Part 3 - Services changes

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 703351 [details] [diff] [review]
Part 2 - Docshell changes

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

This file moving is fine, provided the test passes (I didn't try).
Though we need a Part2.5 to also fix the isVisited calls.

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 703353 [details] [diff] [review]
Part 3 - Services changes

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

::: services/sync/modules/engines/history.js
@@ +325,5 @@
>    itemExists: function HistStore_itemExists(id) {
>      return !!this._findURLByGUID(id);
>    },
>  
> +  urlExists: function HistStore_urlExists(url, callback) {

this method is used only by tests... I wonder why it's in the sync code at all... 

Forwarding the review to Richard to evaluate moving it to the test.

::: services/sync/tests/unit/test_corrupt_keys.js
@@ +146,5 @@
>      // And look! We downloaded history!
>      let store = Service.engineManager.get("history")._store;
> +    store.urlExists("http://foo/bar?record-no--0", function(aURLExists) {
> +      do_check_true(aURLExists);
> +    });

we can't do this since this is asynchronous while the test is syunchronous.

I think at least you should change add_test to add_task, and copy the promiseIsURIVisited helper from Raymond's patch (https://bug752218.bugzilla.mozilla.org/attachment.cgi?id=703143).
then here you need to yield.

This provided it's fine to remove urlExists from Sync and just have promiseIsURIVisited in the test.

Comment 24

[Richard Newman [:rnewman]]

(In reply to Marco Bonardo [:mak] from comment #23)
 
> > +  urlExists: function HistStore_urlExists(url, callback) {
> 
> this method is used only by tests... I wonder why it's in the sync code at
> all... 
> 
> Forwarding the review to Richard to evaluate moving it to the test.

I'm going to call it "historical reasons" :)

Yup, move it.

Will attend to rest of review today.

Comment 25

[Richard Newman [:rnewman]]

Comment on attachment 703353 [details] [diff] [review]
Part 3 - Services changes

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

What mak said: kill urlVisited in history.js, and update the test to use PlacesUtils directly, where it can eliminate the null check or use other handy helpers.

Note that the test is at least using add_test/run_next_test, so a little untangling would allow you to chain async calls.

Marking as f- because it seems semantically closest to "different patch, please"! :)

::: services/sync/modules/engines/history.js
@@ +335,5 @@
> +        callback(aIsVisited);
> +      });
> +    } else {
> +      // Don't call isURIVisited on a null URL to work around crasher bug 492442.
> +      callback(false);

Bug 492442 is resolved, and no longer crashes. But it does throw NS_ERROR_ILLEGAL_VALUE.

Comment 26

[Andres Hernandez [:andreshm]]

Attachment: Part 2 - Docshell changes

(In reply to Marco Bonardo [:mak] from comment #22)
> Comment on attachment 703351 [details] [diff] [review]
> Part 2 - Docshell changes
> 
> Review of attachment 703351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This file moving is fine, provided the test passes (I didn't try).
> Though we need a Part2.5 to also fix the isVisited calls.

Yes, sorry, forget to merge the previous patch. Now it's ready and test runs fine locally.

Comment 27

[Andres Hernandez [:andreshm]]

Attachment: Part 3 - Services changes

Updated to use promiseIsURIVisited. Tests are running fine locally.

Comment 28

[Richard Newman [:rnewman]]

Comment on attachment 703580 [details] [diff] [review]
Part 3 - Services changes

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

r+ with comments addressed.

Please land in services-central, whiteboard [fixed in services]. If you don't want to get an s-c checkout, ping me when you've landed and I'll uplift it for you.

Thanks!

::: services/sync/tests/unit/test_corrupt_keys.js
@@ +197,3 @@
>    } finally {
>      Svc.Prefs.resetBranch("");
>      server.stop(run_next_test);

You need to rework this clause if you use add_task.

add_task runs the next test for you, so you just need to stop the server and reset prefs.

@@ +219,5 @@
> + */
> +function promiseIsURIVisited(aURI) {
> +  if (typeof(aURI) == "string") {
> +    aURI = Utils.makeURI(aURI);
> +  }

This is test code, so just define that the input is a string (it always is) and do the appropriate thing.

(Also, you can drop the aFoo notation in services/, and generally the tree as a whole for new code, unless the owner of that particular module really, really wants to stick with it. It's outdated style, and generally our "thought leaders" have moved on :D)

Comment 29

[Marco Bonardo [:mak]]

Comment on attachment 703559 [details] [diff] [review]
Part 2 - Docshell changes

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

thanks

Comment 30

[Andres Hernandez [:andreshm]]

Attachment: Part 3 - Services changes

> Please land in services-central, whiteboard [fixed in services]. If you
> don't want to get an s-c checkout, ping me when you've landed and I'll
> uplift it for you.

I don't have permission to land a patch. But it will be good if we can land all the r+ patches and keep the bug open for the pending work. 
 
> ::: services/sync/tests/unit/test_corrupt_keys.js
> @@ +197,3 @@
> >    } finally {
> >      Svc.Prefs.resetBranch("");
> >      server.stop(run_next_test);
> 
> You need to rework this clause if you use add_task.

I'm not sure what you mean by this. The finally part is running fine after the yields or in case the try catches something.

Comment 31

[Andres Hernandez [:andreshm]]

Attachment: Part 4 - Browser changes (not all)

Updated the browser_sanitize-timespans.js and browser_library_left_pane_commands.js, there are still 3 tests that need more refactoring and changes. I'll post those in a separate patch.

Comment 32

[Richard Newman [:rnewman]]

(In reply to Andres Hernandez [:andreshm] from comment #30)

> I don't have permission to land a patch. But it will be good if we can land
> all the r+ patches and keep the bug open for the pending work. 

Sure, I'll take care of it.


> > >      server.stop(run_next_test);
> > 
> > You need to rework this clause if you use add_task.
> 
> I'm not sure what you mean by this. The finally part is running fine after
> the yields or in case the try catches something.

add_task is defined as "run this function, processing all the yields; when it returns, call run_next_test()".

You don't need to include run_next_test in the finally block, because the test harness will do it for you. Doing it yourself might work, or it might result in tests being interleaved.

You *might* need to yield some version of server.stop, though, because it's async.

Comment 33

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #32)

> You don't need to include run_next_test in the finally block, because the
> test harness will do it for you. Doing it yourself might work, or it might
> result in tests being interleaved.
> 
> You *might* need to yield some version of server.stop, though, because it's
> async.

I made those changes and landed the modified patch in s-c:

https://hg.mozilla.org/services/services-central/rev/8cc32d6fa707

This'll get merged soon, probably today.

Comment 34

[Andres Hernandez [:andreshm]]

Thanks!

Comment 35

[Marco Bonardo [:mak]]

Comment on attachment 703956 [details] [diff] [review]
Part 3 - Services changes

please use the checkin flag when working in bugs with multiple attachments that land at different times, otherwise it's hard to check what landed and what has to land.

Comment 36

[Marco Bonardo [:mak]]

Comment on attachment 703960 [details] [diff] [review]
Part 4 - Browser changes (not all)

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

::: browser/base/content/test/browser_sanitize-timespans.js
@@ +83,2 @@
>  
> +  waitForAsyncUpdates(function() {

there's a problem with this, waitForAsyncUpdates is able to wait for all writers, but I think (though I didn't verify) readers will ignore the exclusive locking cause we use WAL journaling on the database that grants access to readers at any time.

I think a safer and simpler way to achieve the same result is to use Task.spawn, promiseIsURIVisited (Raymond did that in some tests, you may copy his implementation) and yield.

Honestly, this test is badly written (as you may have noticed) and I'm sort of scared to touch it, so let's try to change the flow less as possible.

::: browser/components/places/tests/browser/browser_library_left_pane_commands.js
@@ +55,5 @@
>         "Delete command is enabled");
>  
>      // Execute the delete command and check visit has been removed.
>      PO._places.controller.doCommand("cmd_delete");
> +    let testURI = PlacesUtils._uri(TEST_URI);

please while changing this, use NetUtil.newURI instead of the internal PlacesUtils._uri

@@ +65,2 @@
>  
> +      historyNode.containerOpen = false;

You can keep the historyNode code out of the isURIvisited callback, since the latter doesn't change anything and doesn't need the node (it's just 2 different ways of checking the same thing)

Comment 37

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/8cc32d6fa707

Comment 38

[Andres Hernandez [:andreshm]]

Attachment: Part 5 - Toolkit changes

Comment 39

[Marco Bonardo [:mak]]

Comment on attachment 704698 [details] [diff] [review]
Part 5 - Toolkit changes

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

::: toolkit/components/places/tests/browser/browser_bug248970.js
@@ +166,1 @@
>    });

fwiw, you may check if head.js in this folder has promiseIsURIVisited, eventually add it and use a Task here... not mandatory though, it would just simplify reading this code.

::: toolkit/components/places/tests/head_common.js
@@ +969,5 @@
> +    deferred.resolve(aIsVisited);
> +  });
> +
> +  return deferred.promise;
> +}

we already have it in head_common.js no need to add it again

@@ +988,5 @@
> +    }
> +  });
> +
> +  return deferred.promise;
> +}

this doesn't belong to head_common, please keep it in test_async_history_api.js

::: toolkit/components/places/tests/unit/test_async_history_api.js
@@ +9,5 @@
>  //// Globals
>  
>  XPCOMUtils.defineLazyServiceGetter(this, "gHistory",
>                                     "@mozilla.org/browser/history;1",
>                                     "mozIAsyncHistory");

this can likely be removed now, since you use your promiseUpdatePlaces helper.

@@ +515,5 @@
> +    "FROM moz_historyvisits " +
> +    "WHERE id = :visit_id"
> +  );
> +  stmt.params.visit_id = placeInfo.visits[0].visitId;
> +  do_check_true(yield stmt.executeStep());

this executeStep call is synchronous, no need to yield.

@@ +548,5 @@
> +    "FROM moz_historyvisits " +
> +    "WHERE id = :visit_id"
> +  );
> +  stmt.params.visit_id = placeInfo.visits[0].visitId;
> +  do_check_true(yield stmt.executeStep());

this executeStep call is synchronous, no need to yield.

@@ +585,5 @@
> +    "FROM moz_historyvisits " +
> +    "WHERE id = :visit_id"
> +  );
> +  stmt.params.visit_id = visit.visitId;
> +  do_check_true(yield stmt.executeStep());

this executeStep call is synchronous, no need to yield.

@@ +607,5 @@
>    let stmt = DBConn().createStatement(
>      "SELECT MAX(session) as max_session " +
>      "FROM moz_historyvisits"
>    );
> +  do_check_true(yield stmt.executeStep());

this executeStep call is synchronous, no need to yield.

@@ +631,5 @@
> +  let stmt = DBConn().createStatement(
> +    "SELECT MAX(session) as max_session " +
> +    "FROM moz_historyvisits"
> +  );
> +  do_check_true(yield stmt.executeStep());

this executeStep call is synchronous, no need to yield.

@@ +695,5 @@
> +    "WHERE place_id = (SELECT id FROM moz_places WHERE url = :page_url) " +
> +    "AND from_visit = 0 "
> +  );
> +  stmt.params.page_url = place.uri.spec;
> +  do_check_true(yield stmt.executeStep());

this executeStep call is synchronous, no need to yield.

@@ +896,5 @@
>        "AND v.visit_date = :visit_date "
>      );
>      stmt.params.page_url = uri.spec;
>      stmt.params.visit_date = visit.visitDate;
> +    do_check_true(yield stmt.executeStep());

well, not going to repeat further, please just remove yield from executeStep calls

@@ +1353,5 @@
>    test_add_visit_invalid_transitionType_throws,
> +  test_title_change_notifies,
> +  test_visit_notifies,
> +  test_callbacks_not_supplied,
> +].forEach(add_test);

it's a bit error-prone to mix up add_test and add_task in the same test, if possible I'd prefer always using add_task... if the function is not a generator it will just execute it commonly, so no problem.

In the cases where we have that finisher() helper, I think you could create a promise and return it from the function, and in the finisher() you could resolve the promise

::: toolkit/components/places/tests/unit/test_removeVisitsByTimeframe.js
@@ +48,5 @@
>  
> +      print("asyncHistory.isURIVisited should return true.");
> +      PlacesUtils.asyncHistory.isURIVisited(TEST_URI, function(aURI, aIsVisited) {
> +        do_check_true(aIsVisited);
> +      });

you can't do this, the test has to wait for the result of isURIVisited, otherwise it may be executed after the test finished and even crash in the worst case.

The same comment is valid for the other instances below.
promiseAsyncUpdates is not going to wait for this afaik.

Comment 40

[Andres Hernandez [:andreshm]]

Attachment: Part 5 - Toolkit changes

Applied suggested changes.

Comment 41

[Andres Hernandez [:andreshm]]

Attachment: Part 4 - Browser changes I

Applied suggested changes.

Comment 42

[Andres Hernandez [:andreshm]]

Attachment: Part 6 - Browser changes II (social)

Comment 43

[Andres Hernandez [:andreshm]]

Attachment: Part 7 - Browser changes III (sanitize)

Last patch, with this one all 'isVisited' calls have been replaced.

Comment 44

[Marco Bonardo [:mak]]

note that it's likely bug 820764 will bitrot some of these patches, and this means you'll (unfortunately) have to go through them a second time to fix changes.
I'm sorry about that, most of the tests are adding and checking visits, I hope the breakage will be limited.

Comment 45

[Marco Bonardo [:mak]]

Comment on attachment 705066 [details] [diff] [review]
Part 5 - Toolkit changes

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

looks good, but the browser-chrome test is wrong

::: toolkit/components/places/tests/browser/browser_bug248970.js
@@ +40,5 @@
>    function checkPlaces(aWindow, aIsPrivate, aCallback) {
>      // Updates the place items count
>      placeItemsCount = getPlacesItemsCount(aWindow);
>      // History items should be retrievable by query
> +    checkHistoryItems();

this doesn't work, it's asynchronous now, so here must wait for the result

::: toolkit/components/places/tests/unit/test_async_history_api.js
@@ +965,3 @@
>  
>      // We need to insert all of our visits before we can test conditions.
>      if (++callbackCount != places.length) {

these are no more callbacks, worth changing the var name to resultCount

@@ +1308,5 @@
>        do_log_info("Could not construct URI for '" + url + "'; ignoring");
>      }
>    });
> +
> +  yield PlacesUtils.asyncHistory.updatePlaces(places, {});

no reason to yield, since this doesn't return a promise.

Comment 46

[Marco Bonardo [:mak]]

Comment on attachment 705555 [details] [diff] [review]
Part 6 - Browser changes II (social)

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

::: browser/base/content/test/social/browser_social_mozSocial_API.js
@@ +54,5 @@
> +          Task.spawn(function() {
> +            ok(true, "got panel message");
> +            // Check the panel isn't in our history.
> +            yield ensureSocialUrlNotRemembered(e.data.location);
> +          });

this is wrong, since task.spawn doesn't actually wait for the task to complete.

Unfortunately I don't know these tests well enough to suggest how to refactor this, I'm forwarding the review to Jared.

::: browser/base/content/test/social/head.js
@@ +4,5 @@
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> +  "resource://gre/modules/commonjs/promise/core.js");
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +  "resource://gre/modules/Task.jsm");

just in case, also import PlacesUtils

@@ +23,5 @@
>  }
>  
>  // Check that a specified (string) URL hasn't been "remembered" (ie, is not
>  // in history, will not appear in about:newtab or auto-complete, etc.)
>  function ensureSocialUrlNotRemembered(url) {

s/ensure/promise/

Comment 47

[Marco Bonardo [:mak]]

Comment on attachment 705211 [details] [diff] [review]
Part 4 - Browser changes I

Clearing until bug 820764 lands and this can be unbitrotted, no reason to review twice.

Comment 48

[Marco Bonardo [:mak]]

Comment on attachment 705966 [details] [diff] [review]
Part 7 - Browser changes III (sanitize)

Clearing until bug 820764 lands and this can be unbitrotted, no reason to review twice.

Comment 49

[Andres Hernandez [:andreshm]]

Ok, no problem, I'll wait for bug 820764 to land.

Comment 50

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 705555 [details] [diff] [review]
Part 6 - Browser changes II (social)

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

We can delay the checking of the history until finishCleanup is called, as long as we can still include checks for the message-panel and chat-window URLs.

Comment 51

[Andres Hernandez [:andreshm]]

Attachment: Part 1 - Remove checks after addVisit

Rebased patch.

Comment 52

[Andres Hernandez [:andreshm]]

Attachment: Part 4 - Browser changes I

Rebased patch.

Comment 53

[Andres Hernandez [:andreshm]]

Attachment: Part 5 - Toolkit changes

Rebased patch and fixed suggestions.

Comment 54

[Andres Hernandez [:andreshm]]

Attachment: Part 6 - Browser changes II (social)

Rebased patch and applied suggestions.

Comment 55

[Andres Hernandez [:andreshm]]

Attachment: Part 7 - Browser changes III (sanitize)

Rebased patch.

Comment 56

[Marco Bonardo [:mak]]

Comment on attachment 707139 [details] [diff] [review]
Part 4 - Browser changes I

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

this one looks great

::: browser/base/content/test/browser_sanitize-timespans.js
@@ +48,5 @@
>    s.range = null;
> +
> +  ok(!(yield promiseIsURIVisited(makeURI("http://10minutes.com"))),
> +     "Pretend visit to 10minutes.com should now be deleted");
> +  ok((yield promiseIsURIVisited(makeURI("http://1hour.com"))),

nit: while the parenthesis look useful in the !yield case, in these positive ok(yield, ) cases they may be omitted.

@@ +64,3 @@
>    if (minutesSinceMidnight > 10)
> +    ok((yield promiseIsURIVisited(makeURI("http://today.com"))),
> +       "Pretend visit to today.com should still exist");

please brace ifs when they change from oneline to more

(not going to repeat for other instances)

Comment 57

[Marco Bonardo [:mak]]

Comment on attachment 707234 [details] [diff] [review]
Part 5 - Toolkit changes

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

this doesn't look like rebased properly on current code, since test_248970.js is no more in the tree (not sure what happened here), but apart that (should be an easy fix) it's fine.

::: toolkit/components/places/tests/browser/browser_bug248970.js
@@ +144,5 @@
> +function checkHistoryItems() {
> +  for (let i = 0; i < visitedURIs.length; i++) {
> +    let visitedUri = visitedURIs[i];
> +    yield promiseIsURIVisited(NetUtil.newURI(visitedUri), true).then(function() {
> +      if (/embed/.test(visitedUri)) {

this looks strange, I suppose you want to check the result of the yield and not use then?

::: toolkit/components/places/tests/unit/test_248970.js
@@ +171,5 @@
>    myBookmarks[0] = create_bookmark(bookmark_A_URI,"title 1", "google");
>  
>    // History items should be retrievable by query
>    visited_URIs.forEach(function (visited_uri) {
> +    do_check_true(yield promiseIsURIVisited(uri(visited_uri)));

looks like this test has been converted to a browser-chrome test and should not be here anymore... could you please check your repository?
maybe you did a rebase that is adding back tests?

::: toolkit/components/places/tests/unit/test_425563.js
@@ +42,5 @@
>    ]);
>  
>    // check that all links are marked as visited
>    count_visited_URIs.forEach(function (visited_uri) {
> +    do_check_eq((yield promiseIsURIVisited(uri(visited_uri))), true);

do_check_true

@@ +47,3 @@
>    });
>    notcount_visited_URIs.forEach(function (visited_uri) {
> +    do_check_eq((yield promiseIsURIVisited(uri(visited_uri))), true);

do_check_true

Comment 58

[Marco Bonardo [:mak]]

Comment on attachment 707255 [details] [diff] [review]
Part 6 - Browser changes II (social)

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

looks god, but want sign-off by Jared.

Comment 59

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 707255 [details] [diff] [review]
Part 6 - Browser changes II (social)

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

r+ with the following changes.

::: browser/base/content/test/social/browser_social_chatwindow.js
@@ -190,5 @@
>            while (chats.selectedChat) {
>              chats.selectedChat.close();
>            }
>            ok(!chats.selectedChat, "chats are all closed");
> -          ensureSocialUrlNotRemembered(chatUrl);

gURLsNotRemembered.append(chatUrl);

::: browser/base/content/test/social/browser_social_mozSocial_API.js
@@ -52,5 @@
>            break;
>          case "got-panel-message":
>            ok(true, "got panel message");
> -          // Check the panel isn't in our history.
> -          ensureSocialUrlNotRemembered(e.data.location);

gURLsNotRemembered.append(e.data.location);

::: browser/base/content/test/social/head.js
@@ +37,3 @@
>  }
>  
>  function runSocialTestWithProvider(manifest, callback) {

Above this function, have:
let gURLsNotRemembered = [];

@@ +43,5 @@
>  
> +  const chatURL =
> +    "https://example.com/browser/browser/base/content/test/social/social_chat.html";
> +  const panelURL =
> +    "https://example.com/browser/browser/base/content/test/social/social_panel.html";

Hardcoding these URLs isn't that good because if the actual URL that gets used is different than these we won't know that there is a leak.

@@ +56,5 @@
>          }
> +      };
> +    }
> +    yield promiseSocialUrlNotRemembered(chatURL);
> +    yield promiseSocialUrlNotRemembered(panelURL);

for (let url of gURLsNotRemembered)
  yield promistSocialUrlNotRemembered(url);
gURLsNotRemembered = [];

Comment 60

[Andres Hernandez [:andreshm]]

Attachment: Part 4 - Browser changes I

(In reply to Marco Bonardo [:mak] from comment #56)
> Comment on attachment 707139 [details] [diff] [review]
> Part 4 - Browser changes I
> 
> Review of attachment 707139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this one looks great
> 
> ::: browser/base/content/test/browser_sanitize-timespans.js
> @@ +48,5 @@
> >    s.range = null;
> > +
> > +  ok(!(yield promiseIsURIVisited(makeURI("http://10minutes.com"))),
> > +     "Pretend visit to 10minutes.com should now be deleted");
> > +  ok((yield promiseIsURIVisited(makeURI("http://1hour.com"))),
> 
> nit: while the parenthesis look useful in the !yield case, in these positive
> ok(yield, ) cases they may be omitted.
> 

There is syntax error when removing the parenthesis: 
SyntaxError: yield expression must be parenthesized

Comment 61

[Andres Hernandez [:andreshm]]

Attachment: Part 5 - Toolkit changes

Fixed suggestions.

Comment 62

[Marco Bonardo [:mak]]

(In reply to Andres Hernandez [:andreshm] from comment #60)
> There is syntax error when removing the parenthesis: 
> SyntaxError: yield expression must be parenthesized

ah, ok, nevermind then.

Comment 63

[Andres Hernandez [:andreshm]]

Attachment: Part 6 - Browser changes II (social)

Applied suggestions.

Comment 64

[Andres Hernandez [:andreshm]]

Please apply patches in order.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a39e48ab00a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fe0fa1d1b9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebc59bd1a516
https://hg.mozilla.org/integration/mozilla-inbound/rev/c45ef9f35b4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2524d2ba315a
https://hg.mozilla.org/integration/mozilla-inbound/rev/743071f62e67

Comment 66

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-1 orange. Please run these through Try before requesting checkin again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbcff2d6edc8

https://tbpl.mozilla.org/php/getParsedLog.php?id=19337768&tree=Mozilla-Inbound

1136 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_bug787619.html | uncaught exception - ReferenceError: XPCOMUtils is not defined at http://mochi.test:8888/tests/browser/base/content/test/head.js:1

Comment 67

[Andres Hernandez [:andreshm]]

Added fixes and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=745c3a6d728a

Comment 68

[Andres Hernandez [:andreshm]]

Attachment: Part 2 - Docshell changes

Rebased patch

Comment 69

[Andres Hernandez [:andreshm]]

Attachment: Part 4 - Browser changes I

Updated patch.

Comment 70

[Andres Hernandez [:andreshm]]

Attachment: Part 6 - Browser changes II (social)

Updated patch.

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Would have been good if your Try push had included the suite that was failing previously.

Comment 72

[Andres Hernandez [:andreshm]]

(In reply to Ryan VanderMeulen from comment #71)
> Would have been good if your Try push had included the suite that was
> failing previously.

Do you mean mochitest-1 instead of mochitest-bc?

Comment 73

[Andres Hernandez [:andreshm]]

All good on try https://tbpl.mozilla.org/?tree=Try&rev=745c3a6d728a, let me know if you want another run on mochistest-1.

Comment 74

[Marco Bonardo [:mak]]

Andres, please run _all_ mochitests on try.

Comment 75

[Andres Hernandez [:andreshm]]

Attachment: Part 5 - Toolkit changes

Rebased patch.

Comment 76

[Andres Hernandez [:andreshm]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=74553ac30155

Comment 77

[Andres Hernandez [:andreshm]]

Attachment: Part 4 - Browser changes I

Rebased and fixed patch

Comment 78

[Andres Hernandez [:andreshm]]

Attachment: Part 7 - Browser changes III (sanitize)

Rebased patch.

Comment 79

[Andres Hernandez [:andreshm]]

Pushed to try fixes: https://tbpl.mozilla.org/?tree=Try&rev=aa698f0ba734

Comment 80

[Andres Hernandez [:andreshm]]

Attachment: Part 4 - Browser changes I

Removed dumps.

Comment 81

[Andres Hernandez [:andreshm]]

Attachment: Part 6 - Browser changes II (social)

Fixed patch.

Comment 82

[Andres Hernandez [:andreshm]]

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=723a3f45324d

Please apply in order.

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9020fabf64c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c82e4ddc1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/432bebf28ec2
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5bbe934f7d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/e79d18652d56
https://hg.mozilla.org/integration/mozilla-inbound/rev/d21ebe0febbe

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9020fabf64c2
https://hg.mozilla.org/mozilla-central/rev/d8c82e4ddc1e
https://hg.mozilla.org/mozilla-central/rev/432bebf28ec2
https://hg.mozilla.org/mozilla-central/rev/b5bbe934f7d7
https://hg.mozilla.org/mozilla-central/rev/e79d18652d56
https://hg.mozilla.org/mozilla-central/rev/d21ebe0febbe