stylo: Stylesheet cloning doesn't seem to work right

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
a month ago
14 hours ago

People

(Reporter: bz, Assigned: bradwerth, NeedInfo)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 9 obsolete attachments)

294 bytes, text/html
Details
339 bytes, text/html
Details
503 bytes, text/html
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
bradwerth
: review?
heycam
Details | Review
59 bytes, text/x-review-board-request
bradwerth
: review?
heycam
Details | Review
59 bytes, text/x-review-board-request
bradwerth
: review?
heycam
Details | Review
59 bytes, text/x-review-board-request
bradwerth
: review?
heycam
Details | Review
59 bytes, text/x-review-board-request
bradwerth
: review?
heycam
Details | Review
Created attachment 8848704 [details]
Testcase

See attached testcase: text should be green.
Summary: Stylesheet cloning doesn't seem to work right → stylo: Stylesheet cloning doesn't seem to work right
Assignee: nobody → bwerth
Priority: -- → P1
Created attachment 8848832 [details]
Another example showing that something is up here.
Though I think that second testcase just shows that we don't do the "copy" part of the copy-on-write cloning setup that stylesheets are supposed to have....  which while clearly broken is not sufficient to explain the first testcase.
Oh, I think I see what's up with the first testcase.  We have two sheets, both sharing a single inner.  We remove one of them, which calls ServoStyleSet::RemoveStyleSheet, which calls:

    Servo_StyleSet_RemoveStyleSheet(mRawSet.get(), aSheet->RawSheet(), !mBatching);

where aSheet->RawSheet() goes via the _inner_.  That is, that pointer is the same for both sheets.  And Servo_StyleSet_RemoveStyleSheet does this:

    data.stylesheets.retain(|x| !arc_ptr_eq(x, sheet));

which removes _both_ copies of the servo-side sheet from the list.

The fundamental problem there is that this mirroring setup is totally broken given how sheet cloning works: it needs to have distinct objects servo-side for distinct sheets, but it doesn't.  Another testcase coming up that shows this.
Created attachment 8848835 [details]
Testcase showing incorrect cascading when a sheet is added dynamically

I'm not sure why the brokenness is when "target" is the first thing.  I would have expected this bit in Servo_StyleSet_InsertStyleSheetBefore:

    let index = data.stylesheets.iter().position(|x| arc_ptr_eq(x, reference)).unwrap();

to find the first index and the bug to therefore arise when inserting before the _second_ clone.  So I don't know quite what's going on there.
Created attachment 8848836 [details]
Correct testcase showing incorrect cascading
Attachment #8848835 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
See Also: → bug 1339629
Because ServoStyleSet keeps its own arrays of sheets (in mSheets), we should be able to know what index in the Servo PerDocumentStyleData.stylesheets Vec that sheets are inserted to and removed from.  Although this might be slightly tricky, since ServoStyleSet keeps the sheets separated by cascade level, and PerDocumentStyleData.stylesheets has them all mixed up together.
But if we can work out the index, we can pass that through the FFI function, so that we can insert before and remove the right copy of a Stylesheet reference in that Vec.
Blocks: 1243581
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)
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

10 days ago
Attachment #8856914 - Flags: review?(cam)
Attachment #8858257 - Flags: review?(cam)
Attachment #8858258 - Flags: review?(cam)
Attachment #8859349 - Flags: review?(cam)
Attachment #8859350 - Flags: review?(cam)
Attachment #8859351 - Flags: review?(cam)
Attachment #8859378 - Flags: review?(cam)

Comment 22

9 days ago
mozreview-review
Comment on attachment 8856914 [details]
Bug 1348481 Part 1: Servo-side changes to StyleSet to track stylesheets by unique ID.

https://reviewboard.mozilla.org/r/128852/#review134120

This is mostly fine, but I'll take a look at another version of the patch.

