Closed Bug 1339629 Opened 7 years ago Closed 7 years ago

stylo: ServoStyleSheets don't handle modification in the presence of cloned inners correctly

Categories

(Core :: CSS Parsing and Computation, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(15 files, 5 obsolete files)

59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
ServoStyleSheets need ability for modification, which implies dirty flags and mInner cloning similar to the implementation in CSSStyleSheet.
See Also: → 1290218
Hm, I'd think that we already support mutation stylo stylesheets via our CSSOM stuff. Is the issue that we just don't expose this to C++ consumers over certain APIs? What are the consumers of those APIs?
Flags: needinfo?(bwerth)
Priority: -- → P2
I think the issue is that they can be modified, but we don't handle this correctly in the presence of style sheet inner sharing.
Ok, that makes sense. P1.
Flags: needinfo?(bwerth)
Priority: P2 → P1
Assignee: nobody → bwerth
See Also: → 1348481
Two things I'd like to know to help me finish this bug...

1) What are the currently-failing reftests we expect should pass? One of the tests added in Bug 1348481 should start working when this is implemented. What others?
2) How much of the work of EnsureUniqueInner at http://searchfox.org/mozilla-central/source/layout/style/CSSStyleSheet.cpp#614 needs to be mirrored in Servo? The first half of that function is refactored into StyleSheet in attachment 8863566 [details], but do we also need to replicate the behavior of ClearRuleCascades() and SetNeedsRestyleAfterEnsureUniqueInner() for ServoStyleSheets?
Flags: needinfo?(cam)
See Also: → 1359491
(In reply to Brad Werth [:bradwerth] from comment #6)
> 1) What are the currently-failing reftests we expect should pass? One of the
> tests added in Bug 1348481 should start working when this is implemented.

From the stylo-failures.md file, I can see there are at least some tests in test_selectors.html that are currently failing due to not cloning on CSSOM access.  There are probably more tests I'm not aware of -- you could do a Gecko try run with say http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/layout/style/CSSStyleSheet.cpp#605 turned into an unconditional return, and see what tests start failing.

> 2) How much of the work of EnsureUniqueInner at
> http://searchfox.org/mozilla-central/source/layout/style/CSSStyleSheet.
> cpp#614 needs to be mirrored in Servo? The first half of that function is
> refactored into StyleSheet in attachment 8863566 [details], but do we also
> need to replicate the behavior of ClearRuleCascades() and
> SetNeedsRestyleAfterEnsureUniqueInner() for ServoStyleSheets?

The ClearRuleCascades() call is used to ensure that the tables that we use for selector matching are cleared out and regenerated on next restyle.  If we don't do that, then restyling an element can pick out a rule object that came from the old, cloned-from sheet's inner, instead of the new, cloned sheet's inner.

