Closed Bug 1377539 Opened 7 years ago Closed 6 years ago

Switch blocklist clients to IndexedDB

Categories

(Toolkit :: Blocklist Implementation, enhancement, P3)

Product:
Toolkit
Component:
Blocklist Implementation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: leplatrem, Assigned: leplatrem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1377539 - Switch blocklists to IndexedDB
59 bytes, text/x-review-board-request
glasserc
: review+
Gijs
: review+
Details 
Two years ago, we wrote a storage adapter to rely on Sqlite. Now that IndexedDB is available on the chrome side, we can get rid of it.
Summary: Get rid of Sqlite storage adapter in Kinto now that IndexedDB is available → Add IndexedDB support to Kinto in addition to Sqlite storage adapter
Assigning priorities to all blocklisting bugs because reasons.
Priority: -- → P3
Depends on: 1447400
Mark mentioned this to me, and while I don't have all the details, I wanted to make sure that we're aware of bug 1425146 and similar issues with indexeddb.
See Also: → 1425146
Assignee: nobody → mathieu
Summary: Add IndexedDB support to Kinto in addition to Sqlite storage adapter → Switch blocklist clients to IndexedDB
Comment on attachment 8961311 [details]
Bug 1377539 - Switch blocklists to IndexedDB

https://reviewboard.mozilla.org/r/229242/#review235774

Looks OK to me, although I don't have strong enough r+ to land this.
Attachment #8961311 - Flags: review?(eglassercamp) → review+
Thanks Ethan and thanks Gijs for reminding me about Bug 1425146, I guess that our usage of IndexedDB in remote settings is a lot lighter though (unidirectional, no concurrent writes etc.)

I submitted a patch, I think the switch to the IndexedDB adapter is worth it, mainly because:
- the code is bit simpler
- all tests passed with no change
- the database will be automatically populated with the local JSON files dumps on sync
- I suppose that the indexeddb blocklist records could be queried from elsewhere with the native IndexedDB API (like for Bug 1447680)

If this patch lands, we should delete the `kinto.sqlite` file in the user profile folder since it will be obsolete. Gijs, is there a specific procedure to clean stuff from profile on version upgrade?

Also, I wonder if we should add a shutdown blocker like we did for SQlite in Bug 1376874. Honestly I would expect the code under the IndexedDB engine to be in charge of this. Marco, can you confirm?
In my previous comment, I forgot to NI you :) Thanks for your answers !
Flags: needinfo?(mak77)
Flags: needinfo?(gijskruitbosch+bugs)
jvarga is a better person to answer questions regarding idb. Alternatively you may ask asuth.
Flags: needinfo?(mak77) → needinfo?(jvarga)
(In reply to Mathieu Leplatre (:leplatrem) from comment #7)
> Thanks Ethan and thanks Gijs for reminding me about Bug 1425146, I guess
> that our usage of IndexedDB in remote settings is a lot lighter though
> (unidirectional, no concurrent writes etc.)
> 
> I submitted a patch, I think the switch to the IndexedDB adapter is worth
> it, mainly because:
> - the code is bit simpler
> - all tests passed with no change
> - the database will be automatically populated with the local JSON files
> dumps on sync
> - I suppose that the indexeddb blocklist records could be queried from
> elsewhere with the native IndexedDB API (like for Bug 1447680)

I have no strong opinion on the internals of how kinto stores this locally - indexeddb or sqlite both work for me. Any direct operations I think would be possible with either, but really, ideally I would like to make sure that the kinto client-side wrapper stuff exposes any APIs we may want from the blocklist side.

> If this patch lands, we should delete the `kinto.sqlite` file in the user
> profile folder since it will be obsolete. Gijs, is there a specific
> procedure to clean stuff from profile on version upgrade?

There's some stuff in nsBrowserGlue.js ( https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/components/nsBrowserGlue.js#1835 ) that depends on an incrementing number where you can then add a one-off cleanup routine. Unfortunately, this is browser-specific so it wouldn't help users of thunderbird/seamonkey. The other thing I'm wondering is if we've actually really created these files for all the users - I thought this was all preffed off on release?

I mean, the other thing you could consider doing is just making the start of the component trigger an idle task that tries to remove the file on a background thread, and silences any errors. Removing a file is relatively cheap, and as long as we don't do it during startup or other critical times, it shouldn't make much of a difference. This would work across consumers.
Flags: needinfo?(gijskruitbosch+bugs)
> just making the start of the component trigger an idle task that tries to remove the file on a background thread

I like this idea but I don't really know how to start an idle task. Do you have an example of something similar? 

Thanks for your help and detailed feedback!
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mathieu Leplatre (:leplatrem) from comment #11)
> > just making the start of the component trigger an idle task that tries to remove the file on a background thread
> 
> I like this idea but I don't really know how to start an idle task. Do you
> have an example of something similar? 

Yep, you can basically just call:

Services.tm.idleDispatchToMainThread(() => {
  // do things in here, something like:
  OS.File.remove(somePath, {thingytomakeitnotthrowifthefiledoesntexist}).catch(Cu.reportError);
});
Flags: needinfo?(gijskruitbosch+bugs)
I added the `idleDispatchToMainThread`, tested it manually, and it worked as expected :) I could not find a way to test it from xpcshell though, as if the test suite never ran the idle tasks.
Would it be a blocker to land this? (try is green otherwise)
Thanks :)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mathieu Leplatre (:leplatrem) from comment #14)
> I added the `idleDispatchToMainThread`, tested it manually, and it worked as
> expected :) I could not find a way to test it from xpcshell though, as if
> the test suite never ran the idle tasks.

