Closed Bug 535041 Opened 12 years ago Closed 12 years ago

Crash [@ nsContentUtils::IsInSameAnonymousTree]

Categories

(Core :: DOM: Editor, defect, P1)

x86
All
defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- alpha1+

People

(Reporter: jruderman, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(4 keywords)

Crash Data

Attachments

(3 files, 3 obsolete files)

Attached file testcase
###!!! ASSERTION: Must have a content to work with: 'aContent', file /Users/jruderman/mozilla-central/content/base/src/nsContentUtils.cpp, line 3983

Followed by a null deref in nsContentUtils::IsInSameAnonymousTree
Attached file stack trace
In nsINode::GetSelectionRootContent, editorRoot is null.  So this crash was introduced by the patch in bug 42676.
Blocks: 42676
blocking2.0: --- → ?
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attached patch Patch v1.0 (obsolete) — Splinter Review
I think that nsIEditor::GetRootElement should use document root element if there is no body element.
Attachment #417868 - Flags: review?(Olli.Pettay)
for reference: the crash on Windows XP: bp-e1cff2d5-643d-41a4-b893-63b872091215
OS: Mac OS X → All
Attachment #417868 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 417868 [details] [diff] [review]
Patch v1.0

> static nsIContent* GetEditorRootContent(nsIEditor* aEditor)
> {
>   nsCOMPtr<nsIDOMElement> rootElement;
>-  aEditor->GetRootElement(getter_AddRefs(rootElement));
>+  nsresult rv = aEditor->GetRootElement(getter_AddRefs(rootElement));
>+  NS_ENSURE_SUCCESS(rv, nsnull);
>   nsCOMPtr<nsIContent> rootContent(do_QueryInterface(rootElement));
>   return rootContent;
> }
No need for this change, do_QueryInterface is null-safe.

>-  nsCOMPtr<nsIDOMHTMLDocument> doc = do_QueryReferent(mDocWeak);
>-  if (!doc) return NS_ERROR_NOT_INITIALIZED;
...
>+  nsCOMPtr<nsIDOMHTMLDocument> htmlDoc = do_QueryReferent(mDocWeak);
>+  if (htmlDoc) {
>+    // Use the HTML documents body element as the editor root if we didn't
>+    // get a root element during initialization.
>+
>+    nsCOMPtr<nsIDOMHTMLElement> bodyElement; 
>+    nsresult rv = htmlDoc->GetBody(getter_AddRefs(bodyElement));
>+    if (NS_SUCCEEDED(rv) && bodyElement)
>+    {
>+      mRootElement = bodyElement;
>+    }
>+  }
>+
>+  // If the document isn't HTML's or there is no HTML body element,
>+  // we should use the document root element instead.
Since you're adding support for non-HTML documents, could you add a testcase for those? Would XML document which has XHTML contenteditable elements work?
Or, safer option would be to still return early if the document isn't HTML document.
Either way, testing or requiring HTML document.
ok, I confirmed that HTML editor in XML document isn't editable. So, I'll change it to early return style.
Attached patch Patch v1.1 (obsolete) — Splinter Review
I'll push this patch.
Attachment #417868 - Attachment is obsolete: true
Attachment #417971 - Flags: review+
Attached patch Patch v1.2 (obsolete) — Splinter Review
This is better.
Attachment #417971 - Attachment is obsolete: true
Attachment #417972 - Flags: review+
You don't need the first nsresult rv variable
Attached patch Patch v1.2.1Splinter Review
thanks.
Attachment #417972 - Attachment is obsolete: true
Attachment #418099 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/b20cff5e2157
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
blocking2.0: ? → alpha1
Priority: -- → P1
Depends on: 545775
This breaks at least some websites.  See bug 545775.
Crash Signature: [@ nsContentUtils::IsInSameAnonymousTree]
You need to log in before you can comment on or make changes to this bug.