Closed
Bug 1287091
Opened 8 years ago
Closed 8 years ago
ContextualIdentityService should use a database to store Containers
Categories
(Core :: DOM: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
References
Details
(Whiteboard: [domsecurity-active])
Attachments
(5 files, 7 obsolete files)
2.85 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
12.92 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
3.15 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
14.35 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
977 bytes,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
in order to support the creation of new containers, we must store them somewhere.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8771430 -
Flags: review?(jvarga)
Attachment #8771430 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 2•8 years ago
|
||
Comment on attachment 8771430 [details] [diff] [review]
about_containers_4.patch
Review of attachment 8771430 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/contextualidentity/content/aboutContainers.js
@@ +20,5 @@
> while (containersList.lastChild) {
> containersList.lastChild.remove();;
> }
>
> + let identities = ContextualIdentityService.getIdentities(identities => {
ContextualIdentityService.getIdentities(identities => {
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8771430 -
Attachment is obsolete: true
Attachment #8771430 -
Flags: review?(jvarga)
Attachment #8771430 -
Flags: review?(gijskruitbosch+bugs)
Attachment #8771447 -
Flags: review?(jvarga)
Attachment #8771447 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 4•8 years ago
|
||
Sorry for the spam.
Attachment #8771447 -
Attachment is obsolete: true
Attachment #8771447 -
Flags: review?(jvarga)
Attachment #8771447 -
Flags: review?(gijskruitbosch+bugs)
Attachment #8771450 -
Flags: review?(jvarga)
Attachment #8771450 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 5•8 years ago
|
||
With this patch we can create/remove/update a container. without UI, without any check. Just a WIP.
Updated•8 years ago
|
Whiteboard: [domsecurity-active]
Comment 6•8 years ago
|
||
Comment on attachment 8771450 [details] [diff] [review]
db.patch
Review of attachment 8771450 [details] [diff] [review]:
-----------------------------------------------------------------
I'm sorry to be a pain, but please could you push this to mozreview? A lot of the changes are whitespace only, or involve minor changes to lines that otherwise remain identical, and it's a pain to review them with the diff visuals that splinter provides. I have to manually check that indented code stays the same (or one variable in the line changed names while the rest stayed the same), and it makes it much harder to spot mistakes and more time-consuming to review.
Generally, can you provide some background as to why we're trying to switch this to an async relational DB system rather than e.g. prefs or an in-memory store that gets serialized to JSON and back? Both of those are in common use and seem like they would support everything we do here - there's no systematic query-ing of the relational DB at all, even where that might be appropriate - we just use getAll() and then filter in JS. :-\
The async-ness worries me. I would prefer having async initialization that blocks startup at some point, rather than this change. It feels like we would not usually notice any difference on our (fast) systems, but on slower systems or in 'interesting' circumstances with multiple DB accesses or whatever, we might be left with un-filled context menus, or non-matching labels / colors. Already the asynchronicity probably introduces more layout/style flushes. For things like synced tabs, we provide a "we're fetching these" view. Not sure if doing that here would be overkill or if we should at least provide some kind of disabled menuitem that says (fetching containers) or the like.
::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +15,5 @@
> XPCOMUtils.defineLazyGetter(this, "gBrowserBundle", function() {
> return Services.strings.createBundle("chrome://browser/locale/browser.properties");
> });
>
> +this.CONTAINERSDB_NAME = "settings";
Seems like we should use something clearer and less generic than "settings" as the name for the file/db.
@@ +21,5 @@
> +this.CONTAINERSSTORE_NAME = "settings";
> +
> +function _ContextualIdentityService() {}
> +_ContextualIdentityService.prototype = {
> + __proto__: IndexedDBHelper.prototype,
Please use Object.create() instead, __proto__ is non-standard.
@@ +72,5 @@
>
> + upgradeSchema: function upgradeSchema(transaction, db, oldVersion,
> + newVersion) {
> + if (oldVersion != 0 || newVersion != 1) {
> + dump("ContextualIdentityService - ERROR - unknown schema version!!!\n");
Please use Cu.reportError() instead.
@@ +86,3 @@
> },
>
> + txn: function(txnType, cb) {
Do we have to mirror the cryptic naming? Can't we just call this newTransaction() ?
@@ +95,5 @@
> },
>
> + getIdentities(cb) {
> + this.txn("readonly", function(txn, store) {
> + store.getAll().onsuccess = function(event) {
What happens if two transactions get created using this method? Do they just get queued? Something else?
@@ +157,3 @@
>
> + if (!identity.alreadyOpened) {
> + identity.alreadyOpened = true;
Where/how does this get reflected back into the DB and/or other objects? All those transactions are labeled "readonly", is that just not true? Does this even want storing in the DB at all? If not, how does that jive with continually re-getting it? Shouldn't that info be local to the JSM and thrown away on exit? That means this might not even need to be async...
Attachment #8771450 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8771450 -
Attachment is obsolete: true
Attachment #8771460 -
Attachment is obsolete: true
Attachment #8771450 -
Flags: review?(jvarga)
Attachment #8772527 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8772529 -
Flags: review?(gijskruitbosch+bugs)
Comment 9•8 years ago
|
||
Comment on attachment 8772527 [details] [diff] [review]
json.patch
Review of attachment 8772527 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/preferences/cookies.js
@@ +12,1 @@
>
This file only loads when you open the preferences, so it seems OK to just leave this as a lazy getter. If you do add it, please keep the list of imports alphabetically sorted.
::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +129,5 @@
> + this.saveSoon();
> + },
> +
> + saveSoon() {
> + this._saver.arm();
Should we file a followup bug to make sure a save blocks shutdown, or do you want to take care of this here?
@@ +133,5 @@
> + this._saver.arm();
> + },
> +
> + save() {
> + let bytes = gTextEncoder.encode(JSON.stringify(this._identities));
As written this will write the 'alreadyOpen' variable and then restore it after startup. That seems wrong.
@@ +135,5 @@
> +
> + save() {
> + let bytes = gTextEncoder.encode(JSON.stringify(this._identities));
> + return OS.File.writeAtomic(this._path, bytes,
> + { tmpPath: this._path + ".tmp" })
Nit: need a semicolon at the end here. Is eslint not running for these files?
Attachment #8772527 -
Flags: review?(gijskruitbosch+bugs) → feedback+
Comment 10•8 years ago
|
||
Comment on attachment 8772529 [details] [diff] [review]
part 2
Review of attachment 8772529 [details] [diff] [review]:
-----------------------------------------------------------------
Ah, this is what I get for reviewing patches one by one... rs=me for using a Set rather than an array - sorry about the mistaken comment about the alreadyOpened thing on the first patch.
::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +74,4 @@
> ],
>
> _identities: null,
> + _openedIdentities: [],
Nit: openedIdentities: new Set(),
@@ +209,5 @@
> if (!identity || !identity.public) {
> return;
> }
>
> + if (this._openedIdentities.indexOf(userContextId) == -1) {
this._openedIdentities.has(userContextId)
@@ +210,5 @@
> return;
> }
>
> + if (this._openedIdentities.indexOf(userContextId) == -1) {
> + this._openedIdentities.push(userContextId);
s/push/add/
Attachment #8772529 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8772527 -
Attachment is obsolete: true
Attachment #8772791 -
Flags: review?(gijskruitbosch+bugs)
Comment 12•8 years ago
|
||
Comment on attachment 8772791 [details] [diff] [review]
part 1 - JSON
Review of attachment 8772791 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +11,3 @@
>
> +const DEFAULT_TAB_COLOR = "#909090";
> +const SAVE_DELAY_MS = 1500
Missing semicolon
Attachment #8772791 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8772838 -
Flags: review?(gijskruitbosch+bugs)
Comment 14•8 years ago
|
||
Comment on attachment 8772838 [details] [diff] [review]
part 3 - versioning
Review of attachment 8772838 [details] [diff] [review]:
-----------------------------------------------------------------
What's the lastUserContextId?
Assignee | ||
Comment 15•8 years ago
|
||
Something that I'll use in a completely different patch. Still uploaded here in less than 1 minute.
Assignee | ||
Comment 16•8 years ago
|
||
This is the last patch for this bug.
It's easier for me to land this block of code also if create/remove/update are not currently in used anywhere. But they will be in about:containers, eventually.
Attachment #8772906 -
Flags: review?(gijskruitbosch+bugs)
Comment 17•8 years ago
|
||
Comment on attachment 8772906 [details] [diff] [review]
part 4 - create/remove/update
Review of attachment 8772906 [details] [diff] [review]:
-----------------------------------------------------------------
r- because the addition code is busted if you've removed items. This should also have some tests. :-)
::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +160,5 @@
> },
>
> + create(name, icon, color) {
> + let identity = {
> + userContextId: ++this._lastUserContextId,
OK, so we're mixing things up a bit here. As written, the IDs are sequential, but the private one can't be removed with remove(), and you're splicing (!) the removed items out. So consider: we start with 5 items:
1: id 1
2: id 2
3: id 3
4: id 4
5: id 5 (and private)
and the length is 5.
So if you add an item immediately, the id would be 6. Let's not do that, it's not very interesting.
Instead, let's say you remove item (3).
Now the length is 4
Then you add a new item.
What ID will we pick?
IOW, I don't think this code works.
I also think it needs unit tests. It's easy to land when nothing exercises it, but we should have slightly higher standards for code on m-c. :-)
@@ +164,5 @@
> + userContextId: ++this._lastUserContextId,
> + public: true,
> + icon: icon,
> + color: color,
> + name: name
nit:
icon,
color,
name
(when the property and the local variable are the same, you don't need to repeat the key: value in ES2015 or whatever version of JS we're up to)
@@ +175,5 @@
> + },
> +
> + update(userContextId, name, icon, color) {
> + let found = false;
> + this._identities.forEach((identity, index) => {
let identity = this._identities.find(identity => identity.userContextid == userContextId && identity.public);
if (identity) {
identity.name = name;
identity.color = color;
// etc.
}
return !!identity;
@@ +180,5 @@
> + if (identity.userContextId == userContextId && identity.public) {
> + this._identities[index].name = name;
> + this._identities[index].color = color;
> + this._identities[index].icon = icon;
> + delete this._identities[index].label;
Can we change "label" to be more explicitly a localization ID, maybe "l10nID" or something? That makes more sense of this code.
Attachment #8772906 -
Flags: review?(gijskruitbosch+bugs) → review-
Comment 18•8 years ago
|
||
Comment on attachment 8772838 [details] [diff] [review]
part 3 - versioning
Review of attachment 8772838 [details] [diff] [review]:
-----------------------------------------------------------------
Clearing review because I think it might end up being this code that you also end up updating to make sure new IDs don't conflict. Because IDs shouldn't change (because they're used internally all over the place), we can't really renumber them. Instead, we should have code that determines the first free ID, or we should assume that the array is sorted and use the last item's ID. I would be OK with either of those, but if we do the latter we should ensure that the IDs are sequential when we load the file and replace them with the defaults if not. Really Bad Things would happen if IDs get overwritten or mixed up (things would (not) be private when we expect them (not) to, or be mislabeled, or...)
Attachment #8772838 -
Flags: review?(gijskruitbosch+bugs)
Comment 19•8 years ago
|
||
Final bit of feedback: we will need observer notifications or other types of communication to ensure that if the user updates labeling/colors/icons in about:containers, the relevant bits of UI actually update. If you're not planning to take care of that in this bug, can you ensure we file a followup bug? :-)
This is particularly complicated for the 'remove' case. What happens to open browsers with that userContextID? What happens when we ask for labels/colors/icons for a userContextID that has been removed?
Assignee | ||
Comment 20•8 years ago
|
||
> What ID will we pick?
6. This is the reason why I use and store lastUsedContextId. I don't count/iterate to know the 'next' one. I store the lastUserContextID into the JSON object in order to avoid this particular issue.
> IOW, I don't think this code works.
I'll add some tests.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 21•8 years ago
|
||
Right. Bug 1288029 has been filed for this reason. What I think it's the correct approach is:
1. you cannot disable Containers from pref if some of them are opened.
2. you cannot delete a container if it's opened.
But this check should not be in the ContextualIdentityService, imo. Maybe some helper function yes, but the logic of remove/update/create should be kept simple.
Comment 22•8 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #20)
> > What ID will we pick?
>
> 6. This is the reason why I use and store lastUsedContextId. I don't
> count/iterate to know the 'next' one. I store the lastUserContextID into the
> JSON object in order to avoid this particular issue.
Ugh, sorry, I got confused because you use the array length when you initially set lastUserContextId. But you never actually update it except by incrementing it.
> > IOW, I don't think this code works.
>
> I'll add some tests.
I think that's still a good idea. :-)
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 23•8 years ago
|
||
With tests.
Attachment #8772906 -
Attachment is obsolete: true
Attachment #8773256 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 24•8 years ago
|
||
Attachment #8773359 -
Flags: review?(gijskruitbosch+bugs)
Comment 25•8 years ago
|
||
Comment on attachment 8773256 [details] [diff] [review]
part 4 - create/remove/update
Review of attachment 8773256 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +1,5 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
> * License, v. 2.0. If a copy of the MPL was not distributed with this
> * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>
> +dump("A\n");
Please remove this.
@@ +165,5 @@
>
> + create(name, icon, color) {
> + let identity = {
> + userContextId: ++this._lastUserContextId,
> + public: true, icon, color, name
Nit: I'd leave them on individual lines, and have a comma after the last one, to make it easy and non-blame-modifying to add more properties.
@@ +182,5 @@
> + identity.name = name;
> + identity.color = color;
> + identity.icon = icon;
> + delete this._identities.l10nID;
> + delete this._identities.accessKey;
These deletes look wrong. You want:
delete identity.l10nID
delete identity.accessKey
right?
@@ +192,5 @@
> +
> + remove(userContextId) {
> + let found = false;
> +
> + this._identities.forEach((identity, index) => {
This can use findIndex, which avoids having a separate bool 'found' and a level of nesting, and looping over the rest of the array once you've found the item you're looking for, ie:
let index = this._identities.findIndex(i => i.userContextId == userContextId && identity.public);
if (index > -1) {
...
}
return index > -1;
@@ +231,5 @@
> },
>
> getIdentities() {
> this.ensureDataReady();
> return this._identities.filter(info => info.public);
Do we need to make sure the stuff we're handing out here is read-only or a copy, rather than references to the original objects? Because right now, you could call this and assign to identities[2].name and it would modify the internal state of the CIS.
::: toolkit/components/contextualidentity/tests/unit/test_basic.html
@@ +46,5 @@
> +});
> +
> +// Remove an identity
> +add_task(function() {
> + equal(cis.getIdentities().length, 5, "Expected 5 containers.");
This depends on the previous task, so it should probably be in the same task function.
@@ +58,5 @@
> +// Update an identity
> +add_task(function() {
> + ok(!!cis.getIdentityFromId(2), "Identity 2 exists");
> +
> + equal(cis.update(-1, "Container", "Icon", "Color"), false, "Update returns true if everything is OK");
Nit: update assertion message here.
::: toolkit/components/contextualidentity/tests/unit/xpcshell.ini
@@ +1,3 @@
> +[DEFAULT]
> +
> +[test_basic.html]
This should be a .js file.
Attachment #8773256 -
Flags: review?(gijskruitbosch+bugs) → review+
Updated•8 years ago
|
Attachment #8772838 -
Flags: review+
Comment 26•8 years ago
|
||
Comment on attachment 8773359 [details] [diff] [review]
part 5 - remove containers for real
Review of attachment 8773359 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/contextualidentity/content/aboutContainers.js
@@ +64,5 @@
> +
> + refreshUI();
> +});
> +
> +function refreshUI() {
We could also just make the UI execute the updates itself instead of re-writing the entire list. Make remove() remove the item, create() just render the new item (for which we already have a helper), and update() just call replaceChild() with a re-rendered new item and a ref to the old one.
@@ +110,5 @@
> + "#00a7e0");
> + refreshUI();
> +}
> +
> +function preferences(userContextId) {
Maybe we can just land this without the "testing" code and buttons, and add this functionality in a followup? Either that or keep iterating, I guess. But I'm assuming you're not expecting me to review the testing code...
@@ +134,3 @@
> }
>
> function remove(userContextId) {
Shouldn't this be checking that there's no open browsers with this userContextId etc.?
@@ +134,4 @@
> }
>
> function remove(userContextId) {
> + ContextualIdentityService.remove(parseInt(userContextId));
Feels like we should make sure the functions here get sanitized arguments, and the click handler gets the usercontextid and makes it sane. :-)
Attachment #8773359 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 27•8 years ago
|
||
Sorry, that was the wrong patch.
Attachment #8773359 -
Attachment is obsolete: true
Attachment #8773744 -
Flags: review?(gijskruitbosch+bugs)
Comment 28•8 years ago
|
||
Comment on attachment 8773744 [details] [diff] [review]
part 5 - remove containers for real
Review of attachment 8773744 [details] [diff] [review]:
-----------------------------------------------------------------
rs=me though this will need rebasing based on the feedback on the earlier patch.
::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +198,5 @@
> this._identities.splice(index, 1);
> this._openedIdentities.delete(userContextId);
> +
> + Services.obs.notifyObservers(null, "clear-origin-data",
> + JSON.stringify({ userContextId }));
What happens with history/formdata etc.? I'm a bit surprised this doesn't use the sanitizer.
Attachment #8773744 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 29•8 years ago
|
||
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acbca9af0a3b
part 1 - ContextualIdentityService should use a storage for the containers, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/939d5064ab91
part 2 - ContextualIdentityService must store telemetry data in a separate array, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed07b35bad6
part 3 - ContextualIdentityService JSON file should have versions, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/edad0174cb12
part 4 - ContextualIdentityService create/remove/update, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c57fd3af416e
part 5 - ContextualIdentityService should dispatch 'clear-origin-data' when a container is deleted, r=Gijs
All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/820961ff0369 for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32459345&repo=mozilla-inbound#L18150
Flags: needinfo?(amarchesini)
Comment 31•8 years ago
|
||
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7c4e797f76
part 1 - ContextualIdentityService should use a storage for the containers, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/93fa501cfd3a
part 2 - ContextualIdentityService must store telemetry data in a separate array, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5efa902cbe5
part 3 - ContextualIdentityService JSON file should have versions, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c01aeb081e2d
part 4 - ContextualIdentityService create/remove/update, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/165e7b455b8e
part 5 - ContextualIdentityService should dispatch 'clear-origin-data' when a container is deleted, r=Gijs
Comment 32•8 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=32499351&repo=mozilla-inbound
Comment 33•8 years ago
|
||
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad8ed662107
Backed out changeset 165e7b455b8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/50b8cc9b8e57
Backed out changeset c01aeb081e2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/37e221f3b0df
Backed out changeset c5efa902cbe5
https://hg.mozilla.org/integration/mozilla-inbound/rev/905027321962
Backed out changeset 93fa501cfd3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0f3616c00a
Backed out changeset bf7c4e797f76 for bc3/bc6 test failures
Comment 34•8 years ago
|
||
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62eb0641f4be
part 1 - ContextualIdentityService should use a storage for the containers, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddc292500b8f
part 2 - ContextualIdentityService must store telemetry data in a separate array, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/57942a8e13b2
part 3 - ContextualIdentityService JSON file should have versions, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/11a988cd506a
part 4 - ContextualIdentityService create/remove/update, r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/f577fea91160
part 5 - ContextualIdentityService should dispatch 'clear-origin-data' when a container is deleted, r=Gijs
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(amarchesini)
Comment 35•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/62eb0641f4be
https://hg.mozilla.org/mozilla-central/rev/ddc292500b8f
https://hg.mozilla.org/mozilla-central/rev/57942a8e13b2
https://hg.mozilla.org/mozilla-central/rev/11a988cd506a
https://hg.mozilla.org/mozilla-central/rev/f577fea91160
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 36•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #6)
> Generally, can you provide some background as to why we're trying to switch
> this to an async relational DB system rather than e.g. prefs or an in-memory
> store that gets serialized to JSON and back? Both of those are in common use
> and seem like they would support everything we do here - there's no
> systematic query-ing of the relational DB at all, even where that might be
> appropriate - we just use getAll() and then filter in JS. :-\
There is no reply to this question in any of these comments. Why was this necessary? What did the usage of prefs not allow?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 37•8 years ago
|
||
> There is no reply to this question in any of these comments. Why was this
> necessary? What did the usage of prefs not allow?
We decided to serialize everything into a JSON file. We also use a optimistic approach where the JSON file is loaded at the first execution of ContextualIdentityService.jsm in order to provide a sync API. If the loading is too slow, we lock the UI as we do for other similar components.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 39•8 years ago
|
||
The reason why I prefer a JSON file is because we are currently storing the 4 default containers, each one with an icon name and a color. The user can also create more containers and maybe in the future, customize the icons. I think it's better to have a separate file for all of these instead use/abuse of prefs.
Flags: needinfo?(amarchesini)
Comment 40•8 years ago
|
||
This added significant code overhead when prefs would have been sufficient. You can see the prefs for browser.contentHandlers.types.*.title for an example of how we could have done this pretty easily with prefs, or we could have stored a JSON blob as a pref value like we do for browser.uiCustomization.state.
Comment 41•8 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #40)
> This added significant code overhead when prefs would have been sufficient.
> You can see the prefs for browser.contentHandlers.types.*.title for an
> example of how we could have done this pretty easily with prefs,
Number of reasons to not use prefs here, mainly perf/convenience/UX-related:
- it's kind of an abuse of prefs to keep values that aren't actually user preference related, such as telemetry identifiers, internal state tracking of which containers are open, etc.
- keeping track of potentially localizable content (labels + access keys) inside prefs is... odd.
- things like the images used for the containers could potentially be quite large if we end up storing image data in data URIs or similar. Depends how we let people pick those images, but that wasn't clear when this patch was written.
- the perf characteristics of prefs (main thread sync IO, writing the entire file every time one part of any Fx/platform pref changes) aren't very nice
- the default containers are removable, so they can't be default pref values because then you can't remove them, which means you have to store all the container info in user prefs (which get re-read/written all the time), so it's not very efficient and there's no good way to "reset" them.
- "good" prefs are synced with the UI, and about:config lets people toggle prefs. If I change the prefs, that takes effect. In the case of containers, when removing custom containers we want to do things like close all the relevant container tabs. There's no way to enforce that with prefs, meaning users could easily trash their browser by flipping a few things in about:config. That's not an attractive prospect either.
> or we could
> have stored a JSON blob as a pref value like we do for
> browser.uiCustomization.state.
That pref is already running up to the "warning" size limitations of prefs on not-a-few profiles. With 20/20 hindsight, I would not have put that in prefs (or found some more condensed storage mechanism). It's not really a "success story" example for putting data in prefs, IMO. Sticking lots of data into prefs slows down both pref reads and writes, which ends up slowing down early startup as well as routine other writing of prefs triggered by e.g. telemetry (yes, it does that...).
You need to log in
before you can comment on or make changes to this bug.
Description
•