Closed Bug 1408311 Opened 7 years ago Closed 7 years ago

make nsTreeSanitizer use Servo

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(5 files, 1 obsolete file)

Bug 1408311 - Part 1: Make Declaration::RemovePropertyByID return whether it did remove a property.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1408311 - Part 2: Add GetDeclarationBlock method to BindingStyleRule.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1408311 - Part 3: Make nsTreeSanitizer Parse style attributes and style sheets with Servo.
59 bytes, text/x-review-board-request
xidorn
: review+
bzbarsky
: review+
Details
Bug 1408311 - Part 4: Fix bug when serializing sanitized style rules.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Servo PR
41 bytes, text/x-github-pull-request
Details | Review 
nsTreeSanitizer uses nsCSSParser for sanitizing whole style sheets and style="" attributes.  We should switch to calling into Servo for these.
Assignee: nobody → cam
Status: NEW → ASSIGNED
I'm a bit lost finding my way around the various style rule and declaration classes and interfaces.  If there's something better to use than the GetDeclarationBlock I added, let me know.
Attachment #8920031 - Flags: review?(xidorn+moz)
(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #6)
> I'm a bit lost finding my way around the various style rule and declaration
> classes and interfaces.  If there's something better to use than the
> GetDeclarationBlock I added, let me know.

You can probably do what you need via BindingStyleRule::Style(), but that may take longer code and less efficient... so I guess GetDeclarationBlock is probably the best thing we can do.
Comment on attachment 8920034 [details]
Bug 1408311 - Part 3: Make nsTreeSanitizer Parse style attributes and style sheets with Servo.

https://reviewboard.mozilla.org/r/191026/#review196568

::: dom/base/nsTreeSanitizer.cpp:1119
(Diff revision 1)
> -  int32_t ruleCount = sheet->StyleRuleCount();
> -  for (int32_t i = 0; i < ruleCount; ++i) {
> -    mozilla::css::Rule* rule = sheet->GetStyleRuleAt(i);
> +  ErrorResult err;
> +  RefPtr<dom::CSSRuleList> rules =
> +    sheet->GetCssRules(*nsContentUtils::SubjectPrincipal(), err);
> +  err.SuppressException();
> +  if (!rules) {
> +    return true;
> +  }

So this is adding an extra security check... I'm not sure whether it's going to work, since it is unclear to me how tree sanitizer is used.

Probably Henri should have a look at this.

::: dom/base/nsTreeSanitizer.cpp:1185
(Diff revision 1)
> -        nsCOMPtr<nsIURI> baseURI = aElement->GetBaseURI();
> +        RefPtr<css::Declaration> geckoDecl;
> +        RefPtr<ServoDeclarationBlock> servoDecl;
> +        DeclarationBlock* decl = nullptr;

It isn't clear to me why you cannot just use
```c++
RefPtr<DeclarationBlock> decl;
```

It seems `servoDecl` and `geckoDecl` are only used once and are just accepting a return value? A `RefPtr<DeclarationBlock>` should be fine.

::: dom/base/nsTreeSanitizer.cpp:1202
(Diff revision 1)
> +          decl = servoDecl;
> +        } else {
> -        // Pass the CSS Loader object to the parser, to allow parser error
> +          // Pass the CSS Loader object to the parser, to allow parser error
> -        // reports to include the outer window ID.
> +          // reports to include the outer window ID.
> -        nsCSSParser parser(document->CSSLoader());
> +          nsCSSParser parser(document->CSSLoader());
> -        nsAutoString value;
> +          nsCOMPtr<nsIURI> baseURI = aElement->GetBaseURI();

It really should use `aElement->GetURLDataForStyleAttr()` as well. It's probably one of the place that I wasn't aware that we are using base URI for style attribute...

Probably we should change this before landing this bug, so that we can track any regression easier, because this would indeed change the current behavior of stylo.
Attachment #8920034 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8920034 [details]
Bug 1408311 - Part 3: Make nsTreeSanitizer Parse style attributes and style sheets with Servo.

https://reviewboard.mozilla.org/r/191026/#review196570

Generally looks fine to me, but need henri's review for the change in `nsTreeSanitizer::SanitizeStyleSheet` I think.
Comment on attachment 8920034 [details]
Bug 1408311 - Part 3: Make nsTreeSanitizer Parse style attributes and style sheets with Servo.

https://reviewboard.mozilla.org/r/191026/#review196568

> So this is adding an extra security check... I'm not sure whether it's going to work, since it is unclear to me how tree sanitizer is used.
> 
> Probably Henri should have a look at this.

Henri is not accepting review request... oh well. Maybe bz then.
Attachment #8920034 - Flags: review?(bzbarsky)
Comment on attachment 8920044 [details]
Bug 1408311 - Part 4: Fix bug when serializing sanitized style rules.

https://reviewboard.mozilla.org/r/191028/#review196574
Attachment #8920044 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8920031 [details]
style: Make Servo_DeclarationBlock_RemovePropertyById return whether it did remove a property.

https://reviewboard.mozilla.org/r/191020/#review196576
Attachment #8920031 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8920032 [details]
Bug 1408311 - Part 1: Make Declaration::RemovePropertyByID return whether it did remove a property.

https://reviewboard.mozilla.org/r/191022/#review196578
Attachment #8920032 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8920031 [details]
style: Make Servo_DeclarationBlock_RemovePropertyById return whether it did remove a property.

https://reviewboard.mozilla.org/r/191020/#review196580

Actually... There would be only one callsite of `HasProperty` (that in part 3), and that is combined with `RemovePropertyById`. Can we instead make `RemovePropertyById` return a `bool` indicating whether there is indeed a property removed, so that we don't need a separate `HasProperty` call?

Servo's `PropertyDeclarationBlock::remove_property` already returns that, so we just need to update the FFI to pass that back. And it shouldn't be too hard for Gecko's `Declaration` to do that as well.

We may not want to remove `Declaration::HasProperty`, though, because `GeckoCSSAnimationBuilder::GetKeyframeTimingFunction` is another callsite to `Declaration::HasProperty`. But we don't use that for Stylo.
Attachment #8920031 - Flags: review+
Comment on attachment 8920032 [details]
Bug 1408311 - Part 1: Make Declaration::RemovePropertyByID return whether it did remove a property.

https://reviewboard.mozilla.org/r/191022/#review196582
Attachment #8920032 - Flags: review+
Comment on attachment 8920033 [details]
Bug 1408311 - Part 2: Add GetDeclarationBlock method to BindingStyleRule.

https://reviewboard.mozilla.org/r/191024/#review196560

::: layout/style/BindingStyleRule.h:58
(Diff revision 1)
>                                    uint64_t* aSpecificity) = 0;
>    virtual nsresult SelectorMatchesElement(dom::Element* aElement,
>                                            uint32_t aSelectorIndex,
>                                            const nsAString& aPseudo,
>                                            bool* aMatches) = 0;
> +  virtual DeclarationBlock* GetDeclarationBlock() const = 0;

This should never return `nullptr` I suppose? Probably call it `DeclarationBlock` instead and maybe decorate the return type with `NotNull`?
Attachment #8920033 - Flags: review?(xidorn+moz) → review+
Blocks: 779058
Depends on: 1410281
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #9)
> It isn't clear to me why you cannot just use
> ```c++
> RefPtr<DeclarationBlock> decl;
> ```
> 
> It seems `servoDecl` and `geckoDecl` are only used once and are just
> accepting a return value? A `RefPtr<DeclarationBlock>` should be fine.

Oh, that would work.  I overlooked the fact that we're doing the mType-based dispatch for AddRef/Release.

> It really should use `aElement->GetURLDataForStyleAttr()` as well. It's
> probably one of the place that I wasn't aware that we are using base URI for
> style attribute...
> 
> Probably we should change this before landing this bug, so that we can track
> any regression easier, because this would indeed change the current behavior
> of stylo.

Filed bug 1410281.
Comment on attachment 8920033 [details]
Bug 1408311 - Part 2: Add GetDeclarationBlock method to BindingStyleRule.

https://reviewboard.mozilla.org/r/191024/#review196560

> This should never return `nullptr` I suppose? Probably call it `DeclarationBlock` instead and maybe decorate the return type with `NotNull`?

I can't name the method the same as the type. :-(  But I'll use NotNull.
Comment on attachment 8920034 [details]
Bug 1408311 - Part 3: Make nsTreeSanitizer Parse style attributes and style sheets with Servo.

https://reviewboard.mozilla.org/r/191026/#review196602

r=me with the principal thing sorted out.

::: dom/base/nsTreeSanitizer.cpp:1121
(Diff revision 2)
>               "should not get marked modified during parsing");
>    sheet->SetComplete();
>    // Loop through all the rules found in the CSS text
> -  int32_t ruleCount = sheet->StyleRuleCount();
> -  for (int32_t i = 0; i < ruleCount; ++i) {
> -    mozilla::css::Rule* rule = sheet->GetStyleRuleAt(i);
> +  ErrorResult err;
> +  RefPtr<dom::CSSRuleList> rules =
> +    sheet->GetCssRules(*nsContentUtils::SubjectPrincipal(), err);

This doesn't look right.  Don't we want `*nsContentUtils::GetSystemPrincipal()` here?  We don't want the behavior to depend on what's on the stack.

But also, the fact that we have to instantiate the CSSRuleList is moderately annoying.  It would be better if we had some sane way to iterate both servo and non-servo stylesheet rules...  Followup is OK for that.
Attachment #8920034 - Flags: review?(bzbarsky) → review+
Comment on attachment 8920034 [details]
Bug 1408311 - Part 3: Make nsTreeSanitizer Parse style attributes and style sheets with Servo.

https://reviewboard.mozilla.org/r/191026/#review196602

> This doesn't look right.  Don't we want `*nsContentUtils::GetSystemPrincipal()` here?  We don't want the behavior to depend on what's on the stack.
> 
> But also, the fact that we have to instantiate the CSSRuleList is moderately annoying.  It would be better if we had some sane way to iterate both servo and non-servo stylesheet rules...  Followup is OK for that.

Makes sense, thanks.

Re the CSSRuleList instantiation -- I guess it's a bit tricky for Servo sheets since the DOM objects are all that we have on the C++.  I'll file a followup.
Comment on attachment 8920034 [details]
Bug 1408311 - Part 3: Make nsTreeSanitizer Parse style attributes and style sheets with Servo.

https://reviewboard.mozilla.org/r/191026/#review196602

> Makes sense, thanks.
> 
> Re the CSSRuleList instantiation -- I guess it's a bit tricky for Servo sheets since the DOM objects are all that we have on the C++.  I'll file a followup.

For Servo, we probably have no choice but to instantiate CSSRuleList if we want to keep the sanitizing code in C++. We can, diverge the code at a higher level, so that we don't need to instantiate any C++ object for this, but I suspect whether it's worth that.
Comment on attachment 8920031 [details]
style: Make Servo_DeclarationBlock_RemovePropertyById return whether it did remove a property.

https://reviewboard.mozilla.org/r/191020/#review196612
Attachment #8920031 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8920032 [details]
Bug 1408311 - Part 1: Make Declaration::RemovePropertyByID return whether it did remove a property.

https://reviewboard.mozilla.org/r/191022/#review196614

::: layout/style/DeclarationBlock.h:136
(Diff revision 2)
>                                 nsAString& aValue) const;
>    inline void GetPropertyValueByID(nsCSSPropertyID aPropID,
>                                     nsAString& aValue) const;
>    inline bool GetPropertyIsImportant(const nsAString& aProperty) const;
>    inline void RemoveProperty(const nsAString& aProperty);
> -  inline void RemovePropertyByID(nsCSSPropertyID aProperty);
> +  inline bool RemovePropertyByID(nsCSSPropertyID aProperty);

Maybe add a comment here stating that the return value means whether any property is actually removed.
Attachment #8920032 - Flags: review?(xidorn+moz) → review+
Attached file Servo PR — 

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8920031 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8920034 [details]
Bug 1408311 - Part 3: Make nsTreeSanitizer Parse style attributes and style sheets with Servo.

https://reviewboard.mozilla.org/r/191026/#review196602

> For Servo, we probably have no choice but to instantiate CSSRuleList if we want to keep the sanitizing code in C++. We can, diverge the code at a higher level, so that we don't need to instantiate any C++ object for this, but I suspect whether it's worth that.

Yeah, if anything moving that part of the sanitizing code to Rust would be thing to do.  Anyway, I think I won't bother with the followup bug.

    
    

    
  
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/062d97bd4e6e
Part 1: Make Declaration::RemovePropertyByID return whether it did remove a property. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/c05093a12b67
Part 2: Add GetDeclarationBlock method to BindingStyleRule. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/2b21d88cc12c
Part 3: Make nsTreeSanitizer Parse style attributes and style sheets with Servo. r=bz,xidorn
https://hg.mozilla.org/integration/autoland/rev/5970cb3d3673
Part 4: Fix bug when serializing sanitized style rules. r=xidorn

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: