The problem is that we clone the child sheets in CSSStyleSheetImpl::CSSStyleSheetImpl and also clone them if we ever create a unique inner (CSSImportRuleImpl::Clone will end up cloning the child sheet). Seems wasteful to me.... Not to mention that editing a sheet you got to through an @import rule will have no effect since the sheets in the cascade are the child sheets of the parent sheet.... Not quite sure what the right thing is to do here, since we lazily clone @import rules (like the whole inner), but want to eagerly clone the child sheets, apparently. Or do we?

Also, CSSImportRuleImpl::Clone() fucks up the media. It appends the media list as a single element! Instead it should use AppendElements (or better yet not use an nsISupportsArray).

And if the Clone() happens _before_ we have our child sheets parsed, some of the clones simply won't see them. See bug 290018. I'm starting to think that the child sheets should just live on the inner; that way we'll get things working right more or less for free, I think. Also, it looks like the media list thing got fixed at some point by dbaron's media list changes...

This isn't perfect (see XXX comments), but it's about as much change of this stuff as I feel we should do on branch and without changing interfaces.

Could you briefly describe what you're changing here, and why?

Yeah. The current setup (before this patch) is that each nsCSSStyleSheet has a list of child nsCSSStyleSheets that it stores. The way this list is built is that when the CSS parser sees an @import rule it tells the CSS loader to load the child sheet and passes in the URI, the rule, and the parent sheet (the nsICSSStyleSheet the parser thinks it's parsing). The CSS loader then creates a new nsCSSStyleSheet, sets it as the @import rule's sheet, and inserts it in the parent sheet. So far so good. The problems start when we clone the parent sheet (for web content because it's loaded in more than one place on the page). Cloning clones all the rules in the CSSStyleSheetInner, and in particular the @import rules. Cloning an @import rule clones its child sheet and sets the clone child sheet in the clone @import rule. Then the parent nsCSSStyleSheet _also_ explicitly clones its list of child sheets and sets those as the child sheets of its own clone. So where before we had: Sheet A contains @import rule R which points to sheet B and has sheet B in the child list. We now have the same thing, plus: Sheet A' contains @import rule R' which points to sheet B' and has sheet B'' in the child list. That's bad enough on its own, but even worse is the fact that if the cloning happens _before_ the @import rule has been parsed, the child sheet will only get inserted in one of the clones by the CSSLoader and the other one won't even know that the child exists. We used to not really hit this except with alternate stylesheets due to parser-blocking, but since we no longer block the parser on stylesheets we might be cloning before we've gotten any data back from the server and so before we parsed the @import rules. So what this patch changes is that instead of storing the child sheets in the nsCSSStyleSheet we store them in the CSSStyleSheetInner. This makes cloning before parsing work, since all the clones share the same inner (we don't allow operations that would EnsureUniqueInner before parsing is done, since that would screw up the list of rules too). That solves most of the issues that were reported. The other change is that when cloning we no longer explicitly clone our child sheets. We just clone the rules list, then walk the @import rules and rebuild our child sheet list, so we're always pointing to the same sheets that our @import rules are pointing to.

(In reply to comment #5) > Cloning > clones all the rules in the CSSStyleSheetInner, and in particular the @import > rules. This only happens if the sheet has been accessed through the CSSOM, right?

The cloning of the @import rules, yes. But the part about what happens if we clone before the @import rule has been parsed requires no CSSOM access. So there are two issues here, really: 1) Cloning a sheet before we parse all the @import rules gives broken child sheet lists. 2) EnsureUniqueInner on a sheet with @import rules completely screws up. The patch fixes both.

Comment on attachment 335550 [details] [diff] [review] Fix Did you consider the possibility of having the inners have a list of child inners, but not child sheets? I'm not sure how hard that would be. Anyway, this is fine. >+static PRBool >+RebuildChildList(nsICSSRule* aRule, void* aBuilder) >+{ >+ PRInt32 type; >+ aRule->GetType(type); >+ if (type == nsICSSRule::CHARSET_RULE) { >+ return PR_TRUE; >+ } >+ >+ if (type != nsICSSRule::IMPORT_RULE) { >+ return PR_FALSE; >+ } Please add a comment in the definition of enum nsCSSSection in nsCSSParser.cpp pointing out the dependency. (This also presumes, I suppose, that there aren't comments in the rule list. It might be better to stop enumeration (return false) only when hitting a style rule, media rule, or namespace rule.) >+ (*builder->sheetSlot) = >+ static_cast<nsCSSStyleSheet*>(static_cast<nsICSSStyleSheet*>(cssSheet.get())); I think the inner cast and the get() are redundant (at least now that nsDerivedSafe is gone). > nsCSSStyleSheetInner::nsCSSStyleSheetInner(nsCSSStyleSheetInner& aCopy, >- nsICSSStyleSheet* aParentSheet) >+ nsCSSStyleSheet* aParentSheet) I wonder if the various aParentSheet arguments to methods on nsCSSStyleSheetInner would be better called aPrimarySheet. (I was getting a little confused.) In nsCSSStyleSheet::AppendStyleSheet(nsICSSStyleSheet* aSheet): >- if (! mFirstChild) { >- mFirstChild = sheet; >+ if (! mInner->mFirstChild) { >+ mInner->mFirstChild = sheet; > } > else { >- nsCSSStyleSheet* child = mFirstChild; >+ nsCSSStyleSheet* child = mInner->mFirstChild; > while (child->mNext) { > child = child->mNext; > } > child->mNext = sheet; > } This if/else could be simplified to: nsRefPtr<nsCSSStyleSheet>* tail = &mInner->mFirstChild; while (*tail) { tail = &(*tail)->mNext; } *tail = sheet; (and somewhat likewise for InsertStyleSheetAt) r+sr=dbaron

Pushed changeset ec0a460fbe1a. The regression tests are flagged on the dependent bugs.

You might want to change the aParentSheet argument names in the header to match the .cpp.

Good catch. Done.

*tail - sheet; didn't work right, as expected. Should be =.

We should fix this on branch too, since the regressions in 1.9 are biting people here.

Comment on attachment 340070 [details] [diff] [review] Roll-up 1.9 branch patch What problems are we solving that people can't wait until 3.1 to get fixed? Are there unstated security concerns here? This really doesn't appear to meet our criteria for branch update releases. If you feel strongly about this bug then it makes a good test case for our new appeal process.

Comment on attachment 340070 [details] [diff] [review] Roll-up 1.9 branch patch > What problems are we solving that people can't wait until 3.1 to get fixed? First and foremost, the fact that sites that worked in Gecko 1.8 broke in Gecko 1.9. I feel pretty strongly that we shouldn't have shipped with that regression to start with; sadly the regression bugs were not filed until after ship. I would have argued for blocker status for them before ship. > Are there unstated security concerns here? I don't _think_ so. The old setup is pretty broken and hard to reason about but I think it avoids dangling pointers, which is the one security, as opposed to actually rendering websites or something, concern I can see here. If our new rules are that correctness regression fixes that are breaking sites but not everyone are not branch-worthy, then I guess this should re-minused... But I must have missed the .planning discussion about that, then. :(

Comment on attachment 340070 [details] [diff] [review] Roll-up 1.9 branch patch Given bug 454578 we should probably hold off on this on branch (and more likely not take it at all).

Apparently VC7.1 has problems casting an nsCSSStyleSheet* to a const nsICSSStyleSheet* ... I thought that a cast would look too ugly.

Comment on attachment 360459 [details] [diff] [review] VC7.1 fix ok, but sane context would be nice next time...

(In reply to comment #19) > (From update of attachment 360459 [details] [diff] [review]) > ok, but sane context would be nice next time... Sorry, I don't know what happened there; my history shows me using -U13 ...

Comment on attachment 360459 [details] [diff] [review] VC7.1 fix Pushed changeset 8f00a4742991 to mozilla-central.