353434 - Remove C++ feed parsing for livemarks on trunk

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Robert Sayre]

The livemark parser is known to be buggy and slow compared the new toolkit component used by the feed preview.

nsILivemarkService and friends seem to be implemented in C++ mostly to reuse the C++ livemark code they inherited. Looks like I can reuse lots of Google code from their moz and js lib to do the alarms and observer stuff. It should result in a big reduction 

The other issue is that Places IDs seem to be 64-bit integers, and that's not safe for JS, which only gets 51 bits of precision. If I just need to pass them around, I can probably use an nsISupportsPrimitive to get it done. I wonder if Places IDs are safe elsewhere in the codebase, though.

Comment 1

[Robert Sayre]

Attachment: stubbed out nsLivemarksService.js, includes Google libs from url-classifier

Comment 2

[Brett Wilson]

51 bits should be fine. The IDs just increment as things get added. We want to handle more than a few billion to keep from crashing in edge cases (probably automatic testing applications are the only things that could get us this high), but both 51 and 64 are way beyond the range where we care.

Comment 3

[Robert Sayre]

Attachment: new livemark listener

Comment 4

[Robert Sayre]

Attachment: new livemark listener

Comment 5

[Robert Sayre]

Attachment: fill in nsLivemarksService methods

Comment 6

[Robert Sayre]

Attachment: fix syntax goof

Comment 7

[Robert Sayre]

Attachment: nsIRemoteContainer implemented, timer and shutdown implemented

Comment 8

[Robert Sayre]

This depends on a "make check" driven unit testing in 354401 for places, so we can have unit tests for the livemark stuff.

Comment 9

[Robert Sayre]

Attachment: almost works perfectly. need unit tests for old and new version.

Comment 10

[Robert Sayre]

Attachment: mostly works... refresh interval and shutdown need work

also need tests and leak checks

Comment 11

[Robert Sayre]

Attachment: add livemarks unit tests

small set to start. already found a possible bug, though.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 241857 [details] [diff] [review]
add livemarks unit tests

looks a-ok.

make check
../../../../dist/bin/run-mozilla.sh ../../../../dist/bin/test_all.sh ../../../../dist/bin/test_places
../../../../dist/bin/test_places/test_bookmarks.js: PASS
../../../../dist/bin/test_places/test_livemarks.js: PASS

yum.

Comment 13

[Robert Sayre]

Attachment: add unit tests to the diff

Comment 14

[Robert Sayre]

Attachment: convert nsLivemarkService to JS

big patch, should get review from two people

Comment 15

[Robert Sayre]

hmm, +722/-1930. Not bad.

Comment 16

[Robert Sayre]

"<philor> and are you sure we want to switch from our current 5 minute retry on fetching errors, and 60 minute retry on parsing errors, to 10 minutes for everything?"

Hmm, no. But that should be easy enough to change.

Comment 17

[Phil Ringnalda (:philor)]

> if (!haveAtLeastOneEntry)

This regresses bug 262798, no?

Comment 18

[Robert Sayre]

(In reply to comment #17)
> > if (!haveAtLeastOneEntry)
> 
> This regresses bug 262798, no?

Yes, it does. I forgot about that fix.

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 242298 [details] [diff] [review]
convert nsLivemarkService to JS

>@@ -1599,18 +1599,18 @@ nsNavBookmarks::WriteContainerContents(P
>                                folderId == mBookmarksRoot)) {
>         // don't write out the bookmarks menu or the toolbar folder from the
>         // places root. When writing to bookmarks.html, these are reparented
>         // to the menu, which is the root of the namespace. This provides
>         // better backwards compatability.
>         continue;
>       }
> 
>-      // it could be a regular folder or it could be a livemark
>-      nsLivemarkService* livemarkService = nsLivemarkService::GetLivemarkService();
>+      // it could be a regular folder or it could be a livemarkx
>+      nsCOMPtr<nsILivemarkService> livemarkService(do_GetService(NS_LIVEMARKSERVICE_CONTRACTID, &rv));

nit: extra "x" in "livemarkx"

