1373484 - Stylesheet loaded from <link> element is always duplicated (temporarily!) when there is any CSSOM access to it

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+11]

From bug 1373193 comment 6:

nsDocument::PreloadStyle triggers a load to linked stylesheet before further construction, and when the corresponding link element is created, it triggers another CreateSheet, which finds the sheet from Loader::mLoadingDatas, and clone from it. At this moment, the inner has two stylesheets associated with, first by preload, and second by the link element.

After everything is loaded, the first stylesheet would probably be owned by Loader::mCompleteSheets, and the second one would be owned by the element. If there is any change to the stylesheet, we would clone the whole stylesheet, while the other stylesheet is not used in styling at all.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

bz, heycam, what do you think about this problem?

Comment 2

[Boris Zbarsky [:bzbarsky]]

> After everything is loaded, the first stylesheet would probably be owned
> by Loader::mCompleteSheets, and the second one would be owned by the element.

No, that shouldn't be what happens.  What should happen is that the second one is owned by mCompleteSheets and the element, while the first one gets dropped completely.

Specifically, see the comment and code at http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/layout/style/Loader.cpp#1936-1948 which was added to fix this precise problem in bug 799816.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

OK, that explains what I saw in bug 1373193 comment 4. So apparently it doesn't work as expected, the first one may still not be released at the moment when script has a chance to run.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Well, right, it's cycle collected.  So it won't get released until the CC runs and whatnot.

I agree that's moderately annoying in terms of us ending up making a copy of data no one really cares about.  I guess we could try harder to disconnect the preload from the inner or something to address that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible fix

idorn, could you check whether this fixes the problem you were seeing?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Olli, are we guaranteed that we won't get calls to our CC Traverse/Unlink after LastRelease?  Or do we need to protect against that?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: With the "use mInner from subclass destructors" thing fixed

Comment 8

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

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #6)
> Olli, are we guaranteed that we won't get calls to our CC Traverse/Unlink
> after LastRelease?  Or do we need to protect against that?

If all unlinking (and Disconnect() usage for weak/raw pointers) was perfect, then it would be guaranteed. But that is not the case in general, unfortunately.
And snowwhite killer explicitly supports the case when refcnt of an object increases from 0.

However if we know all the edges (including weak ones) pointing to this particular object are cut, then
it should be fine. But normally I'd add some null checks.

Comment 9

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

Does anything keep raw or weak references to StyleSheet? Since those are the suspicious ones which might bring a snowwhite object back to life.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Yes, there are lots of raw references to StyleSheet, from objects it owns, including rules, child stylesheets, media list, and rule list.

Rules and child stylesheets are probably fine as Inner should guarantee reparent them when we drop ourselves from it.

Media list and rule list are real problems. If we don't drop their reference, there is a chance that they may bring StyleSheet back to life, since script can hold them.

That says, we probably really need to drop everything StyleSheet owns. That also means we would need a virtual function for it.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1373484 - Drop all reference to StyleSheet when last strong reference is dropped.

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

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

This is basically moving everything from dtor into LastRelease. We should be safe now, otherwise we would already have had dangling pointers somewhere...

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8878947 [details]
Bug 1373484 - Drop all reference to StyleSheet when last strong reference is dropped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150206/diff/1-2/

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8878947 [details]
Bug 1373484 - Drop all reference to StyleSheet when last strong reference is dropped.

https://reviewboard.mozilla.org/r/150206/#review155438

Thsnk your for picking this up!

::: layout/style/CSSStyleSheet.cpp:391
(Diff revision 2)
>    // XXX The document reference is not reference counted and should
>    // not be released. The document will let us know when it is going
>    // away.
>    if (mRuleProcessors) {
>      NS_ASSERTION(mRuleProcessors->Length() == 0, "destructing sheet with rule processor reference");
>      delete mRuleProcessors; // weak refs, should be empty here anyway

Probably want to null it out too, since this is not the destructor...

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8878947 [details]
Bug 1373484 - Drop all reference to StyleSheet when last strong reference is dropped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150206/diff/2-3/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92673a7e3d1f
Drop all reference to StyleSheet when last strong reference is dropped. r=bz

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92673a7e3d1f