336381 - Crash [@ nsRange::InsertNode] involving ranges in detached nodes

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

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

Reread the spec, will write a fix tomorrow :)

Comment 3

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

Attachment: proposed patch

Forcing boundary points to be under document, document fragment or attr.
So in normal cases checking that the element or text node which is used as a 
boundary point must be in document.
(I think there are still more some similar problems in nsRange, but this is 
enough for this bug.)

The change to nsTextControlFrame is needed because otherwise nsEditor would try
to create a range while the anonymous content isn't yet bound to a document.

Comment 4

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

I hope nothing relies on the old (wrong) behaviour where ranges can be 
created even using nodes which aren't in document (or in doc fragments or in attrs).

Comment 5

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

Comment on attachment 221294 [details] [diff] [review]
proposed patch


>+
>+  // DOM 2 Range specification doesn't define the error code for this case,
>+  // but this is the best one we have.
>+  return NS_ERROR_DOM_RANGE_BAD_BOUNDARYPOINTS_ERR;
>+}

Opera seems to use the same error code.

Comment 6

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

Attachment: Additional patch for nsEditor

This removes assertions which editor creates in some cases.
And anyway it looks right to me not to update selection when some nodes
aren't in the document. Not sure who could review this...

Comment 7

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

This patch is likely to break at least editor since it tends to do whacky things to ranges. Not to say that it's not the right thing to do, but are you prepared to hunt down editor regressions as (if) they pop up?

Comment 8

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

(In reply to comment #7)
> This patch is likely to break at least editor since it tends to do whacky
> things to ranges. Not to say that it's not the right thing to do, but are you
> prepared to hunt down editor regressions as (if) they pop up?
> 

Yes :) We must fix ranges, and if someone (who?) is going to (almost) rewrite
them, editor needs to be fixed anyway.
Fortunately that warning I'm adding should catch the problems...

I don't have access to bug 336383, so I don't know whether this is needed for 1.8? (though for 1.8 a simpler null check might be enough)

Comment 9

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

Comment on attachment 221294 [details] [diff] [review]
proposed patch

>Index: content/base/src/nsDocumentFragment.cpp
>+nsDocumentFragment::IsNodeOfType(PRUint32 aFlags) const
>+{
>+  return !(aFlags & ~(eCONTENT | eDOCUMENT_FRAGMENT));
>+}

So currently nsDocumentFragments claim to be eELEMENTs. I do think that the new way is better, and things will probably work fine. But please check the current users of eELEMENT before checking this in.

