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.

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?

(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?

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.

thanks for the test case, looking into it.

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.

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

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 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 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

(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 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.

(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?

(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.

(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?

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 on attachment 702529 [details] [diff] [review] Part 1 - Remove checks after addVisit Review of attachment 702529 [details] [diff] [review]: ----------------------------------------------------------------- thanks

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 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 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.

(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 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.

(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.

Updated to use promiseIsURIVisited. Tests are running fine locally.

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 on attachment 703559 [details] [diff] [review] Part 2 - Docshell changes Review of attachment 703559 [details] [diff] [review]: ----------------------------------------------------------------- thanks

> 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.

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.

(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.

(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.

Thanks!

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 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 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.

Applied suggested changes.

Applied suggested changes.

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

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 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 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 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 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.

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

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.

Rebased patch.

Rebased patch.

Rebased patch and fixed suggestions.

Rebased patch and applied suggestions.

Rebased patch.

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 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 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 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 = [];

(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

Fixed suggestions.

(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.

Applied suggestions.

Please apply patches in order.

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

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

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

Rebased patch

Updated patch.

Updated patch.

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

(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?

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

Andres, please run _all_ mochitests on try.

Rebased patch.

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

Rebased and fixed patch

Rebased patch.

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

Removed dumps.

Fixed patch.

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

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

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