We have the same problem for Servo sheets -- the Stylist's element_map (and other fields) stores rules in it, indexed by element names and other features, and these need to be cleared out so we don't return the old, cloned-from sheet's rules when restyling an element.  We should be able to just call Servo_StyleSet_NoteStyleSheetsChanged / Servo_StyleSet_FlushStyleSheets.  We'll need to do this on every style set that contains the ServoStyleSheet (since they can be in multiple, e.g. the UA sheets we get from nsLayoutStylesheetCache, or sheets that come from addons).  CSSStyleSheet::ClearRuleCascades does this by tracking which nsCSSRuleProcessor objects contain the sheet (since it's the rule processors that contain the cascade data, not the style set itself), in its mRuleProcessors member.  It looks like a CSSStyleSheet also tracks which style sets its in, in its mStyleSets array, so maybe we can uplift that to StyleSheet, and ensure we keep it up to date from our ServoStyleSet sheet addition/removal methods.

SetNeedsRestyleAfterEnsureUniqueInner is used to tell the nsPresContext that if devtools ever requests rules from sheets, it will need to restyle the document first, since the elements are still all using their old rules.  Aside from devtools, it doesn't matter that we leave elements with style contexts that used rules from the old style sheet, since there's no way for the page to get access to the rule that matched an element.  (It can only see what the computed styles are, and they will continue to be accurate until CSSOM actually modifies some rules, at which point we will restyle.)  So I think we do need to call SetNeedsRestyleAfterEnsureUniqueInner from a ServoStyleSheet's EnsureUniqueInner call.


Then of course there is the actual cloning of the Servo Stylesheet object (and thus all of its rules) that must happen under ServoStyleSheetInner::CloneFor.  When we do this, I guess we need to update all of the Stylists that contained the old Stylesheet to give it the new one.  I guess doing this will cause the Stylist to be flushed, so it's really part of the same work as above (the iterating over ServoStyleSheets stuff).

I *think* that the C++-side wrapper objects for rules (like ServoStyleRule) and ServoCSSRuleList don't need to be cloned, since they only get instantiated if CSSOM requests access to them.  And it is a CSSOM access to request them that causes EnsureUniqueInner to be called.  So we should be able to assert that ServoStyleSheet::mRuleList is null when EnsureUniqueInner is called.
Flags: needinfo?(cam)
Some comments about out of date comments from bz in bug bug 1359491 comment #0:

> Now that bug 1290218 is fixed, is the code/comment in
> nsPresContext::EnsureSafeToHandOutCSSRules still correct?  If so, it could
> use an explanation of why there is no need to handle copy-on-write there,
> because it's entirely non-obvious to me: we're sharing a mutable data
> structure.
> 
> Relatedly, the comments on the SheetInfo getters on StyleSheet.h seem to be
> out of date; the info lives on the inner in both cases...
Summary: stylo: ServoStyleSheets don't permit modification → stylo: ServoStyleSheets don't handle modification in the presence of cloned inners correctly
It's not just comments.  nsPresContext::EnsureSafeToHandOutCSSRules needs code changes.
(In reply to Cameron McCormack (:heycam) from comment #7)

> Then of course there is the actual cloning of the Servo Stylesheet object
> (and thus all of its rules) that must happen under
> ServoStyleSheetInner::CloneFor.  When we do this, I guess we need to update
> all of the Stylists that contained the old Stylesheet to give it the new
> one.  I guess doing this will cause the Stylist to be flushed, so it's
> really part of the same work as above (the iterating over ServoStyleSheets
> stuff).

I think the supplied patches implement almost all of the necessary changes, except for two parts. The first part is this one -- the actual cloning of the Servo objects. I'm going to need a primer on the correct approach. Specifically, when looking at the stylesheet structure at http://searchfox.org/mozilla-central/source/servo/components/style/stylesheets.rs#249, is it correct that we need deep copies of rules, shared_lock, and shallow copies of everything else? And how to find the stylists that contain this stylesheet?

The second thing missing is ensuring that the cloning is triggered by relevant CSSOM methods. The testcase I'm using for this is /layout/reftests/bugs/1348481-3.html. When I run that through a Gecko build, the clone is triggered at http://searchfox.org/mozilla-central/source/layout/style/CSSStyleSheet.cpp#110. I'm not sure how to trigger the call to EnsureUniqueInner in a Stylo build. I'd greatly appreciate a sketch of how to do this.
Flags: needinfo?(cam)
(In reply to Brad Werth [:bradwerth] from comment #26)
> I think the supplied patches implement almost all of the necessary changes,
> except for two parts. The first part is this one -- the actual cloning of
> the Servo objects. I'm going to need a primer on the correct approach.
> Specifically, when looking at the stylesheet structure at
> http://searchfox.org/mozilla-central/source/servo/components/style/
> stylesheets.rs#249, is it correct that we need deep copies of rules,
> shared_lock, and shallow copies of everything else?

I think we want to deep clone most things:

* For rules, deep clone.  This probably involves adding a new fn to CssRules, CssRule, and the specific rule types like StyleRule, ImportRule, etc.

* For media, calling clone() on the MediaList should be sufficient, since it doesn't have any shared references to other objects.

* url_data is just a RefPtr to a Gecko object that holds nsIPrincipal / nsIURI objects, and we want to just call clone() on it so we can point to the same object.

* For namespaces, clone() on the Namespaces object is sufficient.

* The rest should be Copy types, so we can just copy them across.

> And how to find the stylists that contain this stylesheet?

Using the mStyleSets member you've uplifted from CSSStyleSheet to StyleSheet.

> The second thing missing is ensuring that the cloning is triggered by
> relevant CSSOM methods. The testcase I'm using for this is
> /layout/reftests/bugs/1348481-3.html. When I run that through a Gecko build,
> the clone is triggered at
> http://searchfox.org/mozilla-central/source/layout/style/CSSStyleSheet.
> cpp#110. I'm not sure how to trigger the call to EnsureUniqueInner in a
> Stylo build. I'd greatly appreciate a sketch of how to do this.

It looks like you're handling all the places where we need to call EnsureUniqueInner, except for the one corresponding to CSSRuleListImpl::IndexedGetter.  For that you need to add the EnsureUniqueInner() call to ServoCSSRuleList::IndexedGetter.
Flags: needinfo?(cam)
It seems I would need EnsureUniqueInner thing for @import rule as well.

(In reply to Cameron McCormack (:heycam) from comment #27)
> It looks like you're handling all the places where we need to call
> EnsureUniqueInner, except for the one corresponding to
> CSSRuleListImpl::IndexedGetter.  For that you need to add the
> EnsureUniqueInner() call to ServoCSSRuleList::IndexedGetter.

Probably we need to EnsureUniqueInner() in constructor of ServoCSSRuleList rather than IndexedGetter, because we want to keep strong reference from the rule list to child sheet from the very beginning, so that they are still accessible as long as rule list is accessible, even after the style sheet gets destroyed.

This is an existing bug in Gecko code, but we can fix it in Stylo from the very beginning. Making behavior depend on timing of CC isn't desirable anyway.
Blocks: 1352968
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #28)
> It seems I would need EnsureUniqueInner thing for @import rule as well.
> 
> Probably we need to EnsureUniqueInner() in constructor of ServoCSSRuleList
> rather than IndexedGetter, because we want to keep strong reference from the
> rule list to child sheet from the very beginning, so that they are still
> accessible as long as rule list is accessible, even after the style sheet
> gets destroyed.

It seems like the first opportunity we have to call EnsureUniqueInner is when the stylesheet is actually set at http://searchfox.org/mozilla-central/source/layout/style/ServoCSSRuleList.cpp#70. I'll add a call to EnsureUniqueInner there in the patches. I don't know if it will address the need you raise with the @import rule. If it won't, please guide me on what else will be needed.
Flags: needinfo?(xidorn+moz)
Attachment #8864679 - Attachment is obsolete: true
Oh, hmmm, yeah, your current approach in part 10 works for me. I forgot that I add a new parameter to the ctor for my @import rule patch, which isn't available on the code at the moment. I'll add EnsureUniqueInner() if I think I need that for my patch. Sorry for the confusion.
Flags: needinfo?(xidorn+moz)
(In reply to Cameron McCormack (:heycam) from comment #27)
> * For rules, deep clone.  This probably involves adding a new fn to
> CssRules, CssRule, and the specific rule types like StyleRule, ImportRule,
> etc.

Current patch doesn't explicitly do a deep clone of rules. But it also passes tests, which indicates to me either:
1) We need more tests. MOST LIKELY
2) Rust semantics are providing us a deep clone automagically since CssRules is a Vec of CssRule and will have Clone trait if CssRule has it. CssRule doesn't appear to have that trait, but my understanding of Rust is shaky: http://searchfox.org/mozilla-central/source/servo/components/script/dom/cssrule.rs#28
3) Deep clone isn't actually necessary.

To settle this, I'd like to build a test that should fail if a deep clone of rules is not happening. Can you help me with that?
Flags: needinfo?(cam)
Attachment #8865710 - Flags: review?(cam)
Attachment #8863565 - Flags: review?(cam)
Attachment #8863566 - Flags: review?(cam)
Attachment #8864677 - Flags: review?(cam)
Attachment #8864678 - Flags: review?(cam)
Attachment #8864680 - Flags: review?(cam)
Attachment #8864681 - Flags: review?(cam)
Attachment #8865006 - Flags: review?(cam)
Attachment #8865711 - Flags: review?(cam)
Attachment #8865712 - Flags: review?(cam)
Does attachment 8848832 [details] on bug 1348481 fail?  Or a similar test where you have e.g.

  @supports (color: green) {
    div { color: green; }
  }

in the two (identical) style sheets, to demonstrate that we clone the contents of grouping rules too.
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #54)
> Or a similar test
> where you have e.g.
> 
>   @supports (color: green) {
>     div { color: green; }
>   }
> 
> in the two (identical) style sheets, to demonstrate that we clone the
> contents of grouping rules too.

I added a test that does this in attachment 8866516 [details], but it passes trivially because the JS call to modify the rule within the group rule, "links[0].sheet.cssRules[0].cssRules[0].style.color = 'red';" has no effect. I'm searching for a bug that covers this failure case, and I'll file a bug if I don't find one.
Attachment #8866516 - Flags: review?(cam)
Did attachment 8848832 [details] pass, or fail due to not cloning, or fail due to the issue you mention in comment 66?  I am reasonably confident that you will need to add more code to handle deep cloning those rules (because the #[derive(Clone)] implementation of CssRule is just going to clone the Arcs inside the enum values, rather than cloning the objects they point to).  So I think it should be failing for some reason or another.  But if it's passing I'd like to know why.
(In reply to Cameron McCormack (:heycam) from comment #67)
> Did attachment 8848832 [details] pass, or fail due to not cloning, or fail
> due to the issue you mention in comment 66?

The reftest http://searchfox.org/mozilla-central/source/layout/reftests/bugs/1348481-3.html replicates that attachment, and it now passes. It's re-enabled in attachment 8865712 [details]. I'll try to come up with a convincing explanation why it passes.
(In reply to Brad Werth [:bradwerth] from comment #66)
> I added a test that does this in attachment 8866516 [details], but it passes
> trivially because the JS call to modify the rule within the group rule,
> "links[0].sheet.cssRules[0].cssRules[0].style.color = 'red';" has no effect.

This was failing because ServoStyleSheetInner copy constructor didn't clone the mURIData structure. Implementing this (as a shallow copy) causes the test to fail, as predicted in comment 54. I'm now working on cloning the CssRule structures Servo-side.
Comment on attachment 8863565 [details]
Bug 1339629 Part 3: Uplift mDirty into StyleSheet.

https://reviewboard.mozilla.org/r/135316/#review141930

::: layout/style/StyleSheet.h:121
(Diff revision 6)
>    virtual already_AddRefed<StyleSheet> Clone(StyleSheet* aCloneParent,
>                                               css::ImportRule* aCloneOwnerRule,
>                                               nsIDocument* aCloneDocument,
>                                               nsINode* aCloneOwningNode) const = 0;
>  
> -  virtual bool IsModified() const = 0;
> +  bool IsModified() const { return mDirty; };

Nit: no ";" after "}".
Attachment #8863565 - Flags: review?(cam) → review+
Comment on attachment 8863566 [details]
Bug 1339629 Part 4: Uplift CloneFor into StyleSheetInfo, and EnsureUniqueInner into StyleSheet.

https://reviewboard.mozilla.org/r/135318/#review141934

::: layout/style/CSSStyleSheet.cpp:367
(Diff revision 6)
>    MOZ_ASSERT(mInner, "We should have an mInner after copy.");
>    MOZ_ASSERT(mInner->mSheets.Contains(this), "Our mInner should include us.");

Should these assertions be uplifted too?

::: layout/style/StyleSheet.h:219
(Diff revision 6)
>  
>    // Changes to sheets should be inside of a WillDirty-DidDirty pair.
>    // However, the calls do not need to be matched; it's ok to call
>    // WillDirty and then make no change and skip the DidDirty call.
> -  inline void WillDirty();
> -  inline void DidDirty();
> +  void WillDirty();
> +  virtual void DidDirty() {};

Nit: no ";" after "}".
Attachment #8863566 - Flags: review?(cam) → review+
Comment on attachment 8864677 [details]
Bug 1339629 Part 5: Change CSSStyleSet::mStyleSets to use StyleSetHandles.

https://reviewboard.mozilla.org/r/136324/#review141942

::: layout/style/CSSStyleSheet.cpp:488
(Diff revision 5)
>  void
>  CSSStyleSheet::AddStyleSet(nsStyleSet* aStyleSet)
>  {
> -  NS_ASSERTION(!mStyleSets.Contains(aStyleSet),
> +  StyleSetHandle setHandle(aStyleSet);
> +  NS_ASSERTION(!mStyleSets.Contains(setHandle),
>                 "style set already registered");
> -  mStyleSets.AppendElement(aStyleSet);
> +  mStyleSets.AppendElement(setHandle);
>  }
>  
>  void
>  CSSStyleSheet::DropStyleSet(nsStyleSet* aStyleSet)
>  {
> -  DebugOnly<bool> found = mStyleSets.RemoveElement(aStyleSet);
> +  StyleSetHandle setHandle(aStyleSet);
> +  DebugOnly<bool> found = mStyleSets.RemoveElement(setHandle);
>    NS_ASSERTION(found, "didn't find style set");
>  }

Note that StyleSetHandle has conversion operators for nsStyleSet/ServoStyleSet defined, so you can probably just leave these functions defined as they are.

::: layout/style/CSSStyleSheet.cpp:603
(Diff revision 5)
>    ClearRuleCascades();
>  
>    // let our containing style sets know that if we call
>    // nsPresContext::EnsureSafeToHandOutCSSRules we will need to restyle the
>    // document
> -  for (nsStyleSet* styleSet : mStyleSets) {
> +  for (StyleSetHandle& setHandle : mStyleSets) {

Since StyleSetHandles are just thin wrappers over pointers, I think we should just write:

  for (StyleSetHandle setHandle : mStyleSets) {
     ...
  }
Attachment #8864677 - Flags: review?(cam) → review+
Comment on attachment 8864678 [details]
Bug 1339629 Part 6: Uplift mStyleSets into StyleSheet.

https://reviewboard.mozilla.org/r/136326/#review141946

::: layout/style/StyleSheet.h:17
(Diff revision 5)
>  #include "mozilla/net/ReferrerPolicy.h"
>  #include "mozilla/StyleBackendType.h"
>  #include "mozilla/CORSMode.h"
>  #include "mozilla/ServoUtils.h"
> +#include "mozilla/StyleSetHandle.h"
> +

Nit: probably don't need a second blank line in here.

::: layout/style/nsStyleSet.cpp:232
(Diff revision 5)
>  
>  nsStyleSet::~nsStyleSet()
>  {
>    for (SheetType type : gCSSSheetTypes) {
>      for (CSSStyleSheet* sheet : mSheets[type]) {
> -      sheet->DropStyleSet(this);
> +      sheet->DropStyleSet(StyleSetHandle(this));

Here and below you can probably drop the explicit StyleSetHandle() calls since the implicit conversion operator should be used.
Attachment #8864678 - Flags: review?(cam) → review+
Comment on attachment 8864680 [details]
Bug 1339629 Part 8: Uplift ClearRuleCascades into StyleSheet.

https://reviewboard.mozilla.org/r/136330/#review141948

::: layout/style/ServoStyleSheet.cpp:220
(Diff revision 5)
> +  // XXX We need to ensure that we eventually flush all the ServoStyleSets
> +  // we noted above, and all the ServoStyleSets noted by our ancestors in
> +  // the implementation of StyleSheet::ClearRuleCascades.
> +  // But we don't want to flush a ServoStyleSet multiple times during this call.
> +  // For now, we don't explicitly flush any of these sets, and instead assume
> +  // that callsites will flush those ServoStyleSets.

What's your plan for this?  I don't see anything in the later patches to do this, and I think we need to in this bug.

In the top level EnsureUniqueInner call, maybe we could iterate over all the sheets we're going to look at, then call BeginUpdate on them all.  And at the end call EndUpdate, which will do the flush?

::: layout/style/StyleSheet.cpp:130
(Diff revision 5)
> +void
> +StyleSheet::ClearRuleCascades()
> +{
> +  if (mParent) {
> +    mParent->ClearRuleCascades();
> +  }
> +}

I wonder if it might be better to split this up into a non-virtual ClearRuleCascades that does:

  ClearRuleCascadesInternal();
  if (mParent) {
    mParent->ClearRuleCascades();
  }

where ClearRuleCascadesInternal is the virtual method that just processes a single sheet.
Comment on attachment 8864681 [details]
Bug 1339629 Part 9: Uplift EnsureUniqueInnerOnCSSSheets and SetNeedsRestyleAfterEnsureUniqueInner into StyleSetHandle, and eliminate CSSStyleSheet::EnsureUniqueInner.

https://reviewboard.mozilla.org/r/136332/#review141950

::: layout/style/ServoStyleSet.h:452
(Diff revision 5)
>    int32_t mBatching;
>    uint32_t mUniqueIDCounter;
>    bool mAllowResolveStaleStyles;
>    bool mAuthorStyleDisabled;
>  
> +  unsigned mNeedsRestyleAfterEnsureUniqueInner : 1;

I'd just make this bool, since unlike in nsStyleSet we don't have a lot of flags we need to pack together here.

::: layout/style/ServoStyleSet.cpp:962
(Diff revision 5)
>  }
>  
> +bool
> +ServoStyleSet::EnsureUniqueInnerOnCSSSheets()
> +{
> +  // This is a stub until more of the funcationality of nsStyleSet is

*functionality
Attachment #8864681 - Flags: review?(cam) → review+
Comment on attachment 8865006 [details]
Bug 1339629 Part 10: Implement ServoStyleSet::EnsureUniqueInnerOnCSSSheets.

https://reviewboard.mozilla.org/r/136662/#review141954

::: layout/style/ServoStyleSet.cpp:969
(Diff revision 4)
> +  // XXX Do we need to replicate the nsStyleSet work of checking
> +  // a nsBindingManager?

We will, but that can be done in bug 1290276.
Attachment #8865006 - Flags: review?(cam) → review+
Comment on attachment 8865711 [details]
Bug 1339629 Part 11: Call EnsureUniqueInner from ServoCSSRuleList when a StyleSheet is set.

https://reviewboard.mozilla.org/r/137338/#review141958

::: layout/style/ServoCSSRuleList.cpp:71
(Diff revision 3)
> -  EnumerateInstantiatedRules([aStyleSheet](css::Rule* rule) {
> -    rule->SetStyleSheet(aStyleSheet);
> +  if (mStyleSheet) {
> +    mStyleSheet->EnsureUniqueInner();
> +  }

I wonder if we should do this up at ServoStyleSheet::GetCssRulesInternal, because in there we get the list of rules, but that list of rules will change if EnsureUniqueInner did actually clone.  So it would be nice to avoid the wasted work of getting the pre-cloned rules.
Comment on attachment 8866516 [details]
Bug 1339629 Part 14: Add reftests to verify we clone css group rules when we clone the stylesheet.

https://reviewboard.mozilla.org/r/138140/#review141960
Attachment #8866516 - Flags: review?(cam) → review+
Comment on attachment 8866516 [details]
Bug 1339629 Part 14: Add reftests to verify we clone css group rules when we clone the stylesheet.

https://reviewboard.mozilla.org/r/138140/#review142150

::: layout/reftests/bugs/reftest.list:2000
(Diff revision 1)
>  == 1348481-3.html 1348481-ref.html
>  == 1352464-1.html 1352464-1-ref.html
>  == 1358375-1.html 1358375-ref.html
>  == 1358375-2.html 1358375-ref.html
>  == 1358375-3.html 1358375-ref.html
> +== 1358375-4.html 1358375-ref.html

1348481-4.html is added, but is not used here?
Attachment #8865710 - Flags: review?(cam)
Attachment #8864680 - Flags: review?(cam)
Attachment #8865711 - Flags: review?(cam)
Attachment #8865712 - Flags: review?(cam)
(In reply to Brad Werth [:bradwerth] from comment #69)
> (In reply to Brad Werth [:bradwerth] from comment #66)
> > I added a test that does this in attachment 8866516 [details], but it passes
> > trivially because the JS call to modify the rule within the group rule,
> > "links[0].sheet.cssRules[0].cssRules[0].style.color = 'red';" has no effect.
> 
> This was failing because ServoStyleSheetInner copy constructor didn't clone
> the mURIData structure. Implementing this (as a shallow copy) causes the
> test to fail, as predicted in comment 54. I'm now working on cloning the
> CssRule structures Servo-side.

Attachment 8867909 [details] implements enough of a deep copy that the new refest layout/reftests/bugs/1348481-4.html now passes. I'll work on a more complete suite of tests to ensure that all the different css rule types are being deep cloned correctly.
Attachment #8865710 - Flags: review?(cam)
Attachment #8867909 - Flags: review?(cam)
Attachment #8865712 - Flags: review?(cam)
Comment on attachment 8865710 [details]
Bug 1339629 Part 1: Servo-side function to clone ServoStyleSheets.

https://reviewboard.mozilla.org/r/137336/#review144810

::: servo/components/style/stylesheets.rs:897
(Diff revision 7)
> +            dirty_on_viewport_size_change: AtomicBool::new(
> +                self.dirty_on_viewport_size_change.load(Ordering::Relaxed)),
> +            disabled: AtomicBool::new(self.disabled.load(Ordering::Relaxed)),

Since we read and write these with SeqCst elsewhere, maybe we should be doing that here too?
Attachment #8865710 - Flags: review?(cam) → review+
Comment on attachment 8867909 [details]
Bug 1339629 Part 2: Servo-side change Stylesheet clone to be a deep copy.

https://reviewboard.mozilla.org/r/139444/#review144812

This is looking great!  But do we need to handle FontFaceRule and CounterStyleRule objects too?  They contain Gecko objects (RefPtr<nsCSSFontFaceRule> and RefPtr<nsCSSCounterStyleRule>) which need to be deeply cloned.

::: servo/components/style/stylesheets.rs:1056
(Diff revision 5)
>      fn clone(&self) -> Stylesheet {
> +        // Create a new lock for our clone.
> +        let lock = self.shared_lock.clone();
> +        let guard = self.shared_lock.read();
> +
> +        // Make a deep clone of the rules, using the new lock.

I think it doesn't matter whether we use self.shared_lock or lock, here, since SharedRwLock just has an Arc<...> in it, which it will clone.  So you might just want to write

  shared_lock: self.shared_lock.clone()

below, and use self.shared_lock for all of the uses of the lock in this function.
Comment on attachment 8864680 [details]
Bug 1339629 Part 8: Uplift ClearRuleCascades into StyleSheet.

https://reviewboard.mozilla.org/r/136330/#review144814
Attachment #8864680 - Flags: review+
Comment on attachment 8865711 [details]
Bug 1339629 Part 11: Call EnsureUniqueInner from ServoCSSRuleList when a StyleSheet is set.

https://reviewboard.mozilla.org/r/137338/#review144816
Attachment #8865711 - Flags: review?(cam) → review+
Comment on attachment 8865712 [details]
Bug 1339629 Part 13: Consolidate stylesheet cloning tests into a named directory.

https://reviewboard.mozilla.org/r/137340/#review144818
Attachment #8865712 - Flags: review?(cam) → review+
Comment on attachment 8867909 [details]
Bug 1339629 Part 2: Servo-side change Stylesheet clone to be a deep copy.

https://reviewboard.mozilla.org/r/139444/#review144812

I'm not sure if we need this or not. I went down the path of making a test to verify the clone-on-write of CounterStyleRule, but found that all the properties are read-only. The read-only nature of the properties is not explicit at https://developer.mozilla.org/en-US/docs/Web/API/CSSCounterStyleRule, but seems to be the case. I'll see if I can get the font face rule properties to change, and if so, I'll add a test that verifies the clone behavior and make necessary code changes.

> I think it doesn't matter whether we use self.shared_lock or lock, here, since SharedRwLock just has an Arc<...> in it, which it will clone.  So you might just want to write
> 
>   shared_lock: self.shared_lock.clone()
> 
> below, and use self.shared_lock for all of the uses of the lock in this function.

I like the explicitness of a "let lock =" here, and it saves keystrokes later. Part of this is over-architected, since the lock is a clone of the old lock, it's not strictly necessary to get the guard from the old lock for the deep clone. Since that assumption is safe for now, I'll strip off the guard parameter to the deep_clone_with_lock calls.
(In reply to Brad Werth [:bradwerth] from comment #163)
> Comment on attachment 8867909 [details]
> Bug 1339629 Part 2: Servo-side change Stylesheet clone to be a deep copy.
> 
> https://reviewboard.mozilla.org/r/139444/#review144812
> 
> I'm not sure if we need this or not. I went down the path of making a test
> to verify the clone-on-write of CounterStyleRule, but found that all the
> properties are read-only. The read-only nature of the properties is not
> explicit at
> https://developer.mozilla.org/en-US/docs/Web/API/CSSCounterStyleRule, but
> seems to be the case. I'll see if I can get the font face rule properties to
> change, and if so, I'll add a test that verifies the clone behavior and make
> necessary code changes.

Descriptors in counter style rule should be all changeable. They haven't been fully implemented for stylo because we need a way to mark stylesheet dirty, which is only added in your patches here IIRC. But other than that, it should have been working well.

And font face rule is much worse on mutability in Gecko currently.
Comment on attachment 8867909 [details]
Bug 1339629 Part 2: Servo-side change Stylesheet clone to be a deep copy.

https://reviewboard.mozilla.org/r/139444/#review144812

I was wrong; I can create a test for counter style and make it fail. Next version of patches will have that test and a code fix.
Attachment #8867909 - Flags: review?(cam)
Attachment #8870222 - Flags: review?(cam)
Attachment #8870223 - Flags: review?(cam)
Attachment #8870224 - Flags: review?(cam)
Comment on attachment 8870222 [details]
Bug 1339629 Part 3: Servo-side define and call Gecko CounterStyle and FontFaceRule clone functions.

https://reviewboard.mozilla.org/r/141676/#review145412
Attachment #8870222 - Flags: review?(cam) → review+
Comment on attachment 8870223 [details]
Bug 1339629 Part 4: Gecko-side implement the CounterStyle and FontFaceRule clone functions.

https://reviewboard.mozilla.org/r/141678/#review145414
Attachment #8870223 - Flags: review?(cam) → review+
Comment on attachment 8867909 [details]
Bug 1339629 Part 2: Servo-side change Stylesheet clone to be a deep copy.

https://reviewboard.mozilla.org/r/139444/#review145416

::: servo/components/style/keyframes.rs:109
(Diff revision 6)
>               .map(KeyframeSelector)
>      }
>  }
>  
>  /// A keyframe.
> -#[derive(Debug)]
> +#[derive(Clone, Debug)]

Since we need the deep clone for this type, I guess we don't need this Clone impl now.
Attachment #8867909 - Flags: review+
Comment on attachment 8870224 [details]
Bug 1339629 Part 15: Add reftests to test other css rules clone properly when stylesheet is cloned.

https://reviewboard.mozilla.org/r/141680/#review145424

::: layout/reftests/stylesheet-cloning/font-face-rule-clone.html:22
(Diff revision 2)
> +
> +    // backup in case the load event is not fired on the link
> +    setTimeout(stop_waiting, 5000);

If the test is currently passing (with your patches), then I think it would be better to remove this setTimeout.  With the build machines, who knows what load on the machine will do to timing, and so maybe 5s is not enough, and the test will intermittently fail.  I think it's fine to leave the test to timeout if it fails.

Alternatively, you could use CSSOM to inspect the second style sheet's @font-face rule, to verify that it hasn't changed, instead of using the actual font load.
Attachment #8870224 - Flags: review?(cam) → review+
Comment on attachment 8870224 [details]
Bug 1339629 Part 15: Add reftests to test other css rules clone properly when stylesheet is cloned.

https://reviewboard.mozilla.org/r/141680/#review145424

> If the test is currently passing (with your patches), then I think it would be better to remove this setTimeout.  With the build machines, who knows what load on the machine will do to timing, and so maybe 5s is not enough, and the test will intermittently fail.  I think it's fine to leave the test to timeout if it fails.
> 
> Alternatively, you could use CSSOM to inspect the second style sheet's @font-face rule, to verify that it hasn't changed, instead of using the actual font load.

Yes, moving this test to a mochitest which just confirms that the reported values of the cloned stylesheet are unchanged.
Blocks: 1367213
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 1 changes to 3 files (+1 heads)
remote: autoland push detected
remote: recorded push in pushlog
remote: 
remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/8894ea7f7f661ec03bbde698f3be0d286d0fec43
remote:   https://hg.mozilla.org/try/rev/560fb706dcd3ff91a3fc95a08d0b0f56f153e46a
remote:   https://hg.mozilla.org/try/rev/8ae732b876376de16db48d2dfa59e73e736f5a47
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ae732b876376de16db48d2dfa59e73e736f5a47
remote: recorded changegroup in replication log in 0.121s
remote: ** unknown exception encountered, please report by visiting
remote: ** https://mercurial-scm.org/wiki/BugTracker
remote: ** Python 2.7.5 (default, Nov  6 2016, 00:28:07) [GCC 4.8.5 20150623 (Red Hat 4.8.5-11)]
remote: ** Mercurial Distributed SCM (version 4.1.2)
remote: ** Extensions loaded: blackbox, clonebundles, obsolescencehacks, pushlog, serverlog, readonly, vcsreplicator
remote: Traceback (most recent call last):
remote:   File "/var/hg/venv_pash/bin/hg", line 45, in <module>
remote:     mercurial.dispatch.run()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 63, in run
remote:     sys.exit((dispatch(request(pycompat.sysargv[1:])) or 0) & 255)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 129, in dispatch
remote:     ret = _runcatch(req)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 219, in _runcatch
remote:     return callcatch(ui, _runcatchfunc)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 227, in callcatch
remote:     return scmutil.callcatch(ui, func)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/scmutil.py", line 152, in callcatch
remote:     return func()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 208, in _runcatchfunc
remote:     return _dispatch(req)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 811, in _dispatch
remote:     cmdpats, cmdoptions)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 563, in runcommand
remote:     ret = _runcommand(ui, options, cmd, d)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 819, in _runcommand
remote:     return cmdfunc()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 808, in <lambda>
remote:     d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/util.py", line 1051, in check
remote:     return func(*args, **kwargs)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/commands.py", line 5824, in serve
remote:     s.serve_forever()
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 320, in serve_forever
remote:     return super(sshserverwrapped, self).serve_forever()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/sshserver.py", line 104, in serve_forever
remote:     while self.serve_one():
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 351, in serve_one
remote:     return super(sshserverwrapped, self).serve_one()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/sshserver.py", line 122, in serve_one
remote:     rsp = wireproto.dispatch(self.repo, self, cmd)
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 343, in dispatch
remote:     return origdispatch(repo, proto, cmd)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/extensions.py", line 223, in closure
remote:     return func(*(args + a), **kw)
remote:   File "/var/hg/version-control-tools/pylib/vcsreplicator/vcsreplicator/hgext.py", line 359, in wireprotodispatch
remote:     return orig(repo, proto, command)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireproto.py", line 569, in dispatch
remote:     return func(repo, proto, *args)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireproto.py", line 982, in unbundle
remote:     proto._client())
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/exchange.py", line 1771, in unbundle
remote:     lockandtr[2].close()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/transaction.py", line 43, in _active
remote:     return func(self, *args, **kwds)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/transaction.py", line 490, in close
remote:     self._postclosecallback[cat](self)
remote:   File "/var/hg/version-control-tools/hgext/pushlog/__init__.py", line 236, in onpostclose
remote:     conn.commit()
remote: sqlite3.OperationalError: database is locked
abort: stream ended unexpectedly (got 0 bytes, expected 4)
Attachment #8870538 - Flags: review?(cam)
Attachment #8870539 - Flags: review?(cam)
Attachment #8870557 - Flags: review?(cam)
Attachment #8870540 - Flags: review?(cam)
See Also: → 1367303
Comment on attachment 8870538 [details]
Bug 1339629 Part 7: Ensure ServoStyleSets keep sheets informed of the stylesets they are part of.

https://reviewboard.mozilla.org/r/141986/#review145814
Attachment #8870538 - Flags: review?(cam) → review+
Comment on attachment 8870539 [details]
Bug 1339629 Part 12: Define UUID for ServoStyleSheet, which is needed for tests.

https://reviewboard.mozilla.org/r/141988/#review145816
Attachment #8870539 - Flags: review?(cam) → review+
Comment on attachment 8870557 [details]
Bug 1339629 Part 16: Mark a crashtest as failing, to be fixed in a later bug.

https://reviewboard.mozilla.org/r/142008/#review145818
Attachment #8870557 - Flags: review?(cam) → review+
Comment on attachment 8870540 [details]
Bug 1339629 Part 17: Add mochitests to test that css rules are distinct after stylesheet cloning.

https://reviewboard.mozilla.org/r/141990/#review145820
Attachment #8870540 - Flags: review?(cam) → review+
Attachment #8870222 - Flags: feedback?(manishearth)
Attachment #8870223 - Flags: feedback?(manishearth)
Attachment #8870222 - Flags: feedback?(manishearth) → feedback?(bbirtles)
Attachment #8870223 - Flags: feedback?(manishearth) → feedback?(bbirtles)
Attachment #8870222 - Flags: feedback?(bbirtles) → feedback+
Attachment #8870223 - Flags: feedback?(bbirtles) → feedback+
Blocks: 1367523
Attachment #8870222 - Attachment is obsolete: true
Attachment #8870223 - Attachment is obsolete: true
Blocks: 1367610
Attachment #8865710 - Attachment is obsolete: true
Attachment #8867909 - Attachment is obsolete: true
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a16551697e09
Part 3: Uplift mDirty into StyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/7a72e493e820
Part 4: Uplift CloneFor into StyleSheetInfo, and EnsureUniqueInner into StyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e0abb8753179
Part 5: Change CSSStyleSet::mStyleSets to use StyleSetHandles. r=heycam
https://hg.mozilla.org/integration/autoland/rev/2cdedac07e3b
Part 6: Uplift mStyleSets into StyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/7f6afbde86e8
Part 7: Ensure ServoStyleSets keep sheets informed of the stylesets they are part of. r=heycam
https://hg.mozilla.org/integration/autoland/rev/72d7b67722d3
Part 8: Uplift ClearRuleCascades into StyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/522b3594b84c
Part 9: Uplift EnsureUniqueInnerOnCSSSheets and SetNeedsRestyleAfterEnsureUniqueInner into StyleSetHandle, and eliminate CSSStyleSheet::EnsureUniqueInner. r=heycam
https://hg.mozilla.org/integration/autoland/rev/597de01f6c17
Part 10: Implement ServoStyleSet::EnsureUniqueInnerOnCSSSheets. r=heycam
https://hg.mozilla.org/integration/autoland/rev/93fd5f6fdd14
Part 11: Call EnsureUniqueInner from ServoCSSRuleList when a StyleSheet is set. r=heycam
https://hg.mozilla.org/integration/autoland/rev/24d9cf9eedae
Part 12: Define UUID for ServoStyleSheet, which is needed for tests. r=heycam
https://hg.mozilla.org/integration/autoland/rev/0b97b39643f6
Part 13: Consolidate stylesheet cloning tests into a named directory. r=heycam
https://hg.mozilla.org/integration/autoland/rev/43f82a9c86d7
Part 14: Add reftests to verify we clone css group rules when we clone the stylesheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/1b67d0bb78da
Part 15: Add reftests to test other css rules clone properly when stylesheet is cloned. r=heycam
https://hg.mozilla.org/integration/autoland/rev/87240fc3dab9
Part 16: Mark a crashtest as failing, to be fixed in a later bug. r=heycam
https://hg.mozilla.org/integration/autoland/rev/8f7d1b244132
Part 17: Add mochitests to test that css rules are distinct after stylesheet cloning. r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: