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

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

VERIFIED FIXED in mozilla1.9.1b3

Status

()

Core
DOM: Core & HTML
--
critical
VERIFIED FIXED
9 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: smaug)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla1.9.1b3
x86
Mac OS X
crash, regression, testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 332165 [details]
testcase (crashes Firefox when loaded with privs)

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

Updated

9 years ago
Assignee: nobody → Olli.Pettay
(Assignee)

Comment 1

9 years ago
The fix for bug 335998 would fix this one too, but let's see if I find some
temporary fix for the crash.
Depends on: 335998
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
Created attachment 332171 [details] [diff] [review]
proposed patch for now
Attachment #332171 - Flags: superreview?(jonas)
Attachment #332171 - Flags: review?(jonas)
(Assignee)

Updated

9 years ago
Attachment #332171 - Flags: superreview?(jonas)
Attachment #332171 - Flags: review?(jonas)
(Reporter)

Comment 4

9 years ago
I assume you meant to refer to bug 332148 in comment 2.
Blocks: 332148
(Assignee)

Comment 5

9 years ago
Created attachment 332173 [details] [diff] [review]
er, this one, fix for now
Attachment #332171 - Attachment is obsolete: true
Attachment #332173 - Flags: superreview?(jonas)
Attachment #332173 - Flags: review?(jonas)
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()?
Seems bad that IsDetached() can return false but mRoot be null. Should we also set mIsDetached in ::NodeWillBeDestroyed?
Or are there more cases when we are detached with a null mRoot so we need to check for that anyway?
(Assignee)

Comment 9

9 years ago
I don't think we want to mark range detached, because that would prevent using
a valid range after mRoot has been deleted.
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)
Attachment #332173 - Flags: superreview?(jonas)
Attachment #332173 - Flags: review?(jonas)
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.
(Assignee)

Comment 12

9 years ago
Ok, I'll make the mRoot strong, after bug 463410.
Depends on: 463410
(Assignee)

Updated

9 years ago
Flags: blocking1.9.1?
(Assignee)

Comment 13

9 years ago
Created attachment 349162 [details] [diff] [review]
Strong mRoot
Attachment #349162 - Flags: superreview?(jonas)
Attachment #349162 - Flags: review?(jonas)
(Assignee)

Comment 14

9 years ago
Created attachment 349163 [details] [diff] [review]
+missing file
Attachment #349163 - Flags: superreview?(jonas)
Attachment #349163 - Flags: review?(jonas)
(Assignee)

Updated

9 years ago
Attachment #349162 - Attachment is obsolete: true
Attachment #349162 - Flags: superreview?(jonas)
Attachment #349162 - Flags: review?(jonas)
Attachment #349163 - Flags: superreview?(jonas)
Attachment #349163 - Flags: superreview+
Attachment #349163 - Flags: review?(jonas)
Attachment #349163 - Flags: review+
(Assignee)

Updated

9 years ago
Attachment #349163 - Flags: approval1.9.1?
Flags: in-testsuite?
(Assignee)

Comment 15

9 years ago
Created attachment 349268 [details] [diff] [review]
mochitest
Attachment #349163 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 349163 [details] [diff] [review]
+missing file

a191=beltzner
(Assignee)

Comment 17

9 years ago
This needs approval for Bug 463410 too.
The patch here has approval, so should be ready to land. Not blocking...
Flags: blocking1.9.1? → blocking1.9.1-
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs-1.9.1-landing]
(Assignee)

Comment 19

9 years ago
fixed on trunk and 1.9.1
Whiteboard: [needs-1.9.1-landing]
Flags: in-testsuite? → in-testsuite+
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1b3
Depends on: 471126
(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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Depends on: 483981
Crash Signature: [@ nsRange::CutContents]
Component: DOM: Traversal-Range → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.