Closed Bug 1373484 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
bz, heycam, what do you think about this problem?
Flags: needinfo?(cam)
Flags: needinfo?(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.
Flags: needinfo?(cam)
Flags: needinfo?(bzbarsky)
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.
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.
Attached patch Possible fix (obsolete) — Splinter Review
idorn, could you check whether this fixes the problem you were seeing?
Flags: needinfo?(xidorn+moz)
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?
Flags: needinfo?(bugs)
Attachment #8878305 - Attachment is obsolete: true
Flags: needinfo?(xidorn+moz)
Summary: Stylesheet loaded from <link> element is always duplicated when there is any CSSOM access to it → Stylesheet loaded from <link> element is always duplicated (temporarily!) when there is any CSSOM access to it
(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.
Flags: needinfo?(bugs)
Does anything keep raw or weak references to StyleSheet? Since those are the suspicious ones which might bring a snowwhite object back to life.
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.
This is basically moving everything from dtor into LastRelease. We should be safe now, otherwise we would already have had dangling pointers somewhere...
Blocks: 1359217
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...
Attachment #8878947 - Flags: review?(bzbarsky) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92673a7e3d1f
Drop all reference to StyleSheet when last strong reference is dropped. r=bz
Assignee: nobody → xidorn+moz
https://hg.mozilla.org/mozilla-central/rev/92673a7e3d1f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.