Closed
Bug 1373193
Opened 8 years ago
Closed 8 years ago
stylo: CSSOM is getting different rule instances than what is used in styling
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: xidorn, Assigned: bradwerth)
References
Details
Attachments
(4 files, 2 obsolete files)
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.
Reporter | ||
Comment 1•8 years ago
|
||
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...)
Reporter | ||
Comment 2•8 years ago
|
||
So, Gecko's ServoStyleSheet gets a different Stylesheet than Servo's stylist, which matches my guess.
Comment hidden (obsolete) |
Reporter | ||
Comment 4•8 years ago
|
||
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 hidden (obsolete) |
Reporter | ||
Comment 6•8 years ago
|
||
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?
Flags: needinfo?(bwerth)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(cam)
![]() |
||
Comment 7•8 years ago
|
||
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.
Reporter | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
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.
Assignee: nobody → bwerth
Blocks: 1367213
Updated•8 years ago
|
Flags: needinfo?(cam)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8879269 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8879267 -
Flags: review?(xidorn+moz)
Attachment #8879268 -
Flags: review?(xidorn+moz)
Attachment #8879270 -
Flags: review?(xidorn+moz)
Reporter | ||
Comment 18•8 years ago
|
||
mozreview-review |
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.
Attachment #8879268 -
Flags: review?(xidorn+moz) → review+
Reporter | ||
Comment 19•8 years ago
|
||
mozreview-review |
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
Attachment #8879267 -
Flags: review?(xidorn+moz) → review+
Reporter | ||
Comment 20•8 years ago
|
||
mozreview-review |
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
Attachment #8879270 -
Flags: review?(xidorn+moz) → review+
Comment 21•8 years ago
|
||
mozreview-review |
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•8 years ago
|
||
mozreview-review |
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•8 years ago
|
||
(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...
Assignee | ||
Comment 24•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 25•8 years ago
|
||
(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.
Flags: needinfo?(bwerth)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8879267 -
Attachment is obsolete: true
Comment 34•8 years ago
|
||
Pushed by bwerth@mozilla.com:
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a2de41b74810
https://hg.mozilla.org/mozilla-central/rev/5bc9622943b1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•