If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[FIXr]Clone() method on nsCSSStyleSheet doesn't null out some stuff

VERIFIED FIXED in mozilla1.3final

Status

()

Core
CSS Parsing and Computation
P1
critical
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({testcase})

Trunk
mozilla1.3final
x86
Linux
testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.3 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed1.3)

Attachments

(4 attachments, 2 obsolete attachments)

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.
Created attachment 98369 [details]
phantom ownerNode on @import-ed sheet
Created attachment 98370 [details]
parentStyleSheet and ownerRule on a top-level sheet.

Updated

15 years ago
Keywords: testcase
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.
Blocks: 183348
Blocks: 192990
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
Attachment #114246 - Attachment is obsolete: true
Severity: normal → critical
Priority: -- → P1
Summary: Clone() method on nsCSSStyleSheet doesn't null out some stuff → [FIX]Clone() method on nsCSSStyleSheet doesn't null out some stuff
Target Milestone: --- → mozilla1.3final
Comment on attachment 114247 [details] [diff] [review]
oops, fix an obvious typo

Well, this works.... should fix the crashes and all.
Attachment #114247 - Flags: superreview?(dbaron)
Attachment #114247 - Flags: review?(dbaron)
Flags: blocking1.3?

Comment 8

15 years ago
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. 
Flags: blocking1.3? → blocking1.3-
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
Attachment #114247 - Attachment is obsolete: true
Attachment #114247 - Flags: superreview?(dbaron)
Attachment #114247 - Flags: review?(dbaron)
Attachment #115273 - Flags: superreview?(dbaron)
Attachment #115273 - Flags: review?(dbaron)
Attachment #115273 - Flags: superreview?(dbaron)
Attachment #115273 - Flags: superreview+
Attachment #115273 - Flags: review?(dbaron)
Attachment #115273 - Flags: review+
Comment on attachment 115273 [details] [diff] [review]
so more like this

looking for approval for 1.3 -- this should fix a topcrash....
Attachment #115273 - Flags: approval1.3?
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.  :(
Summary: [FIX]Clone() method on nsCSSStyleSheet doesn't null out some stuff → [FIXr]Clone() method on nsCSSStyleSheet doesn't null out some stuff

Comment 19

15 years ago
Comment on attachment 115273 [details] [diff] [review]
so more like this

a=asa (on behalf of drivers) for checkin to 1.3
Attachment #115273 - Flags: approval1.3? → approval1.3+
Checked in for 1.3 (both the patch I attached and the one David attached).
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

15 years ago
Whiteboard: fixed1.3

Comment 21

15 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.