The default bug view has changed. See this FAQ.

stylo: Stylesheet cloning doesn't seem to work right

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
5 days ago
2 days ago

People

(Reporter: bz, Assigned: bradwerth)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox55 affected)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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

2 days ago
See Also: → bug 1339629
You need to log in before you can comment on or make changes to this bug.