330872 - nsGenericDOMDataNode::AppendData doesn't send out Begin/EndUpdate notification

Status: RESOLVED FIXED

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

Description

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

nsGenericDOMDataNode::AppendData calls CharacterDataChanged without calling BeginUpdate and EndUpdate around it.

Unfortuantly just adding these calls makes the nsHTMLContentSink go bonkers. It calls AppendData to append new data to an exising textnode, and gets all confused if a BeginUpdate call happens in the middle of that since it'll try to flush the data it has and thereby append the same string again.

The best way to fix this is probably to add an AppendText method to nsITextContent that takes and aNotify parameter

Comment 1

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

A way to reproduce the problem (when the call is added) is to create a script that contains more then 4096 characters. The additional characters will be added twice.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the idea is to have the sink manually call CharacterDataChanged here?

Comment 3

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

Yeah, I guess it would have to. Alternativly, it needs to be prepared for that calling AppendData will call BeginUpdate.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I think we should do the following:

1)  If the text node we're appending has already had
    ContentAppended/ContentInserted called on it, just append with no
    notification.
2)  Otherwise, append with notification and increment the notification counter in
    the sink around that append (so BeginUpdate() knows that we're triggering it
    ourselves).

In either case, we probably need to update our internal state first, since the append can fire mutation events, right?

Comment 5

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

(In reply to comment #4)
> I think we should do the following:
> 
> 1)  If the text node we're appending has already had
>     ContentAppended/ContentInserted called on it, just append with no
>     notification.
> 2)  Otherwise, append with notification and increment the notification counter
>     in the sink around that append (so BeginUpdate() knows that we're
>     triggering it ourselves).

Did you get these reversed? I'd think that if we have notified on it we need to notify about the change too. If we havn't notified then presumably no one knows about its existance and we wouldn't need to notify.

> In either case, we probably need to update our internal state first, since the
> append can fire mutation events, right?

Sort of. We shouldn't really be firing mutation events during loading IMO.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Did you get these reversed?

Er... yes.  :(

The mutation event discussion keeps cropping up.  If we're going to change that behavior, then we should just do it.

Comment 7

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

Attachment: Patch to fix

This patch cleans up nsGenericDOMDataNode such that we have a single function that deals with all modifications and that sends out all appropriate notifications, and adds code to nsHTMLContentSink to deal with the proper notifications being sent.

I also added nsIContent::AppendText with an aNotify parameter so that the sink can avoid notifing when not needed.

Comment 8

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

Comment on attachment 243438 [details] [diff] [review]
Patch to fix

Found a bug in this. New patch in the works

Comment 9

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

Attachment: Patch v1.1

This one should assert less. requesting r= from bz for the changes to nsHTMLContentSink. Though of course feel free to r and/or sr more if you have the time.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 243564 [details] [diff] [review]
Patch v1.1

>Index: content/base/public/nsIContent.h
>    * Set the text to the given value. If aNotify is PR_TRUE then
>    * the document is notified of the content change.
...
>+  virtual nsresult AppendText(const PRUnichar* aBuffer, PRUint32 

I assume the comment text should be changed accordingly?

>Index: content/base/src/nsGenericDOMDataNode.cpp

> nsGenericDOMDataNode::SetNodeValue(const nsAString& aNodeValue)
> {
>-  return SetData(aNodeValue);
>+  return SetTextInternal(0, mText.GetLength(), aNodeValue.BeginReading(),
>+                         aNodeValue.Length(), PR_TRUE);

Why make this change?  The other seems like it's simpler....

>+nsGenericDOMDataNode::SetTextInternal(PRUint32 aOffset, PRUint32 aCount,
>+    // Merging old and new

For the love all that that is holy, let us make this use realloc and memmove as needed!  Followup bug.

>Index: content/base/src/nsGenericDOMDataNode.h

>+  nsresult SetTextInternal(PRUint32 aOffset, PRUint32 aCount,

Please document (esp the aOffset and aCount args).

r+sr=bzbarsky with the comment fixes.  Either way on SetNodeValue.

Comment 11

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

(In reply to comment #10)
> (From update of attachment 243564 [details] [diff] [review] [edit])
> >Index: content/base/public/nsIContent.h
> >    * Set the text to the given value. If aNotify is PR_TRUE then
> >    * the document is notified of the content change.
> ...
> >+  virtual nsresult AppendText(const PRUnichar* aBuffer, PRUint32 
> 
> I assume the comment text should be changed accordingly?

Yup, will do.

> >Index: content/base/src/nsGenericDOMDataNode.cpp
> 
> > nsGenericDOMDataNode::SetNodeValue(const nsAString& aNodeValue)
> > {
> >-  return SetData(aNodeValue);
> >+  return SetTextInternal(0, mText.GetLength(), aNodeValue.BeginReading(),
> >+                         aNodeValue.Length(), PR_TRUE);
> 
> Why make this change?  The other seems like it's simpler....

I thought that it was wasteful to go through another virtual call just to call a function with fewer arguments. Don't care much either way...

> >+nsGenericDOMDataNode::SetTextInternal(PRUint32 aOffset, PRUint32 aCount,
> >+    // Merging old and new
> 
> For the love all that that is holy, let us make this use realloc and memmove 
> as needed!  Followup bug.

Bug 17191 should cover that.

Comment 12

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

Checked in.