Closed Bug 1451050 Opened 6 years ago Closed 6 years ago

Generalize loading of JSON dumps for remote settings

Categories

(Firefox :: Remote Settings Client, enhancement)

Product:
Firefox
Component:
Remote Settings Client
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: leplatrem, Assigned: leplatrem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1451050 - Generalize loading of packaged remote settings dumps
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
Currently we load JSON dumps from a folder called blocklists/

We should generalize that behaviour for any kind of remote settings using the bucket name as the folder name for example.

Plus, currently we only load the dump during the first sync. We should also load the dump during the first call to ``get()``

The job that regularly updates the dumps (see Bug 1451040) should be smart enough to pick the dumps introduced by developers when new remote settings use-cases are shipped.
Component: Blocklisting → Remote Settings Client
Product: Toolkit → Firefox
Target Milestone: --- → Future
Version: 57 Branch → Trunk
Comment on attachment 8974727 [details]
Bug 1451050 - Generalize loading of packaged remote settings dumps

https://reviewboard.mozilla.org/r/243090/#review249240

r=me on the desktop portion, but we should really doublecheck what we do for Android.

::: services/common/docs/RemoteSettings.rst:98
(Diff revision 2)
> +For newly created user profiles, the list of entries returned by the ``.get()`` method will be empty until the first synchronization happens.
> +
> +It is possible to package a dump of the server records that will be loaded into the local database when no synchronization has happened yet. It will thus serve as the default dataset and also reduce the amount of data to be downloaded on the first synchronization.
> +
> +#. Place the JSON dump of the server records in the ``services/settings/dumps/main/`` folder
> +#. Add the filename to the ``FINAL_TARGET_FILES`` list in ``services/settings/dumps/main/moz.build``

This should probably include the installer packaged-files bit as a step as well?