>+
>+var gIoService = Cc[IO_CONTRACTID].getService(Ci.nsIIOService);
>+var gStringBundle;
>+function GetString(name)
>+{
>+  try {
>+    if (!gStringBundle) {
>+      var bundleService = Cc[SB_CONTRACTID].getService(); 
>+      bundleService = bundleService.QueryInterface(Ci.nsIStringBundleService);
>+      gStringBundle = bundleService.createBundle(PLACES_BUNDLE_URI);
>+    }
>+
>+    if (gStringBundle)
>+      return gStringBundle.GetStringFromName(name);
>+  }
>+  catch (ex) {
>+    LOG("Exception loading string bundle: " + ex.message);
>+  }
>+
>+  return null;
>+}

nit: should use same-line start brace, like the rest of the file

>+
>+  _updateLivemarkChildren:
>+  function LS__updateLivemarkChildren(index, forceUpdate) {
>+    if (this._livemarks[index].locked)
>+      return;
>+    
>+    var livemark = this._livemarks[index];
>+    livemark.locked = true;
>+    try {
>+      // Check the TTL/expiration on this.  If there isn't one,
>+      // then we assume it's never been loaded.  We perform this
>+      // check even when the update is being forced, in case the
>+      // livemark has somehow never been loaded.
>+      var exprTime = this._ans.getAnnotationInt64(livemark.feedURI,
>+                                                  LMANNO_EXPIRATION);
>+      if (!forceUpdate && exprTime > Date.now()) {
>+  // no need to refresh
>+  livemark.locked = false;
>+  return;
>+      }
>+    } 

nit: indentation fix

>+
>+  onContainerMoved:
>+  function LS_onContainerMoved(container, newFolder, newIndex) {
>+    var index = this._getLivemarkIndex(container);
>+    
>+    // Update the annotation that maps the folder URI to the livemark feed URI
>+    var newURI = this._bms.getFolderURI(container);
>+    var oldURI = this._livemarks[index].folderURI;
>+    var feedURIString = this._ans.getAnnotationString(oldURI, LMANNO_FEEDURI);
>+    this._ans.removeAnnotation(oldURI, LMANNO_FEEDURI);
>+    this._ans.setAnnotation(newURI, LMANNO_FEEDURI, feedURIString, 0, 
>+                            this._ans.EXPIRE_NEVER);
>+    
>+    // Update the annotation that maps the folder URI to the livemark site URI
>+    var siteURIString;
>+    try {
>+      siteURIString = this._ans.GetAnnotationString(oldURI, LMANNO_SITEURI);
>+    }
>+    catch (ex) {
>+      // will throw if no annotation
>+    }
>+
>+    if (siteURIString) {
>+      this._ans.removeAnnotation(oldURI, LMANNO_SITEURI);
>+      this._ans.setAnnotation(newURI, LMANNO_SITEURI, siteURIString, 0,
>+                              this._ans.EXPIRE_NEVER);
>+    }
>+  },

it seems like the mutability of "place:" URIs is the only reason this code has to exist, which is kinda high-maintenance. or am i misunderstanding what this method is for?

>+  
>+  /**
>+   * See nsIRequestObserver.idl
>+   */
>+  onStopRequest: function LLL_onStopReqeust(request, context, status) {
>+    if (!Components.isSuccessCode(status)) {
>+      // Something went wrong; try to load again in 10 minutes.
>+      this._setResourceTTL(ERROR_EXPIRATION);
>+      this._isAborted = true;
>+    }

nit: just to reduce unnecessary confusion, probably should remove the "in 10 minutes" from the comment, as it sounds like we might change that value at some point

>+
>+  _setResourceTTL: function LLL__setResourceTTL(seconds) {
>+    var lmService = Cc[LS_CONTRACTID].getService(Ci.nsILivemarkService);
>+    var exptime = Date.now() + seconds;
>+    this._ans.setAnnotationInt64(this._livemark.feedURI,
>+                                 LMANNO_EXPIRATION, exptime, 0,
>+                                 Ci.nsIAnnotationService.EXPIRE_NEVER);
>+  },

lmService is defined, but not used for anything here, AFAICT.

r=me w/ these nits fixed

Also: Old livemarks in the moz_bookmarks_folders table still have the old contract ID as their "type" value, and therefore are no longer treated as livemarks after applying this patch. It's not a huge deal, because the only Places builds are not proper releases, so I don't think we need to worry about upgrading existing livemarks.
However, using the contract ID doesn't seem like a good choice for the long-run because of this. Is there a particular reason that the contract ID was used, or could we use the nsILivemarkService interface name instead?

Comment 20

[Robert Sayre]

Attachment: patch w/ dietrich and philor's improvements

Comment 21

[Phil Ringnalda (:philor)]

> +// Check every 1/2 hour
> +const EXPIRATION = 1800000;

If we're going to go all the way from pre-Places' "pref overridden 1 hour default" to Places C++'s "1 hour default with TODO to read the pref" to "30 minute default with no nod to the abandoned pref" I really think we should do it in a separate bug, not in the middle of 100KB of rewrite.

Comment 22

[Robert Sayre]

(In reply to comment #21)
> > +// Check every 1/2 hour
> > +const EXPIRATION = 1800000;
> 
> If we're going to go all the way from pre-Places' "pref overridden 1 hour
> default" to Places C++'s "1 hour default with TODO to read the pref" to "30
> minute default with no nod to the abandoned pref" I really think we should do
> it in a separate bug, not in the middle of 100KB of rewrite.

I think the best course of action is to file a bug. The point of this patch is to get it in JS so we can rewrite (again) quickly.

Comment 23

[Robert Sayre]

Attachment: patch to check in

Comment 24

[Robert Sayre]

Attachment: patch to check in

Comment 25

[Robert Sayre]

(In reply to comment #14)
> Created an attachment (id=242298) [edit]
> convert nsLivemarkService to JS
> 
> big patch, should get review from two people
> 

Eh, this got a lot of review. Yell at me if you want me to back it out. Happy to follow up with tweaks as well.

Comment 26

[Robert Sayre]

Checking in browser/components/places/content/bookmarkProperties.js;
/cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v  <--  bookmarkProperties.js
new revision: 1.25; previous revision: 1.24
done
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.99; previous revision: 1.98
done
Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.58; previous revision: 1.57
done
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.62; previous revision: 1.61
done
Checking in browser/components/places/tests/unit/head_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/head_bookmarks.js,v  <--  head_bookmarks.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/places/tests/unit/test_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks.js,v  <--  test_bookmarks.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/places/tests/unit/test_livemarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_livemarks.js,v  <--  test_livemarks.js
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/components/build/nsToolkitCompsCID.h;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsCID.h,v  <--  nsToolkitCompsCID.h
new revision: 1.21; previous revision: 1.20
done
Checking in toolkit/components/places/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/places/src/Makefile.in,v  <--  Makefile.in
new revision: 1.22; previous revision: 1.21
done
Removing toolkit/components/places/src/nsBookmarksFeedHandler.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsBookmarksFeedHandler.cpp,v  <--  nsBookmarksFeedHandler.cpp
new revision: delete; previous revision: 1.14
done
Checking in toolkit/components/places/src/nsBookmarksHTML.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsBookmarksHTML.cpp,v  <--  nsBookmarksHTML.cpp
new revision: 1.26; previous revision: 1.25
done
Removing toolkit/components/places/src/nsLivemarkService.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.cpp,v  <--  nsLivemarkService.cpp
new revision: delete; previous revision: 1.22
done
Removing toolkit/components/places/src/nsLivemarkService.h;
/cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.h,v  <--  nsLivemarkService.h
new revision: delete; previous revision: 1.12
done
RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.js,v
done
Checking in toolkit/components/places/src/nsLivemarkService.js;
/cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.js,v  <--  nsLivemarkService.js
initial revision: 1.1
done
Checking in toolkit/components/places/src/nsPlacesModule.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsPlacesModule.cpp,v  <--  nsPlacesModule.cpp
new revision: 1.2; previous revision: 1.1
done

Comment 27

[Robert Sayre]

I opened bug 357222 on feed refresh intervals.

Comment 28

[Robert Sayre]

the powers that be want another review here, but it can stay in the tree

Comment 29

[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