Closed Bug 1403077 (stylo-blocklist) Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
57 Branch
Type:
enhancement

Tracking

()

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

People

(Reporter: cpeterson, Assigned: chenpighead)

References

Details

Attachments

(6 files, 2 obsolete files)

Bug 1403077 - add tests for the stylo blocklist mechanism.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1403077 - implement the stylo blocklist mechanism.
59 bytes, text/x-review-board-request
heycam
: review+
ritu
: approval-mozilla-beta+
Details
Bug 1403077 - add two test-only helper functions to access the stylo blocklist.
59 bytes, text/x-review-board-request
heycam
: review+
Details
part1 for beta: implement the stylo blocklist mechanism. r=heycam
10.20 KB, patch
Details | Diff | Splinter Review
part2 for beta: add two test-only helper functions to access the stylo blocklist. r=heycam
5.42 KB, patch
Details | Diff | Splinter Review
part3 for beta: add tests for the stylo blocklist mechanism. r=heycam
6.71 KB, patch
Details | Diff | Splinter Review 
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"
}
Assignee: nobody → jeremychen
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
Attached file Cloudops configuration PR. (obsolete) — 

        Attachment #8912151 -
        Flags: review?(wezhou)

        Attachment #8912151 -
        Flags: review?(jthomas)
Depends on: 1403134

    
    

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

    
  
Depends on: 1403088

    
    

    
  
(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

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
  
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.

    
    

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

          tracking-firefox57:
          --- → ?

    
    

    
  
(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...

    
    

    
  
(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.

    
    

    
  
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.
Flags: needinfo?(bobbyholley)

    
    

    
  
(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.
Flags: needinfo?(bobbyholley)

    
    

    
  
(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
Flags: needinfo?(bobbyholley)

    
    

    
  
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.

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

        Attachment #8912662 -
        Flags: feedback?(cam)

    
    

    
  
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.

        Attachment #8912660 -
        Flags: review?(mathieu) → review+

    
    

    
  
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.

        Attachment #8912662 -
        Flags: feedback?(cam)

    
    

    
  
(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
Flags: needinfo?(bobbyholley)

    
    

    
  
(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.

    
    

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

    
    

    
  
(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.
Flags: needinfo?(bobbyholley)
Depends on: 1404668

    
    

    
  
(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.

    
    

    
  
(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 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 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.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

        Attachment #8912662 -
        Flags: feedback?(cam)

    
    

    
  
(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?
Flags: needinfo?(mathieu)

    
    

    
  
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

    
    

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

    
    

    
  
(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
Flags: needinfo?(bobbyholley)

    
    

    
  
(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.
Flags: needinfo?(bobbyholley)

        Attachment #8912662 -
        Flags: feedback?(cam)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(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.

        Attachment #8912662 -
        Flags: feedback?(cam)

    
    

    
  
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    &
```

    
    

    
  
(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=

    
    

    
  
> > 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()` ?)
Flags: needinfo?(mathieu)

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
(Oh, heycam beat me :)

    
    

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

          tracking-firefox57:
          ? → blocking

    
    

    
  
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.

        Attachment #8912662 -
        Flags: feedback?(cam)

    
    

    
      
  

    
    

    
      
  

        Attachment #8912661 -
        Attachment is obsolete: true

    
    

    
  
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.

        Attachment #8912660 -
        Flags: review+

    
    

    
  
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.

        Attachment #8912151 -
        Attachment is obsolete: true

        Attachment #8912151 -
        Flags: review?(wezhou)

        Attachment #8912151 -
        Flags: review?(jthomas)

    
    

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

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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?

    
    

    
  
(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. :)

    
    

    
  
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 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);

        Attachment #8912662 -
        Flags: review?(cam) → review+

    
    

    
  
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.

    
    

    
  
(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 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"

        Attachment #8916502 -
        Flags: review?(cam) → review+

    
    

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

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

        Attachment #8912660 -
        Flags: review?(cam) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by jichen@mozilla.com:
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 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

        Attachment #8912662 -
        Flags: approval-mozilla-beta?
Depends on: 1407096
Depends on: 1407098

    
    

    
  
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.
Flags: needinfo?(jeremychen)
Flags: needinfo?(cpeterson)

        Attachment #8912662 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
(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.
Flags: needinfo?(jeremychen)
Flags: needinfo?(cpeterson)

    
    

    
  
That sounds like a good plan. Thanks Chris.

    
    

    
  
Needs rebasing for Beta.
Flags: needinfo?(jeremychen)

    
    

    
  


  
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

    
    

    
  


  
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

    
    

    
  


  
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

    
    

    
  
(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.
Flags: needinfo?(jeremychen) → needinfo?(ryanvm)
Flags: needinfo?(ryanvm)

    
    

    
  
(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.
Flags: qe-verify-
Depends on: 1412182

    
  
Blocks: 1426223

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: