587 bytes, text/html
708 bytes, text/html
8.57 KB, patch
|Details | Diff | Splinter Review|
1.03 KB, patch
|Details | Diff | Splinter Review|
The Clone() method on nsCSSStyleSheet doesn't null some stuff out and then the CSSLoader doesn't null it out either. Best-case this can lead to incorrect DOM property values, worst case to crashes (since the pointers in question are weak refs). Testcases coming up.
Some quick notes to go with this: 1) CSSStyleSheetImpl::Clone() clones the child sheets. It also clones the @import rules, which clone their child sheets. We need to do one or the other but not both. 2) 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). Item #1 is a general problem with EnsureUniqueInner -- the result is a sheet whose child sheet list is out of sync with the @import rules. Sounds like the first step here is deciding what Clone() should do exactly, as well as what the copy constructor should do exactly and how they are different....
I've split off the child sheet issues into bug 183348. This bug is basically about ownerNode, parentStyleSheet, ownerRule, and mDocument (we clone _that_ too!) and deciding what this code should really be doing.
Created attachment 114246 [details] [diff] [review] something like this This has not been built or tested yet; I'll try to do that tonight.
Created attachment 114247 [details] [diff] [review] oops, fix an obvious typo
Comment on attachment 114247 [details] [diff] [review] oops, fix an obvious typo Well, this works.... should fix the crashes and all.
This looks small and safe but without review traction. Drivers will consider taking a fully reviewed patch for 1.3 but we're not going to block for this.
Note that this is a trunk topcrash.... but it's fine with me either way.
I find it rather amusing that this constructor is below a comment: // These are not supported and are not implemented! It looks like the operator= actually isn't implemented, which is good, but the comment should be fixed. It's also good that it's private. However, doing this the way this patch does seems like it could hide things in a rather evil way. Perhaps a better solution would be to have additional parameters to Clone, so the caller can at least have to pass in the nulls explicitly?
Hmm... yeah, I could do that (just a boolean arg to Clone()). But I can't think of a single use case where we'd want to have a sheet that has weak refs to things that have no ref to the sheet and so can't tell it they're going away..... If you really feel that there may in the future be Clone() consumers who will want that behavior, I'll add a boolean.
But aren't there likely to be Clone consumers who want to set one of these things you're setting to null to correct values?
Created attachment 115273 [details] [diff] [review] so more like this
Comment on attachment 115273 [details] [diff] [review] so more like this looking for approval for 1.3 -- this should fix a topcrash....
fix checked in for 1.4a; leaving open pending 1.3 approval.
This actually broke the build, in nsCSSRules.cpp, where you need changes to CSSImportRule (can you pass in a different one of the arguments as non-null this time?).
Created attachment 115293 [details] [diff] [review] additional patch I just checked this in to fix the bustage.
Comment on attachment 115293 [details] [diff] [review] additional patch Yeah... I had exactly this in my tree and forgot to diff the file. :(
Comment on attachment 115273 [details] [diff] [review] so more like this a=asa (on behalf of drivers) for checkin to 1.3
Checked in for 1.3 (both the patch I attached and the one David attached).
based on the testcases attachment 98369 [details] and attachment 98370 [details], Verifying the bug on linux and winXO platform witht he 03-31-2003 trunk builds