454977 - remove INSERT OR REPLACE to avoid error prone paths with views

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

This needs some investigation, i thought to this while looking at fsync triggers with Shawn.

morkImporter uses history->AddPageWithVisit
that calls
InternalAddNewPage
that executes the query
mDBAddNewPage

      "INSERT OR REPLACE INTO moz_places "
        "(url, title, rev_host, hidden, typed, frecency) "
      "VALUES (?1, ?2, ?3, ?4, ?5, ?6)"),

now since INSERT OR REPLACE is not able to retain the old id (and i hope places guys will fix it), if the url has alread been added to the db we generate a new place_id, so previously added visits will be unlinked and floating around till expiration/clearhistory.
In other places where we use internalAddVisit we check for url existance before.

So we need to hack it doing:

 "INSERT OR REPLACE INTO moz_places "
        "(id, url, title, rev_host, hidden, typed, frecency) "
      "VALUES ((SELECT id FROM moz_places WHERE url = ?1),?1, ?2, ?3, ?4, ?5, ?6)"),

that will ensure that if url exists we retain the id, if not exists will generate a new one since the query will return null, this hack about is the same that allows sync trigger to work correctly and not generate wrong ids.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

we should also try to not overwrite visit_count and typed

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

we could simply fix the query, but since the sql query will become "smart" we could cleanup AddVisit so that it simply needs to know if the page exists, then insert. With testcase.

If you think this is too much we can still fix only the query, testing will not be easy since it would involve the use of morkImporter (AddPageWithVisit is not available in the interface)

Comment 3