::: services/settings/moz.build:10
(Diff revision 2)
> +with Files('**'):
> +    BUG_COMPONENT = ('Firefox', 'Remote Settings Client')
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android':
> +    DIRS += [
> +        'dumps',

Wait, does this mean we don't ship the blocklists to android? I'm not sure that's right... do we currently package blocklist.xml on Android? Might be worth checking with an android peer...
Attachment #8974727 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs (he/him) from comment #3)
> Comment on attachment 8974727 [details]
> Bug 1451050 - Generalize loading of packaged remote settings dumps
> 
> https://reviewboard.mozilla.org/r/243090/#review249240
> 
> r=me on the desktop portion, but we should really doublecheck what we do for
> Android.

> ::: services/settings/moz.build:10
> (Diff revision 2)
> > +with Files('**'):
> > +    BUG_COMPONENT = ('Firefox', 'Remote Settings Client')
> > +
> > +if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android':
> > +    DIRS += [
> > +        'dumps',
> 
> Wait, does this mean we don't ship the blocklists to android? I'm not sure
> that's right... do we currently package blocklist.xml on Android? Might be
> worth checking with an android peer...

Nick, do you know (who knows) to what extent Android uses the blocklist service? I'm kind of assuming that we want to blocklist anything that's blocklisted on desktop on Android as well, but I'm not really 100% sure on how it works today...
Flags: needinfo?(nalexander)
Comment on attachment 8974727 [details]
Bug 1451050 - Generalize loading of packaged remote settings dumps

https://reviewboard.mozilla.org/r/243090/#review249240

> This should probably include the installer packaged-files bit as a step as well?

The package-manifest.in lists folders only, so we're good IMO:
https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/browser/installer/package-manifest.in#452-453

> Wait, does this mean we don't ship the blocklists to android? I'm not sure that's right... do we currently package blocklist.xml on Android? Might be worth checking with an android peer...

I just kept the current behaviour.
See https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/services/moz.build#15-19

Otherwise, yes the XML seems to be shipped:
https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/mobile/android/installer/package-manifest.in#89

Even though I'm pretty sure the blocklists are not used on Android. I'll get a confirmation.

Thanks!
Sebastian, I feel as if I asked you this a thousands times... do you have addons/plugins/gfx/certificates blocklisting service enabled on Android?

I remember that it was not, but then why is the blocklist.xml file packaged?
https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/mobile/android/installer/package-manifest.in#89

Thanks for your insights ;)
Flags: needinfo?(s.kaspari)
(In reply to :Gijs (he/him) from comment #4)
> (In reply to :Gijs (he/him) from comment #3)
> > Comment on attachment 8974727 [details]
> > Bug 1451050 - Generalize loading of packaged remote settings dumps
> > 
> > https://reviewboard.mozilla.org/r/243090/#review249240
> > 
> > r=me on the desktop portion, but we should really doublecheck what we do for
> > Android.
> 
> > ::: services/settings/moz.build:10
> > (Diff revision 2)
> > > +with Files('**'):
> > > +    BUG_COMPONENT = ('Firefox', 'Remote Settings Client')
> > > +
> > > +if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android':
> > > +    DIRS += [
> > > +        'dumps',
> > 
> > Wait, does this mean we don't ship the blocklists to android? I'm not sure
> > that's right... do we currently package blocklist.xml on Android? Might be
> > worth checking with an android peer...
> 
> Nick, do you know (who knows) to what extent Android uses the blocklist
> service? I'm kind of assuming that we want to blocklist anything that's
> blocklisted on desktop on Android as well, but I'm not really 100% sure on
> how it works today...

I honestly don't know what the situation.  AFAIK, we do the whole "blocklist dance" in Fennec; definitely I see messages in the logcat about trying to do it in my local builds.  At one point, the blocklist ping provided usage data, although I think that is superceded by modern telemetry.  Now, however, we should be using the blocklist for actual Web Extension blocking, and it might be that aswan knows if that's true.

As for:

(In reply to Mathieu Leplatre (:leplatrem) from comment #6)
> Sebastian, I feel as if I asked you this a thousands times... do you have
> addons/plugins/gfx/certificates blocklisting service enabled on Android?
> 
> I remember that it was not, but then why is the blocklist.xml file packaged?
> https://searchfox.org/mozilla-central/rev/
> 3f17a234769d25fca5144ebb8abc8e1cb3c56c16/mobile/android/installer/package-
> manifest.in#89
> 
> Thanks for your insights ;)

This is just history -- that commit is the ancient 500 series ticket that landed "hg cp browser/package-manifest.in mobile/android/package-maniefst.in".  Sadly, reading the package manifest does not say much about intention or expectation :/
Flags: needinfo?(nalexander) → needinfo?(aswan)
(In reply to Nick Alexander :nalexander from comment #7)
> Now,
> however, we should be using the blocklist for actual Web Extension blocking,
> and it might be that aswan knows if that's true.

Short answer: yes

If it helps, desktop and fennec have totally separate front-ends (ie, the UI you see at about:addons) but share the back-end that actually installs, starts, stops, etc. extensions.  And blocklisting happens deep inside that shared backend code.
Flags: needinfo?(aswan)
Ok, thanks for your answers. I'm glad I asked :)

That means we should:

- ship the blocklists JSON dumps in every platform package (eg. android)
- remove the XML from all platforms in Bug 1257565

As for generic remote settings dumps, which are less likely to be used, I think we can wait to have a proper RemoteSettings API available in Android before packaging them.

Giljs, does that sound reasonable?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mathieu Leplatre (:leplatrem) from comment #9)
> Ok, thanks for your answers. I'm glad I asked :)
> 
> That means we should:
> 
> - ship the blocklists JSON dumps in every platform package (eg. android)
> - remove the XML from all platforms in Bug 1257565
> 
> As for generic remote settings dumps, which are less likely to be used, I
> think we can wait to have a proper RemoteSettings API available in Android
> before packaging them.
> 
> Giljs, does that sound reasonable?

Yep! The only caveat here is the size of the JSON dumps and how big the blocklist is. I guess because they're plaintext they'll compress fairly well so maybe apk size isn't a concern... but if we could avoid packaging the plugin or gfx lists on android (which I expect to be unused) then that would be sensible.
Flags: needinfo?(gijskruitbosch+bugs)
I had an error with packaging:

```
[task 2018-05-15T10:30:59.621Z] 10:30:59     INFO -  package> Error: /builds/worker/workspace/build/src/browser/installer/package-manifest.in:454: Missing file(s): bin/browser/defaults/settings/main
[task 2018-05-15T10:30:59.621Z] 10:30:59     INFO -  package> Traceback (most recent call last):
[task 2018-05-15T10:30:59.621Z] 10:30:59     INFO -  package>   File "/builds/worker/workspace/build/src/toolkit/mozapps/installer/packager.py", line 339, in <module>
[task 2018-05-15T10:30:59.621Z] 10:30:59     INFO -  package>     main()
[task 2018-05-15T10:30:59.621Z] 10:30:59     INFO -  package>   File "/builds/worker/workspace/build/src/toolkit/mozapps/installer/packager.py", line 300, in main
[task 2018-05-15T10:30:59.621Z] 10:30:59     INFO -  package>     copier.add(mozpath.join(respath, 'removed-files'), removals)
[task 2018-05-15T10:30:59.621Z] 10:30:59     INFO -  package>   File "/usr/lib/python2.7/contextlib.py", line 24, in __exit__
[task 2018-05-15T10:30:59.622Z] 10:30:59     INFO -  package>     self.gen.next()
```
So I created an empty dump in services/settings/dumps/tippytop.json, which will be one of our first use-cases of remote settings v2.
Keywords: checkin-needed
Target Milestone: Future → Firefox 62
Flags: needinfo?(s.kaspari)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/be23dc007b53
Generalize loading of packaged remote settings dumps r=Gijs
Keywords: checkin-needed
Hmm, sorry about that. Test on try was passing :|

Build error is:

```
[task 2018-05-16T10:20:49.986Z] 10:20:49     INFO -  package> WARNING: Found 26 duplicated files taking 339348 bytes (uncompressed)
[task 2018-05-16T10:20:49.986Z] 10:20:49     INFO -  package> ERROR: The following duplicated files are not allowed:
[task 2018-05-16T10:20:49.986Z] 10:20:49     INFO -  package> browser/defaults/settings/pinning/pins.json
[task 2018-05-16T10:20:49.986Z] 10:20:49     INFO -  package> browser/defaults/settings/main/tippytop.json
```
Flags: needinfo?(mathieu)
Blocks: 1451040
Keywords: checkin-needed
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e4cdaab8e5c7
Generalize loading of packaged remote settings dumps r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e4cdaab8e5c7
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/cec714cd0bdf
Port bug 1451050 - fix paths of blocklists, pinning, add main. rs=bustage-fix
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4588a69b6962
Port bug 1451050 - add to allowed-dupes.mn. rs=bustage-fix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: