263053 - createContextualFragment fails when the string contains <endnote> tags

Status: VERIFIED FIXED

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

Description

[steve.swanson]

This is a follow-up to bug 133827.

createContextualFragment uses an <endnote> tag to separate the context from 
the content.  Unfortunately, this design has a problem if the content is 
arbitrary XML (which might have an <endnote> tag).

Comment 1

[steve.swanson]

Attachment: testcase

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

-> me. I'll have a patch tomorrow.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This gets rid of <endnote> entirely. I had to make a bunch of changes to the
XML fragment sink. I think it all works, I've tested it as well as I could and
everything seems to work.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 170506 [details] [diff] [review]
patch v1

I forgot to diff mozilla/content/html/content/src/nsHTMLAtoms.cpp
in this patch (it references endnote) but I've removed the line locally.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'll try to look at this soon, but no guarantees.  :(

Comment 6

[steve.swanson]

Nicely done.

The patch works for me.  And all the testcases in bug 133827 continue to work.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 170506 [details] [diff] [review]
patch v1

I like it, I think I understand it too :-).

>Index: content/xml/document/src/nsXMLFragmentContentSink.cpp
>===================================================================

>@@ -235,15 +228,21 @@ nsXMLFragmentContentSink::CreateElement(

>+  // Make sure that scripts are added immediately, not on close.
>+  *aAppendContent = PR_TRUE;
>+
>+  // However, because we never open a doc element, we run into trouble on the
>+  // first element, so we don't append, and simply push this onto the content
>+  // stack.
>+  if (mContentStack.Count() == 0) {

This will only happen if !mAllContent, right?

>Index: content/html/document/src/nsHTMLFragmentContentSink.cpp
>===================================================================

>@@ -191,26 +194,27 @@ NS_NewHTMLFragmentContentSink(nsIFragmen
> {
>   return NewHTMLFragmentContentSinkHelper(PR_FALSE,aResult);
> }
> 
> nsHTMLFragmentContentSink::nsHTMLFragmentContentSink(PRBool aAllContent)
>+  : mAllContent(aAllContent),
>+    mRoot(nsnull),
>+    mParser(nsnull),
>+    mCurrentForm(nsnull),
>+    mCurrentMap(nsnull),

You should just make mRoot, mParser, mCurrentForm and mCurrentMap nsCOMPtr's.

>+    mContentStack(nsnull),
>+    mText(nsnull),
>+    mTextLength(0),
>+    mTextSize(0)
> {
>   if (aAllContent) {
>-    mHitSentinel = PR_TRUE;
>+    mProcessing = PR_TRUE;
>     mSeenBody = PR_FALSE;
>   } else {
>-    mHitSentinel = PR_FALSE;
>+    mProcessing = PR_FALSE;
>     mSeenBody = PR_TRUE;
>   }

This could be
    mProcessing (aAllContent),
    mSeenBody(!aAllContent)

>@@ -596,11 +594,11 @@ nsHTMLFragmentContentSink::AddLeaf(const
>           }
> 
>           if(nodeType == eHTMLTag_script    ||
>              nodeType == eHTMLTag_style     ||
>              nodeType == eHTMLTag_textarea  ||
>-             nodeType == eHTMLTag_xmp) {
>+             nodeType == eHTMLTag_server) {

Is that supposed to be part of this patch? Or just a small fix you want to
include?

>@@ -710,10 +708,31 @@ nsHTMLFragmentContentSink::SetTargetDocu

>+NS_IMETHODIMP
>+nsHTMLFragmentContentSink::WillBuildContent()
>+{
>+  if (!mAllContent) {
>+    mProcessing = PR_TRUE;
>+  }

You could drop the |if (!mAllContent)| here? (if mAllContent is PR_TRUE than
mProcessing will be too)

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

This patch includes everything, including my changes to the HTML fragment sink.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v3

Lets hope I finally got all of the files in this patch!

This patch already has r=peterv over IRC.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 171961 [details] [diff] [review]
patch v3

>Index: content/xml/document/src/nsXMLFragmentContentSink.cpp
>+  // However, when we aren't grabbing all of the content we, never open a doc

Remove comma after "we".

>+  // element, we run into trouble on the first element, so we 

"hence we run into trouble"

>Index: content/html/document/src/nsHTMLFragmentContentSink.cpp

>+  PRBool mAllContent;
>+  PRBool mProcessing;
>   PRBool mSeenBody;

Smack these to be PRPackedBool while you're here?

>Index: parser/htmlparser/src/nsParser.cpp
>@@ -1781,29 +1801,44 @@ nsParser::ParseFragment(const nsAString&

>+  NS_ASSERTION(fragSink, "ParseFragment requires a fragment content sink");

Should probably document that in nsIParser.

sr=bzbarsky.  I keep expecting your patches to be more complicated than they
are... :(

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 12

[Mats Palmgren (inactive)]

This caused bug 283036

Comment 13

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

Attachment: Screenshot

Comment 14

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

See screenshot above (attachment 176234 [details]), which was

the testcase here:

https://bugzilla.mozilla.org/attachment.cgi?id=161189

This is Verified FIXED using build 2005-03-03-05 on Windows XP Seamonkey trunk.