[Dietrich Ayala (:dietrich)]

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

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
>--- a/toolkit/components/places/src/nsNavHistory.cpp
>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -1044,20 +1044,34 @@ nsNavHistory::InitStatements()
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>       "UPDATE moz_places "
>       "SET hidden = ?2, typed = ?3 "
>       "WHERE id = ?1"),
>     getter_AddRefs(mDBUpdatePageVisitStats));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // mDBAddNewPage (see InternalAddNewPage)
>+  // Since we are using INSERT OR REPLACE due to the unique url constraint,
>+  // we should try to hold the correct place id, since this statement would
>+  // generate a new one on conflict. For the same reason we should also retain
>+  // old attributes, or update them correctly.
>+  // Note, we want to unhide any hidden pages that the user explicitly types
>+  // (aTransitionType == TRANSITION_TYPED) so that they will appear in
>+  // the history UI (sidebar, history menu, url bar autocomplete, etc)
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>       "INSERT OR REPLACE INTO moz_places "
>-      "(url, title, rev_host, hidden, typed, frecency) "
>-      "VALUES (?1, ?2, ?3, ?4, ?5, ?6)"),
>+        "(id, url, title, rev_host, hidden, typed, frecency, visit_count) "
>+      "VALUES ((SELECT id FROM moz_places WHERE url = ?1), "
>+              "?1, ?2, ?3, "
>+              "COALESCE((SELECT 0 WHERE ?5 = 1), " // typed = 1 => hidden = 0
>+                       "(SELECT hidden FROM moz_places WHERE url = ?1), "
>+                       "?4), "
>+              "MAX(IFNULL((SELECT typed FROM moz_places WHERE url = ?1), 0), ?5), "
>+              "?6, "
>+              "IFNULL((SELECT visit_count FROM moz_places WHERE url = ?1), 0))"),
>     getter_AddRefs(mDBAddNewPage));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // mDBVisitToURLResult, should match kGetInfoIndex_* (see GetQueryResults)
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>       "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
>         SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" )
>         ", f.url, null, null "
>@@ -2492,109 +2506,46 @@ nsNavHistory::AddVisit(nsIURI* aURI, PRT
>   PRBool canAdd = PR_FALSE;
>   nsresult rv = CanAddURI(aURI, &canAdd);
>   NS_ENSURE_SUCCESS(rv, rv);
>   if (!canAdd) {
>     *aVisitID = 0;
>     return NS_OK;
>   }
> 
>-  // This will prevent corruption since we have to do a two-phase add.
>   // Generally this won't do anything because AddURI has its own transaction.
>   mozStorageTransaction transaction(mDBConn, PR_FALSE);
> 
>-  // see if this is an update (revisit) or a new page
>-  mozStorageStatementScoper scoper(mDBGetPageVisitStats);
>-  rv = BindStatementURI(mDBGetPageVisitStats, 0, aURI);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-  PRBool alreadyVisited = PR_FALSE;
>-  rv = mDBGetPageVisitStats->ExecuteStep(&alreadyVisited);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>+  PRBool newItem = PR_FALSE; // used to send out notifications at the end
>+  {
>+    // see if this page already exists
>+    PRBool itemExists = PR_FALSE;
>+    mozStorageStatementScoper scoper(mDBGetPageVisitStats);
>+    rv = BindStatementURI(mDBGetPageVisitStats, 0, aURI);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    PRBool alreadyVisited = PR_FALSE;

huh?

>+    rv = mDBGetPageVisitStats->ExecuteStep(&itemExists);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    newItem = !itemExists;
>+  }
>+
>+  // Note that we test the redirect flag and not for the redirect transition
>+  // type. The transition type refers to how we got here, and whether a page
>+  // is shown does not depend on whether you got to it through a redirect.
>+  // Rather, we want to hide pages that do not themselves redirect somewhere
>+  // else, which is what the redirect flag means.
>   PRInt64 pageID = 0;
>-  PRBool hidden; // XXX fix me, this should not be a PRBool, as we later do BindInt32Parameter()
>-  PRBool typed;  // XXX fix me, this should not be a PRBool, as we later do BindInt32Parameter()
>-  PRBool newItem = PR_FALSE; // used to send out notifications at the end
>-  if (alreadyVisited) {
>-    // Update the existing entry...
>-    rv = mDBGetPageVisitStats->GetInt64(0, &pageID);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    PRInt32 oldVisitCount = 0;
>-    rv = mDBGetPageVisitStats->GetInt32(1, &oldVisitCount);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    PRInt32 oldTypedState = 0;
>-    rv = mDBGetPageVisitStats->GetInt32(2, &oldTypedState);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    PRBool oldHiddenState = 0;
>-    rv = mDBGetPageVisitStats->GetInt32(3, &oldHiddenState);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    // free the previous statement before we make a new one
>-    mDBGetPageVisitStats->Reset();
>-    scoper.Abandon();
>-
>-    // embedded links and redirects will be hidden, but don't hide pages that
>-    // are already unhidden.
>-    //
>-    // Note that we test the redirect flag and not for the redirect transition
>-    // type. The transition type refers to how we got here, and whether a page
>-    // is shown does not depend on whether you got to it through a redirect.
>-    // Rather, we want to hide pages that do not themselves redirect somewhere
>-    // else, which is what the redirect flag means.
>-    //
>-    // note, we want to unhide any hidden pages that the user explicitly types
>-    // (aTransitionType == TRANSITION_TYPED) so that they will appear in
>-    // the history UI (sidebar, history menu, url bar autocomplete, etc)
>-    hidden = oldHiddenState;
>-    if (hidden && (!aIsRedirect || aTransitionType == TRANSITION_TYPED) &&
>-        aTransitionType != TRANSITION_EMBED)
>-      hidden = PR_FALSE; // unhide
>-
>-    typed = oldTypedState || (aTransitionType == TRANSITION_TYPED);
>-
>-    // some items may have a visit count of 0 which will not count for link
>-    // visiting, so be sure to note this transition
>-    if (oldVisitCount == 0)
>-      newItem = PR_TRUE;
>-
>-    // update with new stats
>-    mozStorageStatementScoper updateScoper(mDBUpdatePageVisitStats);
>-    rv = mDBUpdatePageVisitStats->BindInt64Parameter(0, pageID);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    rv = mDBUpdatePageVisitStats->BindInt32Parameter(1, hidden);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-    rv = mDBUpdatePageVisitStats->BindInt32Parameter(2, typed);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    rv = mDBUpdatePageVisitStats->Execute();
>-    NS_ENSURE_SUCCESS(rv, rv);
>-  } else {
>-    // New page
>-    newItem = PR_TRUE;
>-
>-    // free the previous statement before we make a new one
>-    mDBGetPageVisitStats->Reset();
>-    scoper.Abandon();
>-
>-    // Hide only embedded links and redirects
>-    // See the hidden computation code above for a little more explanation.
>-    hidden = (aTransitionType == TRANSITION_EMBED || aIsRedirect);
>-
>-    typed = (aTransitionType == TRANSITION_TYPED);
>-
>-    // set as visited once, no title
>-    nsString voidString;
>-    voidString.SetIsVoid(PR_TRUE);
>-    rv = InternalAddNewPage(aURI, voidString, hidden, typed, 1, PR_TRUE, &pageID);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-  }
>+  PRBool hidden = (aTransitionType == TRANSITION_EMBED || aIsRedirect);
>+  PRBool typed = (aTransitionType == TRANSITION_TYPED);
>+
>+  // set as visited once, no title
>+  nsString voidString;
>+  voidString.SetIsVoid(PR_TRUE);
>+  rv = InternalAddNewPage(aURI, voidString, hidden, typed, 1, PR_TRUE, &pageID);
>+  NS_ENSURE_SUCCESS(rv, rv);
> 
>   // Get the place id for the referrer, if we have one
>   PRInt64 referringVisitID = 0;
>   PRInt64 referringSessionID;
>   if (aReferringURI &&
>       !FindLastVisit(aReferringURI, &referringVisitID, &referringSessionID)) {
>     // Add the referrer
>     rv = AddVisit(aReferringURI, aTime - 1, nsnull, TRANSITION_LINK, PR_FALSE,
>diff --git a/toolkit/components/places/tests/unit/test_454977.js b/toolkit/components/places/tests/unit/test_454977.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/tests/unit/test_454977.js
>@@ -0,0 +1,113 @@
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Bug 454977 code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Corp.
>+ * Portions created by the Initial Developer are Copyright (C) 2008
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *  Marco Bonardo <mak77bonardo.net> (Original Author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+// Get services
>+try {
>+  var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
>+           getService(Ci.nsINavHistoryService);
>+  var mDBConn = hs.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;
>+} catch(ex) {
>+  do_throw("Could not get services\n");
>+}
>+
>+var visit_count = 0;
>+
>+function add_visit(aURI, aVisitDate, aVisitType) {
>+  var isRedirect = aVisitType == hs.TRANSITION_REDIRECT_PERMANENT ||
>+                   aVisitType == hs.TRANSITION_REDIRECT_TEMPORARY;
>+  var visitId = hs.addVisit(aURI, aVisitDate, null,
>+                            aVisitType, isRedirect, 0);
>+  do_check_true(visitId > 0);
>+  // Increase visit_count if applicable
>+  if (aVisitType != 0 &&
>+      aVisitType != hs.TRANSITION_EMBED &&
>+      aVisitType != hs.TRANSITION_DOWNLOAD)
>+    visit_count ++;
>+  // Get the place id
>+  var sql = "SELECT place_id FROM moz_historyvisits WHERE id = ?1";
>+  var stmt = mDBConn.createStatement(sql);
>+  stmt.bindInt64Parameter(0, visitId);
>+  do_check_true(stmt.executeStep());
>+  var placeId = stmt.getInt64(0);
>+  do_check_true(placeId > 0);
>+  return placeId;
>+}
>+
>+function check_results(aExpectedCount, aExpectedCountWithHidden) {
>+  var query = hs.getNewQuery();
>+  // used to check visit_count
>+  query.minVisits = visit_count;
>+  query.maxVisits = visit_count;
>+  var options = hs.getNewQueryOptions();
>+  options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY;
>+  result = hs.executeQuery(query, options);
>+  var root = result.root;
>+  root.containerOpen = true;
>+  // Children without hidden ones
>+  do_check_eq(root.childCount, aExpectedCount);
>+  root.containerOpen = false;
>+  // Execute again with includeHidden = true
>+  options.includeHidden = true;
>+  result = hs.executeQuery(query, options);
>+  var root = result.root;
>+  root.containerOpen = true;
>+  // Children with hidden ones
>+  do_check_eq(root.childCount, aExpectedCountWithHidden);
>+  root.containerOpen = false;
>+}
>+
>+// main
>+function run_test() {
>+  var testURI = uri("http://test.mozilla.org/");
>+
>+  // Add a visit that force hidden
>+  var placeId = add_visit(testURI, Date.now()*1000, hs.TRANSITION_EMBED);
>+  check_results(0, 1);
>+
>+  // Add a visit that force unhide and check place id
>+  // - We expect that the place gets hidden = 0 while retaining the same
>+  //   place_id and a correct visit_count.
>+  do_check_eq(add_visit(testURI, Date.now()*1000, hs.TRANSITION_TYPED), placeId);
>+  check_results(1, 1);

wouldn't the the place be unhidden at this point?

>+
>+  // Add a visit, check that hidden is not overwritten and check place id
>+  // - We expect that the place has still hidden = 0, while retaining the same
>+  //   place_id and a correct visit_count.
>+  do_check_eq(add_visit(testURI, Date.now()*1000, hs.TRANSITION_EMBED), placeId);
>+  check_results(1, 1);
>+}

Comment 4

[Dietrich Ayala (:dietrich)]

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

talked over irc, r=me

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

fixed review comments

i'm going to wait for fsync stuff decisions before landing this since it will bitrot patch in bug 450705.
So do not land until a landing time is setup for fsync stuff.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

nominating for branch, users migrating from fx2 to fx3 are crashing against this (partially loosing their visits)

Comment 7

[Daniel Veditz [:dveditz]]

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

Not taking for 1.9.0.x until it's taken and tested on trunk. It's also possibly out of scope for a stability release.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

needed for correct history migration, asking blocking. Will need to be unbitrotted after fsync stuff.

Comment 9

[Dietrich Ayala (:dietrich)]

not blocking on this, but please do land it regardless.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

actually this is problematic with temp tables and views, so i have to find a different way around this.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2.0

unless i'm blind (and could be), history.dat contains visit count, first and last visit time, but no informations on other visits, so there's no way we can end up with a correctly imported history (with all visits and a correct visit_count).

As of now we end up having all history entries, each one has 1 visit (the last one) and visit_count = 1, the only way to fix the visit_count would be to add enough dummy visits to set a correct count, but that sounds bad and useless.

So the only thing i think we should do is completely get rid of INSERT OR REPLACE since with views it's hard to tell what it will do, and probably slower and error prone. So we should check for entry existance in all methods that use internalAddNewPage, luckily the only one is import, that actually will never do that for the above reason, but it's better to change it still for sanity.

One test checks that history.dat is imported, one test checks that on adding a visit hidden and typed are correctly changed or retained based on the fact page exists or not.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #12)
> internalAddNewPage, luckily the only one is import

i mean "the only one that does not check for existance"

Comment 14

[Dietrich Ayala (:dietrich)]

(In reply to comment #12)
> As of now we end up having all history entries, each one has 1 visit (the last
> one) and visit_count = 1, the only way to fix the visit_count would be to add
> enough dummy visits to set a correct count, but that sounds bad and useless.

why is it bad and useless? while the dates will not be correct for pre-Fx3 history entries, this would almost immediately make the awesomebar push your Fx2 "most visited" up to the top, making the transition far easier for sensitive upgraders.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #14)
> why is it bad and useless? while the dates will not be correct for pre-Fx3
> history entries, this would almost immediately make the awesomebar push your
> Fx2 "most visited" up to the top, making the transition far easier for
> sensitive upgraders.

conceptually a visit is something identified by an uri and a time, having many identical visits is not so good in such a vision, personally i don't like having many visits with the same time. Frecency is however a good point to add a correct number of visits, maybe we could add first visit, last one and set remaining visits in the past by a certain number of microseconds before last one

Comment 16

[Dietrich Ayala (:dietrich)]

(In reply to comment #15)
> (In reply to comment #14)
> > why is it bad and useless? while the dates will not be correct for pre-Fx3
> > history entries, this would almost immediately make the awesomebar push your
> > Fx2 "most visited" up to the top, making the transition far easier for
> > sensitive upgraders.
> 
> conceptually a visit is something identified by an uri and a time, having many
> identical visits is not so good in such a vision, personally i don't like
> having many visits with the same time. Frecency is however a good point to add
> a correct number of visits, maybe we could add first visit, last one and set
> remaining visits in the past by a certain number of microseconds before last
> one

the most common visual interface for these visits is the awesomebar. i'm less worried about a bunch of visits next to each other in the history sidebar than i am about representing the user's behavior properly in the data. the closer we get to properly modeling the user's behavior, the better our predictive assertions will be, thereby improving people's experience with the awesomebar.

your suggestion to artificially stagger the visits sounds good to me.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2.1

This implements the artificial spread of visits, i add first visit, last visit and other visits are shifted of N microseconds (where N is the visit number) from last visit. Includes a test to check that addVisit makes the correct thing with typed/hidden and a test that imports an history.dat and checks visit count and visit times.

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 355999 [details] [diff] [review]
patch v2.1


>+nsNavHistory::AddPageWithVisits(nsIURI *aURI,
>+                                const nsString &aTitle,
>+                                PRInt32 aVisitCount,
>+                                PRInt32 aTransitionType,
>+                                PRTime aFirstVisitDate,
>+                                PRTime aLastVisitDate)
> {
>   PRBool canAdd = PR_FALSE;
>   nsresult rv = CanAddURI(aURI, &canAdd);
>   NS_ENSURE_SUCCESS(rv, rv);
>   if (!canAdd) {
>     return NS_OK;
>   }
> 
>-  PRInt64 pageID;
>-  // we don't want to calculate frecency here for each uri we import
>-  rv = InternalAddNewPage(aURI, aTitle, aHidden, aTyped, aVisitCount, PR_FALSE, &pageID);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_SUCCESS(rv, rv);

not needed?

>diff --git a/toolkit/components/places/tests/unit/test_454977.js b/toolkit/components/places/tests/unit/test_454977.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/tests/unit/test_454977.js
>@@ -0,0 +1,123 @@
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Bug 454977 code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Corp.
>+ * Portions created by the Initial Developer are Copyright (C) 2008
>+ * the Initial Developer. All Rights Reserved.

2009

>diff --git a/toolkit/components/places/tests/unit/test_history.js b/toolkit/components/places/tests/unit/test_history.js
>--- a/toolkit/components/places/tests/unit/test_history.js
>+++ b/toolkit/components/places/tests/unit/test_history.js
>@@ -230,9 +230,45 @@ function run_test() {
>   file.copyTo(histFile, "history.dat");
>   histFile.append("history.dat");
>   do_check_true(histFile.exists());
> 
>   var globalHistory = Components.classes["@mozilla.org/browser/global-history;2"]
>                                 .getService(Components.interfaces.nsIBrowserHistory);
>   globalHistory.removeAllPages();
>   do_check_false(histFile.exists());
>+
>+  // test import history
>+  var file = do_get_file("toolkit/components/places/tests/unit/history_import_test.dat");
>+  globalHistory.importHistory(file);
>+  var uri1 = uri("http://www.mozilla.org/");
>+  do_check_true(uri_in_db(uri1));
>+
>+  // Check visit count
>+  var options = histsvc.getNewQueryOptions();
>+  options.sortingMode = options.SORT_BY_DATE_DESCENDING;
>+  options.resultType = options.RESULTS_AS_VISIT;
>+  var query = histsvc.getNewQuery();
>+  query.minVisits = 4;
>+  query.maxVisits = 4;
>+  query.uri = uri1;
>+  var result = histsvc.executeQuery(query, options);
>+  var root = result.root;
>+
>+  root.containerOpen = true;
>+  var cc = root.childCount;
>+  do_check_eq(cc, 4);
>+
>+  // Check first and last visits times are correct
>+  var lastVisitDate = root.getChild(0).time;
>+  do_check_eq(lastVisitDate, 1230666859910484);
>+  var firstVisitDate = root.getChild(3).time;
>+  do_check_eq(firstVisitDate, 1230666845910353);
>+
>+  // Check other visits have different times and are between first and last ones
>+  do_check_true(root.getChild(1).time < lastVisitDate &&
>+                root.getChild(1).time > firstVisitDate);
>+  do_check_true(root.getChild(2).time < lastVisitDate &&
>+                root.getChild(2).time > firstVisitDate);
>+  do_check_true(root.getChild(1).time != root.getChild(2).time);
>+
>+  root.containerOpen = false;
> }

this should probably be in it's own test_history_import.js.

r=me otherwise

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2.2

addressed comments

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/0524c8f29855

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 356617 [details] [diff] [review]
patch v2.2

asking approval, this fixes history import from firefox 2 and removes an error prone path for future development, risk is medium but comes with tests.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 356617 [details] [diff] [review]
patch v2.2

Drivers have decided that the method change and invasiveness of this patch isn't worth the benefit at this time.

Comment 23

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv