Closed Bug 1348481 Opened 7 years ago Closed 7 years ago

stylo: Stylesheet cloning doesn't seem to work right

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bradwerth)

References

Details

Attachments

(9 files, 10 obsolete files)

Testcase
294 bytes, text/html
Details
Another example showing that something is up here.
339 bytes, text/html
Details
Correct testcase showing incorrect cascading
503 bytes, text/html
Details
Bug 1348481 Part 2: Gecko-side track unique IDs for each stylesheet and send them to Servo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1348481 Part 3: Fix a dangerous typo in StyleSetHandleInlines (replaced one variable with another) and add assert to catch the problem it created.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1348481 Part 4: Turn back on unexpected pass reftests.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1348481 Part 5: Add new reftests to test servo stylesheet set integrity and cloning behavior.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1348481 Part 6: Adjust expected error counts for mochitests (generally greatly reduced).
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1348481 Part 1b: Generalize FindDocStyleSheetInsertionPoint so it doesn't require an array of RefPtrs.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Attached file 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
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.
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.
Attachment #8848835 - Attachment is obsolete: true
See Also: → 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.
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 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 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...
Attachment #8858258 - Attachment is obsolete: true
Attachment #8858258 - Flags: review?(cam)
Attachment #8859349 - Attachment is obsolete: true
Attachment #8859349 - Flags: review?(cam)
Attachment #8859350 - Attachment is obsolete: true
Attachment #8859350 - Flags: review?(cam)
Attachment #8859351 - Attachment is obsolete: true
Attachment #8859351 - Flags: review?(cam)
Attachment #8859378 - Attachment is obsolete: true
Attachment #8859378 - Flags: review?(cam)
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+
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)
(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);
}
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: → 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...
(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 :)
(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.
Attachment #8858257 - Attachment is obsolete: true
Attachment #8861627 - Attachment is obsolete: true
Attachment #8861627 - Flags: review?(cam)
Attachment #8861628 - Attachment is obsolete: true
Attachment #8861628 - Flags: review?(cam)
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)
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 on attachment 8861266 [details]
Bug 1348481 Part 2: Gecko-side track unique IDs for each stylesheet and send them to Servo.

https://reviewboard.mozilla.org/r/133210/#review137972

::: layout/style/ServoStyleSet.cpp:52
(Diff revision 4)
>    mRawSet.reset(Servo_StyleSet_Init(aPresContext));
>  
>    mPresContext->DeviceContext()->InitFontCache();
>    gfxPlatformFontList::PlatformFontList()->InitLangService();
>  
> +  mUniqueIDCounter = 0;

We should only ever call Init on a given ServoStyleSet once, so we probably don't need to reset the counter here in addition to initializing it in the constructor.

::: layout/style/ServoStyleSet.cpp:712
(Diff revision 4)
>    RefPtr<StyleSheet> strong(aSheet);
>  
> -  nsTArray<RefPtr<ServoStyleSheet>>& sheetsArray = mSheets[SheetType::Doc];
> +  uint32_t oldUniqueID = RemoveSheetOfType(SheetType::Doc, aSheet);
>  
> -  sheetsArray.RemoveElement(aSheet);
> +  // Build an array of sheets so FindDocStyleSheetInsertionPoint can inspect it.
> +  nsTArray<RefPtr<ServoStyleSheet>> sheetsArray;

It would be nice if we didn't have to bump the reference counts on all the sheets just to search through the array.  You could make FindDocStyleSheetInsertionPoint take |const nsTArray<U>&| instead, which would allow us to pass in an nsTArray<ServoStyleSheet*> here.

(Looking at the way FindDocStyleSheetInsertionPoint is defined, you *could* actually add an |operator ServoStyleSheet*() const| to your Entry type, and then you could just pass mEntries[SheetType::Doc] in directly...)

::: layout/style/ServoStyleSet.cpp:713
(Diff revision 4)
>  
> -  nsTArray<RefPtr<ServoStyleSheet>>& sheetsArray = mSheets[SheetType::Doc];
> +  uint32_t oldUniqueID = RemoveSheetOfType(SheetType::Doc, aSheet);
>  
> -  sheetsArray.RemoveElement(aSheet);
> +  // Build an array of sheets so FindDocStyleSheetInsertionPoint can inspect it.
> +  nsTArray<RefPtr<ServoStyleSheet>> sheetsArray;
> +  for (auto& entry: mEntries[SheetType::Doc]) {

Nit: space before ":".
Attachment #8861266 - Flags: review?(cam) → review+
Comment on attachment 8861629 [details]
Bug 1348481 Part 3: Fix a dangerous typo in StyleSetHandleInlines (replaced one variable with another) and add assert to catch the problem it created.

https://reviewboard.mozilla.org/r/133618/#review137978
Attachment #8861629 - Flags: review?(cam) → review+
Comment on attachment 8861630 [details]
Bug 1348481 Part 4: Turn back on unexpected pass reftests.

https://reviewboard.mozilla.org/r/133620/#review137980

This will need to be updated to modify reftest.list, now that we've migrated back to it from reftest-stylo.list.
Attachment #8861630 - Flags: review?(cam) → review+
Comment on attachment 8861631 [details]
Bug 1348481 Part 5: Add new reftests to test servo stylesheet set integrity and cloning behavior.

https://reviewboard.mozilla.org/r/133622/#review137982

Some for the reftest-stylo.list changes here.

::: layout/reftests/bugs/1348481-1.html:5
(Diff revision 2)
> +<!DOCTYPE html>
> +<link id="target" rel="stylesheet" href="data:text/css,">
> +<link rel="stylesheet" href="data:text/css,div { color: green }">
> +<link rel="stylesheet" href="data:text/css,">
> +<div attr>This should be green</div>

Does the "attr" do something?  If not, just drop it.  (Here and in the other tests.)
Attachment #8861631 - Flags: review?(cam) → review+
Comment on attachment 8861632 [details]
Bug 1348481 Part 6: Adjust expected error counts for mochitests (generally greatly reduced).

https://reviewboard.mozilla.org/r/133624/#review137984

Great!  Thanks for fixing this!
Attachment #8861632 - Flags: review?(cam) → review+
Attachment #8863527 - Flags: review?(cam)
Comment on attachment 8863527 [details]
Bug 1348481 Part 1b: Generalize FindDocStyleSheetInsertionPoint so it doesn't require an array of RefPtrs.

https://reviewboard.mozilla.org/r/135296/#review138242
Attachment #8863527 - Flags: review?(cam) → review+
Flags: needinfo?(cam)
It seems to me that the unique id added here would be a necessary part for correctly implementing CSSOM support of @import rule. Adding dependency.
Blocks: 1352968
Attachment #8856914 - Attachment is obsolete: true
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aad0f4d1b858
Part 1b: Generalize FindDocStyleSheetInsertionPoint so it doesn't require an array of RefPtrs. r=heycam
https://hg.mozilla.org/integration/autoland/rev/161914658020
Part 2: Gecko-side track unique IDs for each stylesheet and send them to Servo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/66b66b26e20f
Part 3: Fix a dangerous typo in StyleSetHandleInlines (replaced one variable with another) and add assert to catch the problem it created. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a788db04cc2c
Part 4: Turn back on unexpected pass reftests. r=heycam
https://hg.mozilla.org/integration/autoland/rev/10eb2b938a36
Part 5: Add new reftests to test servo stylesheet set integrity and cloning behavior. r=heycam
https://hg.mozilla.org/integration/autoland/rev/8bb839a45259
Part 6: Adjust expected error counts for mochitests (generally greatly reduced). r=heycam
Depends on: 1363572
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: