680687 - Crash [@ nsSVGSwitchElement::FindActiveChild] after GC

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires extension for GC) (crashes when loaded)

1. Install https://www.squarefree.com/extensions/domFuzzLite2.xpi
2. Load the testcase.

Result:
  Debug: Crash [@ nsSVGSwitchElement::FindActiveChild] calling 0x0 ?
  Opt:   Crash [@ nsNodeUtils::ContentRemoved]         calling bogus ?

The GC pattern makes me wonder if this is related to bug 335998 being fixed.

Comment 1

[Jesse Ruderman]

Attachment: stack trace (mac, debug, gdb)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Um, nsSVGSwitchElement overrides InsertChildAt/RemoveChildAt

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Follow the COM rules

The changes shouldn't be in hot paths, and this is the right thing to do.
Yet, it is unfortunate to add new Addref/releases.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

...so I patched all the cases I found where similar problem could occur,
not only nsGenericElement.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

And note, in parser eTreeOpAppendChildrenToNewParent
it is really the node which needs to be strong.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/071d9c997f3d

Comment 7

[Johnny Stenback (:jst)]

Olli, this is something we could take in 8, right?

Comment 8

[Daniel Veditz [:dveditz]]

I could not reproduce the crash using the testcase on Firefox 8 (beta), is it possible this was introduced by something in Firefox 9 rather than bug 335998 as Jesse guessed? Didn't see any script errors running the testcase so I think I ran it correctly, but I'd be happier if Jesse concurred. It's also possible something else that landed on both Fx9 and 8 masked/fixed this, or something that landed earlier on Fx9 that un-masked the underlying problem.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

The patch for bug 335998 is not in FF8.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I don't see any reason to take this to FF8.

Comment 11

[Johnny Stenback (:jst)]

Excellent, thanks Olli!

Comment 12

[Marcia Knous [:marcia]]

Verified fixed using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0a1) Gecko/20111122 Firefox/11.0a1. I verified by following the steps in Comment 0.

Comment 13

[u279076]

(In reply to Marcia Knous [:marcia] from comment #12)
> Verified fixed using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0a1)
> Gecko/20111122 Firefox/11.0a1. I verified by following the steps in Comment
> 0.

Thanks Marcia. If you have time, could you please also verify on Firefox 9 and 10?

Comment 14

[Marcia Knous [:marcia]]

Verified fixed using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101 Firefox/9.0. The extension is not compatible with Aurora, so I am not sure if forcing compat would be a fair test.