Closed Bug 1407911 (stylo-blocklist-domains) Opened 7 years ago Closed 7 years ago

stylo-blocklist: update stylo-blocklist preference using a system addon

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 blocking fixed
firefox58 --- fixed

People

(Reporter: chenpighead, Assigned: chenpighead)

References

Details

(Keywords: meta)

Attachments

(4 files, 3 obsolete files)

PR for styloblocklist system addon
57 bytes, text/x-github-pull-request
Details | Review
styloblocklist.xpi (v0.2, unsigned)
1.87 KB, application/x-xpinstall
chenpighead
: review+
Details
signed.8919985.xpi
5.78 KB, application/x-xpinstall
Details
XPI unsigned .png
75.22 KB, image/png
Details 
Note that at the time while I'm filing this bug, we don't have any domain in our Radar. This is still a precaution mechanism. Couple reasons why I'm filing this bug:
 
1. We'll need a tracking bug for add/remove domains once we start to use the stylo-blocklist mechanism.

2. As a system addon, it takes time for writing the add-on, QA sign off, deploying to the balrog update server, and waiting for users to download the update. So, if we can get some steps ready in advance, the update process will be much more efficient in future.


So, I'm going to refer to https://wiki.mozilla.org/Firefox/Go_Faster/System_Add-ons/Process and write a template patch here.
Attachment #8917743 - Attachment description: use system addon to set/reset stylo blocklist. → [template] use system addon to set/reset stylo blocklist.
Attachment #8917743 - Attachment is obsolete: true
Comment on attachment 8917746 [details]
Bug 1407911 - [template] use system addon to set/reset stylo blocklist.

Hi Felipe, could you help review this template and its commit message? Thanks.

Also, I wonder if we should deploy a separated pref-flip patch to Release channel apart from this add-on, so users can get the correct pref if they update to a dot release?

The scenario is, for example, a user who's in 57.01 and his pref has been updated by the add-on already. What will the pref looks like if he updates to 57.02?


if we decide to flip the pref through a system add-on,
Attachment #8917746 - Flags: review?(felipc)
Keywords: meta
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #3)
> if we decide to flip the pref through a system add-on,

Forgot to remove this line.... please ignore it.
Assignee: nobody → jeremychen
status-firefox56: --- → unaffected
status-firefox57: --- → wontfix
status-firefox-esr52: --- → unaffected
Priority: -- → P2
[Tracking Requested - why for this release]:

Jeremy is preparing a system add-on to update the Stylo blocklist pref. We want to have the add-on code ready in case we need it after Firefox 57 ships.

Ritu, how do you recommend we track this bug? I don't know if we need to uplift these patches to the mozilla-beta or mozilla-release repos to ship the system add-on to Firefox 57 users.
status-firefox57: wontfix → affected
tracking-firefox57: --- → ?
Depends on: stylo-blocklist
Flags: needinfo?(rkothari)
Thanks Chris, let's track this as blocking 57. When the system add-on is ready and shipping in a 57 beta build, we can resolve this one as fixed.
tracking-firefox57: ? → blocking
Flags: needinfo?(rkothari)
Comment on attachment 8917746 [details]
Bug 1407911 - [template] use system addon to set/reset stylo blocklist.

This is in the right direction, but some changes are needed:

1- The main important point is that a system add-on don't need to be pre-shipped with the build to allow it to be updated.. We can ship new system addons at any time. So there's no need to make this live in the tree and ship to all users even with an empty list.  We can have the code ready and only ship if/when we do need to add something to the lists.

This will make it easier because we don't need to track this, nor do uplifts. But in this case, the code shouldn't be part of mozilla-central. It should go here: https://github.com/mozilla/one-off-system-add-ons

2- The "defaults" pref need to be updated on every startup, and I think for this case it's better to use a regular pref. That will have two benefits:
 - only call it once from install(), and not startup()
 - when users update to e.g. 57.0.2 as you asked, the pref will remain with its value, so there's nothing else to worry about.
