1369903 - stylo: Audit cycle-collector handling

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Bobby Holley (:bholley)]

(note that the stylo: prefix is what gets things on the radar, moreso than bug dependencies).

Comment 2

[Emilio Cobos Álvarez [:emilio]]

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.

Comment 3

[Brad Werth [:bradwerth]]

I'll go through the IsGecko/IsServo/GetAsGecko/GetAsServo calls and remove them where possible.

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1369903 Part 1: Remove over-specified stylesheet calls in nsDocument.

Review commit: https://reviewboard.mozilla.org/r/145858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145858/

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1369903 Part 2: Allow HTMLEditor to enable/disable any type of stylesheet.

Review commit: https://reviewboard.mozilla.org/r/145860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145860/

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1369903 Part 3: Ensure that Loader enables all cloned stylesheets, and checks modified state of all stylesheets, regardless of backend.

Review commit: https://reviewboard.mozilla.org/r/145862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145862/

Comment 7

[Emilio Cobos Álvarez [:emilio]]

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.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

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

Comment 9

[Emilio Cobos Álvarez [:emilio]]

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?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ea5ac39685cd
https://hg.mozilla.org/mozilla-central/rev/66a0bce9a299
https://hg.mozilla.org/mozilla-central/rev/42669f4e317b