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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
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.
So the idea is to have the sink manually call CharacterDataChanged here?
Flags: blocking1.9a1?
Yeah, I guess it would have to. Alternativly, it needs to be prepared for that calling AppendData will call BeginUpdate.
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?
(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.
> 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.
Flags: blocking1.9a1? → blocking1.9-
Whiteboard: [wanted-1.9]
Created attachment 243438 [details] [diff] [review]
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.
Assignee: general → bugmail
Status: NEW → ASSIGNED
Attachment #243438 - Flags: superreview?(bzbarsky)
Attachment #243438 - Flags: review?(bzbarsky)
Comment on attachment 243438 [details] [diff] [review]
Patch to fix

Found a bug in this. New patch in the works
Attachment #243438 - Flags: superreview?(bzbarsky)
Attachment #243438 - Flags: review?(bzbarsky)
Attachment #243438 - Flags: review-
Created attachment 243564 [details] [diff] [review]
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.
Attachment #243438 - Attachment is obsolete: true
Attachment #243564 - Flags: review?(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.
Attachment #243564 - Flags: superreview+
Attachment #243564 - Flags: review?(bzbarsky)
Attachment #243564 - Flags: review+
(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.
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
You need to log in before you can comment on or make changes to this bug.