The only thing is that in the future we'd need to a) ship an updated version to clear the pref, or b) make the code in question ignore the pref once it's no longer needed.. which both are simpler
Attachment #8917746 - Flags: review?(felipc) → feedback+
(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> Comment on attachment 8917746 [details]
> Bug 1407911 - use system addon to set/reset stylo blocklist.
> 
> This is in the right direction, but some changes are needed:
> 
> 1- The main important point is that a system add-on don't need to be
> pre-shipped with the build to allow it to be updated.. We can ship new
> system addons at any time. So there's no need to make this live in the tree
> and ship to all users even with an empty list.  We can have the code ready
> and only ship if/when we do need to add something to the lists.

Yes, I understand. The patch is just a template for future use, and we don't need to ship anything right now.

> This will make it easier because we don't need to track this, nor do
> uplifts. But in this case, the code shouldn't be part of mozilla-central. It
> should go here: https://github.com/mozilla/one-off-system-add-ons

Are you suggesting that we should create a PR for https://github.com/mozilla/one-off-system-add-ons once we're going to ship the add-on?

> 2- The "defaults" pref need to be updated on every startup, and I think for
> this case it's better to use a regular pref. That will have two benefits:
>  - only call it once from install(), and not startup()
>  - when users update to e.g. 57.0.2 as you asked, the pref will remain with
> its value, so there's nothing else to worry about.
> The only thing is that in the future we'd need to a) ship an updated version
> to clear the pref, or b) make the code in question ignore the pref once it's
> no longer needed.. which both are simpler

Ok, I'll update the template with:

* use a regular pref
* only call it once from install() and flush the preferences to disk

I wonder when will an add-on been uninstalled? Is it possible we just uninstall the add-on once we'd like to clear/reset the pref?
Comment on attachment 8917746 [details]
Bug 1407911 - [template] use system addon to set/reset stylo blocklist.

needinfo for the questions in comment 8, and ask for review for the feedback in comment 7.
Flags: needinfo?(felipc)
Attachment #8917746 - Flags: review?(felipc)
(In reply to Jeremy Chen [:jeremychen] UTC+8 (away 24 Oct – 12 Nov) from comment #8)
> > This will make it easier because we don't need to track this, nor do
> > uplifts. But in this case, the code shouldn't be part of mozilla-central. It
> > should go here: https://github.com/mozilla/one-off-system-add-ons
> 
> Are you suggesting that we should create a PR for
> https://github.com/mozilla/one-off-system-add-ons once we're going to ship
> the add-on?

Yep, that's right. The main point is to _not_ land this on mozilla-central. Only keep it locally to you so that you can build it to get the post-processed install.rdf.  (Sometimes I do maxVersion/minVersion on install.rdf manually to avoid the need for this. Note that you'd need to build using mozilla-release in order to get the right versions in the post-processed file)

FWIW, if you prefer, you don't need to wait to have to ship it to create the first PR. You could do it now and then later you just create a new PR to update the list.


> I wonder when will an add-on been uninstalled? Is it possible we just
> uninstall the add-on once we'd like to clear/reset the pref?

It's possible to trigger an uninstall of the addon, but uninstall() also runs whenever there's a version update (because it uninstalls the updated version to install the built-in version, which is presumably newer).  So it's not easy to count on the uninstall() function to do this.. It's simpler to just ship something new to clear the pref, or leave it there and ignore it.
Flags: needinfo?(felipc)
Comment on attachment 8917746 [details]
Bug 1407911 - [template] use system addon to set/reset stylo blocklist.

https://reviewboard.mozilla.org/r/188676/#review194934

If you were to land this on central, you'd also need to add an entry at http://searchfox.org/mozilla-central/source/testing/talos/talos/xtalos/xperf_whitelist.json
But that's not necessary, so this is just informative

::: browser/extensions/styloblocklist/bootstrap.js:18
(Diff revision 2)
> +
> +const PREF_STYLO_BLOCKLIST_ENABLED = "layout.css.stylo-blocklist.enabled";
> +const PREF_STYLO_BLOCKLIST_DOMAINS = "layout.css.stylo-blocklist.blocked_domains";
> +// TODO: Put a comma-separated string here to set/reset the stylo blocklist.
> +//       e.g., "example1.com,example2.com,example3.com"
> +const BLOCKED_DOMAINS = "";