::: layout/style/ServoStyleSet.h:354
(Diff revision 3)
>     */
>    void PreTraverse(dom::Element* aRoot = nullptr);
>    // Subset of the pre-traverse steps that involve syncing up data
>    void PreTraverseSync();
>  
> +  typedef struct {

In C++ |struct RawIndexedSheet { ... }| is sufficient for not needing to write out |struct RawIndexedSheet| when referring to the type.

::: layout/style/ServoStyleSet.h:355
(Diff revision 3)
> +    size_t rawIndex;
> +    RefPtr<ServoStyleSheet> sheet;

Nit: I would still use the mRawIndex / mSheet naming convention here.

Please also add a comment here describing that the mRawIndex member is the index of the sheet in the PerDocumentStyleData::stylesheets vector.

::: layout/style/ServoStyleSet.h:358
(Diff revision 3)
>  
> +  typedef struct {
> +    size_t rawIndex;
> +    RefPtr<ServoStyleSheet> sheet;
> +  } RawIndexedSheet;
> +  static const size_t NO_RAW_INDEX = SIZE_MAX;

Nit: probably kNoRawIndex, to follow the style guidelines for (non-macro) constants.

I also find it a little confusing that we're using the same constant for the return values of AppendSheetOfType, InsertSheetOfType, etc., since there the indexes are local indexes, not raw indexes.  Should we just have a separate constant for kNoLocalIndex?

::: layout/style/ServoStyleSet.h:364
(Diff revision 3)
> +
> +  /**
> +   * Add a sheet of type at the end, and return a reference to its rawIndex,
> +   * which is initially set to NO_RAW_INDEX.
> +   */
> +  size_t& AppendSheetOfType(ServoStyleSheet *aSheet, SheetType aType);

Nit: "*" next to type, here and below.

::: layout/style/ServoStyleSet.cpp:59
(Diff revision 3)
> -      MOZ_ASSERT(sheet->RawSheet(), "We should only append non-null raw sheets.");
> -      Servo_StyleSet_AppendStyleSheet(mRawSet.get(), sheet->RawSheet(), false);
> +      MOZ_ASSERT(rawIndexedSheet.sheet->RawSheet(),
> +                 "We should only append non-null raw sheets.");

(Hmm, if RawSheet() is allowed to return null in some cases, then maybe it should be called GetRawSheet().)

::: layout/style/ServoStyleSet.cpp:594
(Diff revision 3)
>    if (mRawSet) {
> -    for (ServoStyleSheet* sheet : mSheets[aType]) {
> -      Servo_StyleSet_RemoveStyleSheet(mRawSet.get(), sheet->RawSheet(), false);
> +    for (auto& rawIndexedSheet : mSheets[aType]) {
> +      Servo_StyleSet_RemoveStyleSheet(mRawSet.get(),
> +                                      rawIndexedSheet.sheet->RawSheet(),
> +                                      false);
>      }
>    }

(Since we're removing all sheets, this makes me wonder if we should should have a single function call to do this removal, since in the worst case, always removing the first element of the vector on the Servo side is going to result in quadratic time behaviour in the number of sheets.)

::: layout/style/ServoStyleSet.cpp:633
(Diff revision 3)
> -  mSheets[aType].RemoveElement(aNewSheet);
> -  size_t idx = mSheets[aType].IndexOf(aReferenceSheet);
> +  size_t idx = FindLocalIndexOfSheetOfType(aReferenceSheet, aType);
> +  if (idx == NO_RAW_INDEX) {
> -  if (idx == mSheets[aType].NoIndex) {
>      return NS_ERROR_INVALID_ARG;
>    }

I think we need to compute the index after we've potentially removed aNewSheet from its current position, since if it appears before the reference sheet, our index will be off by one.

::: layout/style/ServoStyleSet.cpp:685
(Diff revision 3)
> -  sheetsArray.RemoveElement(aSheet);
> +  nsTArray<RefPtr<ServoStyleSheet>> sheetsArray;
> +  FillArrayOfSheetsOfType(sheetsArray, SheetType::Doc);

It seems a shame to have to bump the refcounts up and down for all of the document sheets we have.  How about making this an array of raw ServoStyleSheet pointers, and adjusting nsIDocument::FindDocStyleSheetInsertionPoint to take a |const nsTArray<U>& aDocSheets| array?

::: layout/style/ServoStyleSet.cpp:915
(Diff revision 3)
>      ptr = nullptr;
>    }
>  }
>  
> +size_t&
> +ServoStyleSet::AppendSheetOfType(ServoStyleSheet *aSheet, SheetType aType)

Nit: "*" next to type, here and below.

::: layout/style/ServoStyleSet.cpp:936
(Diff revision 3)
> +  size_t i = 0;
> +  for (const RawIndexedSheet& rawIndexedSheet : mSheets[aType]) {

Nit: feels a bit funny to me to keep a loop counter and use an iterator.  Maybe just use the for loop to increment the counter, and ditch the ranged for loop syntax?  (And below.)
Attachment #8856914 - Flags: review?(cam) → review-

Comment 23

9 days ago
mozreview-review
Comment on attachment 8858257 [details]
Bug 1348481 Part 3: Servo-side implementation of the styleset regenerate function.

https://reviewboard.mozilla.org/r/130238/#review134232

(I haven't looked at AddDocStyleSheet so far.)

I'm a little concerned with the difficulty in keeping the indexes correct while performing these operations.  It seems like it is easy to make mistakes and for the two sides to get out of sync.  At the very least, I think we need to add a bunch of assertions after each of these operations is performed that the indexes/sheets on both sides match up.

But I want to make a suggestion, and I apologise for not thinking of it and suggesting it earlier: instead of getting indexes back from the Servo side, and patching up  the indexes we store in the C++ side, what do you think about simply passing the complete array of RawServoStyleSheet pointers for the entire style set after processing one of these insertion/removal operations on the C++ side arrays?  That would obviate the need to store any indexes on the C++ side, and we could simplify the interface to the Servo side by removing the Servo_StyleSet_Append/InsertBefore/Prepend/Remove_XXX functions.  WDYT?  Can you see any downsides to this, apart from extra refcount traffic on the Arc<Stylesheets>, and the potential to do O(n^2) work when we add a bunch of style sheets in a row?

::: layout/style/ServoStyleSet.cpp:653
(Diff revision 3)
>    if (idx == NO_RAW_INDEX) {
>      return NS_ERROR_INVALID_ARG;
>    }
> -  MOZ_ASSERT(aReferenceSheet->RawSheet(), "Reference sheet should have a raw sheet.");
> +  size_t& referenceRawIndex = mSheets[aType][idx].rawIndex;
>  
> -  RemoveSheetOfType(aNewSheet, aType);
> +  size_t oldRawIndex = RemoveSheetOfType(aNewSheet, aType);

Won't this call to RemoveSheetOfType invalidate the |idx| that we computed above?
Attachment #8858257 - Flags: review?(cam) → review-
(In reply to Cameron McCormack (:heycam) from comment #23)
> Can you see any downsides to this, apart from extra
> refcount traffic on the Arc<Stylesheets>, and the potential to do O(n^2)
> work when we add a bunch of style sheets in a row?

And maybe we should use mBatching to avoid sending over the list of style sheets, doing it in EndUpdate if we are batching...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 days ago
Attachment #8858258 - Attachment is obsolete: true
Attachment #8858258 - Flags: review?(cam)
(Assignee)

Updated

9 days ago
Attachment #8859349 - Attachment is obsolete: true
Attachment #8859349 - Flags: review?(cam)
(Assignee)

Updated

9 days ago
Attachment #8859350 - Attachment is obsolete: true
Attachment #8859350 - Flags: review?(cam)
(Assignee)

Updated

9 days ago
Attachment #8859351 - Attachment is obsolete: true
Attachment #8859351 - Flags: review?(cam)
(Assignee)

Updated

9 days ago
Attachment #8859378 - Attachment is obsolete: true
Attachment #8859378 - Flags: review?(cam)

Comment 27

9 days ago
mozreview-review
Comment on attachment 8856914 [details]
Bug 1348481 Part 1: Servo-side changes to StyleSet to track stylesheets by unique ID.

https://reviewboard.mozilla.org/r/128852/#review134648

This looks great, thanks.  A later patch will call this function?
Attachment #8856914 - Flags: review?(cam) → review+
(Assignee)

Comment 28

8 days ago
mozreview-review-reply
Comment on attachment 8856914 [details]
Bug 1348481 Part 1: Servo-side changes to StyleSet to track stylesheets by unique ID.

https://reviewboard.mozilla.org/r/128852/#review134648

Yeah, Part 2 (servo implementation) is failing to compile with type problems. After I get that working, I'll restructure the patches to isolate the servo stuff from the gecko stuff, and make a clearer invocation story on the gecko side. I'll clear review flags for now.
Attachment #8858257 - Flags: review?(cam)
Brad, have you figured out why the Part 2 is failing to compile? While poking bug 1290276, I also need to to pass an nsTArary<const RawServoStyleSheet*> to the servo side and iterate each sheet in the array by casting them into servo type Stylesheet. I haven't figured how to do that yet ... If you have any progress, that will be very helpful.
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)
(Assignee)

Comment 36

3 days ago
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #29)
> Brad, have you figured out why the Part 2 is failing to compile? While
> poking bug 1290276, I also need to to pass an nsTArary<const
> RawServoStyleSheet*> to the servo side and iterate each sheet in the array
> by casting them into servo type Stylesheet. I haven't figured how to do that
> yet ... If you have any progress, that will be very helpful.

Ting, I figured out something dangerous-looking that seems to work. I don't know if this is best practice, but it does work:

for raw_sheet in raw_sheets.iter() {
    // This transmute is necessary to convert the value from the iter
    // into the proper borrowed type.
    let unsafeSheet: RawServoStyleSheetBorrowed =
    unsafe {
        mem::transmute::<
            &RawServoStyleSheet,
            RawServoStyleSheetBorrowed>(&**raw_sheet)
    };
    let sheet = HasArcFFI::as_arc(&unsafeSheet);
    data.stylesheets.append_stylesheet(sheet);
}
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)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 days ago
Attachment #8861266 - Flags: review?(cam)
Attachment #8861627 - Flags: review?(cam)
Attachment #8861628 - Flags: review?(cam)
Attachment #8861629 - Flags: review?(cam)
Attachment #8861630 - Flags: review?(cam)
Attachment #8861631 - Flags: review?(cam)
Attachment #8861632 - Flags: review?(cam)
This approach doesn't play well at all with bug 1357583 :(.
See Also: → bug 1357583
Can't we give each sheet an unique id and give it to servo instead?
I propose to store a Vec<(Arc<Stylesheet>, StylesheetId)>, instead of a Vec<Arc<Stylesheet>>, with the stylesheet id being some sort of unique thing (can be something monotonically increasing, but also perhaps the pointer identity of the ServoStyleRule object).

Brad told me that it was his original approach, but Cameron said to go to the rebuilding approach. I wonder why?
Flags: needinfo?(cam)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #48)
> I propose to store a Vec<(Arc<Stylesheet>, StylesheetId)>, instead of a
> Vec<Arc<Stylesheet>>, with the stylesheet id being some sort of unique thing
> (can be something monotonically increasing, but also perhaps the pointer
> identity of the ServoStyleRule object).

Err, I mean ServoStyleSheet.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #48)
> Brad told me that it was his original approach, but Cameron said to go to
> the rebuilding approach. I wonder why?

The original suggestion I made was slightly different from that -- rather than having unique identifiers per added sheet, we kept a track of the index in the Vec that the Stylesheet was added at, and back on the C++ side when we performed insertion/removal operations, we adjusted the indexes appropriately according to the particular insertion/removal operation.  This seemed pretty fragile to me, after I saw the patches, and I was worried that we could get our indexes out of sync easily.

Using a unique ID per added sheet would be better than that.

I am kind of reluctant to ask Brad to change it all again, though. :-)  For the moment, what do you think about going with the current approach, and detecting on the Servo side that the new sheet array contains new sheets (and no removed sheets), and skipping restyling in that case?  (Or perhaps tracking that in ServoStyleSet while batching, and passing a parameter once we do set the new sheet array, if we only had sheet additions, and which ones they were?)
Flags: needinfo?(cam) → needinfo?(emilio+bugs)
I'd really prefer to get it right from the beginning, to be honest.

I can diff the arrays, but that's way more non-trivial than what we do now... Sounds like using an id wouldn't be a huge deal and would be a less breaking change?

Anyway, I guess it's fine, I can always rework this when we get to bug 1357583, I guess :/
Flags: needinfo?(emilio+bugs)
I guess that's kind of annoying though, because random rules can be mutated via CSSOM, so we need to keep track of whether there've been only stylesheet addition/removals, and only then diffing the arrays...
(Assignee)

Comment 53

2 days ago
(In reply to Cameron McCormack (:heycam) from comment #50)
> Using a unique ID per added sheet would be better than that.
> 
> I am kind of reluctant to ask Brad to change it all again, though. :-)  For
> the moment, what do you think about going with the current approach, and
> detecting on the Servo side that the new sheet array contains new sheets
> (and no removed sheets), and skipping restyling in that case?  (Or perhaps
> tracking that in ServoStyleSet while batching, and passing a parameter once
> we do set the new sheet array, if we only had sheet additions, and which
> ones they were?)

It seems a shame for this patch to strip out all the servo_style_set functions if Bug 1357583 will just require them to be put back again. I'm not completely convinced that turning certain stylesheet additions into a no-op is that difficult with the regenerative approach in the current patches -- it seems we would just not call the servo_styleset_regnerate function in such a case. It would get called eventually when a does-affect-the-page stylesheet change is made. Why would that not work?

Anyway, I'm willing to revise my original set of patches to go with the unique ID approach, but I want to make sure I understand how you want it work. Once we have unique monotonic IDs generated on the gecko side, what's the organizing data structure on the servo side? To be specific, if we have 3 added stylesheets in the order 1-3-2 (first 1 and 2 are appended, then 3 is inserted before 2), on the servo side we would do an insertbefore unique ID 2 and do what with it? Look into a hashmap of unique IDs to array indices? If that's the intent, then we'd have to do fixup array indices on servo-side, right?
Flags: needinfo?(bwerth) → needinfo?(cam)
(In reply to Brad Werth [:bradwerth] from comment #53)
> It seems a shame for this patch to strip out all the servo_style_set
> functions if Bug 1357583 will just require them to be put back again. I'm
> not completely convinced that turning certain stylesheet additions into a
> no-op is that difficult with the regenerative approach in the current
> patches -- it seems we would just not call the servo_styleset_regnerate
> function in such a case. It would get called eventually when a
> does-affect-the-page stylesheet change is made. Why would that not work?

I'm not sure I've got this. The point of bug 1357583 is avoiding styling the whole document when stylesheets change, given you can know by inspecting the DOM that a given stylesheet doesn't affect anything.

To do that, you need to know precisely the stuff that has been added. Otherwise you can't really know.


> Anyway, I'm willing to revise my original set of patches to go with the
> unique ID approach, but I want to make sure I understand how you want it
> work. Once we have unique monotonic IDs generated on the gecko side, what's
> the organizing data structure on the servo side? To be specific, if we have
> 3 added stylesheets in the order 1-3-2 (first 1 and 2 are appended, then 3
> is inserted before 2), on the servo side we would do an insertbefore unique
> ID 2 and do what with it? Look into a hashmap of unique IDs to array
> indices? If that's the intent, then we'd have to do fixup array indices on
> servo-side, right?


I was suggesting storing the id along the stylesheet (e.g., |struct StylesheetEntry(StyleSheetId, Arc<StyleSheet>)|, or something like that). 

That way, what you have is a Vec<StylesheetEntry>.

When you add 1, and 3, you have a list that looks like [(1, <style>), (2, <style>)]. When you insert 3 before 2, you just go through the list looking for the id number 2, and find the position (the second in this case), so you end up with [(1, <style>), (3, <style>), (2, <style>)].

It'd be really similar to how it works now, just instead of looking by arc_ptr_eq, we'd be looking by StyleSheetId.

Also, probably you don't need a monotonic counter, you can probably just use the Gecko object pointer identity as the id, which would be slightly simpler to map, but that's another matter :)
(Assignee)

Comment 55

2 days ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #54) 
> I was suggesting storing the id along the stylesheet (e.g., |struct
> StylesheetEntry(StyleSheetId, Arc<StyleSheet>)|, or something like that). 
> 
> That way, what you have is a Vec<StylesheetEntry>.

Thanks for detailing the vision. I'll prepare a patch that does this.
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

14 hours ago
Attachment #8858257 - Attachment is obsolete: true
(Assignee)

Updated

14 hours ago
Attachment #8861627 - Attachment is obsolete: true
Attachment #8861627 - Flags: review?(cam)
(Assignee)

Updated

14 hours ago
Attachment #8861628 - Attachment is obsolete: true
Attachment #8861628 - Flags: review?(cam)
(Assignee)

Updated

14 hours ago
Attachment #8861266 - Flags: review?(cam)
Attachment #8861629 - Flags: review?(cam)
Attachment #8861630 - Flags: review?(cam)
Attachment #8861631 - Flags: review?(cam)
Attachment #8861632 - Flags: review?(cam)
(Assignee)

Updated

14 hours ago
Attachment #8861266 - Flags: review?(cam)
Attachment #8861629 - Flags: review?(cam)
Attachment #8861630 - Flags: review?(cam)
Attachment #8861631 - Flags: review?(cam)
Attachment #8861632 - Flags: review?(cam)
Comment hidden (mozreview-request)
You need to log in before you can comment on or make changes to this bug.