Closed Bug 582703 Opened 9 years ago Closed 9 years ago

Improve concurrency of location bar searches

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: sdwilsh, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fixed-in-places])

Attachments

(2 files, 6 obsolete files)

We can improve the concurrency of location bar searches and ensure that we do not ever have lock contention with the searching thread and the main thread by spinning up a second connection for it.  This is pretty easy to do but does mean we need to open with the shared cache instead of using the unshared connection.  This isn't a big deal in practice.

Requesting blocking because this should help out bug 581978, or at the very least help with bug 563538.  Weather or not that means it actually blocks though, I'll let someone else decide.
Depends on: 583882
Actually, shared cache means lock contention still (in the shared cache).  But we'd have to drop our exclusive lock.  Marco, thoughts on this?
on dropping the exclusive lock? no, we will suffer a bit on perf, but it's a minimal win. I still guess if wal would not be enough allowing to read and write at the same time (And iirc it also uses the shared cache).
Do want
blocking2.0: --- → betaN+
Depends on: 552023
Whiteboard: [needs dependent bugs]
Attached patch v1.0 (obsolete) — Splinter Review
In theory, this will work once we nuke the temp tables (dependent bug).  Something that this patch doesn't do is increase the default cache size.  We might need to consider doing that, but that can also be done in a follow-up bug.
Whiteboard: [needs dependent bugs] → [needs bug 552023]
(In reply to comment #0)
> We can improve the concurrency of location bar searches and ensure that we do
> not ever have lock contention with the searching thread and the main thread by
> spinning up a second connection for it.  This is pretty easy to do but does
> mean we need to open with the shared cache instead of using the unshared
> connection.  This isn't a big deal in practice.
> 
> Requesting blocking because this should help out bug 581978, or at the very
> least help with bug 563538.  Weather or not that means it actually blocks
> though, I'll let someone else decide.
I'm actually not convinced anymore that this would help with what people are seeing in bug 563538.  The stacks there are mostly the annotation service, although I just posted instructions on how to get the SQL running in the background thread.  I find it difficult to believe that it'd be location bar searches (unless people are seeing the hangs during the searches, but that would be news to me).

Removing dependency, and renoming.
No longer blocks: 563538
blocking2.0: betaN+ → ?
FWIW the time I most often hang and see from the stack that it is DB related is when I am typing in the address bar.
(In reply to comment #6)
> FWIW the time I most often hang and see from the stack that it is DB related is
> when I am typing in the address bar.
Right, but in this case, we should really fix the main-thread caller.  This would be a nice to have, but fixing the main-thread caller is more important.  If you can, getting the stack as described in bug 563538 comment 60 would be a huge help (but put the stack in that bug).
All the reporters of the problem seem to be saying the same thing: It happens when they type in the location bar. So I find it hard to believe it wouldn't be related somehow. Until we get solid evidence proving that location bar searches are not involved, we should keep this open, and keep researching in bug 563538.

Maybe you should walk over to Dolske or Mossop's desk and debug it there, to find out what exactly is going on?
Reblocking until we find out exactly what is causing the hang.
blocking2.0: ? → betaN+
(In reply to comment #8)
> All the reporters of the problem seem to be saying the same thing: It happens
> when they type in the location bar. So I find it hard to believe it wouldn't be
> related somehow. Until we get solid evidence proving that location bar searches
> are not involved, we should keep this open, and keep researching in bug 563538.
I'm not sure I agree.  While it's true that this fix would mitigate the problem (and even then only if we have WAL enabled), the underlying issue here is doing I/O on the main thread with places.sqlite.

> Maybe you should walk over to Dolske or Mossop's desk and debug it there, to
> find out what exactly is going on?
I've asked for stacks and given instructions on what to do if you can get the hang in the debugger.  With that information, we can likely make some good progress.
Comment on attachment 468544 [details] [diff] [review]
v1.0

This won't work because we use an in-memory table for opened tabs.  This bug will be a bit more complicated to fix, sadly...
Attachment #468544 - Attachment is obsolete: true
Whiteboard: [needs bug 552023]
dolske and I sat down on Friday to debug this with a debug build.  The case we looked at was typing in the location bar, pressing enter, and then hang.  I'm attaching our notes, that includes some backtraces, but here's the high level summary:

The main thread was stuck in trabbrowser.xml.  Specifically, at this line:
browserHistory.unregisterOpenPage(this.mBrowser.registeredOpenURI);

On our background thread, we were stuck at this line:
while ((srv = ::sqlite3_step(aStatement)) == SQLITE_LOCKED_SHAREDCACHE)
[http://hg.mozilla.org/mozilla-central/annotate/23dcbd0c286c/storage/src/mozStoragePrivateHelpers.cpp#l283]
Running this query:
SELECT h.url, h.title, f.url, (SELECT b.parent FROM
moz_bookmarks b JOIN moz_bookmarks t ON t.id = b.parent AND t.parent != :parent WHERE b.type = 1 AND b.fk = h.id ORDER BY b.lastModified DESC LIMIT
(note: this doesn't appear to be a proper query).  This query is called in autocomplete code here:
http://hg.mozilla.org/mozilla-central/annotate/23dcbd0c286c/toolkit/components/places/src/nsPlacesAutoComplete.js#l220

From what I can tell, one of two things is happening:
1) The background thread is holding the lock, and not giving it up and keeps getting SQLITE_LOCKED_SHAREDCACHE (or we just kept breaking there).
2) Reading from disk on the background thread is taking a long time, and the lock is held during that time.

In both cases, the main thread is trying to get the lock to write to the in-memory table.  The background thread is clearly holding onto it, however.

We added the code in case (1) in bug 571599.  I'm a bit confused as to why we might be getting SQLITE_LOCKED_SHAREDCACHE since places doesn't use the shared cache at all.  We can probably yield in the linked code if we get that, and then try again when we get called back.

Regardless, we can work around this by using a different database connection, which is largely what attachment 468544 [details] [diff] [review] does.

Need to investigate the storage bits too, though.
(In reply to comment #12)
> We added the code in case (1) in bug 571599.  I'm a bit confused as to why we
> might be getting SQLITE_LOCKED_SHAREDCACHE since places doesn't use the shared
> cache at all.  We can probably yield in the linked code if we get that, and
> then try again when we get called back.

Your stacks don't indicate that you're stuck in that while loop, rather that sqlite3_step is just taking a long time (see the 'id' param for seekAndRead in both stacks, they're the same). Without profiling data showing multiple calls to sqlite3_step I'm betting you're not receiving SQLITE_LOCKED_SHAREDCACHE at all.

> 2) Reading from disk on the background thread is taking a long time, and
> the lock is held during that time.

This seems most likely.

> In both cases, the main thread is trying to get the lock to write to the
> in-memory table. 

What in-memory table? Aren't both messing with the places database on disk?

> Regardless, we can work around this by using a different database connection,
> which is largely what attachment 468544 [details] [diff] [review] does.

How will that help? Unless I'm missing something it looks like you're doing a long read from the places DB on the async thread and then you're doing a sync write from the main thread. That has to wait for the in-progress read to finish, regardless of how many connections you have. Right?
(In reply to comment #13)
> Your stacks don't indicate that you're stuck in that while loop, rather that
> sqlite3_step is just taking a long time (see the 'id' param for seekAndRead in
> both stacks, they're the same). Without profiling data showing multiple calls
> to sqlite3_step I'm betting you're not receiving SQLITE_LOCKED_SHAREDCACHE at
> all.
Right, hence the "(or we just kept breaking there)" disclaimer in (1).

> What in-memory table? Aren't both messing with the places database on disk?
No, the code on the main thread is only hitting an in-memory table that we use to keep track of open tabs (for switch to tab integration in the location bar).

> How will that help? Unless I'm missing something it looks like you're doing a
> long read from the places DB on the async thread and then you're doing a sync
> write from the main thread. That has to wait for the in-progress read to
> finish, regardless of how many connections you have. Right?
Even if we were writing on the main thread (which we luckily are not in this case), we will be using WAL (do already on the places branch), which means readers don't block writers and writers don't block readers (on different connections in both cases).  Because we use one connection on trunk, and all access to each connection is serialized, we end up blocking even with WAL.
(In reply to comment #14)
> No, the code on the main thread is only hitting an in-memory table that we use
> to keep track of open tabs (for switch to tab integration in the location bar).

Really? All of the documentation on temp tables says that they're kept in a temporary file on disk, not in memory. Am I missing something?

> Even if we were writing on the main thread (which we luckily are not in this
> case)

Hm? I see nsNavHistory::UnregisterOpenPage running this query on the main thread:

  "UPDATE moz_openpages_temp "
  "SET open_count = open_count - 1 "
  "WHERE url = :page_url"

That's going to require a write transaction, yes?

> we will be using WAL (do already on the places branch), which means
> readers don't block writers and writers don't block readers (on different
> connections in both cases).

Are you sure? http://www.sqlite.org/tempfiles.html says that temp databases always use journal_mode=PERSIST. I'm not sure exactly how that works out if the rest of the db is opened with WAL, but I think this needs some investigation.
(In reply to comment #15)
> Really? All of the documentation on temp tables says that they're kept in a
> temporary file on disk, not in memory. Am I missing something?

fwiw, you can choose the temporary store with PRAGMA temp_store.
(In reply to comment #16)
> fwiw, you can choose the temporary store with PRAGMA temp_store.

Aha! Ok, that ties up that loose end, thanks.

So, maybe the real problem is that we're creating temp tables (to reduce IO) with the same connection that we're using to do real IO. Can we just move all the temp tables into a separate connection? Like, give it its own empty filename and all?
(In reply to comment #17)
> So, maybe the real problem is that we're creating temp tables (to reduce IO)
> with the same connection that we're using to do real IO. Can we just move all
> the temp tables into a separate connection? Like, give it its own empty
> filename and all?
Not really in the general case, but we want to get rid of our temporary tables anyway.  Moving to a separate connection that autocomplete uses (this bug), and still having the temp table (but ensure that it is in memory) for the open pages is just fine (we don't want to persist it).  I'm also making us write, even to an in-memory table, asynchronously with this approach so we don't end up competing with the background thread, even for this.  I suppose we could create a database connection that was only in memory for this too, but then it gets complicated when we want to query both databases for autocomplete results.
Attached patch v2.0 (obsolete) — Splinter Review
This is basically it, but test_frecency.js is failing without the dump_table statements.  I fear that this is because the OS is reordering writes/reads (the last addBookmark always fails to get picked up by the autocomplete controller), but I ran out of time to confirm it today.  If someone else has any insight, feel free to comment.

Blair - this should be reviewable enough for you.  I'll have mak take a look when I get tests passing.

Try server run: http://tbpl.mozilla.org/?tree=MozillaTry&rev=52a5bc954641
Attachment #490722 - Flags: review?(bmcbride)
Comment on attachment 490722 [details] [diff] [review]
v2.0

I don't really understand the reasons behind needing this, but the code itself looks sound. Just a couple of small things:

>+   * @note that when Private Browsing mode is entered/exited, pages need to be
>+   * manually unregistered/registered.
>+   *
>+   * @note this is just an alias for mozIPlacesAutoComplete::unregisterOpenPage.
>+   */

Nit: Capitalize and make into full sentances (applies to the other @note comments too).

>+  nsCOMPtr<mozIPlacesAutoComplete> ac =
>+    do_GetService("@mozilla.org/autocomplete/search;1?name=history");
>+  NS_ENSURE_TRUE(ac, NS_ERROR_UNEXPECTED);
>+
>+  nsresult rv = ac->RegisterOpenPage(aURI);

I wonder if this should be cached, since it will be getting the service twice on every page load.

>+  // Note: this should be kept up-to-date with the definition in
>+  //       nsPlacesTables.h.
>+  let sql = "CREATE TEMP TABLE moz_openpages_temp ("
>+          + "  url TEXT PRIMARY KEY"
>+          + ", open_count INTEGER"
>+          + ")";
>+  aDatabase.executeSimpleSQL(sql);
>+
>+  // Note: this should be kept up-to-date with the definition in
>+  //       nsPlacesTriggers.h.
>+  sql = "CREATE TEMPORARY TRIGGER moz_openpages_temp_afterupdate_trigger "
>+      + "AFTER UPDATE OF open_count ON moz_openpages_temp FOR EACH ROW "
>+      + "WHEN NEW.open_count = 0 "
>+      + "BEGIN "
>+      +   "DELETE FROM moz_openpages_temp "
>+      +   "WHERE url = NEW.url;"
>+      + "END";
>+  aDatabase.executeSimpleSQL(sql);

Why do these still need to exist in nsPlacesTables.h?
Attachment #490722 - Flags: review?(bmcbride) → review+
(In reply to comment #20)
> Why do these still need to exist in nsPlacesTables.h?
Largely because it's our one-stop-shop for our schema.  When I can't remember what some table looks like, it's a quick reference and I don't have to think "where is that table defined at again".
Comment on attachment 490722 [details] [diff] [review]
v2.0

I admit I've not read Blair's review, but this is a feature, not a bug :)

>diff --git a/toolkit/components/places/public/mozIPlacesAutoComplete.idl b/toolkit/components/places/public/mozIPlacesAutoComplete.idl
>- * Mozilla Corporation.
>+ * Mozilla Foundation.

nit: "the Mozilla Foundation".

>-[scriptable, uuid(a5ae8332-333c-412a-bb02-a35df8247714)]
>+[scriptable, uuid(3bf895a0-d6d9-4ba7-b8db-f2f0e0f32d23)]
> interface mozIPlacesAutoComplete : nsISupports
> {
>   //////////////////////////////////////////////////////////////////////////////
>@@ -110,10 +113,25 @@ interface mozIPlacesAutoComplete : nsISu

>+  /**
>+   * Mark a page as being currently open.
>+   *
>+   * @note this is just an alias for mozIPlacesAutoComplete::registerOpenPage.

this IS mozIPlacesAutoComplete...

>+  void registerOpenPage(in nsIURI aURI);
>+
>+  /**
>+   * Mark a page as no longer being open (either by closing the window or tab,
>+   * or by navigating away from that page).
>+   *
>+   * @note that when Private Browsing mode is entered/exited, pages need to be
>+   * manually unregistered/registered.

nit: I'd love if we coud clarify this comment like "pages won't be automatically unregistered when entering pb mode thus they should be manually..."

>+   *
>+   * @note this is just an alias for mozIPlacesAutoComplete::unregisterOpenPage.

ditto

>diff --git a/toolkit/components/places/public/nsIBrowserHistory.idl b/toolkit/components/places/public/nsIBrowserHistory.idl
>     /**
>      * Mark a page as being currently open.
>+     *
>+     * @note this is just an alias for mozIPlacesAutoComplete::registerOpenPage.
>      */
>     void registerOpenPage(in nsIURI aURI);

please mark it as deprecated so we can remove it on next version.

>@@ -166,8 +168,11 @@ interface nsIBrowserHistory : nsIGlobalH

>+     * @note this is just an alias for
>+     *       mozIPlacesAutoComplete::unregisterOpenPage.
>      */
>     void unregisterOpenPage(in nsIURI aURI);

ditto

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>+  nsCOMPtr<mozIPlacesAutoComplete> ac =
>+    do_GetService("@mozilla.org/autocomplete/search;1?name=history");
>+  NS_ENSURE_TRUE(ac, NS_ERROR_UNEXPECTED);

nit: if you use unexpected, just NS_ENSURE_STATE, otherwise use OUT_OF_MEMORY as we usually do.
Same for unregister

>diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js

>+function initTempTable(aDatabase)

nit: you use tables (plural) everywhere but in the name of the function

>+{
>+  // Keep our temporary tables in memory.
>+  aDatabase.executeSimpleSQL("PRAGMA temp_store = MEMORY");
>+
>+  // Note: this should be kept up-to-date with the definition in
>+  //       nsPlacesTables.h.

sigh

>+/**
>+ * @returns true if private browsing is active, false otherwise.

@return, @returns is wrong

>+function inPrivateBrowsingMode()
>+{
>+  return Cc["@mozilla.org/privatebrowsing;1"].
>+         getService(Ci.nsIPrivateBrowsingService).
>+         privateBrowsingEnabled;

you really want to lazyGetter private browsing service, or add a pb observer and cache pb status when it changes (could be cheaper).

>   XPCOMUtils.defineLazyGetter(this, "_db", function() {
>-    return Cc["@mozilla.org/browser/nav-history-service;1"].
>-           getService(Ci.nsPIPlacesDatabase).
>-           DBConnection;
>+    let db = Cc["@mozilla.org/browser/nav-history-service;1"].
>+             getService(Ci.nsPIPlacesDatabase).
>+             DBConnection.
>+             clone(true);

Add a comment above explaining why we use a cloned read-only connection.
PS: this is read-only but we actually write openpages to it?

>+  XPCOMUtils.defineLazyGetter(this, "_registerOpenPageQuery", function() {

nit: I don't recall the code style in this file, maybe you want brace on new line

>+  XPCOMUtils.defineLazyGetter(this, "_unregisterOpenPageQuery", function() {

nit: ditto

>   //////////////////////////////////////////////////////////////////////////////
>+  //// mozIPlacesAutoComplete
>+
>+  registerOpenPage: function PAC_registerOpenPage(aURI)

>+    let stmt = this._registerOpenPageQuery;
>+    stmt.params.page_url = aURI.spec;
>+    stmt.executeAsync();

ah, you are changing this from sync to async, I see where your oranges come from.
There is no way to know when a page has been registered.

>+  unregisterOpenPage: function PAC_unregisterOpenPage(aURI)

>+    let stmt = this._unregisterOpenPageQuery;
>+    stmt.params.page_url = aURI.spec;
>+    stmt.executeAsync();
>+  },

and same here, there is no way to know when it's unregistered.

Is only test_frecency that is failing? did you execute a full tryrun? I'd expect more tests to fail.
We could want to fire a openPageRegistration notification with a true/false data, it's a pity we have to handle this now, if this could be sync would solve our headaches, but then we would again create the lock contention :(

>diff --git a/toolkit/components/places/src/nsPlacesTables.h b/toolkit/components/places/src/nsPlacesTables.h

>+// Note: this should be kept up-to-date with the definition in
>+//       nsPlacesAutoComplete.js.
> #define CREATE_MOZ_OPENPAGES_TEMP NS_LITERAL_CSTRING( \
>   "CREATE TEMP TABLE moz_openpages_temp (" \
>     "  url TEXT PRIMARY KEY" \

I hate stuff that we have to keep in sync :)
It would be extremely more hepful to have a schema dumper utility and publish schema in a wiki page or mdc.
An add-on could do it!

>diff --git a/toolkit/components/places/src/nsPlacesTriggers.h b/toolkit/components/places/src/nsPlacesTriggers.h

>+ * @note this should be kept up-to-date with the definition in
>+ *       nsPlacesAutoComplete.js

ditto
ah you also want to change browser to use this interface rather than the old one, for performance reasons (no interfaces redirecting)
this is the effect of bailing out in register and unregister open pages on top of Places branch.
On some platform numbers are interesting, looks like it causes interesting thread movements, nothing major but numbers seem to assest. Btw, we want this regardless for the locking when new pages are opened, I see that pretty often and looks like I cannot reproduce in pb mode, thus must be this thing.
http://tinyurl.com/376shfh
PS: if you want to concentrate on Bug 612617 I can eventually steal this one, fix comments and go through tests.
also, for testing open pages we'll need a isOpenPageRegistered(aURI) API, otherwise browser_tabMatchesInAwesomebar.js (or whatever it is called) cannot query the table.
(In reply to comment #25)
> PS: if you want to concentrate on Bug 612617 I can eventually steal this one,
> fix comments and go through tests.
It's already done, so I can continue this for now.

(In reply to comment #26)
> also, for testing open pages we'll need a isOpenPageRegistered(aURI) API,
> otherwise browser_tabMatchesInAwesomebar.js (or whatever it is called) cannot
> query the table.
We can just refactor it to do an autocomplete query, no?
(In reply to comment #22)
> Add a comment above explaining why we use a cloned read-only connection.
> PS: this is read-only but we actually write openpages to it?
We write to our very own in-memory table, which is fine.

> >+  XPCOMUtils.defineLazyGetter(this, "_registerOpenPageQuery", function() {
> 
> nit: I don't recall the code style in this file, maybe you want brace on new
> line
This is the style in the file, and in Places code in general when we have an inline function.

> >+    let stmt = this._registerOpenPageQuery;
> >+    stmt.params.page_url = aURI.spec;
> >+    stmt.executeAsync();
> 
> ah, you are changing this from sync to async, I see where your oranges come
> from.
Which oranges?  test_frecency.js uses an autocomplete query, which is also async.  We need to use async queries here, or use a different db connection all together so we don't block when adding.  async queries should be find for adding (even to the in-memory table) because we also read async on the same thread, so everything will be serialized.'

> Is only test_frecency that is failing? did you execute a full tryrun? I'd
> expect more tests to fail.
Full try run reports these test failures:
test_000_frecency.js
test_frecency.js
browser_tabMatchesInAwesomebar.js
The last one looks to be due to a statement not compiling, which is likely because it cannot find the temp tables any more.  *sigh*

We can fix it by making it do an autocomplete query.  The other two fail in more subtle ways.

> I hate stuff that we have to keep in sync :)
> It would be extremely more hepful to have a schema dumper utility and publish
> schema in a wiki page or mdc.
> An add-on could do it!
Right, but until we have that, I think it's worth keeping there (see my comments to Blair's comments).
(In reply to comment #20)
> I wonder if this should be cached, since it will be getting the service twice
> on every page load.
No, because we'll update the caller to use the new one.  This is just a compatibility shim for any add-ons that might have started to use this.
Attached patch v2.1 (obsolete) — Splinter Review
This only addresses the review comments so far.  Need to do more test fixes still.
Attachment #490722 - Attachment is obsolete: true
Attached patch v2.2 (wip) (obsolete) — Splinter Review
More work.  mak is likely going to steal this from me while I tackle something else, so putting this here now.  Things to do still:
1) fix test failures
2) update tabbrowser.xml to use new interface
3) get sr
Attachment #491269 - Attachment is obsolete: true
stealing.
Assignee: sdwilsh → mak77
Attached patch patch v2.3 (obsolete) — Splinter Review
This should address all we know so far, I've pushed it to try to get further confirms before asking review.
While fixing it I've found a couple minor bugs in autocomplete (now with tests) causing oranges.
Also added a util to wait async statements to our head_common, for complicated cases where common waitFor methods are hard to use.
Attachment #491400 - Attachment is obsolete: true
Attached patch patch v2.4 (obsolete) — Splinter Review
This one should be the "final", I've done a further push to try, but I think I have fixed both the failing tests. I'm now double-checking xpcshell and oth on try.
I've slightly changed browser_tabMatchesInAwesombar to wait for both tab load and the visit since I was seeing random failures.
I had to slightly change the way we track duplicates: originally we were tracking by place id, but not all pages have one, thus in patch v2.3 I changed to track urls. This was still wrong since keywords entries are dinamically changed to include the search string so that was causing both entries (the %s and the changed one) to appear. The final design registers ids for pages having a place id, urls for others, this fixes both issues and is faster then always using urls. This tracking is well tests in /autocomplete/ tests.
Attachment #491652 - Attachment is obsolete: true
Attachment #491810 - Flags: review?(sdwilsh)
fwiw, tryrun went pretty much green, random failures disappeared too.
Comment on attachment 491810 [details] [diff] [review]
patch v2.4

>+++ b/toolkit/components/places/src/nsPlacesAutoComplete.js
>@@ -304,17 +363,17 @@ function nsPlacesAutoComplete()
>     +   ") AS rank, place_id "
>     +   "FROM moz_inputhistory i "
>     +   "GROUP BY i.place_id "
>     +   "HAVING rank > 0 "
>     + ") AS i "
>     + "JOIN moz_places h ON h.id = i.place_id "
>     + "LEFT JOIN moz_favicons f ON f.id = h.favicon_id "
>     + "LEFT JOIN moz_openpages_temp t ON t.url = h.url "
>-    + "WHERE AUTOCOMPLETE_MATCH(:searchString, h.url, "
>+    + "WHERE AUTOCOMPLETE_MATCH(NULL, h.url, "
>     +                          "IFNULL(bookmark, h.title), tags, "
>     +                          "h.visit_count, h.typed, parent, "
>     +                          "t.open_count, "
>     +                          ":matchBehavior, :searchBehavior) "
>     + "ORDER BY rank DESC, h.frecency DESC "
>     );
>   });
> 

>+    // the result.  _usedPlaces is an Object that is being used as a set.
>+    // Not all entries have a place id, thus we fallback to the url for them.
>+    // We cannot use onlye the url since keywords entries are modified to
nit: only no onlye

>+function waitForAsyncUpdates(aCallback, aScope, aArguments) {
nit: brace on new line

>+  let scope = aScope || this;
>+  let argument = aArguments || [];
>+  let stmt = DBConn().createAsyncStatement(
>+    "BEGIN EXCLUSIVE;COMMIT;"
>+  );
This shouldn't be doing what you expect.  We can't do more than one statement (even if there are ; in there) due to how the SQLite C API works.  However, you can do this instead:
let db = DBConn();
db.createAsyncStatement("BEGIN EXCLUSIVE").executeAsync();
db.createAsyncStatement("COMMIT").executeAsync({
  handleCompletion: function() aCallback.apply(scope, arguments)
});
(Note, do not use db.executeAsync as that will get you a transaction.

>+++ b/toolkit/components/places/tests/unit/test_frecency.js
>   input.onSearchComplete = function() {
>     do_check_eq(numSearchesStarted, 1);
>     do_check_eq(controller.searchStatus,
>                 Ci.nsIAutoCompleteController.STATUS_COMPLETE_MATCH);
>+    print("Expecting:");
>+    for (let j = 0; j < uris.length; j++) {
>+      print(uris[j].spec);
>+    }
>+    print("Got:");
>+    for (let j = 0; j < controller.matchCount; j++) {
>+      print(controller.getValueAt(j));
>+    }
remove this please; it was only added for debugging purposes

r=sdwilsh

This patch was surprisingly easy to review given it's size.  Blair should look over it one more time too, and rs can do the sr I guess.
Attachment #491810 - Flags: superreview?(robert.bugzilla)
Attachment #491810 - Flags: review?(sdwilsh)
Attachment #491810 - Flags: review?(bmcbride)
Attachment #491810 - Flags: review+
Comment on attachment 491810 [details] [diff] [review]
patch v2.4

Reviewed only the api changes and they are fine
Attachment #491810 - Flags: superreview?(robert.bugzilla) → superreview+
(In reply to comment #36)
> Comment on attachment 491810 [details] [diff] [review]
> patch v2.4
> 
> >+++ b/toolkit/components/places/src/nsPlacesAutoComplete.js
> >@@ -304,17 +363,17 @@ function nsPlacesAutoComplete()
> >     +   ") AS rank, place_id "
> >     +   "FROM moz_inputhistory i "
> >     +   "GROUP BY i.place_id "
> >     +   "HAVING rank > 0 "
> >     + ") AS i "
> >     + "JOIN moz_places h ON h.id = i.place_id "
> >     + "LEFT JOIN moz_favicons f ON f.id = h.favicon_id "
> >     + "LEFT JOIN moz_openpages_temp t ON t.url = h.url "
> >-    + "WHERE AUTOCOMPLETE_MATCH(:searchString, h.url, "
> >+    + "WHERE AUTOCOMPLETE_MATCH(NULL, h.url, "
> >     +                          "IFNULL(bookmark, h.title), tags, "
> >     +                          "h.visit_count, h.typed, parent, "
> >     +                          "t.open_count, "
> >     +                          ":matchBehavior, :searchBehavior) "
> >     + "ORDER BY rank DESC, h.frecency DESC "
> >     );
> >   });
Forgot to add a comment about this...
If we pass in NULL for the search string in the adaptive query, we will always return no results, won't we?
Note that Blair is out until Tuesday.  I'm fine with this landing on the places branch before his review is complete, as long as we address any comments before we merge into mozilla-central.
(In reply to comment #36)
> >+  let scope = aScope || this;
> >+  let argument = aArguments || [];
> >+  let stmt = DBConn().createAsyncStatement(
> >+    "BEGIN EXCLUSIVE;COMMIT;"
> >+  );
> This shouldn't be doing what you expect.  We can't do more than one statement
> (even if there are ; in there) due to how the SQLite C API works.  However, 

ah, I had the same doubt but then I was seeing tests waiting something and I thought it was working! I could debug it but I don't have the will right now :)
(In reply to comment #38)
> Forgot to add a comment about this...
> If we pass in NULL for the search string in the adaptive query, we will always
> return no results, won't we?

passing null there does not change a thing, since we were not binding that param it was binded to null. We just don't filter on the string if we don't pass a string. the real search_string for adaptive queries is called :search_string (cool eh) and it does a query in the adaptive table.
(In reply to comment #41)
> passing null there does not change a thing, since we were not binding that
> param it was binded to null. We just don't filter on the string if we don't
> pass a string. the real search_string for adaptive queries is called
> :search_string (cool eh) and it does a query in the adaptive table.
Oy, right.  The location bar is complicated :)
Attached patch patch v2.5Splinter Review
I agree, considering GUIDS work, we are still a week far from a probable merge, thus there is all the time for a post-review. He reviewed the first changes already, I'm asking ask post-review on these new ones and will push a review changeset if needed.
Attachment #491810 - Attachment is obsolete: true
Attachment #491997 - Flags: review?(bmcbride)
Attachment #491810 - Flags: review?(bmcbride)
http://hg.mozilla.org/projects/places/rev/f118db7595c6
Whiteboard: [fixed-in-places][needs post-review Blair]
Blocks: 555694
Comment on attachment 491997 [details] [diff] [review]
patch v2.5

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml

>+      <field name="mPlacesAutocomplete" readonly="true">
>+         Components.classes["@mozilla.org/autocomplete/search;1?name=history"]
>+                   .getService(Components.interfaces.mozIPlacesAutoComplete);

_placesAutocomplete, please!
I did not make _placesAutocomplete because previously there was mBrowserHistory there with the same functionality (not _browserHistory).
Btw, I'm fine with the change, I will coalesce it with Blair's review comments if any, along this week.
Comment on attachment 491997 [details] [diff] [review]
patch v2.5

Sorry for the delay :(

Looks good to me.
Attachment #491997 - Flags: review?(bmcbride) → review+
Whiteboard: [fixed-in-places][needs post-review Blair] → [fixed-in-places]
Depends on: 619659
http://hg.mozilla.org/mozilla-central/rev/1818e5fee66a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
Depends on: 620290
Depends on: 728997
You need to log in before you can comment on or make changes to this bug.