suggestion for when you populate this list:

const BLOCKED_DOMAINS = [
  "example1.com",
  "example2.com",
].join(",");

::: browser/extensions/styloblocklist/moz.build:10
(Diff revision 2)
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DEFINES['MOZ_APP_VERSION'] = CONFIG['MOZ_APP_VERSION']
> +DEFINES['MOZ_APP_MAXVERSION'] = CONFIG['MOZ_APP_MAXVERSION']
> +
> +FINAL_TARGET_FILES.features['asyncrendering@mozilla.org'] += [

need to update asyncrendering@mozilla.org to styloblocklist@mozilla.org
Attachment #8917746 - Flags: review?(felipc) → review+
(In reply to :Felipe Gomes (needinfo me!) from comment #11)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 (away 24 Oct – 12 Nov) from
> comment #8)
> > > This will make it easier because we don't need to track this, nor do
> > > uplifts. But in this case, the code shouldn't be part of mozilla-central. It
> > > should go here: https://github.com/mozilla/one-off-system-add-ons
> > 
> > Are you suggesting that we should create a PR for
> > https://github.com/mozilla/one-off-system-add-ons once we're going to ship
> > the add-on?
> 
> Yep, that's right. The main point is to _not_ land this on mozilla-central.
> Only keep it locally to you so that you can build it to get the
> post-processed install.rdf.  (Sometimes I do maxVersion/minVersion on
> install.rdf manually to avoid the need for this. Note that you'd need to
> build using mozilla-release in order to get the right versions in the
> post-processed file)

Ok, if I understand correctly, the only difference between install.rdf and install.rdf.in is maxVersion/minVersion, so I should be able to just hardcode maxVersion/minVersion and rename the file to get install.rdf, right?

Then, I guess I just need to zip the bootstrap.js and install.rdf files and renamed the zipped file to styloblocklist.xpi to complete making the package.

Hmmm... however, I tried to install styloblocklist.xpi (made by manually zipping and renaming) in my nightly (through "install add-on from file"), it says the file is corrupt. Could you help on how to make an .xpi package from bootstrap.js and install.rdf.in files? The process is not clear to me, and the documentation work is listed in TODO section on https://github.com/mozilla/one-off-system-add-ons... :(

> FWIW, if you prefer, you don't need to wait to have to ship it to create the
> first PR. You could do it now and then later you just create a new PR to
> update the list.
> 

Ok, I'll create a PR once I can successfully make a .xpi package and test it works as expected.

(In reply to :Felipe Gomes (needinfo me!) from comment #12)
> Comment on attachment 8917746 [details]
> Bug 1407911 - [template] use system addon to set/reset stylo blocklist.
> 
> https://reviewboard.mozilla.org/r/188676/#review194934
> 
> If you were to land this on central, you'd also need to add an entry at
> http://searchfox.org/mozilla-central/source/testing/talos/talos/xtalos/
> xperf_whitelist.json
> But that's not necessary, so this is just informative

Thank you for the info.

> ::: browser/extensions/styloblocklist/bootstrap.js:18
> (Diff revision 2)
> > +
> > +const PREF_STYLO_BLOCKLIST_ENABLED = "layout.css.stylo-blocklist.enabled";
> > +const PREF_STYLO_BLOCKLIST_DOMAINS = "layout.css.stylo-blocklist.blocked_domains";
> > +// TODO: Put a comma-separated string here to set/reset the stylo blocklist.
> > +//       e.g., "example1.com,example2.com,example3.com"
> > +const BLOCKED_DOMAINS = "";
> 
> suggestion for when you populate this list:
> 
> const BLOCKED_DOMAINS = [
>   "example1.com",
>   "example2.com",
> ].join(",");

Will put this in the template.

> ::: browser/extensions/styloblocklist/moz.build:10
> (Diff revision 2)
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +DEFINES['MOZ_APP_VERSION'] = CONFIG['MOZ_APP_VERSION']
> > +DEFINES['MOZ_APP_MAXVERSION'] = CONFIG['MOZ_APP_MAXVERSION']
> > +
> > +FINAL_TARGET_FILES.features['asyncrendering@mozilla.org'] += [
> 
> need to update asyncrendering@mozilla.org to styloblocklist@mozilla.org

Oops, good catch!
Flags: needinfo?(felipc)
(In reply to Jeremy Chen [:jeremychen] UTC+8 (away 24 Oct – 12 Nov) from comment #13)
> 
> Ok, if I understand correctly, the only difference between install.rdf and
> install.rdf.in is maxVersion/minVersion, so I should be able to just
> hardcode maxVersion/minVersion and rename the file to get install.rdf, right?

Yep, but there's also the line "#filter substitution" that should be removed from install.rdf.in. That's very likely the reason why you were getting a corrupt file.  Other than that, the .xpi is just a renamed .zip file, with these two files in the root of the zip.  Let me know if fixing that makes it work!
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes (needinfo me!) from comment #14)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 (away 24 Oct – 12 Nov) from
> comment #13)
> > 
> > Ok, if I understand correctly, the only difference between install.rdf and
> > install.rdf.in is maxVersion/minVersion, so I should be able to just
> > hardcode maxVersion/minVersion and rename the file to get install.rdf, right?
> 
> Yep, but there's also the line "#filter substitution" that should be removed
> from install.rdf.in. That's very likely the reason why you were getting a
> corrupt file.  Other than that, the .xpi is just a renamed .zip file, with
> these two files in the root of the zip.  Let me know if fixing that makes it
> work!

It works!!!!! Thank you so much.
Then, in our case, I think we should just maintain the install.rdf file instead of install.rdf.in.
Set the version number to v0.1 for now, because we haven't yet need to set/reset styloblocklist with this addon.

If one day we really need to use this add-on to set/reset stylo blocklist, we
should publish v1.0 and so on. Couple things to do before publishing:

1. update BLOCKED_DOMAINS variable in bootstrap.js
2. double check minVersion/maxVersion/version in install.rdf
3. re-pack styloblocklist.xpi package file by zipping bootstrap.js and install.rdf and renaming the zipped file to styloblocklist.xpi
4. get the add-on signed
5. QA it
6. deploy it by requesting approval-mozilla-release?
Attachment #8917746 - Attachment is obsolete: true
I'm setting maxVersion to 59 for now, so we can do some tests on Nightly with this v0.1 add-on. I can install it on my Nightly by setting "xpinstall.signatures.required" to false. The add-on resets my stylo-blocklist pref to empty string successfully.
@ Jeremy: is the styloblocklist.xpi add-on ready to be signed for QA testing?

@ Felipe: does QA test system add-ons by installing them locally or do we need to stage the add-on on an QA update server? The wiki is not clear:

https://wiki.mozilla.org/Firefox/Go_Faster/System_Add-ons/Process#QA_and_Code_Review
Flags: needinfo?(jeremychen)
Flags: needinfo?(felipc)
Ok, here is the thing, we have different settings in beta57 and nightly58:

        | stylo-blocklist pref | can install unsigned add-on?
--------+----------------------+------------------------------------------------
beta    | ""                   | No
--------+----------------------+------------------------------------------------
nightly | "arewestyloyet.rs"   | Yes, set "xpinstall.signatures.required" pref to false


So, I'm going to upload a v0.2 add-on, which sets stylo-blocklist pref to "arewestyloyet.rs", so we can get it signed/tested on beta 57.

I'm also going to land a patch to reset nightly's stylo-blocklist pref to empty string, so the signed v0.2 can be tested in Nightly as well.

I'm not sure if this bug should block stylo-release, since this is a tracking bug for all potential future shippings of the add-on, the dependency setting seems a bit confusing to me.
Flags: needinfo?(jeremychen)
This add-on will reset pref("layout.css.stylo-blocklist.blocked_domains") to "arewestyloyet.rs".

This add-on is ready for signing and QA, I've verified its functionality on my Nightly.
Attachment #8919582 - Attachment is obsolete: true
Depends on: 1409958
(In reply to Chris Peterson [:cpeterson] from comment #19)
> @ Felipe: does QA test system add-ons by installing them locally or do we
> need to stage the add-on on an QA update server? The wiki is not clear:

The add-on can be simply installed locally (as long as it's signed) to test that it works properly.

Once this level of testing is done, it's usually nice to test it through the update process. Sometimes we ask the people in balrog to put it on a testing channel, but what I usually do, which is easier, is to just host a simple update.xml file and the xpi myself,  and point the pref extensions.systemAddon.update.url to it.

For example, on bug 1390703 I hosted this xml:

https://felipc.github.io/ctp25/update.xml

On the xml you just need to set all the data correctly: version, filesize, url and hash of the file
Flags: needinfo?(felipc)
(In reply to Jeremy Chen [:jeremychen] UTC+8 (away 24 Oct – 12 Nov) from comment #21)
> Created attachment 8919985 [details]
> styloblocklist.xpi (v0.2, unsigned)
> 
> This add-on will reset pref("layout.css.stylo-blocklist.blocked_domains") to
> "arewestyloyet.rs".
> 
> This add-on is ready for signing and QA, I've verified its functionality on
> my Nightly.

Hi Felipe, any idea who I should reach to get this add-on signed (for QA purpose)?
Flags: needinfo?(felipc)
(In reply to Jeremy Chen [:jeremychen] UTC+8 (away 24 Oct – 12 Nov) from comment #23)
> 
> Hi Felipe, any idea who I should reach to get this add-on signed (for QA
> purpose)?

:wezhou or :jason
Flags: needinfo?(felipc)
Comment on attachment 8919985 [details]
styloblocklist.xpi (v0.2, unsigned)

Carrying forward r+
Attachment #8919985 - Flags: review+
Hi Jason, could I get this XPI signed please?
Flags: needinfo?(jthomas)
Flags: needinfo?(jthomas) → needinfo?(wezhou)
Attached file signed.8919985.xpi — 
Attachment signed. Please test.
Flags: needinfo?(wezhou)
Attached image XPI unsigned .png — 
I tried to install this .xpi to FF beta 57.0b10, FF Nightly 58.0a1(2017-10-22) and FF release 56 on Mac 10.10 and Windows 10, but I can't, I receive a message saying that this is not compatible, please see the attachment.
Flags: needinfo?(jeremychen)
(In reply to ovidiu boca[:Ovidiu] from comment #28)
> Created attachment 8920994 [details]
> XPI unsigned .png
> 
> I tried to install this .xpi to FF beta 57.0b10, FF Nightly
> 58.0a1(2017-10-22) and FF release 56 on Mac 10.10 and Windows 10, but I
> can't, I receive a message saying that this is not compatible, please see
> the attachment.

Hi Felipe, do you think this is something related to the version settings? What version should I use to make it compatible with Beta 57? Probably use 57.* in minVersion?

FYI, the .rdf setting in the attached v0.2 (see PR in [1]) is:

```
<em:minVersion>57.0</em:minVersion>
<em:maxVersion>59.*</em:maxVersion>
```


[1] https://github.com/mozilla/one-off-system-add-ons/pull/66/commits/fb5b1248825348d517af1939b0447dfa8804d8a1
Flags: needinfo?(jeremychen) → needinfo?(felipc)
Ah, what is happening is that installing by drag'n'drop makes it work just as any normal add-ons, and it's being blocked by being a legacy addon. On Nightly that can be worked around by toggling extensions.legacy.enabled, but I believe that won't have any effect on Beta. You'll need to publish a simple update.xml file like the example from comment 22 to test on Beta.

Note 1: I found out this by enabling the pref "extensions.logging.enabled", which gives a lot of details about the installation process in the Browser Console.
Flags: needinfo?(felipc)
Hi Ovidiu. In the meantime, can you re-test doing the following:

- On Nightly only
- changing the pref extensions.legacy.enabled to true
- making sure you're using the signed file  (signed.8919985.xpi)
Flags: needinfo?(ovidiu.boca)
Hi Felipe, I tried this and it works. Will it be ok to continue testing this add-on by changing this pref, or should I wait until the add-on is signed? Thanks
Flags: needinfo?(ovidiu.boca) → needinfo?(felipc)
Hi Felipe, I tried to host a update.xml in https://chenpighead.github.io/styloblocklistaddonhost/update.xml and point the pref extensions.systemAddon.update.url to it (as comment 22 said), but I still can't install the add-on. Could you enlighten me a bit more? Thanks.
Hey Jeremy, your update.xml worked correctly for me. Maybe you're missing a way to force an addon update check [1]. (Hmm we should have this somewhere in the documentation.. I'll add it).

[1] https://wiki.mozilla.org/Add-ons/Hotfix#Triggering_the_update_check
Hey Ovidiu, you can now start testing using the update.xml that Jeremy set up. That way you won't need to change that extnsions.legacy.enabled pref.

Steps for teting:

- On a fresh profile:
  - set extensions.logging.enabled to true
  - set devtools.chrome.enabled to true
  - set extensions.systemAddon.update.url to "https://chenpighead.github.io/styloblocklistaddonhost/update.xml"  (without the quotes)

- Verify that the pref layout.css.stylo-blocklist.blocked_domains is empty
- Verify that on https://arewestyloyet.rs/ , it says "This page is styled with Stylo"

- Now trigger an addon update check with the snippet from the link in comment 34

- After the logging in the browser console stops, verify if the addon worked:

 - check that the pref layout.css.stylo-blocklist.blocked_domains was changed
 - check that in about:support, the system addon "Update Stylo Blocklist" appears in the "Features" section
 - restart the browser and check that on https://arewestyloyet.rs/ it says "This page is not styled by Stylo"
Flags: needinfo?(felipc)
Depends on: 1411517
Hi,
I finish testing the scenario from comment 35, and I didn't find any issues. The test was run on Nightly 58.0a1(2017-10-24) on Mac OS X 10.10, Ubuntu 16.04(x64), Windows 10(x32 and x64), Windows 7(x32 and x64)
(In reply to ovidiu boca[:Ovidiu] from comment #36)
> I finish testing the scenario from comment 35, and I didn't find any issues.
> The test was run on Nightly 58.0a1(2017-10-24) on Mac OS X 10.10, Ubuntu
> 16.04(x64), Windows 10(x32 and x64), Windows 7(x32 and x64)

Thanks, Ovidiu. I didn't realize that Jeremy had already set up a test server. Do you have time to try the test server with Beta 57? Since you already confirmed that the blocklist code works for each Mac/Linux/Windows platform in Nightly 58, you could test Beta 57 on just one platform instead of all of them.
Flags: needinfo?(ovidiu.boca)
Hi Chris,
I tested with Firefox beta 57.0b11 on Mac Os X 10.10, Ubuntu 16.04 and Windows 10 and I can confirm that the blocklist code works.
Flags: needinfo?(ovidiu.boca)
(In reply to ovidiu boca[:Ovidiu] from comment #38)
> I tested with Firefox beta 57.0b11 on Mac Os X 10.10, Ubuntu 16.04 and
> Windows 10 and I can confirm that the blocklist code works.

Awesome! I think we can resolve this bug as fixed because Jeremy built the system add-on and Ovidiu verified that it works. If we need to deploy the system add-on after Firefox 57 is released, we can file a new bug.
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
status-firefox58: affected → fixed
Resolution: --- → FIXED
(In reply to Chris Peterson [:cpeterson] from comment #39)
> (In reply to ovidiu boca[:Ovidiu] from comment #38)
> > I tested with Firefox beta 57.0b11 on Mac Os X 10.10, Ubuntu 16.04 and
> > Windows 10 and I can confirm that the blocklist code works.
> 
> Awesome! I think we can resolve this bug as fixed because Jeremy built the
> system add-on and Ovidiu verified that it works. If we need to deploy the
> system add-on after Firefox 57 is released, we can file a new bug.

In this case, I think this bug is no longer a meta.
Summary: [meta] stylo-blocklist: update stylo-blocklist preference using a system addon → stylo-blocklist: update stylo-blocklist preference using a system addon
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: