Last Comment Bug 559505 - localstore.rdf kills ponies
: localstore.rdf kills ponies
Status: RESOLVED FIXED
[ponies]
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: RDF (show other bugs)
: Trunk
: All All
: -- normal with 6 votes (vote)
: mozilla34
Assigned To: Roberto Agostino Vitillo (:rvitillo)
:
Mentors:
Depends on: 1057137 1058436
Blocks: 833098 987728 742529 1056199 1056635 1056649 1056948 1069110 1077959
  Show dependency treegraph
 
Reported: 2010-04-14 22:48 PDT by Justin Dolske [:Dolske]
Modified: 2016-03-06 00:44 PST (History)
63 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (WIP) (32.01 KB, patch)
2010-04-14 22:48 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.1 (WIP) (42.61 KB, patch)
2010-04-16 19:15 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2 (WIP) (52.71 KB, patch)
2010-04-17 18:13 PDT, Justin Dolske [:Dolske]
neil: feedback+
Details | Diff | Splinter Review
Patch v.3 (66.95 KB, patch)
2010-04-24 20:44 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Reformed Patch, work in progress (76.14 KB, patch)
2013-10-28 12:51 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Reformed Patch, updated, work in progress (83.96 KB, patch)
2013-10-30 11:54 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
localstore (102.61 KB, patch)
2013-11-08 09:25 PST, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Bug 559505 - localstore.rdf kills ponies, v7 (132.08 KB, patch)
2014-06-20 10:55 PDT, Roberto Agostino Vitillo (:rvitillo)
no flags Details | Diff | Splinter Review
Bug 559505 - localstore.rdf kills ponies, v8 (132.09 KB, patch)
2014-06-25 12:51 PDT, Roberto Agostino Vitillo (:rvitillo)
enndeakin: review-
Details | Diff | Splinter Review
fred.txt (3.21 KB, text/plain)
2014-07-10 12:01 PDT, Neil Deakin (away until Oct 3)
no flags Details
C++ implementation (18.21 KB, patch)
2014-07-18 07:02 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Bug 559505 - localstore.rdf kills ponies, v9 (104.88 KB, patch)
2014-07-21 04:54 PDT, Roberto Agostino Vitillo (:rvitillo)
enndeakin: review+
Details | Diff | Splinter Review
Bug 559505 - localstore.rdf kills ponies, v10 (104.50 KB, patch)
2014-07-28 13:27 PDT, Roberto Agostino Vitillo (:rvitillo)
benjamin: superreview+
Details | Diff | Splinter Review
Bug 559505 - localstore.rdf kills ponies, v11 (104.77 KB, patch)
2014-08-19 10:31 PDT, Roberto Agostino Vitillo (:rvitillo)
no flags Details | Diff | Splinter Review
Talos: Allow IO on xulstore.json (1.97 KB, patch)
2014-08-20 09:39 PDT, Roberto Agostino Vitillo (:rvitillo)
jmaher: review+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2010-04-14 22:48:52 PDT
Created attachment 439182 [details] [diff] [review]
Patch (WIP)

Localstore.rdf kills ponies. Dead. We've grumbled for a while about wanting to kill this off (or at least kill off the common uses of it), I sat down and took a rough shot at it.

Checkpointing a hacky, doesn't-quite-compile WIP, since it seems far enough along that I think I see what remains to be done (famous last words).
Comment 1 Justin Dolske [:Dolske] 2010-04-16 19:15:20 PDT
Created attachment 439662 [details] [diff] [review]
Patch v.1 (WIP)

Looks like this is working, although I'm not saving values and just return dummy data.
Comment 2 Justin Dolske [:Dolske] 2010-04-17 18:13:24 PDT
Created attachment 439740 [details] [diff] [review]
Patch v.2 (WIP)

Functional replacement for localstore.rdf as used by XUL documents, still a few other places in the tree to switch over (now marked with FIXME).
Comment 3 Justin Dolske [:Dolske] 2010-04-20 17:01:53 PDT
Comment on attachment 439740 [details] [diff] [review]
Patch v.2 (WIP)

Before I go any further with completing this patch, I'd like to get some general feedback from The Neils, regarding the general suitability of what I'm doing here. Not sure if Seamonkey / Thunderbird / other apps are doing anything of special concern here... My Thunderbird localstore.rdf doesn't have anything surprising in it, and a brief skim of the comm-central tree seems to indicate that moving SM/TB over to this scheme shouldn't be very hard.

Main TODOs as of Patch v.2:
* write the code to migrate existing localstore.rdf data to the new format.
* TB/SM reqs & patches
* tests
Comment 4 neil@parkwaycc.co.uk 2010-04-21 02:22:42 PDT
Comment on attachment 439740 [details] [diff] [review]
Patch v.2 (WIP)

>+    nsresult rv = GetElementById(aID, getter_AddRefs(domelement));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    NS_ENSURE_TRUE(domelement, NS_OK);
This is a public API. Do we really want to be whining like this?

>+DIRS = public src
PARALLEL_DIRS?

>+interface nsISimpleEnumerator;
nsIStringEnumerator would be easier to use.

>+        const FILE_PERMS    = 0644;
This is almost always wrong. Either use 0600 (private) or 0666 (ask umask)

>+            // XXX why does this work even if I don't call safeStream.finish() in the callback?
Because NS_AsyncCopy already knows about safe output streams.

