627772 - AutocompleteCache should read from disk async

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Shawn Wilsher :sdwilsh]

In loadCache (https://hg.mozilla.org/mobile-browser/annotate/ad6d810a22bc/components/AutoCompleteCache.js#l196), we should be using async disk I/O to read the data in instead of doing it synchronously.

Fix is fairly straightforward (I say this without knowing how the dependent code uses this, of course):
loadCache: function loadCache() {
  try {
    NetUtil.asyncFetch(this.cacheFile, function(aInputStream, aResultCode) {
      if (Components.isSuccessCode(aResultCode) {
        let cache = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON).
                    decodeFromStream(aInputStream, aInputStream.available());
        ...

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

Works fine.

Comment 2

[Shawn Wilsher :sdwilsh]

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

>+      NetUtil.asyncFetch(this.cacheFile, function(aInputStream, aResultCode) {
>+        if (Components.isSuccessCode(aResultCode)) {
You don't seem to log an error here in case this isn't a success code.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #2)
> Comment on attachment 505854 [details] [diff] [review]
> patch
> 
> >+      NetUtil.asyncFetch(this.cacheFile, function(aInputStream, aResultCode) {
> >+        if (Components.isSuccessCode(aResultCode)) {
> You don't seem to log an error here in case this isn't a success code.

Added locally. Thanks for pointing out and filing this async issue Shawn.

Comment 4

[Matt Brubeck (:mbrubeck)]

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

>+      NetUtil.asyncFetch(this.cacheFile, function(aInputStream, aResultCode) {
>+        if (Components.isSuccessCode(aResultCode)) {
>+          let cache = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON).
>+                      decodeFromStream(aInputStream, aInputStream.available());
> 
>+          if (cache.version != CACHE_VERSION) {
>+            this.fetch(this.query);
>+            return;
>+          }
>+          this.cache = new cacheResult(cache.result.searchString, cache.result.data);
>+        }
>+      });

Looks like "this" is not set correctly inside the callback.

How do I test to see if this is working?  Can we get a browser-chrome test?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

Updated to use "self" in the closure.

I made a test for this. Just a simple mock JSON cache file is be loaded and we check for the mock values. The problem was getting the timing right. I had to add some notifications, which seem pretty good for general use:
* cache is loaded
* cache is saved
* force a reload of the cache

With these notifs, I was able to make the test and it passes.

Lastly, I changed the delay used to refresh the cache from 10secs to 5secs. 10secs just seems crazy long. We have had bugs where people added a bookmark, then open the awesomebar, but the bookmark is not shown yet. The cache was stale. I don't think 10 -> 5 will cause any adverse side effects.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/7af22bb64ec7