1403077 - (stylo-blocklist) stylo-blocklist: add support for a list of domains that should use the old Gecko style system instead of Stylo

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

We want a list for domains that should use the old Gecko style system instead of Stylo. We want this list to be sync'd from a server so we can quickly update the list to unbreak a website without needing to release a Firefox dot release.

We plan to use the Kinto client/server system to maintain this domain list.

Stylo's Kinto requirements and notes:
https://docs.google.com/document/d/1IMHr_Fxe-oXC0wPnQWqV4bBs5E7HhnZZVDd4UPcC960/

Proposed Kinto JSONSchema:

{
  "properties": {
    "bug": {
        "bug": "Bug",
        "description": "Bug number",
        "type": "integer"
    },
      "domain": {
        "description": "Disable Stylo on this domain",
        "domain": "Domain",
        "type": "string"
    },
    "reason": {
        "type": "string",
        "description": "Reason why Stylo was disabled on this domain",
        "reason": "Reason"
    }
  },
  "type": "object"
}

Comment 1

[Chris Peterson [:cpeterson]]

I don't know of any documentation for Kinto client code in Firefox, but the patches in resistFingerprinting bug 1336208 might be a good example. In particular:

Part 3: Adding a new BlocklistClient for font fingerprinting.
https://reviewboard.mozilla.org/r/173122/diff/3#index_header

Part 6: Apply the font whitelist when pref 'privacy.resistFingerprinting' is true.
https://reviewboard.mozilla.org/r/173128/diff/4#index_header

Comment 2

[Rémy Hubscher (:natim)]

Attachment: Cloudops configuration PR.

Comment 3

[Rémy Hubscher (:natim)]

This documentation might also be useful to you: https://wiki.mozilla.org/Firefox/Kinto

Comment 4

[Jeremy Chen ([e-mail])]