>+        if (!(id in obj))
>+            obj[id] = {};
Hmm, this is going to fail for an id of, say, "watch".
Comment 5 neil@parkwaycc.co.uk 2010-04-21 02:28:20 PDT
(In reply to comment #3)
> * write the code to migrate existing localstore.rdf data to the new format.
Careful here, because we have two sorts of data in the existing rdf; attribute persist data, which is grouped by document URI, and RDF tree twisty persist data, which always has the value NC:open="true" but can have any URI.
Comment 6 Justin Dolske [:Dolske] 2010-04-24 19:44:00 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > * write the code to migrate existing localstore.rdf data to the new format.
> Careful here, because we have two sorts of data in the existing rdf; attribute
> persist data, which is grouped by document URI, and RDF tree twisty persist
> data, which always has the value NC:open="true" but can have any URI.

This had looked easy to store with the current API; I was thinking of doing persister.persistValue("containers", containerName, "open", "true").

But on closer inspection, nsIXULTreeBuilder and nsIXULTemplateBuilder both expose the RDF goop through the API. So if, for example, I switch nsXULTreeBuilder.cpp over to this new code, what should it do when someone calls getResourceAtIndex()?

Maybe it's possible to create dummy RDF objects such that getResourceAtIndex() / getIndexOfResource() roundtrips correctly, but nothing else RDF-y works? I'm not really sure how this stuff gets used.
Comment 7 Justin Dolske [:Dolske] 2010-04-24 20:44:18 PDT
Created attachment 441329 [details] [diff] [review]
Patch v.3

Checkpointing; compiles but is completely untested.
Comment 8 Serge Gautherie (:sgautherie) 2011-01-31 23:38:21 PST
Ftr,
http://mxr.mozilla.org/comm-central/find?text=&string=localstore.rdf
{
/mozilla/xulrunner/app/profile/localstore.rdf
/mozilla/browser/locales/generic/profile/localstore.rdf
/mail/app/profile/localstore.rdf
/mail/test/mozmill/migration-from-tb2/localstore.rdf
/suite/locales/generic/profile/localstore.rdf
}
Comment 9 Ed Morley [:emorley] 2011-12-10 06:49:19 PST
Being able to kill off RDF would really help with bug 709193.
Comment 10 Justin Dolske [:Dolske] 2013-03-13 18:00:12 PDT
I'm exceedingly unlikely to have the time to finish this. :(

IIRC the main hangup was the XUL tree stuff noted in comment 6. RDF is baked into those APIs, so either we need some kind of compatibility shim to not break the API, or actually change the API.
Comment 11 Neil Deakin (away until Oct 3) 2013-10-28 12:51:28 PDT
Created attachment 823525 [details] [diff] [review]
Reformed Patch, work in progress
Comment 12 Neil Deakin (away until Oct 3) 2013-10-30 11:54:41 PDT
Created attachment 824809 [details] [diff] [review]
Reformed Patch, updated, work in progress

Adds tests for tree builders, makes get/remove methods ignore missing keys.
Comment 13 Philip Chee 2013-10-30 20:57:22 PDT
+++ b/toolkit/components/xulstore/XULStore.js
@@ -0,0 +1,399 @@
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

+++ b/toolkit/components/xulstore/mozIXULStore.idl
@@ -0,0 +1,53 @@
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Please use MPLv2. Thanks muchly.
Comment 14 Neil Deakin (away until Oct 3) 2013-11-05 08:10:48 PST
Some performance tests, where the time is for 1000 iterations:

Loading a fairly small file, which would normally be done at startup:
  Existing code using synchronous RDF - 2100ms
  Using nsIFile and json.decodeFromStream - 1400ms
  Using nsIFile and JSON.parse - 1600ms
  Using OSFile, with no json parsing - 5300ms
  Parsing only using JSON.parse - 600ms

Setting an attribute that needs to be persisted:
  Old code storing in rdf localstore - 1300ms
  New code storing using xulstore.jsm - 1960ms

The conclusion is that the new patch is 33% faster at reading a small file. It is also faster at reading larger files, although I didn't write down the numbers. However, the new patch makes setAttribute calls that require persistence 50% slower.
Comment 15 Neil Deakin (away until Oct 3) 2013-11-08 09:25:38 PST
Created attachment 829378 [details] [diff] [review]
localstore

This patch:
 - uses osfile for writing
 - fixes bookmarks
 - code cleanup and documentation
 - work in progress attempt at backward compatibilty

Still to do:
 - test toolbar migration (from nsBrowserGlue.js)
 - fix browser_toolbar_migration.js test
 - write file on exit not working yet
Comment 16 Neil Deakin (away until Oct 3) 2013-12-03 07:45:08 PST
> Setting an attribute that needs to be persisted:
>   Old code storing in rdf localstore - 1300ms
>   New code storing using xulstore.jsm - 1960ms

A very basic C++ implementation is twice as fast as xulstore.jsm at setting attributes.
Comment 17 Nathan Froyd [:froydnj] 2013-12-03 07:48:32 PST
(In reply to Neil Deakin from comment #14)
> Some performance tests, where the time is for 1000 iterations:
> 
> Loading a fairly small file, which would normally be done at startup:
>   Existing code using synchronous RDF - 2100ms
>   Using nsIFile and json.decodeFromStream - 1400ms
>   Using nsIFile and JSON.parse - 1600ms
>   Using OSFile, with no json parsing - 5300ms
>   Parsing only using JSON.parse - 600ms

Do you have a testcase for the OSFile numbers?  That's really high, and we should try to improve the performance of OSFile if possible.
Comment 18 Neil Deakin (away until Oct 3) 2013-12-03 08:14:28 PST
The following was used on a 1830 byte file:

var olddate;
var count = 0;
 
function readFile()
{
  let filename = "xulstore.json";
  let file = Services.dirsvc.get("ProfD", Components.interfaces.nsIFile);
  file.append(filename);

  let decoder = new TextDecoder();
  let promise = OS.File.read(file.path);
  promise = promise.then(
    function onSuccess(array) {
      var txt = decoder.decode(array);
      let jsobj = JSON.parse(txt);
      count++;
      if (count > 1000) {
        var newdate = new Date();
        dump(newdate - olddate);
      }
      else {
        readFile();
      }
    }
  );
}

function readXULStore()
{
  olddate = new Date();
  readFile();
}

Note that for this bug we can't use an asynchronous api so the osfile number isn't as important.
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-12-12 10:59:18 PST
It's not really fair to directly compare async OS.File read performance with synchronous I/O equivalents, but it may be worth a followup to look into some of these measurements in more detail. It's also possible that the 1000-iteration workload incurs overhead with OS.File that wouldn't otherwise occur in more realistic scenarios.
Comment 20 David Teller [:Yoric] (please use "needinfo") 2013-12-12 12:40:03 PST
If the benchmark was taken during an actual startup, I imagine that the main difference is that OS.File has to wait until the main thread is available to post its results. Since startup is mostly sequential, that can make a huge difference.
Comment 21 Neil Deakin (away until Oct 3) 2013-12-12 13:22:07 PST
(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #20)
> If the benchmark was taken during an actual startup

The test above was not. The test I did ran the code in comment 18 ran when pressing a button.
Comment 22 Roberto Agostino Vitillo (:rvitillo) 2014-06-20 10:55:58 PDT
Created attachment 8443610 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v7

Getting there. I fixed all remaining failing tests and added some more (https://tbpl.mozilla.org/?tree=Try&rev=38beff07dd77).

I still need to address properly the background compatibility for add-ons that require localstore.rdf; I would be inclined to leave the current implementation baked in and issue a warning, would that make sense?
Comment 23 Roberto Agostino Vitillo (:rvitillo) 2014-06-25 12:51:55 PDT
Created attachment 8446082 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v8

Restored localstore.rdf support for backward compatibility.
Comment 24 Axel Hecht [:Pike] 2014-07-03 07:30:40 PDT
I'm pretty sure you don't request l10n feedback. For RDF, please use axel@pike.org.

Also, you didn't really ask anything?
Comment 25 Roberto Agostino Vitillo (:rvitillo) 2014-07-03 07:32:07 PDT
Axel, please have a look at comment 22.
Comment 26 Axel Hecht 2014-07-03 08:41:09 PDT
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #22)
> I still need to address properly the background compatibility for add-ons
> that require localstore.rdf; I would be inclined to leave the current
> implementation baked in and issue a warning, would that make sense?

I wonder if Jorge can help to drill in to that. Do you have stats to figure out how many add-ons use localstore, and what prominent use cases are out there, if still?
Comment 27 Jorge Villalobos [:jorgev] 2014-07-03 10:09:03 PDT
Add-ons shouldn't access localstore.rdf directly. Looking around the Add-ons MXR, I saw a couple of old add-ons that maybe would break if localstore.rdf is removed.

Are there other changes I should be looking into? Any API changes?
Comment 29 Jorge Villalobos [:jorgev] 2014-07-03 10:37:57 PDT
(In reply to :Gijs Kruitbosch from comment #28)
> I think all of these:
> 
> 
> https://mxr.mozilla.org/addons/search?string=rdf%3Alocal-
> store&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons
> 
> would break.

Thanks. I see that most of them haven't been updated in years, so they're probably already incompatible. I also see the Brand Thunder themes, so I'm CCing mkaply so he can look into this.

I think the important question here for compatibility is whether there is a suitable API that can be used to replace what these add-ons are doing. If there is, then it's okay to break compatibility. We can notify the affected developers so they update their code.
Comment 30 Mike Kaply [:mkaply] 2014-07-03 10:42:08 PDT
That's not a public MXR so I can't see the results.
Comment 31 Jorge Villalobos [:jorgev] 2014-07-03 10:45:47 PDT
The query is for rdf:local-store

It matches /chrome/chromeFiles/content/btUtilities.js, line 542 in BT add-ons like https://addons.mozilla.org/addon/bt-fantasyfishing/
Comment 32 Mike Kaply [:mkaply] 2014-07-03 10:50:08 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #31)
> The query is for rdf:local-store
> 
> It matches /chrome/chromeFiles/content/btUtilities.js, line 542 in BT
> add-ons like https://addons.mozilla.org/addon/bt-fantasyfishing/

Yeah. Those are mega old and don't work on current Firefox and even if they did, they are in a try/catch. So we're good with this.
Comment 33 Serge Gautherie (:sgautherie) 2014-07-03 11:41:36 PDT
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #22)
> I would be inclined to leave the current
> implementation baked in and issue a warning, would that make sense?

If this is not too complicated to do then I assume it is always a good idea to have 1+ "transition" release(s).
(Maybe with some related "telemetry" reports, if applicable.)

At least, it can be good to have that in aurora (+ beta) cycle(s), to give (Thunderbird/SeaMonkey/...) a bit of time to adapt.
Comment 34 Neil Deakin (away until Oct 3) 2014-07-10 11:58:23 PDT
Comment on attachment 8446082 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v8

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>-    this._rdf = Cc["@mozilla.org/rdf/rdf-service;1"].getService(Ci.nsIRDFService);
>-    this._dataSource = this._rdf.GetDataSource("rdf:local-store");
>-    this._dirty = false;
>+    xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.mozIXULStore);
> 

let xulStore = ...


>     if (currentUIVersion < 5) {
>       // This code uncollapses PersonalToolbar if its collapsed status is not
>       // persisted, and user customized it or changed default bookmarks.
>-      let toolbarResource = this._rdf.GetResource(BROWSER_DOCURL + "PersonalToolbar");
>-      let collapsedResource = this._rdf.GetResource("collapsed");
>-      let collapsed = this._getPersist(toolbarResource, collapsedResource);
>+      let collapsed = xulStore.getValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed");
>       // If the user does not have a persisted value for the toolbar's
>       // "collapsed" attribute, try to determine whether it's customized.
>-      if (collapsed === null) {
>+      if (collapsed === "") {

Can we use hasValue here? Similarly in a few other places in this file?


>diff --git a/browser/components/places/tests/browser/browser_toolbar_migration.js b/browser/components/places/tests/browser/
>@@ -93,43 +56,52 @@ function test_many_bookmarks_toolbar()
>   ids.push(
>     PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
>                                           PlacesUtils.bookmarks.DEFAULT_INDEX)
>   );
>   ids.push(
>     PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
>                                           PlacesUtils.bookmarks.DEFAULT_INDEX)
>   );
>+  ids.push(
>+    PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
>+                                          PlacesUtils.bookmarks.DEFAULT_INDEX)
>+  );

Why do you add this?


>-
>-        for (int32_t i = int32_t(cnt) - 1; i >= 0; --i) {
>-            nsCOMPtr<nsIContent> element = aElements.SafeObjectAt(i);
...
>+        for (uint32_t i = aElements.Count(); i > 0; i--) {
>+            nsCOMPtr<nsIContent> element = aElements.SafeObjectAt(i - 1);

I think the loop was better before.


> {
>-  gOriginalMigrationVersion = Services.prefs.getIntPref("browser.migration.version");
>+  try {
>+    gOriginalMigrationVersion = Services.prefs.getIntPref("browser.migration.version");
>+  } catch(e) {
>+    gOriginalMigrationVersion = 0;
>+  }

You should probably leave this as is.


>diff --git a/content/xul/templates/src/nsXULContentUtils.h b/content/xul/templates/src/nsXULContentUtils.h
>     static nsresult
>-    GetElementResource(nsIContent* aElement, nsIRDFResource** aResult);
>-
>-    static nsresult

You should also be able to remove the MakeElementURI, MakeElementResource and MakeElementID methods which are now unused.


>
>-    if (aIsTrusted) {
>-        // If we're a privileged (e.g., chrome) document, then add the
>-        // local store as the first data source in the db. Note that
>-        // we _might_ not be able to get a local store if we haven't
>-        // got a profile to read from yet.
>-        nsCOMPtr<nsIRDFDataSource> localstore;
>-        rv = gRDFService->GetDataSource("rdf:local-store",
>-                                        getter_AddRefs(localstore));
>-        NS_ENSURE_SUCCESS(rv, rv);
>-
>-        rv = compDB->AddDataSource(localstore);
>-        NS_ASSERTION(NS_SUCCEEDED(rv), "unable to add local store to db");
>-        NS_ENSURE_SUCCESS(rv, rv);
>-    }
>-

There may be a compatiblity issue here, as I recall long ago that some people would assume that the first datasource was the local store. You could leave this code in for now.


>diff --git a/content/xul/templates/src/nsXULTreeBuilder.cpp b/content/xul/templates/src/nsXULTreeBuilder.cpp
> NS_IMPL_CYCLE_COLLECTION_INHERITED(nsXULTreeBuilder, nsXULTemplateBuilder,
>                                    mBoxObject,
>                                    mSelection,
>                                    mPersistStateStore,
>+				   mLocalStore,
>                                    mObservers)

Fix indenting


>+      mLocalStore = do_GetService("@mozilla.org/xul/xulstore;1");
>+      if(NS_WARN_IF(!mLocalStore)){
>+	return NS_ERROR_NOT_INITIALIZED;
>+      }

and here.


>         if (isOpen) {
>-            if (hasProperty) {
>-                mPersistStateStore->Unassert(container,
>-                                             nsXULContentUtils::NC_open,
>-                                             nsXULContentUtils::true_);
>-            }
>-
>+	    mLocalStore->RemoveValue(docURI, nodeid, NS_LITERAL_STRING("open"));
>             CloseContainer(aIndex);
>-        }
>-        else {
>-            if (! hasProperty) {
>-                mPersistStateStore->Assert(container,
>-                                           nsXULContentUtils::NC_open,
>-                                           nsXULContentUtils::true_,
>-                                           true);
>-            }
>+        } else {
>+	    mLocalStore->SetValue(docURI, nodeid, NS_LITERAL_STRING("open"),
>+				  NS_LITERAL_STRING("true"));
> 
>             OpenContainer(aIndex, result);
>         }
>     }

and both mLocalStore-> lines above.


>     return NS_OK;
> }
> 
>@@ -1215,20 +1186,19 @@ nsXULTreeBuilder::ReplaceMatch(nsIXULTem
> 
>             if (result != mRootResult) {
>                 // don't open containers if child processing isn't allowed
>                 bool mayProcessChildren;
>                 nsresult rv = result->GetMayProcessChildren(&mayProcessChildren);
>                 if (NS_FAILED(rv) || ! mayProcessChildren) return NS_OK;
>             }
> 
>-            bool open;
>-            IsContainerOpen(result, &open);
>-            if (open)
>+            if (IsContainerOpen(result)) {
>                 OpenContainer(iter.GetRowIndex(), result);
>+	    }
>         }
>     }

and here.

>diff --git a/rdf/base/src/nsRDFService.cpp b/rdf/base/src/nsRDFService.cpp
>-        // Strip params to get ``base'' contractID for data source.
>-        int32_t p = contractID.FindChar(char16_t('&'));
>-        if (p >= 0)
>-            contractID.Truncate(p);
>+	    ds = mLocalStore;
>+	}

Fix indenting here.


>+        else {
>+	    // It's a built-in data source. Convert it to a contract ID.
>+	    nsAutoCString contractID(
>+		NS_LITERAL_CSTRING(NS_RDF_DATASOURCE_CONTRACTID_PREFIX) +
>+		Substring(spec, 4, spec.Length() - 4));
> 
>-        ds = do_GetService(contractID.get(), &rv);
>-        if (NS_FAILED(rv)) return rv;
>+            // Strip params to get ``base'' contractID for data source.
>+            int32_t p = contractID.FindChar(PRUnichar('&'));
>+            if (p >= 0)
>+                contractID.Truncate(p);
> 
>-        nsCOMPtr<nsIRDFRemoteDataSource> remote = do_QueryInterface(ds);
>-        if (remote) {
>-            rv = remote->Init(spec.get());
>+	    ds = do_GetService(contractID.get(), &rv);
>             if (NS_FAILED(rv)) return rv;

and here.

I recall when I worked on this that mLocalStore or something would leak when I added the compatibility code here. Does that no longer happen?
I don't remember the details but I believe the window mediator would be holding a reference or something.

>diff --git a/rdf/datasource/public/moz.build b/rdf/datasource/public/moz.build
>-EXPORTS += [
>-    'nsILocalStore.h',
>-]
>+EXPORTS += []

You should be able to remove the entire line, no?


>diff --git a/toolkit/components/xulstore/XULStore.js b/toolkit/components/xulstore/XULStore.js
>+XULStore.prototype = {
>+  classID: XULSTORE_CID,
>+  classInfo: XPCOMUtils.generateCI({classID: XULSTORE_CID,
>+                                    contractID: XULSTORE_CONTRACTID,
>+                                    classDescription: "XULStore",
>+                                    interfaces: [Ci.mozIXULStore]}),
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.mozIXULStore]),


I recall from testing that I needed to implement nsISupportsWeakReference in order to get the observer to work.


>+  observe: function(subject, topic, data) {
>+    if (topic == "profile-before-change") {
>+      writeFile();

this.writeFile()


>+      let data = JSON.stringify(this._data);
>+      let encoder = new TextEncoder();
>+
>+      data = encoder.encode(data);
>+      OS.File.writeAtomic(this._storeFile.path, data, { tmpPath: this._storeFile.path + ".tmp" });

writeAtomic no longer needs the tmpPath.


>+    } catch (e) {
>+      this.log("Failed to write xulstore.json: " + e);
>+      throw e;
>+    }
>+  },
>+
>+  markAsChanged : function() {
>+    this._needsSaving = true;
>+    if (this._writeTimer)
>+      return;
>+
>+    let callback = () => {
>+      this._writeTimer = null;
>+      this.writeFile();
>+    };
>+
>+    // Don't write the file more than once every 30 seconds.
>+    this._writeTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+    this._writeTimer.initWithCallback(callback, WRITE_DELAY, Ci.nsITimer.TYPE_ONE_SHOT);
>+  },
>+
>+  /* ---------- interface implementation ---------- */
>+
>+  setValue : function (docURI, id, attr, value) {
>+    if (docURI instanceof Ci.nsIURI) {
>+      docURI = docURI.spec;
>+    }
>+
>+    // bug 319846 -- don't save really long attributes or values.
>+    if (id.length > 1024 || attr.length > 1024 || value.length > 1024)
>+      throw Components.Exception("id, attribute, or value too long", Cr.NS_ERROR_ILLEGAL_VALUE);
>+
>+    this.log("Saving " + attr + "=" + value + " for id=" + id + ", doc=" + docURI);
>+
>+    let obj = this._data;
>+    if (!(docURI in obj)) {
>+      obj[docURI] = {};
>+    }
>+    obj = obj[docURI];
>+    if (!(id in obj)) {
>+      obj[id] = {};
>+    }
>+    obj = obj[id];
>+
>+    // Don't set the value if it already set to avoid saving the file.

if it *is* already set


>+    if (attr in obj && obj[attr] == value)
>+      return;
>+
>+    obj[attr] = value; //IE, this._data[docURI][id][attr] = value;
>+
>+    this.markAsChanged();
>+  },
>+
>+  getIDsEnumerator : function (docURI) {
>+    let key = docURI.spec;

Change 'key' to docURI or something to be clearer.


>+    if (!(key in this._data))
>+      return new nsStringEnumerator([]);
>+
>+    this.log("Getting ID enumerator for doc=" + key);
>+
>+    let ids = [];
>+    let docs = this._data[key];
>+    if (docs) {
>+      for (let id in this._data[key]) {
>+        ids.push(id);
>+      }
>+    }

'docs' should instead be called 'ids' (and the array called something else) since the data is a list of ids not documents.

>+
>+    return new nsStringEnumerator(ids);
>+  },
>+
>+  getAttrsEnumerator : function (docURI, id) {
>+    let key = docURI.spec;

Same here as above as well as the remaining functions.


>+  getValue : function (docURI, id, attr) {
>+    let key = docURI.spec;
>+    this.log("get store value for id=" + id + ", attr=" + attr + ", doc=" + key);
>+
>+    let docs = this._data[key];
>+    if (docs) {
>+      let ids = docs[id];
>+      if (ids) {
>+        return ids[attr] || "";
>+      }
>+    }

Again, 'docs' should be called 'ids', and 'ids' should be called 'attrs'


>+
>+    return "";
>+  },
>+
>+  hasValue : function (docURI, id, attr) {
>+    let key = docURI.spec;
>+    this.log("has store value for id=" + id + ", attr=" + attr + ", doc=" + key);
>+
>+    let docs = this._data[key];
>+    if (docs) {
>+      let ids = docs[id];
>+      if (ids) {
>+        return attr in ids;
>+      }
>+    }

Same here.


>+
>+    return false;
>+  },
>+
>+  removeValue : function (docURI, id, attr) {
>+    let key = docURI.spec;
>+    this.log("remove store value for id=" + id + ", attr=" + attr + ", doc=" + key);
>+
>+    let docs = this._data[key];
>+    if (docs) {
>+      let ids = docs[id];
>+      if (ids && attr in ids) {
>+        delete ids[attr];

And here.

>+
>+        if (Object.getOwnPropertyNames(ids).length == 0) {
>+          delete docs[id];
>+        }

This should also delete from this._data if that is empty.

Can you reorder the functions to match that in the interface file?


>diff --git a/toolkit/components/xulstore/tests/chrome/window_persistence.xul b/toolkit/components/xulstore/tests/chrome/window_persistence.xul
...
>+function runTest(treeView)
>+{
>+  var firstRun = window.arguments[0];
>+  if (firstRun) {
>+    document.getElementById("button1").setAttribute("value", "Pressed");
>+    document.getElementById("button2").removeAttribute("value");
>+
>+    document.getElementById("button2").setAttribute("foo", "bar");
>+    document.persist("button2", "foo");
>+    document.getElementById("button2").removeAttribute("foo");
>+    document.persist("button2", "foo");

You should verify that the data is saved into the store in-between the two persist calls, and that it is removed afterwards.


>diff --git a/toolkit/components/xulstore/tests/xpcshell/test_XULStore.js b/toolkit/components/xulstore/tests/xpcshell/test_XULStore.js
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cu = Components.utils;
>+const Cr = Components.results;
>+
>+Components.utils.import("resource://gre/modules/Services.jsm");
>+Components.utils.import("resource://gre/modules/osfile.jsm")

You don't use Cu or Cr. But you do use Components.utils again.


>+add_task(function* testImport(){
>+  let src = "localstore.rdf";
>+  let dst = OS.Path.join(OS.Constants.Path.profileDir, src);
>+
>+  yield OS.File.copy(src, dst);
>+
>+  XULStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.mozIXULStore);
>+  checkOldStore();
>+});

You might want to add a comment here that this importing test relies on the xul store not yet being loaded/initialized before this point.


>diff --git a/xulrunner/app/profile/Makefile.in b/xulrunner/app/profile/Makefile.in
...
> FILES := \
>-	localstore.rdf \
> 	$(NULL)
> 
> libs:: $(FILES)
> 	$(INSTALL) $^ $(DIST)/bin/defaults/profile
> 	$(INSTALL) $^ $(DIST)/bin/defaults/profile/US

You should be able to remove the entire file. Also, I don't see the removed localstore.rdf in this patch.
Comment 35 Neil Deakin (away until Oct 3) 2014-07-10 12:01:50 PDT
Created attachment 8453947 [details]
fred.txt

This was a test that I had wrote a while ago which could be combined with your test_XULStore.js as it tests a few more things. testEnumerator is similar to your checkArrays.

I know that some of the comments above apply to code that I wrote. Once this is more polished we probably want another reviewer to take a quick look as well.
Comment 36 Neil Deakin (away until Oct 3) 2014-07-15 08:16:08 PDT
I tested the performance with the latest patch, similar to comment 14 above:

Setting an attribute that needs to be persisted:
  Existing code - 1400ms
  With this patch - 3000ms

Not sure what happened, but this version is significantly slower.

I also put together a quick webidl implementation and it was slightly faster at 2800ms.
Comment 37 Neil Deakin (away until Oct 3) 2014-07-15 08:18:22 PDT
Comment on attachment 8446082 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v8

Two other comments:

1. We should change the interface to be nsIXULStore rather than mozIXULStore.
2. It would be nice to have the interface take a document uri string rather than an nsIURI, since the implementation uses a string anyway.
Comment 38 Roberto Agostino Vitillo (:rvitillo) 2014-07-15 08:37:09 PDT
(In reply to Neil Deakin from comment #36)
> I tested the performance with the latest patch, similar to comment 14 above:
> 
> Setting an attribute that needs to be persisted:
>   Existing code - 1400ms
>   With this patch - 3000ms
> 
> Not sure what happened, but this version is significantly slower.
> 
> I also put together a quick webidl implementation and it was slightly faster
> at 2800ms.

I wonder though how much of a performance concern this really is; how often do we set attributes?
Comment 39 :Gijs Kruitbosch 2014-07-15 08:40:37 PDT
Quite a lot, cf.

http://mxr.mozilla.org/mozilla-central/search?string=%20persist%3D

ie whenever those are changed / the window is closed, and then:

http://mxr.mozilla.org/mozilla-central/search?string=document.persist

whenever that code is called.
Comment 40 Neil Deakin (away until Oct 3) 2014-07-18 07:02:27 PDT
Created attachment 8458681 [details] [diff] [review]
C++ implementation

It isn't going to be a noticeable performance issue to users, but I don't think we should be regressing performance if it isn't needed.

I found the unfinished C++ implementation I has started and have attached it, for future work.
Comment 41 Roberto Agostino Vitillo (:rvitillo) 2014-07-21 04:54:19 PDT
Created attachment 8459529 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v9

(In reply to Neil Deakin from comment #35)

Please note that in the updated patch I left the current implementation of locastore.rdf as it is for background compatibility. A warning message is issued when a client tries to access it though.

> >diff --git a/browser/components/places/tests/browser/browser_toolbar_migration.js b/browser/components/places/tests/browser/
> >@@ -93,43 +56,52 @@ function test_many_bookmarks_toolbar()
> >   ids.push(
> >     PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
> >                                           PlacesUtils.bookmarks.DEFAULT_INDEX)
> >   );
> >   ids.push(
> >     PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
> >                                           PlacesUtils.bookmarks.DEFAULT_INDEX)
> >   );
> >+  ids.push(
> >+    PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
> >+                                          PlacesUtils.bookmarks.DEFAULT_INDEX)
> >+  );
> 
> Why do you add this?

To comply with the condition in nsBrowserGlue.js, i.e.:

>        if (toolbarIsCustomized || getToolbarFolderCount() > 3) {
>          xulStore.setValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed", "false");
>        }

Note that the test fails on my machine even without the patch.

(In reply to Neil Deakin from comment #35)
> Created attachment 8453947 [details]
> fred.txt
> 
> This was a test that I had wrote a while ago which could be combined with
> your test_XULStore.js as it tests a few more things. testEnumerator is
> similar to your checkArrays.

I have added more tests to the test suite.
Comment 42 Roberto Agostino Vitillo (:rvitillo) 2014-07-22 05:08:08 PDT
https://tbpl.mozilla.org/?tree=Try&rev=7cf4f8129591
Comment 43 Birunthan Mohanathas [:poiru] 2014-07-22 11:08:51 PDT
FYI, bug 1040774 moved to rdf/base/src/nsRDFService.cpp to rdf/base/nsRDFService.cpp.
Comment 44 Marco Bonardo [::mak] 2014-07-23 08:17:41 PDT
(In reply to Neil Deakin from comment #36)
> I tested the performance with the latest patch, similar to comment 14 above:
> 
> Setting an attribute that needs to be persisted:
>   Existing code - 1400ms
>   With this patch - 3000ms

None of the 2 implementations will do any IO in this time, so looks like it's all overhead.  Did you profile where this time is spent with the js implementation? Before moving to cpp we should know why js is so much slower in this simple case.

Out of curiosity I tried calling Assert and (a locally built version of) setValue in a 1000 iterations loop but I'm very far from 3000ms, I'm around 7ms for both :/ If I increase iterations the js version wins (though I'm skipping xpcom overhead here but I suspect js to cpp is more expensive than js to js). May you share your perf testing code?
Comment 45 neil@parkwaycc.co.uk 2014-07-23 12:36:14 PDT
Comment on attachment 8459529 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v9

>-        // Get the datasource we intend to use to remember open state.
>-        nsAutoString datasourceStr;
>-        mRoot->GetAttr(kNameSpaceID_None, nsGkAtoms::statedatasource, datasourceStr);
>-
>-        // since we are trusted, use the user specified datasource
>-        // if non specified, use localstore, which gives us
>-        // persistence across sessions
>-        if (! datasourceStr.IsEmpty()) {
>-            gRDFService->GetDataSource(NS_ConvertUTF16toUTF8(datasourceStr).get(),
>-                                       getter_AddRefs(mPersistStateStore));
>-        }
I don't think this removal will break anything, but you should document it.
Comment 46 Neil Deakin (away until Oct 3) 2014-07-23 13:11:02 PDT
> Out of curiosity I tried calling Assert and (a locally built version of)
> setValue in a 1000 iterations loop but I'm very far from 3000ms, I'm around
> 7ms for both :/ If I increase iterations the js version wins (though I'm
> skipping xpcom overhead here but I suspect js to cpp is more expensive than
> js to js). May you share your perf testing code?

I am calling setAttribute 10000 times. This is C++ calling into the JS component.
Comment 47 Neil Deakin (away until Oct 3) 2014-07-23 13:12:33 PDT
>-nsresult
>-nsXULTreeBuilder::IsContainerOpen(nsIRDFResource* aResource, bool* aOpen)
>-{
>-    if (mPersistStateStore)
>-        mPersistStateStore->HasAssertion(aResource,
>-                                         nsXULContentUtils::NC_open,
>-                                         nsXULContentUtils::true_,
>-                                         true,
>-                                         aOpen);
>-    else
>-        *aOpen = false;
>+  nsIURI* docURI = doc->GetDocumentURI();
> 
>-    return NS_OK;
>+  nsAutoString nodeid;
>+  nsresult rv = aResult->GetId(nodeid);
>+  if (NS_FAILED(rv)) {
>+    return false;
>+  }
>+
>+
>+  nsAutoCString utf8uri;

Remove the extra blank line.


>diff --git a/rdf/base/src/nsRDFService.cpp b/rdf/base/src/nsRDFService.cpp
>     // Nope. So go to the repository to try to create it.
>     nsCOMPtr<nsIRDFDataSource> ds;
>     if (StringBeginsWith(spec, NS_LITERAL_CSTRING("rdf:"))) {
>+        if (spec.Equals("rdf:local-store")) {
>+            nsCOMPtr<nsIConsoleService> consoleService =
>+                do_GetService(NS_CONSOLESERVICE_CONTRACTID);
>+
>+            if (consoleService) {
>+                consoleService->LogStringMessage(NS_LITERAL_STRING(
>+                    "Warning: rdf:local-store is deprecated, please use XULStore.").get());
>+            }
>+        }
>+

Since you didn't remove the automatic local-store usages from content/xul/templates, this error will occur without a means to change it.
So I don't think you should add this warning.


>+  getIDsEnumerator : function (docURI) {
>+    if (!(docURI in this._data))
>+      return new nsStringEnumerator([]);
>+
>+    this.log("Getting ID enumerator for doc=" + docURI);

Let's be consistent and move the log line before the condition. Similarly for getAttrsEnumerator.


>+interface nsIXULStore: nsISupports
>+{
>+  /**
>+   * Sets a value in the store.
>+   *
>+   * @param docURI - document URI
>+   * @param id - identifier of the node
>+   * @param attr - attribute to store
>+   * @param value - value of the attribute
>+   */
>+  void setValue(in AString doc, in AString id, in AString attr, in AString value);

You use 'docURI' in the comment text but the argument is 'doc'. Same with the other functions.


>+  /**
>+   * Returns true if the store contains a value for attr.
>+   *
>+   * @param docURI - nsIURI of the document

This isn't an nsIURI any more.


>+add_task(function* testGetValue() {
>+  // Get non-existing property
>+  checkValue(browserURI, "side-window", "height", "");
>+
>+  // Get existing property
>+  checkValue(browserURI, "main-window", "width", "994");
>+});

hasValue already has these checks. Perhaps remove them there.


>+add_task(function* testSetValue() {
...
>+  // Add another attribute
>+  checkValue(browserURI, "side-bar", "heigth", "");

You've spelled 'height' wrong.


>+  XULStore.setValue(browserURI, "side-bar", "heigth", "1000");
>+  checkValue(browserURI, "side-bar", "heigth", "1000");
>+  checkArrays(["addon-bar", "main-window", "side-bar", "sidebar-title"], getIDs(browserURI));
>+  checkArrays(["width", "heigth"], getAttributes(browserURI, 'side-bar'));
>+});
>+
>+add_task(function* testRemoveValue() {
>+  // Remove first attribute
>+  checkValue(browserURI, "side-bar", "width", "1024");
>+  XULStore.removeValue(browserURI, "side-bar", "width")
>+  checkValue(browserURI, "side-bar", "width", "");

Add a hasValue check here as well. All of your removeValue lines should end with semicolons.


>+  // Removing from an id that doesn't exists shouldn't fail
>+  XULStore.removeValue(browserURI, "foo", "bar")

exists -> exist


>+
>+  // Removing from a document that doens' exists shouldn't fail
>+  let nonDocURI = "chrome://example/content/other.xul";
>+  XULStore.removeValue(nonDocURI, "foo", "bar")

doesn't exist


Add a test for getAttributes (getAttrsEnumerator) for non-existing documents and ids.

While thinking about this, getAttrsEnumerator should be renamed and spelled out as getAttributeEnumerator.

You might consider changing the interface to take string rather than AString to avoid having to convert UTF16 to UTF8 yourself.
Comment 48 Marco Bonardo [::mak] 2014-07-24 05:22:31 PDT
(In reply to Neil Deakin from comment #46)
> I am calling setAttribute 10000 times. This is C++ calling into the JS
> component.

I see, doesn't look like cpp to js is the more common case we have in the tree though, is it?
Comment 49 Marco Bonardo [::mak] 2014-07-24 06:14:50 PDT
Comment on attachment 8459529 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v9

Review of attachment 8459529 [details] [diff] [review]:
-----------------------------------------------------------------

Just some drive_by comments, I looked at certain parts better than others, so don't count on this as an official review :)

::: browser/components/places/content/treeView.js
@@ +17,5 @@
>    this._rows = [];
>    this._flatList = aFlatList;
>    this._openContainerCallback = aOnOpenFlatContainer;
>    this._controller = aController;
> +  this._xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);

please make this a lazy getter.

@@ +1119,4 @@
>    {
> +    let ioService = Cc["@mozilla.org/network/io-service;1"].
> +                    getService(Ci.nsIIOService);
> +    return ioService.newURI(document.URL, null, null);

I guess you might make documentURI a getter and memoize it? I don't expect it to change

::: content/xul/document/src/XULDocument.cpp
@@ +1335,5 @@
>      aElement->GetAttr(kNameSpaceID_None, aAttribute, valuestr);
>  
> +    nsresult rv;
> +    nsAutoCString utf8uri;
> +    rv = mDocumentURI->GetSpec(utf8uri);

nit: declare rv on first use

@@ +2095,5 @@
>      nsCOMArray<nsIContent> elements;
>  
> +    nsAutoCString utf8uri;
> +    nsresult rv;
> +    rv = mDocumentURI->GetSpec(utf8uri);

nit: declare at first use

@@ +2136,3 @@
>      }
>  
> +    return rv;

return NS_OK; errors are handled inline

@@ +2145,5 @@
>  {
>      nsresult rv;
>  
> +    nsAutoCString utf8uri;
> +    rv = mDocumentURI->GetSpec(utf8uri);

nit: ditto :)

@@ +2183,3 @@
>          uint32_t cnt = aElements.Count();
>  
> +        for (int32_t i = int32_t(cnt) - 1 ; i >= 0; --i) {

nit: remove whitespace before ;

@@ +2187,5 @@
> +            if (!element) {
> +                 continue;
> +            }
> +
> +            rv = element->SetAttr(kNameSpaceID_None, attr, value, PR_TRUE);

you are ignoring this error and overwriting it, it should be handled instead.

@@ +2192,4 @@
>          }
>      }
>  
> +    return rv;

NS_OK

::: content/xul/document/src/XULDocument.h
@@ +319,1 @@
>      bool                       mApplyingPersistedAttrs;

fix indentation per code module convention

::: toolkit/components/xulstore/XULStore.js
@@ +5,5 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;
> +
> +const debugMode = false;

please add comment about what this does

@@ +9,5 @@
> +const debugMode = false;
> +
> +// Delay when a change is made to when the file is saved.
> +// 30 milliseconds normally, or 3 milliseconds for testing
> +const WRITE_DELAY = (debugMode ? 3 : 30) * 1000;

As far as I can tell, this is 3 seconds or 30 seconds, not milliseconds (please fix the comment)
would also be nice to specify the unit in the const name, like WRITE_DELAY_MS

@@ +18,5 @@
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/NetUtil.jsm");
> +Components.utils.import("resource://gre/modules/osfile.jsm")

please lazyModuleGetter NetUtil and osfile

@@ +31,5 @@
> +                                    contractID: XULSTORE_CONTRACTID,
> +                                    classDescription: "XULStore",
> +                                    interfaces: [Ci.nsIXULStore]}),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsIXULStore,
> +                                         Ci.nsISupportsWeakReference]),

if it's expected to behave like a service you'd better add
_xpcom_factory: XPCOMUtils.generateSingletonFactory(XULStore),

unless you expect it might be useful to have multiple instances.

@@ +47,5 @@
> +   *      "chrome://foopy/b.xul" :  { ... },
> +   *      ...
> +   *  }
> +   */
> +  _data     : {},

multiple instances would also share this object since it's defined in the prototype (wanted or mistake? It's usually better to define local cache objects in the constructor)

@@ +52,5 @@
> +  _storeFile : null,
> +  _needsSaving : false,
> +
> +  init : function () {
> +    Services.obs.addObserver(this, "profile-before-change", false);

why did you add nsISupportsWeakReference but then add a strong ref? weak ref should be fine here (indeed you then forgot to removeObserver)

@@ +54,5 @@
> +
> +  init : function () {
> +    Services.obs.addObserver(this, "profile-before-change", false);
> +
> +    let filename = STOREDB;

what's the point? just name the const better if it's for readability, like STOREDB_FILENAME?

@@ +65,5 @@
> +      this.readFile();
> +    }
> +  },
> +
> +  observe: function(subject, topic, data) {

nit: you are mixing up styles, like "init : function ()" and "observe: function()", please be coherent (I think the latter style is more common)

@@ +67,5 @@
> +  },
> +
> +  observe: function(subject, topic, data) {
> +    if (topic == "profile-before-change") {
> +      this.writeFile();

would be nice to also stop accepting requests and warn quite loudly if something tries to add data after this point (also, avoid setting up timers from now on)

@@ +103,5 @@
> +      let resource = resources.getNext().QueryInterface(Ci.nsIRDFResource);
> +      let uri;
> +
> +      try {
> +        uri = Services.io.newURI(resource.ValueUTF8, "", null);

NetUtil.newURI(resource.ValueUTF8);

@@ +164,5 @@
> +      let data = JSON.stringify(this._data);
> +      let encoder = new TextEncoder();
> +
> +      data = encoder.encode(data);
> +      OS.File.writeAtomic(this._storeFile.path, data);

honestly, I'd pass tmpPath option here, it would make the likelyhoo of storing a corrupt store less likely, for a small price.
see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_async_front.jsm#1164

you should also handle error here, not just the exception but an eventual promise rejection should be handled and reportError'ed

@@ +182,5 @@
> +      this.writeFile();
> +    };
> +
> +    // Don't write the file more than once every 30 seconds.
> +    this._writeTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

you don't need to create a new timer instance every time, you can just create one and reuse it
Comment 50 Roberto Agostino Vitillo (:rvitillo) 2014-07-28 13:27:34 PDT
Created attachment 8463572 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v10

https://tbpl.mozilla.org/?tree=Try&rev=20887d6a7500
Comment 51 Roberto Agostino Vitillo (:rvitillo) 2014-07-28 13:28:50 PDT
Thanks for all your comments.
Comment 52 Günter 2014-07-29 04:30:35 PDT
Just to be sure, Reminderfox for Firefox/Thunderbird is using
> .getService(Components.interfaces.nsIRDFService);
to resolve the Thunderbird msgfolder from uri.

Would that be affected by this change?
Comment 53 Roberto Agostino Vitillo (:rvitillo) 2014-07-29 04:33:18 PDT
(In reply to Günter from comment #52)
> Just to be sure, Reminderfox for Firefox/Thunderbird is using
> > .getService(Components.interfaces.nsIRDFService);
> to resolve the Thunderbird msgfolder from uri.
> 
> Would that be affected by this change?

It wouldn't.
Comment 54 Roberto Agostino Vitillo (:rvitillo) 2014-07-29 04:38:37 PDT
Comment on attachment 8463572 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v10

Neil, you mentioned we should let this be reviewed by someone else too. Whom do you suggest? What about Marco, he already made some good remarks.
Comment 55 Neil Deakin (away until Oct 3) 2014-07-29 06:08:42 PDT
Marco would be ok . Or from a superreviewer (I would suggest bsmedberg or gavin).
Comment 56 Roberto Agostino Vitillo (:rvitillo) 2014-08-05 07:56:35 PDT
Jorge, Benjamin suggested to add a rule in the AMO automated checking/validation to warn addons that use localstore. Could you give me some pointers on the best way to do that? Thanks!
Comment 57 Jorge Villalobos [:jorgev] 2014-08-05 14:53:11 PDT
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #56)
> Jorge, Benjamin suggested to add a rule in the AMO automated
> checking/validation to warn addons that use localstore. Could you give me
> some pointers on the best way to do that? Thanks!

We got this. Just make sure the milestone is properly set when the bug lands.
Comment 58 Benjamin Smedberg [:bsmedberg] 2014-08-07 09:40:06 PDT
Comment on attachment 8463572 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v10

I'm a little sad about the nsIStringEnumerator stuff here. I wonder if you can do better by declaring this in webidl instead of xpidl and returning arrays of strings?

This currently imports from localstore using the C++ RDF stuff. Can you file a followup bug to import using a .jsm instead, so we can think about removing the C++ RDF bits?

Are you set on the name "xulstore"? This is basically just a key/key/value datastore, which if it's relatively persistent is likely to be useful for things other than XUL.

I didn't do a detailed code review here, but I did read over the interface/tests usage and I don't see problems with this.
Comment 59 Roberto Agostino Vitillo (:rvitillo) 2014-08-12 07:19:58 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #58)
> Are you set on the name "xulstore"? This is basically just a key/key/value
> datastore, which if it's relatively persistent is likely to be useful for
> things other than XUL.

Do you have any particular name in mind?
Comment 60 Benjamin Smedberg [:bsmedberg] 2014-08-12 09:44:47 PDT
kvstore?
Comment 61 Marco Bonardo [::mak] 2014-08-12 12:31:24 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #58)
> This is basically just a key/key/value
> datastore, which if it's relatively persistent is likely to be useful for
> things other than XUL.

I honestly hope nobody is going to use this synchronous store on main-thread apart from localstore.  And it can't be used off the main thread cause it's a plain js component.  Naming it too generically like keyValueStore or such, would make it look like a one-size-fits-all solution to store any kind of thing. We know it's not the kind of kv store we want add-ons to use. I'd personally prefer a dedicated name like xulPersistenceStore than a generic one.
Comment 62 Olli Pettay [:smaug] (TPAC) 2014-08-15 12:42:26 PDT
Implementing a service using webidl sounds rather odd. What would be the parent object for such thing?
We could hack somehow that the service would get junk scope as parent, but I doubt
bholley would accept that.
Comment 63 Roberto Agostino Vitillo (:rvitillo) 2014-08-18 05:29:50 PDT
Benjamin, please have a look at comment 61 and 62. I am suggesting to leave the name as is and not using a webidl interface, unless there is a clean way to do it.
Comment 64 Benjamin Smedberg [:bsmedberg] 2014-08-18 13:51:02 PDT
ok, if this is really not intended to be a generic kvstore, then the current name is fine.

As for webidl, bholley explicitly said that he thought we could use .webidl for JS components and it improves the array junk significantly. But the current version is fine especially if this is meant to be internal-only.
Comment 65 Roberto Agostino Vitillo (:rvitillo) 2014-08-19 10:31:11 PDT
Created attachment 8475301 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v11

Unbitrotted. https://tbpl.mozilla.org/?tree=Try&rev=54213567e4ad
Comment 66 Ryan VanderMeulen [:RyanVM] 2014-08-20 07:25:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/143ae44587b2
Comment 67 neil@parkwaycc.co.uk 2014-08-20 08:09:36 PDT
Comment on attachment 8475301 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v11

>+  // Retrieves an nsIURI for the document
It just occurs to me that maybe document.documentURIObject is what you want?
Comment 69 Roberto Agostino Vitillo (:rvitillo) 2014-08-20 09:39:23 PDT
Created attachment 8476012 [details] [diff] [review]
Talos: Allow IO on xulstore.json
Comment 70 Joel Maher ( :jmaher) 2014-08-20 09:51:01 PDT
Comment on attachment 8476012 [details] [diff] [review]
Talos: Allow IO on xulstore.json

Review of attachment 8476012 [details] [diff] [review]:
-----------------------------------------------------------------

thanks, please land this on the talos repo.
Comment 71 Roberto Agostino Vitillo (:rvitillo) 2014-08-20 09:52:49 PDT
Ryan, could you please land Attachment #8476012 [details] [diff] on the talos repo?
Comment 72 Ryan VanderMeulen [:RyanVM] 2014-08-20 10:16:53 PDT
I think Joel's taking care of pushing the talos patch.
Comment 73 Ryan VanderMeulen [:RyanVM] 2014-08-20 10:59:02 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/25c918c5f3e1
Comment 74 Ryan VanderMeulen [:RyanVM] 2014-08-20 13:43:10 PDT
https://hg.mozilla.org/mozilla-central/rev/25c918c5f3e1
Comment 75 Serge Gautherie (:sgautherie) 2014-08-21 02:21:31 PDT
(In reply to neil@parkwaycc.co.uk from comment #67)
> Comment on attachment 8475301 [details] [diff] [review]
> Bug 559505 - localstore.rdf kills ponies, v11
> 
> >+  // Retrieves an nsIURI for the document
> It just occurs to me that maybe document.documentURIObject is what you want?
Comment 76 Roberto Agostino Vitillo (:rvitillo) 2014-08-21 02:23:52 PDT
I have seen that, I am going to create a follow up bug to clean this up.
Comment 77 Thomas D. (currently busy elsewhere; needinfo?me) 2014-08-22 00:18:48 PDT
Looks like this breaks things in Thunderbird which must be addressed asap.

-------- Forwarded Message --------
Subject: 	Re: Breakage between 08-19 and 08-21
Date: 	Fri, 22 Aug 2014 09:09:11 +0300
From: 	Magnus Melin
To: 	tb-planning

Bug 559505 should account for at least part of the breakage I think.

 -Magnus

On 22.8.2014 08:13, Jonathan Protzenko wrote:
> Hi,
>
> I noticed several things breaking all at once over the past few days.
> - In 08-19, everything is fine as far as I can tell.
> - In 08-20, the call I am making (in Conversations) to
> nsIMsgCompose::initEditor fails with an error "Failure: arg 0" in the error
> console. Do you if something changed in this area?
> http://hg.mozilla.org/comm-central/pushloghtml gives little clues, so I
> suspect it must be in mozilla-central.
> - In 08-21, several things are broken at once: the thread summary no longer
> works, tabs are not saved, the state of the folder pane (unified vs. all vs.
> ...) is not saved either. Do you have any hints as to what bugs could be
> causing this?
>
> In the meantime, I would advise _not_ to use 08-21 because it seems badly
> impacted. I can file bugs if that helps.
>
> Cheers,
>
> ~ jonathan
Comment 78 Sebastian Zartner [:sebo] 2014-08-22 05:36:41 PDT
Looks like this broke Firebug, which relies on PlacesUIUtils.localStore. This change should at least be documented at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Places_utilities_for_JavaScript.

Sebastian
Comment 79 Roberto Agostino Vitillo (:rvitillo) 2014-08-22 05:54:15 PDT
(In reply to Sebastian Zartner from comment #78)
> Looks like this broke Firebug, which relies on PlacesUIUtils.localStore.
> This change should at least be documented at
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/
> Places_utilities_for_JavaScript.
> 
> Sebastian

Done, thanks. How does Firebug use localstore? Do you use it to access properties set by other components or do you just use it to store Firebug-local data?
Comment 80 Sebastian Zartner [:sebo] 2014-08-22 15:00:55 PDT
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #79)
> (In reply to Sebastian Zartner from comment #78)
> > Looks like this broke Firebug, which relies on PlacesUIUtils.localStore.
> > This change should at least be documented at
> > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/
> > Places_utilities_for_JavaScript.
> > 
> > Sebastian
> 
> Done, thanks.

Thanks. Of course nsIXULStore still needs to be described at MDN. Referring people to 'xulstore' without giving any hint of how it works is a bit bad.

> How does Firebug use localstore? Do you use it to access
> properties set by other components or do you just use it to store
> Firebug-local data?

Just to store Firebug-local data[1], specifically the width and height of the Firebug UI[2].
For reference, I created an issue for that in the Firebug issue tracker[3]. A quick hint on how to change the current code would be nice.

Sebastian

[1] https://github.com/firebug/firebug/blob/0f024e7fee4c208a641f73db645084b8e9a38c95/extension/content/firebug/firefox/browserOverlayLib.js#L273
[2] https://github.com/firebug/firebug/blob/0f024e7fee4c208a641f73db645084b8e9a38c95/extension/content/firebug/firefox/browserOverlay.js#L167
[3] https://code.google.com/p/fbug/issues/detail?id=7640
Comment 81 Roberto Agostino Vitillo (:rvitillo) 2014-08-22 15:17:34 PDT
(In reply to Sebastian Zartner from comment #80)
> Thanks. Of course nsIXULStore still needs to be described at MDN. Referring
> people to 'xulstore' without giving any hint of how it works is a bit bad.

As xulstore is meant as a strict replacement for localstore.rdf and not a generic kv-store, I wouldn't want to advertise it too much. Please have a look at the interface documentation at http://dxr.mozilla.org/mozilla-central/source/toolkit/components/xulstore/nsIXULStore.idl.

> Just to store Firebug-local data[1], specifically the width and height of
> the Firebug UI[2].
> For reference, I created an issue for that in the Firebug issue tracker[3].
> A quick hint on how to change the current code would be nice.

I will comment on the issue tracker asap.
Comment 82 Neil Deakin (away until Oct 3) 2014-08-24 13:46:42 PDT
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #81)
> As xulstore is meant as a strict replacement for localstore.rdf and not a
> generic kv-store, I wouldn't want to advertise it too much.

Note though that localstore.rdf is meant to be a generic datastore, so there may some addons that use it that way.

The links pointed to in comment 80 though suggest they are just updating persist data using the rdf api rather than setting attributes and so forth, so it should be a simple conversion to use nsIXULStore instead.

But I would suggest creating a short migration guide.
Comment 83 neil@parkwaycc.co.uk 2014-08-25 15:05:20 PDT
Comment on attachment 8475301 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v11

>+          let docURI = this._getDocumentURI();
>+          let val = this._xulStore.getValue(docURI, uri, "open");
...
>+  // Retrieves an nsIURI for the document
>+  _documentURI: null,
>+  _getDocumentURI: function()
>   {
>-    let uri = aNode.uri;
>-    NS_ASSERT(uri, "if there is no uri, we can't persist the open state");
>-    return uri ? PlacesUIUtils.RDF.GetResource(uri) : null;
>+    if (!this._documentURI) {
>+      let ioService = Cc["@mozilla.org/network/io-service;1"].
>+                      getService(Ci.nsIIOService);
>+      this._documentURI = ioService.newURI(document.URL, null, null);
>+    }
>+    return this._documentURI;
>   },
...
>+  /**
>+   * Retrieves a value in the store, or an empty string if it does not exist.
>+   *
>+   * @param doc - document URI
>+   * @param id - identifier of the node
>+   * @param attr - attribute to retrieve
>+   *
>+   * @returns the value of the attribute
>+   */
>+  AString getValue(in AString doc, in AString id, in AString attr);

I just noticed comment 47 - you don't need the document URI as an nsIURI, so just document.URL suffices, i.e.
let val = this._xulStore.getValue(document.URL, uri, "open");
Comment 84 neil@parkwaycc.co.uk 2014-08-25 15:17:43 PDT
I see that the old Places persist data doesn't get "upgraded" into xulstore.json (I'm not going to lose sleep over it but I didn't notice it mentioned before.)

The old Places code also tried to share the persist of the twisty open states between the sidebar and the library. The new code seems to achieve this by virtue of the bug mentioned in my previous comment.
Comment 85 neil@parkwaycc.co.uk 2014-08-25 15:22:16 PDT
Ah, maybe that doesn't apply to the Library, only the old Bookmarks and History windows, so it makes no difference there.

Note You need to log in before you can comment on or make changes to this bug.