>Index: content/base/src/nsRange.cpp
>+  // Elements etc. must be in document or in document fragment,
>+  // text nodes in document, in document fragment or in attribute.
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
>+  if (content) {
>+    if (content->IsInDoc() ||
>+        (content->GetNodeParent() &&
>+         content->GetNodeParent()->IsNodeOfType(nsINode::eATTRIBUTE))) {
>+      return NS_OK;
>+    }
>+
>+    nsIContent* parent = content->GetParent();
>+    while (parent) {
>+      if (parent->IsNodeOfType(nsINode::eDOCUMENT_FRAGMENT)) {
>+        return NS_OK;
>+      }
>+      parent = parent->GetParent();
>+    }

Write this loop using GetNodeParent since GetNodeParent is slightly faster than GetParent. Also cache the result of the initial call to GetNodeParent since it is used for both the attribute check and for while-loop.

Also note that we'll currently never return an attribute node from GetNodeParent, but that'll hopefully get fixed at some point.

>Index: layout/forms/nsTextControlFrame.cpp

Are these changes related to this patch? If so, I'd rather that someone that knows layout better reviews that.

r=me on the other changes with the above comments.

Comment 10

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

(In reply to comment #9)
> 
> >Index: layout/forms/nsTextControlFrame.cpp
> 
> Are these changes related to this patch? If so, I'd rather that someone that
> knows layout better reviews that.

Yes, it is related. anonymous content needs to be in document before attaching
editor. 

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 221331 [details] [diff] [review]
Additional patch for nsEditor

r/moa=[e-mail]

Comment 12

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

Attachment: using GetNodeParent

The changes to nsTextControlFrame are just moving the initialization of the editor
to a place where native anonymous content is already bound to document.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Could you even move all the editor hookup code into InitEditor()?  Or no?

I'll try to give this a more thorough review in the next few days, but just asking...

Comment 14

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

Having the code in nsTextControlFrame::CreateFrameFor makes it easy to
access the anonymous div (which is created in nsTextControlFrame::CreateAnonymousContent).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  I'd forgotten that we don't have a member for that; I've had one in my tree for at least a year.  Merging is going to be exciting.  ;)

I'm hoping to get to this by the end of the weekend.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 221446 [details] [diff] [review]
using GetNodeParent

>Index: content/base/src/nsRange.cpp
>+  if (content) {

Why not just do an early return if (!content) and outdent all this stuff?

>+#ifdef DEBUG
>+    printf("nsRange::IsValidBoundary: node is not a valid boundary point [%s]\n",
>+           NS_ConvertUTF16toUTF8(name).get());

Is this really something everyone should be seeing in debug builds?  Or should this be DEBUG_smaug?

If everyone should see this, please use NS_WARNING or NS_ERROR depending on severity, not printf.

>Index: layout/forms/nsTextControlFrame.cpp

> void nsTextControlFrame::PostCreateFrames() {
>   InitEditor();
> }
> 
> NS_IMETHODIMP
> nsTextControlFrame::CreateFrameFor(nsPresContext*   

Document that this method MUST set *aFrame to null?

>+  nsIPresShell *shell = aPresContext->GetPresShell();
>+  if (!shell)
>+    return NS_ERROR_FAILURE;
>+  
>+  nsCOMPtr<nsIDOMDocument> domdoc = do_QueryInterface(shell->GetDocument());

Can't you remove the equivalent code from CreateAnonymousContent?  Certainly domdoc is no longer used there.

Add a debug-only member to assert that this method only gets called once or something?

>@@ -1615,199 +1799,17 @@ nsTextControlFrame::CreateAnonymousConte

>+  return aChildList.AppendElement(divContent);

Mmmm... I guess this suckage was already here.  :(  AppendElement returns a boolean, but pretends that it returns an nsresult.  :(  I guess let's leave it as is.

sr=bzbarsky with the nits picked.

Comment 17

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

Attachment: this was checked in

and also the patch for editor

Comment 18

[Peter van der Woude [:Peter6]]

It looks like this caused Bug 338087

Comment 19

[Boris Zbarsky [:bzbarsky]]

So wait.  Did this just break innerHTML on nodes that are not in the document?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Ah, so apparently it did not, but only because it's allowing the boundary points to end up outside a Document/DocumentFragment/Attr if selectNodeContents is used.

Other issues I see, now that I read this more carefully:

1)  SetStart doesn't enforce that the two endpoints are in the same document
    fragment, does it?  In fact, note the business with null GetDocument()
    returns in nsContentUtils::InSameDoc and the comments about editor.
2)  SetEnd has the same thing.
3)  SelectNode has the same issue as SelectNodeContents.

Comment 21

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

(In reply to comment #20)
> Ah, so apparently it did not, but only because it's allowing the boundary
> points to end up outside a Document/DocumentFragment/Attr if selectNodeContents
> is used.
> 

Argh.

> Other issues I see, now that I read this more carefully:
> 
> 1)  SetStart doesn't enforce that the two endpoints are in the same document
>     fragment, does it?  In fact, note the business with null GetDocument()
>     returns in nsContentUtils::InSameDoc and the comments about editor.
> 2)  SetEnd has the same thing.
> 3)  SelectNode has the same issue as SelectNodeContents.
> 

I was planning to file new bugs related to these other similar 
problems in nsRange. But if so many things relies on being able
to create ranges even using nodes which aren't in the document,
I guess I could back this out and fix the crash with a null check
in InsertNode. Though, making sure that ranges are always in
document/documentfragment forces people to write better code, imo :)

Comment 22

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

Fortunately nsGenericHTMLElement::SetInnerHTML is easy to fix if
SelectNodeContents forces node to be in document.

Comment 23

[Boris Zbarsky [:bzbarsky]]

As long as we don't break SetInnerHTML, I'm all happy.

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2006-05-16-07 of SeaMonkey trunk on Windows XP with the testcase: https://bugzilla.mozilla.org/attachment.cgi?id=220612.

No crash.

Comment 25

[Jesse Ruderman]

Instead of a crashtest, this should get a mochitest checking that the "range endpoints are in the document" rule is enforced for various methods of setting the endpoints.

Comment 26

[Jesse Ruderman]

Or, if bug 409380 is fixed, I guess a crashtest would be best.

Comment 27

[Jesse Ruderman]

Bug 409380 got fixed, so I checked in this bug's testcase as a crashtest.