(In reply to Rémy Hubscher (:natim) from comment #3)
> This documentation might also be useful to you:
> https://wiki.mozilla.org/Firefox/Kinto

Thanks Rémy!! I'll read it.

(In reply to Chris Peterson [:cpeterson] from comment #1)
> I don't know of any documentation for Kinto client code in Firefox, but the
> patches in resistFingerprinting bug 1336208 might be a good example. In
> particular:
> 
> Part 3: Adding a new BlocklistClient for font fingerprinting.
> https://reviewboard.mozilla.org/r/173122/diff/3#index_header
> 
> Part 6: Apply the font whitelist when pref 'privacy.resistFingerprinting' is
> true.
> https://reviewboard.mozilla.org/r/173128/diff/4#index_header

Thanks Chris, this is helpful.



So, in the client side, I think we can refer to bug 1336208 to implement the fetch/download the stylo-blocklist from Kinto. Then, we might need to read the stylo-blocklist, compare the domains of current document, and then flip the backend type in nsIDocument::UpdateStyleBackendType() [1] if the current document's URI belongs to one of the domains on the stylo-blocklist.

Not sure if we should add a separate pref to enable this feature, e.g., layout.css.stylo-blocklist.enabled ...


[1] https://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/base/nsDocument.cpp#13522

Comment 5

[Jeremy Chen ([e-mail])]

Attachment: Bug 1403077 - add tests for the stylo blocklist mechanism.

This patch adds a new BlocklistClient 'StyloBlocklistClient' for synchronizing
stylo blocklist with Kinto server. After it syncs up with Kinto, it will write
the stylo blocklist into the profile folder.

Review commit: https://reviewboard.mozilla.org/r/183986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183986/

Comment 6

[Jeremy Chen ([e-mail])]

Attachment: Bug 1403077 - add a initial stylo blocklist file which ships with the release.

This patch adds a initial stylo blocklist file that will ship with releases.
This file will be used before the first sync with Kinto server and it is
beneficial for reducing the traffic loading for the first sync.

Review commit: https://reviewboard.mozilla.org/r/183988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183988/

Comment 7

[Jeremy Chen ([e-mail])]

Attachment: Bug 1403077 - implement the stylo blocklist mechanism.

In this patch, we read the stylo blocklist into nsLayoutUtils's global static
vector during nsLayoutUtils::Initialize(). So, we can decide if we should
fallback to use Gecko backend while updating style backend for a document.

TODO:
* implement some public API so we don't need to expose the private sStyloBlocklist member
* implement the file loading/storing for stylo blocklist, i.e., LoadStyloBlocklist()
* do we need a pref for enabling this feature?
* should we replace nsTArray with hashtable?

Review commit: https://reviewboard.mozilla.org/r/183990/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183990/

Comment 8

[Bobby Holley (:bholley)]

Having a pref for the blocklist seems prudent. We could probably live without it, but it's easy to add, and might be useful if the blocklist starts misbehaving.

Also, please be sure to avoid doing a lot of work in the common case for UpdateStyleBackendType. Ideally all the information would be processed and cached once at startup, and doing so would be cheap if there are no domains in the blocklist.

Comment 9

[Jet Villegas (inactive)]

[Tracking Requested - why for this release]: This one will need an uplift to ship with Stylo.

Comment 10

[Jeremy Chen ([e-mail])]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8)
> Having a pref for the blocklist seems prudent. We could probably live
> without it, but it's easy to add, and might be useful if the blocklist
> starts misbehaving.

Ok, then I'll proceed w/o adding a pref for now.

> Also, please be sure to avoid doing a lot of work in the common case for
> UpdateStyleBackendType. Ideally all the information would be processed and
> cached once at startup, and doing so would be cheap if there are no domains
> in the blocklist.

Good point. I'm planning to use a hashtable instead of a vector, which is the current wip does. So, the cost would be just a hash look up, which should be cheap.

I also discussed a little bit with heycam about the scenario that a page has one or more iframes that their domains are different with their parent document's. For example, an embedded youtube video. So far, we probably would make the logic like:

```
if current document's domain is in the blocklist {
  fallback to Gecko backend for the current document
} else {
  match the current document's backend type with the parent document's one
}
```

Not sure if having different style backend types for a parent document and its children documents could be a problem though...

Comment 11

[Bobby Holley (:bholley)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #10)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8)
> > Having a pref for the blocklist seems prudent. We could probably live
> > without it, but it's easy to add, and might be useful if the blocklist
> > starts misbehaving.
> 
> Ok, then I'll proceed w/o adding a pref for now.

To be clear, my comment above was in favor of making it prefable. I'm not saying we _must_, but that we probably should unless it makes things more complicated somehow.

> 
> > Also, please be sure to avoid doing a lot of work in the common case for
> > UpdateStyleBackendType. Ideally all the information would be processed and
> > cached once at startup, and doing so would be cheap if there are no domains
> > in the blocklist.
> 
> Good point. I'm planning to use a hashtable instead of a vector, which is
> the current wip does. So, the cost would be just a hash look up, which
> should be cheap.

The most important thing by far would be to optimize for the common case where there is nothing in the blocklist. I think it's pretty unlikely that we'd ever have more than a few entries, though we might as well use a hashtable to be safe.

> 
> I also discussed a little bit with heycam about the scenario that a page has
> one or more iframes that their domains are different with their parent
> document's. For example, an embedded youtube video. So far, we probably
> would make the logic like:
> 
> ```
> if current document's domain is in the blocklist {
>   fallback to Gecko backend for the current document
> } else {
>   match the current document's backend type with the parent document's one
> }
> ```
> 
> Not sure if having different style backend types for a parent document and
> its children documents could be a problem though...

We should operate at the granularity of effectiveTLD + 1, which is the same limit used for document.domain (and cookies). That will prevent subframes from ever becoming same-origin via document.domain.

Comment 12

[Cameron McCormack (:heycam)]

What about for cases like an about:blank iframe or an iframe with a srcdoc?  (Or other cases I'm forgetting where the outer page can reach into the iframe.)  Is there some function we can call to check this, since checking the eTLD+1 won't be enough.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #12)
> What about for cases like an about:blank iframe or an iframe with a srcdoc? 
> (Or other cases I'm forgetting where the outer page can reach into the
> iframe.)  Is there some function we can call to check this, since checking
> the eTLD+1 won't be enough.

I think we want to check the origin of the document principal, rather than the document URI per se. That will let us piggy-back off the existing principal relationships and symmetries.

Comment 14

[Jeremy Chen ([e-mail])]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #10)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8)
> > > Having a pref for the blocklist seems prudent. We could probably live
> > > without it, but it's easy to add, and might be useful if the blocklist
> > > starts misbehaving.
> > 
> > Ok, then I'll proceed w/o adding a pref for now.
> 
> To be clear, my comment above was in favor of making it prefable. I'm not
> saying we _must_, but that we probably should unless it makes things more
> complicated somehow.

Oh, thanks for the clarification. I indeed misread your comment before.

> > 
> > > Also, please be sure to avoid doing a lot of work in the common case for
> > > UpdateStyleBackendType. Ideally all the information would be processed and
> > > cached once at startup, and doing so would be cheap if there are no domains
> > > in the blocklist.
> > 
> > Good point. I'm planning to use a hashtable instead of a vector, which is
> > the current wip does. So, the cost would be just a hash look up, which
> > should be cheap.
> 
> The most important thing by far would be to optimize for the common case
> where there is nothing in the blocklist. I think it's pretty unlikely that
> we'd ever have more than a few entries, though we might as well use a
> hashtable to be safe.

Yes, this could be done by setting a condition like "isBlockListEmpty()" or something like this.

> > 
> > I also discussed a little bit with heycam about the scenario that a page has
> > one or more iframes that their domains are different with their parent
> > document's. For example, an embedded youtube video. So far, we probably
> > would make the logic like:
> > 
> > ```
> > if current document's domain is in the blocklist {
> >   fallback to Gecko backend for the current document
> > } else {
> >   match the current document's backend type with the parent document's one
> > }
> > ```
> > 
> > Not sure if having different style backend types for a parent document and
> > its children documents could be a problem though...
> 
> We should operate at the granularity of effectiveTLD + 1, which is the same
> limit used for document.domain (and cookies). That will prevent subframes
> from ever becoming same-origin via document.domain.

Ok, thanks for bringing up the concept of effectiveTLD + 1. Here's a case that I can think of:

Step1: open a page with "mail.google.com"
Step2: the page somehow execute the script "document.domain = google.com"
Step3: the page already has a child iframe with "maps.google.com"

If maps.google.com is already in our blocklist, then the initial  backend for the iframe would be Gecko. However, this would make an issue about letting parent document (stylo backend) access its child (gecko), which across the style backend boundary. And I think we've already add some release assertions to avoid this kind of situations, right?

One way that we could avoid this issue is by only allow setting effectiveTLD + 1 domains in the blocklist, which in the example above, the entry in the blocklist would be "google.com". In this way, both parent and its child will be styled with the same backend type. The disadvantage would be that we might overly block too many pages in the same effectiveTLD + 1 domain.


(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> (In reply to Cameron McCormack (:heycam) from comment #12)
> > What about for cases like an about:blank iframe or an iframe with a srcdoc? 
> > (Or other cases I'm forgetting where the outer page can reach into the
> > iframe.)  Is there some function we can call to check this, since checking
> > the eTLD+1 won't be enough.
> 
> I think we want to check the origin of the document principal, rather than
> the document URI per se. That will let us piggy-back off the existing
> principal relationships and symmetries.

Ok, then we probably should create a principal object for each domain in the blocklist, and store these principals in an nsTArray. The hashtable might not be appropriate for complicated objects like principal I guess.

Is this subsumesConsideringDomain() [1] the right function that we should use for this purpose?

[1] https://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/caps/nsIPrincipal.idl#108

Comment 15

[Jeremy Chen ([e-mail])]

Oh, also, I wonder if someone could provide suggestions about the automated testing? I can refer to some known behavior change cases (between Gecko and Stylo) and do some manual tests for sure. However, it's unclear for me to write automated tests if we don't have a Web API to get the style backend.

Comment 16

[Jeremy Chen ([e-mail])]

Since I'm not sure if we'll need a initial stylo blocklist file which ships with the release, I'm going to hold this part for now, and ask for review/feedback for the other two parts.

Comment 17

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183986/diff/1-2/

Comment 18

[Jeremy Chen ([e-mail])]

Comment on attachment 8912661 [details]
Bug 1403077 - add a initial stylo blocklist file which ships with the release.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183988/diff/1-2/

Comment 19

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183990/diff/1-2/

Comment 20

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

https://reviewboard.mozilla.org/r/183986/#review190118

::: services/common/blocklist-clients.js:490
(Diff revision 2)
> +this.StyloBlocklistClient = new BlocklistClient(
> +  Services.prefs.getCharPref(PREF_BLOCKLIST_STYLO_COLLECTION),
> +  PREF_BLOCKLIST_STYLO_CHECKED_SECONDS,
> +  (records) => updateJSONBlocklist(this.StyloBlocklistClient.filename, records),
> +  Services.prefs.getCharPref(PREF_BLOCKLIST_BUCKET)
> +);

You might want to send an event or notify other components that a file was dumped on disk.

Otherwise, I guess you could also import your interface here and call internal APIs to update the internal state.

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

https://reviewboard.mozilla.org/r/183990/#review190274

::: dom/base/nsDocument.cpp:13524
(Diff revision 2)
> +  // We don't want to access elements between different style backends.
> +  // So, the fallback logic is, fallback if current document belongs to the
> +  // blocklist, otherwise, match current document's backend to its parent's
> +  // one.
> +  return nsLayoutUtils::IsInStyloBlocklist(NodePrincipal()) ||
> +         (GetParentDocument() &&
> +          GetParentDocument()->mStyleBackendType == StyleBackendType::Gecko);

I think we do still need some sort of "parent document" check like this, since the parent document from an origin in the blocklist could have an about:blank iframe.

But I don't know that GetParentDocument() is enough, since I think that is only for <iframe> parents, and the page could also do window.open("about:blank") and poke at that new window's document.

::: layout/base/nsLayoutUtils.h:3021
(Diff revision 2)
>    static bool sInterruptibleReflowEnabled;
>    static bool sSVGTransformBoxEnabled;
>    static bool sTextCombineUprightDigitsEnabled;
>  #ifdef MOZ_STYLO
>    static bool sStyloEnabled;
> +  static nsTArray<nsCOMPtr<nsIPrincipal>> sStyloBlocklist;

I'm not sure if we have checks for this, but we should avoid adding new static variables that need constructors to run before main.  (It affects startup time and I think the order of static constructors running is not well defined.)

So instead, let's make this variable a pointer, initialized to nullptr in its definition, and just create a new nsTArray when we load and process the blocklist.  And delete the nsTArray in nsLayoutUtils::Shutdown of course.

(Using UniquePtr instead of a raw pointer would also be fine, since its constructor is constexpr, and so doesn't cause a construct call during static initialization before main.  But I don't think UniquePtr has much advantage here.  We'd need to clear / delete the array before the UniquePtr structor would run (after main) to avoid the nsIPrincipal objects from being reported as leaked.)

::: layout/base/nsLayoutUtils.cpp:7955
(Diff revision 2)
> +  // XXX: Do we need this assertion?
> +  NS_ASSERTION(rv == NS_OK, "something's wrong while loading stylo blocklist");

It's worth some sort of error reporting like this, even though the best option is just to continue as if the blocklist was empty.

Asserting NS_SUCCEEDED(rv) is better than checking for NS_OK explicitly.  (In theory, there are nsresult values other than NS_OK that are also "success" values.)

::: layout/base/nsLayoutUtils.cpp:7982
(Diff revision 2)
> +  if (!sStyloBlocklist.IsEmpty()) {
> +    sStyloBlocklist.Clear();
> +  }

No need to check for whether the array is empty before calling Clear.

::: layout/base/nsLayoutUtils.cpp:8008
(Diff revision 2)
> +  if (sStyloBlocklist.IsEmpty()) {
> +    return false;
> +  }

No need for this either.  The iteration over sStyloBlocklist will quickly determine that that are no elements to iterate.

Though once we change sStyloBlocklist to a pointer, you can just check it for null.

::: layout/base/nsLayoutUtils.cpp:8012
(Diff revision 2)
> +{
> +  if (sStyloBlocklist.IsEmpty()) {
> +    return false;
> +  }
> +
> +  for (nsCOMPtr<nsIPrincipal> blockPrincipals : sStyloBlocklist) {

This should be:

  for (nsIPrincipal* blockPrincipals ...

to avoid bumping the refcounts of each principal that we iterate over.

::: layout/base/nsLayoutUtils.cpp:8013
(Diff revision 2)
> +  if (sStyloBlocklist.IsEmpty()) {
> +    return false;
> +  }
> +
> +  for (nsCOMPtr<nsIPrincipal> blockPrincipals : sStyloBlocklist) {
> +    if (aPrincipal->SubsumesConsideringDomain(blockPrincipals)) {

I think this needs to be the other way around.  sStyloBlocklist will contain eTLD+1s, and we want to check whether any of those origins subsume the one for the document we're testing.  (For example, we want to return true if the blocklist contains "example.com" and the document is "abc.example.com".)

I do wonder now whether we need to call the "ConsiderDomain" version of the function here.  If the blocklist only ever contains eTLD+1s, per your comment 14, then I suppose it's not possible for an iframe to become accessible to the different-style-backend parent document after messing with document.domain.

Although I saw some warning in the HTML spec about ports being ignored after document.domain is set (but then following the algorithms couldn't see anything about that), so I'm not sure.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14)
> One way that we could avoid this issue is by only allow setting effectiveTLD
> + 1 domains in the blocklist, which in the example above, the entry in the
> blocklist would be "google.com". In this way, both parent and its child will
> be styled with the same backend type.

Yes, this is what I had in mind.

> The disadvantage would be that we
> might overly block too many pages in the same effectiveTLD + 1 domain.

Sure, but I think we'll have to accept that. There's no other way to do it sanely without risking the origins becoming same-origin via document.domain.

> 
> 
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> > (In reply to Cameron McCormack (:heycam) from comment #12)
> > > What about for cases like an about:blank iframe or an iframe with a srcdoc? 
> > > (Or other cases I'm forgetting where the outer page can reach into the
> > > iframe.)  Is there some function we can call to check this, since checking
> > > the eTLD+1 won't be enough.
> > 
> > I think we want to check the origin of the document principal, rather than
> > the document URI per se. That will let us piggy-back off the existing
> > principal relationships and symmetries.
> 
> Ok, then we probably should create a principal object for each domain in the
> blocklist, and store these principals in an nsTArray. The hashtable might
> not be appropriate for complicated objects like principal I guess.

I don't know if we really need principals here. I think an array if nsIURIs might be sufficient? See the mFileURIWhitelist stuff  we have already [1], which is probably a decent starting point to look at.

> 
> Is this subsumesConsideringDomain() [1] the right function that we should
> use for this purpose?

No, I think we instead want to just extract the eTLD+1 of the document principal and compare it against the entries in our whitelist.

[1] http://searchfox.org/mozilla-central/rev/c296ed9811319cdd61ac35e9e648f95639cda726/caps/nsScriptSecurityManager.h#131

Comment 23

[Chris Peterson [:cpeterson]]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #15)
> Oh, also, I wonder if someone could provide suggestions about the automated
> testing? I can refer to some known behavior change cases (between Gecko and
> Stylo) and do some manual tests for sure. However, it's unclear for me to
> write automated tests if we don't have a Web API to get the style backend.

Can you use a mochitest-chrome or browser-chrome test to access privileged information about the page's style system?


(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #16)
> Since I'm not sure if we'll need a initial stylo blocklist file which ships
> with the release, I'm going to hold this part for now, and ask for
> review/feedback for the other two parts.

For manual testing of the stylo-blocklist while Stylo is in Beta 57, we can temporarily add some test domains (like example.com) to the blocklist on the Kinto server. But we should empty the stylo-blocklist before Stylo ships to Release to avoid client overhead from a non-empty blocklist.

Comment 24

[Cameron McCormack (:heycam)]

Bobby, is my concern about window.open("about:blank") in comment 21 warranted?

Comment 25

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #24)
> Bobby, is my concern about window.open("about:blank") in comment 21
> warranted?

I think we should keep the browsing context hierarchy out of this decision. As long as you use eTLD+1, it shouldn't matter whether any parent/child document uses the same style system, because the access will never be same-origin and the style backend isn't observable over cross-origin-exposed properties.

The one thing to consider is that document URI/principal can be reset via HTMLDocument::Open (document.open). I believe all the content is ejected there, but we should update the style backend, and verify that we end up with a new style set. Automated tests for this would probably be good.

Comment 26

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #25)
> I think we should keep the browsing context hierarchy out of this decision.
> As long as you use eTLD+1, it shouldn't matter whether any parent/child
> document uses the same style system, because the access will never be
> same-origin and the style backend isn't observable over cross-origin-exposed
> properties.

But how do about:blank documents fit into our eTLD+1 test?  Don't we need to ensure that any about:blank documents accessible by the top level document have a matching style backend?

> The one thing to consider is that document URI/principal can be reset via
> HTMLDocument::Open (document.open). I believe all the content is ejected
> there, but we should update the style backend, and verify that we end up
> with a new style set. Automated tests for this would probably be good.

Filed bug 1404668 for that.

Comment 27

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #26)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #25)
> > I think we should keep the browsing context hierarchy out of this decision.
> > As long as you use eTLD+1, it shouldn't matter whether any parent/child
> > document uses the same style system, because the access will never be
> > same-origin and the style backend isn't observable over cross-origin-exposed
> > properties.
> 
> But how do about:blank documents fit into our eTLD+1 test?  Don't we need to
> ensure that any about:blank documents accessible by the top level document
> have a matching style backend?

Any about:blank doc that is accessible from the parent will have, by definition, inherited that parent's principal. So as long as we base the check on the principal's origin URI, rather than the document URI, we should be fine.

Comment 28

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

https://reviewboard.mozilla.org/r/183990/#review190274

> I think we do still need some sort of "parent document" check like this, since the parent document from an origin in the blocklist could have an about:blank iframe.
> 
> But I don't know that GetParentDocument() is enough, since I think that is only for <iframe> parents, and the page could also do window.open("about:blank") and poke at that new window's document.

Per comment 27 from Bobby, I think we should be fine with the existing check for "parent document".

> I'm not sure if we have checks for this, but we should avoid adding new static variables that need constructors to run before main.  (It affects startup time and I think the order of static constructors running is not well defined.)
> 
> So instead, let's make this variable a pointer, initialized to nullptr in its definition, and just create a new nsTArray when we load and process the blocklist.  And delete the nsTArray in nsLayoutUtils::Shutdown of course.
> 
> (Using UniquePtr instead of a raw pointer would also be fine, since its constructor is constexpr, and so doesn't cause a construct call during static initialization before main.  But I don't think UniquePtr has much advantage here.  We'd need to clear / delete the array before the UniquePtr structor would run (after main) to avoid the nsIPrincipal objects from being reported as leaked.)

Ok, I'll make this static variable a pointer.

> It's worth some sort of error reporting like this, even though the best option is just to continue as if the blocklist was empty.
> 
> Asserting NS_SUCCEEDED(rv) is better than checking for NS_OK explicitly.  (In theory, there are nsresult values other than NS_OK that are also "success" values.)

I agree that we should just continue as if the blocklist was empty.

I think we can remove this assertion for now, and consider to add some error reporting into LoadAndCreateStyloBlocklist() while implementing it.

> No need to check for whether the array is empty before calling Clear.

Ok.

> No need for this either.  The iteration over sStyloBlocklist will quickly determine that that are no elements to iterate.
> 
> Though once we change sStyloBlocklist to a pointer, you can just check it for null.

Ok.

> This should be:
> 
>   for (nsIPrincipal* blockPrincipals ...
> 
> to avoid bumping the refcounts of each principal that we iterate over.

Ok.

> I think this needs to be the other way around.  sStyloBlocklist will contain eTLD+1s, and we want to check whether any of those origins subsume the one for the document we're testing.  (For example, we want to return true if the blocklist contains "example.com" and the document is "abc.example.com".)
> 
> I do wonder now whether we need to call the "ConsiderDomain" version of the function here.  If the blocklist only ever contains eTLD+1s, per your comment 14, then I suppose it's not possible for an iframe to become accessible to the different-style-backend parent document after messing with document.domain.
> 
> Although I saw some warning in the HTML spec about ports being ignored after document.domain is set (but then following the algorithms couldn't see anything about that), so I'm not sure.

Yeah, since we all agree that the blocklist will only contain eTLD+1s, I think we can use some sort of string matching to do the check. So, we don't need this kind of APIs anymore.

Comment 29

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

https://reviewboard.mozilla.org/r/183990/#review190274

> Per comment 27 from Bobby, I think we should be fine with the existing check for "parent document".

Oops, misread!! Per comment 25, we don't need the GetParentDocument() checks at all.

Comment 30

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183986/diff/2-3/

Comment 31

[Jeremy Chen ([e-mail])]

Comment on attachment 8912661 [details]
Bug 1403077 - add a initial stylo blocklist file which ships with the release.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183988/diff/2-3/

Comment 32

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183990/diff/2-3/

Comment 33

[Jeremy Chen ([e-mail])]

(In reply to Mathieu Leplatre (:leplatrem) from comment #20)
> Comment on attachment 8912660 [details]
> Bug 1403077 - add a new BlocklistClient for stylo-blocklist.
> 
> https://reviewboard.mozilla.org/r/183986/#review190118
> 
> ::: services/common/blocklist-clients.js:490
> (Diff revision 2)
> > +this.StyloBlocklistClient = new BlocklistClient(
> > +  Services.prefs.getCharPref(PREF_BLOCKLIST_STYLO_COLLECTION),
> > +  PREF_BLOCKLIST_STYLO_CHECKED_SECONDS,
> > +  (records) => updateJSONBlocklist(this.StyloBlocklistClient.filename, records),
> > +  Services.prefs.getCharPref(PREF_BLOCKLIST_BUCKET)
> > +);
> 
> You might want to send an event or notify other components that a file was
> dumped on disk.
> 
> Otherwise, I guess you could also import your interface here and call
> internal APIs to update the internal state.

Hi Mathieu, thanks for the review.

I thought updateJSONBlocklist() already handled the event notification stuff.
Could you enlighten me a bit more about what specific component that I should notify?

Comment 34

[Bobby Holley (:bholley)]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

https://reviewboard.mozilla.org/r/183990/#review190766

::: layout/base/nsLayoutUtils.cpp:8013
(Diff revision 3)
> +/* static */
> +bool
> +nsLayoutUtils::IsInStyloBlocklist(const nsCString* aBaseDomain)
> +{
> +  if (IsStyloBlocklistEmpty()) {
> +    return false;
> +  }

I don't think we should be doing the string comparisons ourselves. Why don't we do something similar to what we do in IsInFileURIWhitelist?

http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/caps/nsScriptSecurityManager.cpp#1178

Comment 35

[Bobby Holley (:bholley)]

More specifically, I mean that we should probably be going through nsScriptSecurityManager::SecurityCompareURIs.

Comment 36

[Jeremy Chen ([e-mail])]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #34)
> Comment on attachment 8912662 [details]
> Bug 1403077 - (wip) apply the stylo blocklist.
> 
> https://reviewboard.mozilla.org/r/183990/#review190766
> 
> ::: layout/base/nsLayoutUtils.cpp:8013
> (Diff revision 3)
> > +/* static */
> > +bool
> > +nsLayoutUtils::IsInStyloBlocklist(const nsCString* aBaseDomain)
> > +{
> > +  if (IsStyloBlocklistEmpty()) {
> > +    return false;
> > +  }
> 
> I don't think we should be doing the string comparisons ourselves. Why don't
> we do something similar to what we do in IsInFileURIWhitelist?
> 
> http://searchfox.org/mozilla-central/rev/
> 298033405057ca7aa5099153797467eceeaa08b5/caps/nsScriptSecurityManager.
> cpp#1178

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #35)
> More specifically, I mean that we should probably be going through
> nsScriptSecurityManager::SecurityCompareURIs.

Yes, nsScriptSecurityManager::SecurityCompareURIs is what I've looked before, but it seems not taking the subdomain (ex. google.com and mail.google.com) into consideration. So, I decides to extract eTLD+1 from each principal through GetBaseDomain [1], which returns a nsCString.

If we'd like to go with URIs and reuse some existing codes, then probably EqualOrSubdomain() [2] in nsScriptSecurityManager::InFileURIWhitelist(), which you just mentioned above, would be a good fit?


[1] http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/netwerk/dns/nsIEffectiveTLDService.idl#39-79
[2] http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/caps/nsScriptSecurityManager.cpp#607

Comment 37

[Bobby Holley (:bholley)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #36)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #34)
> > Comment on attachment 8912662 [details]
> > Bug 1403077 - (wip) apply the stylo blocklist.
> > 
> > https://reviewboard.mozilla.org/r/183990/#review190766
> > 
> > ::: layout/base/nsLayoutUtils.cpp:8013
> > (Diff revision 3)
> > > +/* static */
> > > +bool
> > > +nsLayoutUtils::IsInStyloBlocklist(const nsCString* aBaseDomain)
> > > +{
> > > +  if (IsStyloBlocklistEmpty()) {
> > > +    return false;
> > > +  }
> > 
> > I don't think we should be doing the string comparisons ourselves. Why don't
> > we do something similar to what we do in IsInFileURIWhitelist?
> > 
> > http://searchfox.org/mozilla-central/rev/
> > 298033405057ca7aa5099153797467eceeaa08b5/caps/nsScriptSecurityManager.
> > cpp#1178
> 
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #35)
> > More specifically, I mean that we should probably be going through
> > nsScriptSecurityManager::SecurityCompareURIs.
> 
> Yes, nsScriptSecurityManager::SecurityCompareURIs is what I've looked
> before, but it seems not taking the subdomain (ex. google.com and
> mail.google.com) into consideration. So, I decides to extract eTLD+1 from
> each principal through GetBaseDomain [1], which returns a nsCString.
> 
> If we'd like to go with URIs and reuse some existing codes, then probably
> EqualOrSubdomain() [2] in nsScriptSecurityManager::InFileURIWhitelist(),
> which you just mentioned above, would be a good fit?

Hm, I guess you're right that doing it with URIs is kind of a pain if the eTLD service returns a string. I guess string comparisons are probably fine here, given that we don't really care about all the various other security edge cases that NS_SecurityCompareURIs handles.

Comment 38

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183986/diff/3-4/

Comment 39

[Jeremy Chen ([e-mail])]

Comment on attachment 8912661 [details]
Bug 1403077 - add a initial stylo blocklist file which ships with the release.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183988/diff/3-4/

Comment 40

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183990/diff/3-4/

Comment 41

[Jeremy Chen ([e-mail])]

(In reply to Chris Peterson [:cpeterson] from comment #23)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #15)
> > Oh, also, I wonder if someone could provide suggestions about the automated
> > testing? I can refer to some known behavior change cases (between Gecko and
> > Stylo) and do some manual tests for sure. However, it's unclear for me to
> > write automated tests if we don't have a Web API to get the style backend.
> 
> Can you use a mochitest-chrome or browser-chrome test to access privileged
> information about the page's style system?

Yes, I'll try to add an internal JS API, which only expose to chrome priviledge, to get the style backend.

> 
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #16)
> > Since I'm not sure if we'll need a initial stylo blocklist file which ships
> > with the release, I'm going to hold this part for now, and ask for
> > review/feedback for the other two parts.
> 
> For manual testing of the stylo-blocklist while Stylo is in Beta 57, we can
> temporarily add some test domains (like example.com) to the blocklist on the
> Kinto server. But we should empty the stylo-blocklist before Stylo ships to
> Release to avoid client overhead from a non-empty blocklist.

Sounds like a plan. We can definitely do this once we uplift the patchset to Beta 57.

Comment 42

[Jan Keromnes [:janx]]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

https://reviewboard.mozilla.org/r/183990/#review190990

Quick note: Our static analysis bot found a small code issue in this patch. Publishing it as a drive-by review.

::: layout/base/nsLayoutUtils.cpp:8025
(Diff revision 4)
> +{
> +  if (IsStyloBlocklistEmpty()) {
> +    return false;
> +  }
> +
> +  for (nsCString domains : *sStyloBlocklist) {

Warning: Loop variable is copied but only used as const reference; consider making it a const [clang-tidy: performance-for-range-copy]

```
for (nsCString domains : *sStyloBlocklist) {
                 ^
       const    &
```

Comment 43

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #41)
> (In reply to Chris Peterson [:cpeterson] from comment #23)
> > (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #15)
> > > Oh, also, I wonder if someone could provide suggestions about the automated
> > > testing? I can refer to some known behavior change cases (between Gecko and
> > > Stylo) and do some manual tests for sure. However, it's unclear for me to
> > > write automated tests if we don't have a Web API to get the style backend.
> > 
> > Can you use a mochitest-chrome or browser-chrome test to access privileged
> > information about the page's style system?
> 
> Yes, I'll try to add an internal JS API, which only expose to chrome
> priviledge, to get the style backend.
> 

Per xidorn's suggestion, we can use the existing DOMWindowUtils.isStyledByServo [1] to get the style backend while testing, so probably no need to add a new one.

Here are the steps of the tests in my mind:

1. test for blocked document
1.1. open a document whose domain is in the blocklist
1.2. check if the current document's style backend does fallback to Gecko

2. test for non-blocked document
2.1. open a document whose domain is not in the blocklist
2.2. check if the current document's style backend is Servo


Q1: Should we use the real blocklist JSON file, or use a mock one?
Ans: We can probably use a mock one since we're using a test-profile while testing, so dumping a mock JSON file and removing it after testing may be fine.

Q2: What document should we open?
Ans: Since we can't get to external links while testing, we probably have to use http server and open a file through "`http://localhost", which means we should put "localhost" into our mock blocklist JSON.


Hmmm, just saw that Bug 1257565 is not landed yet, which means I might need to put some extra codes to ensure we do update the stylo blocklist JSON file. I'll try manually testing once I finish the rest of the client side codes. Any feedback about writing automated tests are appreciated.


[1] http://searchfox.org/mozilla-central/search?q=isStyledByServo&case=true&regexp=false&path=

Comment 44

[Mathieu Leplatre [:leplatrem]]

> > You might want to send an event or notify other components that a file was
> > dumped on disk.
> > 
> > Otherwise, I guess you could also import your interface here and call
> > internal APIs to update the internal state.
> 
> Hi Mathieu, thanks for the review.
> 
> I thought updateJSONBlocklist() already handled the event notification stuff.
> Could you enlighten me a bit more about what specific component that I
> should notify?

Yes indeed you're right, you'll receive a "Blocklist:reload-from-disk" event. When subscribing to it, you can ignore events whose `data.filename` do not match yours. 

The specific component you would have to notify is the one you are implementing in this bug, the one that compares the domain to load with the entries in the blocklist (`nsLayoutUtils::LoadAndCreateStyloBlocklist()` ?)

Comment 45

[Cameron McCormack (:heycam)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #43)
> Q2: What document should we open?
> Ans: Since we can't get to external links while testing, we probably have to
> use http server and open a file through "`http://localhost", which means we
> should put "localhost" into our mock blocklist JSON.

Will we only have once chance in the whole test session to read the blocklist file?  If so, then we should choose a hostname that isn't used in any other tests.  You can make the test harness HTTP server recognise new hostnames by adding them here:

  http://searchfox.org/mozilla-central/source/build/pgo/server-locations.txt

(In reply to Mathieu Leplatre (:leplatrem) from comment #44)
> > > You might want to send an event or notify other components that a file was
> Yes indeed you're right, you'll receive a "Blocklist:reload-from-disk"
> event. When subscribing to it, you can ignore events whose `data.filename`
> do not match yours. 

I think we will need to ignore any blocklist reloads that happen, because otherwise we could have a top level page that uses Stylo (if its load happens before the blocklist reload) and a child iframe that uses Gecko (if its load happens after the blocklist reload).  This is similar to the pref change problem we're hypothesising is the cause of bug 1404020.

Comment 46

[Xidorn Quan [:xidorn] UTC+10]

Can we avoid updating blocklist after startup? I mean, after we open any page. If some page contains both backends, and they try to move element across backend boundary, it could crash (see bug 1404020). It has been happening because of stylo pref experiment. I hope we don't trigger the same issue in release because of that.

Comment 47

[Xidorn Quan [:xidorn] UTC+10]

(Oh, heycam beat me :)

Comment 48

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I am told this is a must for 57 and therefore tracking this as a blocking issue.

Comment 49

[Jeremy Chen ([e-mail])]

Per discussion with Chirs and the Release Management team, we might want to consider replacing the Kinto list with a simple list of domains in a pref string. We can then update the pref for Release channel users with a system add-on.

Pros:
1. parse a pref list should be easier than reading Kinto's JSON files
2. make local and automated testing easier
3. no need to ask the Cloud Operations team for code reviews every time we want to change the domain blocklist.

If we planned to keep this domain blocklist forever (hopefully not), then a robust solution like Kinto might make sense. But a simple pref is probably good enough for us.

So, I'll update the patchset in this way.

(In reply to Cameron McCormack (:heycam) from comment #45)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #43)
> > Q2: What document should we open?
> > Ans: Since we can't get to external links while testing, we probably have to
> > use http server and open a file through "`http://localhost", which means we
> > should put "localhost" into our mock blocklist JSON.
> 
> Will we only have once chance in the whole test session to read the
> blocklist file?  If so, then we should choose a hostname that isn't used in
> any other tests.  You can make the test harness HTTP server recognise new
> hostnames by adding them here:
> 
>   http://searchfox.org/mozilla-central/source/build/pgo/server-locations.txt
> 

Ok, I'll add new mock hostnames just for testing stylo blocklist.

> (In reply to Mathieu Leplatre (:leplatrem) from comment #44)
> > > > You might want to send an event or notify other components that a file was
> > Yes indeed you're right, you'll receive a "Blocklist:reload-from-disk"
> > event. When subscribing to it, you can ignore events whose `data.filename`
> > do not match yours. 
> 
> I think we will need to ignore any blocklist reloads that happen, because
> otherwise we could have a top level page that uses Stylo (if its load
> happens before the blocklist reload) and a child iframe that uses Gecko (if
> its load happens after the blocklist reload).  This is similar to the pref
> change problem we're hypothesising is the cause of bug 1404020.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #46)
> Can we avoid updating blocklist after startup? I mean, after we open any
> page. If some page contains both backends, and they try to move element
> across backend boundary, it could crash (see bug 1404020). It has been
> happening because of stylo pref experiment. I hope we don't trigger the same
> issue in release because of that.

Cameron and Xidorn, thank you for the pointers. I agree that we should avoid moving element across backend boundary. In the current plan, we only read the blocklist (from JSON before, but from a pref now) once, at the time that nsLayoutUtils is initialized. The blocklist doesn't response to any pref change event, and is keeping unchanged within nsLayoutUtils. Since the blocklist can't be updated unless we restart/reinitialize nsLayoutUtils, we should be fine.

Comment 50

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183990/diff/4-5/

Comment 51

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183986/diff/4-5/

Comment 52

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Clear the r+ manually, since this is a whole new patch.

Comment 53

[Jeremy Chen ([e-mail])]

Comment on attachment 8912151 [details] [review]
Cloudops configuration PR.

According to offline discussion and comment 49, I think we can obsolete this patch now.

Rémy, very appreciate your help.

Comment 54

[Rémy Hubscher (:natim)]

You can obsolete it here but we still need to merge it I have updated the PR to remove stylo blocklists from it.

Comment 55

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183990/diff/5-6/

Comment 56

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183986/diff/5-6/

Comment 57

[Chris Peterson [:cpeterson]]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

https://reviewboard.mozilla.org/r/183990/#review192398

::: layout/base/nsLayoutUtils.cpp:6149
(Diff revisions 5 - 6)
>      else
>        shadowColor = aForegroundColor;
>  
>      // Webrender just needs the shadow details
>      if (auto* textDrawer = aContext->GetTextDrawer()) {
> -      wr::TextShadow wrShadow;
> +      wr::Shadow wrShadow;

Is this wr::Shadow change might be an unintentional change after resolving a merge conflict?

Comment 58

[Jeremy Chen ([e-mail])]

(In reply to Chris Peterson [:cpeterson] from comment #57)
> Comment on attachment 8912662 [details]
> Bug 1403077 - implement the stylo blocklist mechanism.
> 
> https://reviewboard.mozilla.org/r/183990/#review192398
> 
> ::: layout/base/nsLayoutUtils.cpp:6149
> (Diff revisions 5 - 6)
> >      else
> >        shadowColor = aForegroundColor;
> >  
> >      // Webrender just needs the shadow details
> >      if (auto* textDrawer = aContext->GetTextDrawer()) {
> > -      wr::TextShadow wrShadow;
> > +      wr::Shadow wrShadow;
> 
> Is this wr::Shadow change might be an unintentional change after resolving a
> merge conflict?

Yeah, I think so. I removed it on my local, but not updated it to reviewboard yet.
Thanks for the pointer. :)

Comment 59

[Jeremy Chen ([e-mail])]

Looks like using testing profile preferences for testing might not work. I think we read the pref pretty early and only ONCE, so we can never get to use the testing profile preferences to do the test.

Here's a previous try for my WIP testcase:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7388e03f0fcdbd5d51c5494f8d07600fc034c36b

So, I wonder if we can put a mock test domain in user preferences, i.e., all.js?

Comment 60

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183990/diff/6-7/

Comment 61

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183986/diff/6-7/

Comment 62

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183990/diff/7-8/

Comment 63

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183986/diff/7-8/

Comment 64

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183986/diff/8-9/

Comment 65

[Jeremy Chen ([e-mail])]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4acd68e7bc5a402e74743c36f4d75de6e2a02528

Comment 66

[Cameron McCormack (:heycam)]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

https://reviewboard.mozilla.org/r/183990/#review192600

::: dom/base/nsDocument.cpp:13670
(Diff revision 8)
>      }
>    }
>    return false;
>  }
> +
> +// Principal-based blacklist for stylo.

"blocklist" (so we have consistent naming blocklist vs blacklist)

::: dom/base/nsDocument.cpp:13675
(Diff revision 8)
> +static bool
> +IsBlockedByStyloBlocklist(nsIPrincipal* aPrincipal)

I would probably move the contents of this function into nsLayoutUtils::IsInStyloBlocklist.  Then you don't need to have a public IsStyloBlocklistEmpty function.

::: dom/base/nsDocument.cpp:13693
(Diff revision 8)
> +  // XXX: Is it possible that we can't get the eTLD service?
> +  //      If so, we might want to fallback to use GetAsciiHost(), and extract
> +  //      eTLD + 1 domain manually.

I think services like this are only unavailable if the browser is broken, or if we're trying to use it very early (before it's initialized) or very late (after it's shut down).  I think it's fine to warn and return false like you are doing here.

::: layout/base/nsLayoutUtils.cpp:7977
(Diff revision 8)
> +  Preferences::AddBoolVarCache(&sStyloBlocklistEnabled,
> +                               "layout.css.stylo-blocklist.enabled");
> +  if (sStyloBlocklistEnabled &&

I think we don't need to use AddBoolVarCache for this pref, since we also only need to look at this pref's value once, here.

::: layout/base/nsLayoutUtils.cpp:7992
(Diff revision 8)
> +        const nsCString& domain = PromiseFlatCString(domainString);
> +        sStyloBlocklist->AppendElement(domain);

I think it should be fine to do:

  sStyloBlocklist->AppendElement(domainString);

Comment 67

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

https://reviewboard.mozilla.org/r/183990/#review192600

> I would probably move the contents of this function into nsLayoutUtils::IsInStyloBlocklist.  Then you don't need to have a public IsStyloBlocklistEmpty function.

Ok, will do.

> I think we don't need to use AddBoolVarCache for this pref, since we also only need to look at this pref's value once, here.

Yes, you're right. Will fix it!

> I think it should be fine to do:
> 
>   sStyloBlocklist->AppendElement(domainString);

Ok.

Comment 68

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #59)
> Looks like using testing profile preferences for testing might not work. I
> think we read the pref pretty early and only ONCE, so we can never get to
> use the testing profile preferences to do the test.
> 
> Here's a previous try for my WIP testcase:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7388e03f0fcdbd5d51c5494f8d07600fc034c36b
> 
> So, I wonder if we can put a mock test domain in user preferences, i.e.,
> all.js?

Per discussed w/ Cameron, I'll use one or two nsIDOMWindowUtils APIs to update the blocklist for testing purpose, so probably no need to put mock domains in user preferences. Patches on the way.

Comment 69

[Jeremy Chen ([e-mail])]

Attachment: Bug 1403077 - add two test-only helper functions to access the stylo blocklist.

In the current blocklist impelemtation, we read the stylo blocklist from the user
preferences very early and only once, which even earlier than the test preferences
updating happens.

So, to be able to test the functionality of the stylo blocklist, we add these two
nsIDOMWindowUtils APIs to be able to add/remove a mock domain to the existing
blocklist.

Review commit: https://reviewboard.mozilla.org/r/187638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187638/

Comment 70

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183990/diff/8-9/

Comment 71

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183986/diff/9-10/

Comment 72

[Cameron McCormack (:heycam)]

Comment on attachment 8916502 [details]
Bug 1403077 - add two test-only helper functions to access the stylo blocklist.

https://reviewboard.mozilla.org/r/187638/#review192608

::: commit-message-2fccd:3
(Diff revision 1)
> +Bug 1403077 - add a test-only helper function to append the stylo blocklist.
> +
> +In the current blocklist impelemtation, we read the stylo blocklist from the user

implementation

::: commit-message-2fccd:4
(Diff revision 1)
> +Bug 1403077 - add a test-only helper function to append the stylo blocklist.
> +
> +In the current blocklist impelemtation, we read the stylo blocklist from the user
> +preferences very early and only once, which even earlier than the test preferences

"which is even earlier" or just "even earlier"

Comment 73

[Cameron McCormack (:heycam)]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

https://reviewboard.mozilla.org/r/183986/#review192610

Comment 74

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183990/diff/9-10/

Comment 75

[Jeremy Chen ([e-mail])]

Comment on attachment 8916502 [details]
Bug 1403077 - add two test-only helper functions to access the stylo blocklist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187638/diff/1-2/

Comment 76

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183986/diff/10-11/

Comment 77

[Jeremy Chen ([e-mail])]

Comment on attachment 8916502 [details]
Bug 1403077 - add two test-only helper functions to access the stylo blocklist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187638/diff/2-3/

Comment 78

[Jeremy Chen ([e-mail])]

Comment on attachment 8912660 [details]
Bug 1403077 - add tests for the stylo blocklist mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183986/diff/11-12/

Comment 79

[Jeremy Chen ([e-mail])]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbe035acc4d78670443fd49bab68a108c1791fc1

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08363e433669
implement the stylo blocklist mechanism. r=heycam
https://hg.mozilla.org/integration/autoland/rev/abf7029dbcd0
add two test-only helper functions to access the stylo blocklist. r=heycam
https://hg.mozilla.org/integration/autoland/rev/13255dcbdaf6
add tests for the stylo blocklist mechanism. r=heycam,leplatrem

Comment 81

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/08363e433669
https://hg.mozilla.org/mozilla-central/rev/abf7029dbcd0
https://hg.mozilla.org/mozilla-central/rev/13255dcbdaf6

Comment 82

[Jeremy Chen ([e-mail])]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

Approval Request Comment
[Feature/Bug causing the regression]: A precaution mechanism to disable stylo (and fall back to the old Gecko style system) for broken websites without a dot release.
[User impact if declined]: Need a dot release to disable stylo if necessary.
[Is this code covered by automated tests?]: Yes, the tests are in another part of the patchset.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low
[Why is the change risky/not risky?]: We have a pref to disable the mechanism if necessary.
[String changes made/needed]: None

Comment 83

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8912662 [details]
Bug 1403077 - implement the stylo blocklist mechanism.

This is a must fix, beta57+

Hi Jeremy, Chris, what mechanism will we use to do this pref change when we hit release? Will it be a new system add-on or an existing one? Shield can be used for pref flips but it has a limitation wherein it cannot reach a 100% of the channel population.

Comment 84

[Chris Peterson [:cpeterson]]

(In reply to Ritu Kothari (:ritu) from comment #83)
> Hi Jeremy, Chris, what mechanism will we use to do this pref change when we
> hit release? Will it be a new system add-on or an existing one? Shield can
> be used for pref flips but it has a limitation wherein it cannot reach a
> 100% of the channel population.

We plan to use a system add-on to change this Stylo pref, if necessary. I'll follow up with Jeremy about preparing a new system add-on so it's ready if we need it.

Comment 85

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

That sounds like a good plan. Thanks Chris.

Comment 86

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Beta.

Comment 87

[Jeremy Chen ([e-mail])]

Attachment: part1 for beta: implement the stylo blocklist mechanism. r=heycam

In this patch, we read the stylo blocklist into nsLayoutUtils's global static
variable during nsLayoutUtils::Initialize(). So, we can decide if we should
fallback to use Gecko backend while updating style backend for a document.

We add "layout.css.stylo-blocklist.blocked_domains" and
"layout.css.stylo-blocklist.enabled" to ContentPrefs.cpp because they are read
very early (during nsLayoutUtils::Initialize).

MozReview-Commit-ID: IiTVf6qD9uO

Comment 88

[Jeremy Chen ([e-mail])]

Attachment: part2 for beta: add two test-only helper functions to access the stylo blocklist. r=heycam

In the current blocklist implementation, we read the stylo blocklist from the user
preferences very early and only once, even earlier than the test preferences
updating happens.

So, to be able to test the functionality of the stylo blocklist, we add these two
nsIDOMWindowUtils APIs to be able to add/remove a mock domain to the existing
blocklist.

MozReview-Commit-ID: 1iRnGmyTJgC

Comment 89

[Jeremy Chen ([e-mail])]

Attachment: part3 for beta: add tests for the stylo blocklist mechanism. r=heycam

In this patch, we add 3 tests:
1. test for blocked domain
2. test for blocked sub-domain
3. test for non-blocked domain

MozReview-Commit-ID: 54DDxE77Yif

Comment 90

[Jeremy Chen ([e-mail])]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #86)
> Needs rebasing for Beta.

Rebased to Beta. Please refer to the following three attached patches:

part1 for beta: implement the stylo blocklist mechanism. r=heycam
part2 for beta: add two test-only helper functions to access the stylo blocklist. r=heycam
part3 for beta: add tests for the stylo blocklist mechanism. r=heycam

Thanks.

Comment 91

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c2c10508ec8d
https://hg.mozilla.org/releases/mozilla-beta/rev/1059f19c8136
https://hg.mozilla.org/releases/mozilla-beta/rev/f4ba99b41183

Comment 92

[Andrei Vaida [:avaida]]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #82)
> [User impact if declined]: Need a dot release to disable stylo if necessary.
> [Is this code covered by automated tests?]: Yes, the tests are in another
> part of the patchset.
> [Needs manual test from QE? If yes, steps to reproduce]:

Setting qe-verify- based on the fact that this fix has automated coverage.