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

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: phantom ownerNode on @import-ed sheet

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: parentStyleSheet and ownerRule on a top-level sheet.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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....

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: something like this

This has not been built or tested yet; I'll try to do that tonight.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: oops, fix an obvious typo

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114247 [details] [diff] [review]
oops, fix an obvious typo

Well, this works.... should fix the crashes and all.

Comment 8

[Asa Dotzler [:asa]]

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. 

Comment 9

[Boris Zbarsky [:bzbarsky]]

Note that this is a trunk topcrash.... but it's fine with me either way.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: so more like this

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

looking for approval for 1.3 -- this should fix a topcrash....

Comment 15

[Boris Zbarsky [:bzbarsky]]

fix checked in for 1.4a; leaving open pending 1.3 approval.

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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?).

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: additional patch

I just checked this in to fix the bustage.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 115293 [details] [diff] [review]
additional patch

Yeah... I had exactly this in my tree and forgot to diff the file.  :(

Comment 19

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to 1.3

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.3 (both the patch I attached and the one David attached).

Comment 21

[Madhur Bhatia]

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