Huh. That's a bit surprising - but I don't think it's important to verify that the removal works in a test.

> Would it be a blocker to land this? (try is green otherwise)
> Thanks :)

Hm, no, I think that's fine.

One thing is that re-reading my own suggestion, I'm starting to doubt myself in terms of whether it's really a good idea to try to remove this non-existant file on every startup... Florian, do you know if there's a better idea here, especially one that works for toolkit apps? I guess we can track whether we did the removal in our own pref but that feels super ugly too.

Also, Mathieu, do we need to remove the json files as well? Or will we keep exporting those? Or something else? :-)
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(florian)
Comment on attachment 8961311 [details]
Bug 1377539 - Switch blocklists to IndexedDB

https://reviewboard.mozilla.org/r/229242/#review241798

This is basically an r+ but I'm hoping Florian can help us figure out where best to remove the files.

::: services/common/remote-settings.js
(Diff revision 3)
> -ChromeUtils.defineModuleGetter(this, "FirefoxAdapter",
> -                               "resource://services-common/kinto-storage-adapter.js");

I guess we should `hg remove` this file, too?

::: services/common/tests/unit/test_blocklist_clients.js
(Diff revision 3)
> -let gBlocklistClients;
>  let server;
> +let gBlocklistClients;

Nit: just leave this in the same place? :-)
Attachment #8961311 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs (he/him) from comment #15)

> One thing is that re-reading my own suggestion, I'm starting to doubt myself
> in terms of whether it's really a good idea to try to remove this
> non-existant file on every startup... Florian, do you know if there's a
> better idea here, especially one that works for toolkit apps?

We typically do these removals in the UI migration code: https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/browser/components/nsBrowserGlue.js#2151-2154

How much do we care about removing these files for non-browser toolkit apps?
Flags: needinfo?(florian)
Comment on attachment 8961311 [details]
Bug 1377539 - Switch blocklists to IndexedDB

https://reviewboard.mozilla.org/r/229242/#review241798

> I guess we should `hg remove` this file, too?

We can't ;) It's used in WebExtensions storage.sync() API https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/toolkit/components/extensions/ExtensionStorageSync.jsm#54
> I'm starting to doubt myself in terms of whether it's really a good idea to try to remove this non-existant file on every startup...

It won't really be on startup, but on the first call to nsBlocklist::notify()

> do we need to remove the json files as well? Or will we keep exporting those? Or something else? :-)

We'll keep them for now (we can think of something better in Bug 1451050).


> How much do we care about removing these files for non-browser toolkit apps?

Maybe we don't indeed...
Blocks: 1345334
Comment on attachment 8961311 [details]
Bug 1377539 - Switch blocklists to IndexedDB

https://reviewboard.mozilla.org/r/229242/#review242246

::: browser/components/nsBrowserGlue.js:1841
(Diff revision 5)
>      }
>    },
>  
>    // eslint-disable-next-line complexity
>    _migrateUI: function BG__migrateUI() {
> -    const UI_VERSION = 67;
> +    // Use an arbitrary number to keep track of the current migration state.

Nit: s/arbitrary/increasing/ .

Maybe also add something like: "Completely unrelated to the current Firefox release number". :-)
Attachment #8961311 - Flags: review?(gijskruitbosch+bugs) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/840e168e50ab
Switch blocklists to IndexedDB r=Gijs,glasserc
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/840e168e50ab
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: needinfo?(jvarga)
Component: Blocklist Policy Requests → Blocklist Implementation
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: