448993 - Crash [@ nsRange::CutContents] using deleteContents after extractContents and GC

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded with privs)

I've been hitting this crash a lot during automated testing.

Comment 1

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

The fix for bug 335998 would fix this one too, but let's see if I find some
temporary fix for the crash.

Comment 2

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

So bug 335998 changed us to cut nodes, which means that just
before CC b's parentNode is c, which parentNode is a document fragment, which 
isn't kept alive.
Before bug 335998, c's parent is null.

Keeping the document fragment alive by changing
r.extractContents();
to
var df = r.extractContents();
prevents the crash.

Comment 3

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: proposed patch for now

Comment 4

[Jesse Ruderman]

I assume you meant to refer to bug 332148 in comment 2.

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: er, this one, fix for now

Comment 6

[Alex Vincent [:WeirdAl]]

Whoops.

I know the point was made in bug 332148 about making mochitests instead of xpcshell tests.  Do we plan on adding this testcase as a mochitest and granting privileges, or should we perhaps use a xpcshell test and just call gc()?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Seems bad that IsDetached() can return false but mRoot be null. Should we also set mIsDetached in ::NodeWillBeDestroyed?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Or are there more cases when we are detached with a null mRoot so we need to check for that anyway?

Comment 9

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

I don't think we want to mark range detached, because that would prevent using
a valid range after mRoot has been deleted.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But you can't really use the range if the root goes away anyway since lots of the nodes between the start and end point might have gone away (at which point the endpoints would no longer be connected).

Should we make ranges hold a strong reference to the root? (effectively we'd do that anyway once we make nodes hold strong reference to their parent)

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 332173 [details] [diff] [review]
er, this one, fix for now

Resetting request, i think we'll want to detach or hold a strong reference to the root.

Comment 12

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Ok, I'll make the mRoot strong, after bug 463410.

Comment 13

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: Strong mRoot

Comment 14

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: +missing file

Comment 15

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: mochitest

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 349163 [details] [diff] [review]
+missing file

a191=beltzner

Comment 17

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

This needs approval for Bug 463410 too.

Comment 18

[Johnny Stenback (:jst)]

The patch here has approval, so should be ready to land. Not blocking...

Comment 19

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

fixed on trunk and 1.9.1

Comment 20

[Tony Chung [:tchung]]

(In reply to comment #19)
> fixed on trunk and 1.9.1

Please include links to changesets to trunk and 1.9.1 fixes in the comments next time.  Thanks.

Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090310 Minefield/3.2a1pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090310 Shiretoko/3.1b4pre Ubiquity/0.1.5