bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

stylo: Audit cycle-collector handling

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: smaug, Assigned: bradwerth)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

bradwerth was debugging a stylo leak which happened when one added an expando to a stylesheet, and a worrisome place in code is
http://searchfox.org/mozilla-central/rev/3efd1caff252061946f0773df68bdd50de5eb756/layout/style/Loader.cpp#2656-2658

This hints that CC handling in Stylo could use some auditing.
Summary: Audit Stylo's CC handling → stylo: Audit cycle-collector handling
(note that the stylo: prefix is what gets things on the radar, moreso than bug dependencies).
We should audit all the IsGecko/IsServo/GetAsGecko/GetAsServo calls, pretty much, see http://searchfox.org/mozilla-central/rev/3b4f95ac5946b639827358d93b5258d837a5ddde/dom/base/nsDocument.cpp#10113, for an example that could/should be fixed now too.

At a glance there aren't that many, which is good.
(Assignee)

Comment 3

a year ago
I'll go through the IsGecko/IsServo/GetAsGecko/GetAsServo calls and remove them where possible.
Assignee: nobody → bwerth
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Priority: -- → P2
(Assignee)

Updated

a year ago
Attachment #8874488 - Flags: review?(emilio+bugs)
Attachment #8874489 - Flags: review?(emilio+bugs)
Attachment #8874490 - Flags: review?(emilio+bugs)

Comment 7

a year ago
mozreview-review
Comment on attachment 8874489 [details]
Bug 1369903 Part 2: Allow HTMLEditor to enable/disable any type of stylesheet.

https://reviewboard.mozilla.org/r/145860/#review150052

This is fine, but we should make sure that disabled sheets work properly in stylo. I think they do because how they affect the StyleSet, but we do have a boolean in servo's stylesheet that we definitely check. I think that may be false always right now, but worth documenting it, getting rid of it.
Attachment #8874489 - Flags: review?(emilio+bugs) → review+

Comment 8

a year ago
mozreview-review
Comment on attachment 8874490 [details]
Bug 1369903 Part 3: Ensure that Loader enables all cloned stylesheets, and checks modified state of all stylesheets, regardless of backend.

https://reviewboard.mozilla.org/r/145862/#review150054
Attachment #8874490 - Flags: review?(emilio+bugs) → review+

Comment 9

a year ago
mozreview-review
Comment on attachment 8874488 [details]
Bug 1369903 Part 1: Remove over-specified stylesheet calls in nsDocument.

https://reviewboard.mozilla.org/r/145858/#review150056

So, these ones are for printing, aren't there any tests that fail without this?
Attachment #8874488 - Flags: review?(emilio+bugs) → review+

Comment 10

a year ago
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea5ac39685cd
Part 1: Remove over-specified stylesheet calls in nsDocument. r=emilio
https://hg.mozilla.org/integration/autoland/rev/66a0bce9a299
Part 2: Allow HTMLEditor to enable/disable any type of stylesheet. r=emilio
https://hg.mozilla.org/integration/autoland/rev/42669f4e317b
Part 3: Ensure that Loader enables all cloned stylesheets, and checks modified state of all stylesheets, regardless of backend. r=emilio

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea5ac39685cd
https://hg.mozilla.org/mozilla-central/rev/66a0bce9a299
https://hg.mozilla.org/mozilla-central/rev/42669f4e317b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.