1373193 - stylo: CSSOM is getting different rule instances than what is used in styling

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase

See the testcase. The text inside should turn to green, however, in stylo, the change to color doesn't affect styling.

What seems to happen behind is that the C++ wrapper object gets a different Servo rule object than what is used in styling.

It is not clear why this happens, but it blocks bug 1359217 because it is not able to match all styles.

I suspect this is a recent regression.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Looks like it isn't a very recent regression at least... I can reproduce this from https://hg.mozilla.org/mozilla-central/rev/b42d50cafb154e3e50fdde3ca853635a000cb219

(I probably should setup a Linux VM and download the binaries from TreeHerder rather than building code myself...)

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

So, Gecko's ServoStyleSheet gets a different Stylesheet than Servo's stylist, which matches my guess.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

So there are two problems:
1. The stylesheet inner is cloned somehow, because it is used by two stylesheets (which doesn't make sense because there is only one reference to the stylesheet in the document).
2. The cloned stylesheet is not added back to stylist, thus it would never actually take effect.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

And more interestingly that, the other stylesheet which holds this inner is not owned by the document, and is actually not referenced by anyone. Its mRefCntAndFlags = 3, which, according to [1], means refcnt = 0, but it is "purple" and "in purple buffer". I don't know what that really means, but I believe that indicates it is totally a waste to clone the inner in this case.

[1] https://dxr.mozilla.org/mozilla-central/rev/da66c4a05fda49d457d9411a7092fed87cf9e53a/xpcom/base/nsISupportsImpl.h#182-186

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

OK, so the first stylesheet, which is created but never inserted into the document is from nsDocument::PreloadStyle. I think we should not clone it in that case, but instead just set document to it.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

So the two problems are:


First:

nsDocument::PreloadStyle triggers a load to linked stylesheet before further construction, and when the corresponding link element is created, it triggers another CreateSheet, which finds the sheet from Loader::mLoadingDatas, and clone from it. At this moment, the inner has two stylesheets associated with, first by preload, and second by the link element.

After everything is loaded, the first stylesheet would probably be owned by Loader::mCompleteSheets, and the second one would be owned by the element. If there is any change to the stylesheet, we would clone the whole stylesheet, while the other stylesheet is not used in styling at all.

This is probably a pre-existing problem for Gecko that, the cache means to deduplicate stylesheet, but it actually duplicate its content unnecessarily in some case.

This problem is probably out of scope of Stylo, but is something we probably should fix for Gecko.


Second:

When ServoStyleSheet clones the inner, it doesn't notify ServoStyleSet about that, and consequently the cloned Stylesheet is not added into stylist when styling.

This problem is orthogonal to the first problem, because even if we can avoid cloning stylesheet for that case, we probably cannot avoid cloning when the stylesheet is really reused in the same document.


The second problem is probably the real problem for this bug. The first one should probably go to a separate bug.


Brad, heycam, what do you think about the second problem?

Comment 7

[Boris Zbarsky [:bzbarsky]]

As noted in bug 1373484, the first problem is not actually a problem.  It _used_ to be a problem, but we fixed it in bug 799816.  There are in fact two clones, but when the load completes the actually-used clone is what's placed in mCompleteSheets and the other clone is discarded.  At least that's how it's meant to work; it's possible that there are bugs.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase 2

So after bug 1373484 gets fixed, the original testcase would start working correctly (because we no longer clone for single link element case), so attach another testcase for this bug.

Comment 9

[Brad Werth [:bradwerth]]

This seems to be the source of another problem I was working on. Thank you for identifying the root cause. I'll work on a patch for testcase 2.

Comment 11

[Brad Werth [:bradwerth]]

Attachment: Bug 1373193 Part 1: Servo-side add an update_stylesheet method to StylesheetSet.

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

Comment 12

[Brad Werth [:bradwerth]]

Attachment: Bug 1373193 Part 2: Create and call a new UpdateStyleSheet method on ServoStyleSet when a ServoStyleSheet has its inner cloned.

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

Comment 13

[Brad Werth [:bradwerth]]

Attachment: Bug 1373193 Part 3: Fix assert logic in ServoCSSRuleList.

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

Comment 14

[Brad Werth [:bradwerth]]

Attachment: Bug 1373193 Part 3: Add a test of insert-after-clone correctness, and remove redundant crashtest.

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

Comment 15

[Brad Werth [:bradwerth]]

Comment on attachment 8879267 [details]
Bug 1373193 Part 1: Servo-side add an update_stylesheet method to StylesheetSet.

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

Comment 16

[Brad Werth [:bradwerth]]

Comment on attachment 8879268 [details]
Bug 1373193 Part 2: Create and call a new UpdateStyleSheet method on ServoStyleSet when a ServoStyleSheet has its inner cloned.

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

Comment 17

[Brad Werth [:bradwerth]]

Comment on attachment 8879270 [details]
Bug 1373193 Part 3: Add a test of insert-after-clone correctness, and remove redundant crashtest.

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

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8879268 [details]
Bug 1373193 Part 2: Create and call a new UpdateStyleSheet method on ServoStyleSet when a ServoStyleSheet has its inner cloned.

https://reviewboard.mozilla.org/r/150554/#review155382

::: layout/style/StyleSheet.cpp:457
(Diff revision 2)
>    // nsPresContext::EnsureSafeToHandOutCSSRules we will need to restyle the
>    // document
>    for (StyleSetHandle& setHandle : mStyleSets) {
> +    if (ServoStyleSet* servoSet = setHandle->GetAsServo()) {
> +      MOZ_ASSERT(IsServo(), "Only servo sheets should be in servo stylesets.");
> +      servoSet->UpdateStyleSheet(GetAsServo());

I'm a bit concerned since `EnsureUniqueInner` can be invoked by the constructor. It's probably fine here, but we need to keep an eye on this.

Actually, in that case, we probably don't need to call this at all. Not sure whether that would make anything better, though.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8879267 [details]
Bug 1373193 Part 1: Servo-side add an update_stylesheet method to StylesheetSet.

https://reviewboard.mozilla.org/r/150552/#review155424

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8879270 [details]
Bug 1373193 Part 3: Add a test of insert-after-clone correctness, and remove redundant crashtest.

https://reviewboard.mozilla.org/r/150558/#review155426

Comment 21

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8879267 [details]
Bug 1373193 Part 1: Servo-side add an update_stylesheet method to StylesheetSet.

https://reviewboard.mozilla.org/r/150552/#review155492

::: servo/components/style/stylesheet_set.rs:153
(Diff revision 2)
> +        debug!("StylesheetSet::update_stylesheet");
> +        if let Some(entry) = self.entries.iter_mut().find(
> +            |e| e.unique_id == unique_id) {
> +            entry.sheet = sheet.clone();
> +
> +            self.dirty = true;

There should be no need to call dirty and process_invalidations_for. it's ok if we use the old inner data for a little longer, since it's a clone.  As soon as it changes it'll be updated on ththhtehte next flush.

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8879268 [details]
Bug 1373193 Part 2: Create and call a new UpdateStyleSheet method on ServoStyleSet when a ServoStyleSheet has its inner cloned.

https://reviewboard.mozilla.org/r/150554/#review155500

::: layout/style/ServoStyleSet.cpp:794
(Diff revision 2)
> +  if (mRawSet) {
> +    // Inform servo that the underlying raw sheet has changed.
> +    Servo_StyleSet_UpdateStyleSheet(mRawSet.get(),
> +                                    aSheet->RawSheet(),
> +                                    UniqueIDForSheet(aSheet));
> +    SetStylistStyleSheetsDirty();

Ditto for this, but add aa comment on why.

Comment 23

[Emilio Cobos Álvarez [:emilio]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #21)
>  ththhtehte next flush.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> add aa comment on why.

Can't really write review comments from my phone, apparently, d'oh...

Comment 24

[Brad Werth [:bradwerth]]

Comment on attachment 8879267 [details]
Bug 1373193 Part 1: Servo-side add an update_stylesheet method to StylesheetSet.

https://reviewboard.mozilla.org/r/150552/#review155492

> There should be no need to call dirty and process_invalidations_for. it's ok if we use the old inner data for a little longer, since it's a clone.  As soon as it changes it'll be updated on ththhtehte next flush.

This function doesn't guarantee that the updated stylesheet is a clone of the old stylesheet. It seems either we should keep this dirty = true, or provide a parameter that makes the setting of dirty = true optional. I'll pursue this second strategy.

Comment 25

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #24) 
> This function doesn't guarantee that the updated stylesheet is a clone of
> the old stylesheet. It seems either we should keep this dirty = true, or
> provide a parameter that makes the setting of dirty = true optional. I'll
> pursue this second strategy.

After a conversation with emilio, he convinced me that it's sufficient to document this assumption in the function and leave the function signature alone.

Comment 26

[Brad Werth [:bradwerth]]

Comment on attachment 8879267 [details]
Bug 1373193 Part 1: Servo-side add an update_stylesheet method to StylesheetSet.

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

Comment 27

[Brad Werth [:bradwerth]]

Comment on attachment 8879268 [details]
Bug 1373193 Part 2: Create and call a new UpdateStyleSheet method on ServoStyleSet when a ServoStyleSheet has its inner cloned.

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

Comment 28

[Brad Werth [:bradwerth]]

Comment on attachment 8879270 [details]
Bug 1373193 Part 3: Add a test of insert-after-clone correctness, and remove redundant crashtest.

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

Comment 29

[Brad Werth [:bradwerth]]

Comment on attachment 8879267 [details]
Bug 1373193 Part 1: Servo-side add an update_stylesheet method to StylesheetSet.

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

Comment 30

[Brad Werth [:bradwerth]]

Comment on attachment 8879268 [details]
Bug 1373193 Part 2: Create and call a new UpdateStyleSheet method on ServoStyleSet when a ServoStyleSheet has its inner cloned.

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

Comment 31

[Brad Werth [:bradwerth]]

Comment on attachment 8879270 [details]
Bug 1373193 Part 3: Add a test of insert-after-clone correctness, and remove redundant crashtest.

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

Comment 32

[Brad Werth [:bradwerth]]

Comment on attachment 8879268 [details]
Bug 1373193 Part 2: Create and call a new UpdateStyleSheet method on ServoStyleSet when a ServoStyleSheet has its inner cloned.

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

Comment 33

[Brad Werth [:bradwerth]]

Comment on attachment 8879270 [details]
Bug 1373193 Part 3: Add a test of insert-after-clone correctness, and remove redundant crashtest.

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

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2de41b74810
Part 2: Create and call a new UpdateStyleSheet method on ServoStyleSet when a ServoStyleSheet has its inner cloned. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/5bc9622943b1
Part 3: Add a test of insert-after-clone correctness, and remove redundant crashtest. r=xidorn

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a2de41b74810
https://hg.mozilla.org/mozilla-central/rev/5bc9622943b1