Closed Bug 352219 Opened 14 years ago Closed 13 years ago

potential crash [@ nsDocument::AdoptNode] dereferencing oldDocument

Categories

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

x86
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: timeless)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity)

Attachments

(2 obsolete files)

 
Attached patch optimistic (obsolete) — Splinter Review
Assignee: general → timeless
Status: NEW → ASSIGNED
Attachment #238874 - Flags: review?(mats.palmgren)
Attached patch pesimistic (obsolete) — Splinter Review
Attachment #238875 - Flags: review?(mats.palmgren)
Comment on attachment 238875 [details] [diff] [review]
pesimistic

We would definitely need the pessimistic patch, or we would crash in
nsContentUtils::GetContextFromDocument():
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/base/src/nsContentUtils.cpp&rev=1.184&root=/cvsroot&mark=934#910
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/base/src/nsContentUtils.cpp&rev=1.184&root=/cvsroot&mark=861#859

I'm not sure we want to continue if |oldDocument| is null though -
won't nsNodeUtils::Adopt() think this is the "sameDocument" case
when the JS context/scopes are null?
I don't know this code well enough to say what should happen...

I'm guessing something like this would be the safest approach:

  nsIDocument *oldDocument = adoptedNode->GetOwnerDoc();
+ if (!oldDocument) {
+   BlastSubtreeToPieces(adoptedNode);
+   return NS_ERROR_xxx;
+ }
Attachment #238875 - Flags: review?(mats.palmgren) → review?(peterv)
Attachment #238874 - Attachment is obsolete: true
Attachment #238874 - Flags: review?(mats.palmgren)
Comment on attachment 238875 [details] [diff] [review]
pesimistic

nsNodeUtils::Adopt will work correctly (no old ownerDocument, so no need to reparent wrappers).
Attachment #238875 - Flags: superreview+
Attachment #238875 - Flags: review?(peterv)
Attachment #238875 - Flags: review+
Summary: hopefully useless null check in [@ nsDocument::AdoptNode] of oldDocument → potential crash [@ nsDocument::AdoptNode] dereferencing oldDocument
Comment on attachment 238875 [details] [diff] [review]
pesimistic

mozilla/content/base/src/nsDocument.cpp 	3.686
Attachment #238875 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.