172372 - DOMParser parseFromString method fails to halt on XHTML with script tag

Status: RESOLVED FIXED

(Core :: XML, defect, P3)

Description

[Doug Daniels]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2a) Gecko/20020910
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2a) Gecko/20020910

The DOMParser's parseFromString method does not halt when passed a valid XHTML
String containing a script tag in the head.  This is using the "text/xml"
parsing mode, which I assume is correct for XHTML.  

A test case follows.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Doug Daniels]

Attachment: XHTML file to load the testcase javascript

Comment 2

[Doug Daniels]

Attachment: javascript to demonstrate the bug

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: testcase

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: testcase

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: js

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: testcase

Ok, use this. The original used dialogs and I was concerned that could have
affected the results.It does not seem to matter, but it is better to use this
instead in case it might...

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: testcase

Argh, I do this every time :( In XML gotta remember to escape &

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Holy cow! The JS that gets loaded when DOMParser parseFromString is called
actually gets executed!

I am going to fix it so that scripts and stylesheets will not be loaded nor
executed (when inline) when loaded as data.

Comment 9

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

this could be a regression from my fix for bug 26790 or bug 153600. There is a
suspend() function on the scriptloader and bz says that it should be trivial to
add the same thing to the stylesheetloader.

XSLT would also have good use of this

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: css

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: test scripts and styles

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix v1.0

This patch contains the fix for bug 169980 (the part about loadgroups), you can
ignore it for now...

There are three effective parts in the patch: 

1) In XML content sink we bail out if we try to ProcessStyleLink() we bail out
if we were loading as data. This takes care of xml-stylesheet processing
instructions and cookie-initiated stylesheet loads. This is the only way we can
process XSLT stylesheets at the moment so this takes care of XSLT styles for
good.

2) In XML document's StartDocumentLoad() we suspend the script loader. This
prevents both external scripts from loading as well as inline scripts from
executing.

3) In the same place we also suspend the CSS loader, which basically does the
same thing as 2) but for styles

In addition to the effective parts I did a little cleanup and found a minimal
footprint gain. I also realized the the existing Suspend/Resume in the script
loader really was only used as enable/disable, so I made it clear. I then
introduced the same pattern on the CSS loader.

I still need to test that everything really happens as I described above :)

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Heh, things look a bit funky. Turns out XBL is lying: it says loadAsData but it
really does want to load styles and I assume scripts also. A proof of concept
fix is to make XBL call us with "loadAsXBL" load type and then in
nsXMLDocument::StartDocumentLoad() add else-if to the initial if-clause:

  } else if (nsCRT::strcmp("loadAsXBL", aCommand) == 0) {
    aCommand = kLoadAsData; // XBL needs scripts and styles
  }

I should perhaps say "loadAsDataWithScriptsAndStyles" because I don't want XML
to know anything about XBL (after all, XBL inherits from XML). Any better
suggestions for command name?

Comment 14

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

maybe "loadAsInteractiveData"?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> +   * Is the loader enabled or not.

"Whether the loader is enabled or not."

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix v1.1

loadAsInteractiveData then. 

Reviews? (Ignore the load group stuff, or better yet, go and review that in bug
169980)

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Forgot to say that this patch actually may cause bloat in non-XBL loading cases
because now we force the creation of script loader and CSS loader for all
documents loaded as data. Previously they would have been created only if
someone asked for them (for example when there was a script or style element).
It would be possible to store the "loadAsData" bit on the document, but the
current solution feels more "right" to me. Besides the size of these objects
compared to the full document object including its children is minimal.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> +  NS_ENSURE_ARG_POINTER(aEnabled);

I'm of the "If they can't get basic COM right, assert and crash" school of
though myself... ;)  But either way.

I'm not that familiar with the HTML content sink change... could we get the
following sequence of calls (in the old world):

disable()
disable()
enable()
enable()

?  That is, a nested disable even once we're disabled?  If we could, for any
reason, that's a problem.... otherwise, should be good.

I'm not sure we should block LoadChildSheet... in my mind that's similar to
disabling "already loading styles", I guess... Either way, though.

The loadAsInteractiveData change doesn't seem to be in the patch... The rest
looks really good, though.

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: interactive, v1.2

Dunno how that happened, let's see if this one has it...

Comment 20

[Boris Zbarsky [:bzbarsky]]

sr=me assuming the 

disable()
disable()
enable()
enable()

thing could never happen (and it's up to you how you want to handle child sheets)

Comment 21

[Heikki Toivonen (remove -bugzilla when emailing directly)]

We discussed this with Harish, and we can't see how we could get that call
sequence. Suspend/resume is only called in ProcessSCRIPTTag(). The only way we
could get recursive calls on that would be to have a script include
document.write that wrote out script. But since we suspend at the outer script,
document.write will never be executed and hence we will never get a recursive
call. Here is the HTML that tests this code:

<frameset>
</frameset>
<script>
document.write(
  "<script>
     document.write('<p>Hello</p>')
   </script>"
);
</script>

I opted to put the mEnabled check in all the functions that are part of the
public interface. If I left some open, I am afraid someone could bypass the
disabled state. So in theory someone may have been able to start stylesheet
load, then get suspended, and fail to load child sheets. But I think this is an
unlikely situation, and shouldn't happen in the current codebase at all.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 102104 [details] [diff] [review]
interactive, v1.2

sr=bzbarsky

Comment 23

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

Comment on attachment 102104 [details] [diff] [review]
interactive, v1.2

>Index: content/xbl/src/nsXBLService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLService.cpp,v
>retrieving revision 1.157
>diff -u -r1.157 nsXBLService.cpp
>--- content/xbl/src/nsXBLService.cpp	7 Sep 2002 17:08:43 -0000	1.157
>+++ content/xbl/src/nsXBLService.cpp	7 Oct 2002 21:15:49 -0000
>@@ -1248,7 +1248,7 @@
>     return NS_ERROR_FAILURE;
> 
>   // Call StartDocumentLoad
>-  if (NS_FAILED(rv = xmlDoc->StartDocumentLoad("loadAsData", 
>+  if (NS_FAILED(rv = xmlDoc->StartDocumentLoad("loadAsInteractiveData", 
>                                                channel, 
>                                                nsnull, 
>                                                nsnull, 

This should be left as is. (This is a function for loading plain XML documents
and isn't XBL-related at all).

>+nsresult 
>+nsXMLDocument::GetLoadGroup(nsILoadGroup **aLoadGroup)
>+{
>+  NS_ENSURE_ARG_POINTER(aLoadGroup);
>+  *aLoadGroup = nsnull;
>+
>+  if (mScriptContext) {
>+    nsCOMPtr<nsIScriptGlobalObject> global;
>+    mScriptContext->GetGlobalObject(getter_AddRefs(global));
>+    nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(global);
>+    if (window) {
>+      nsCOMPtr<nsIDOMDocument> domdoc;
>+      window->GetDocument(getter_AddRefs(domdoc));
>+      nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc);
>+      if (doc) {
>+        doc->GetDocumentLoadGroup(aLoadGroup);
>+      }
>+    }
>+  }
>+
>+  return NS_OK;
>+}

This seems strange. What if you're viewing an css-styled XML document? Can't
you just call GetDocumentLoadGroup directly? Or maybe just go through the
scriptcontext if GetDocumentLoadGroup returns null?

However i think this was part of the stuff that was not going to land now?


>+  } else if (nsCRT::strcmp("loadAsInteractiveData", aCommand) == 0) {
>+    aCommand = kLoadAsData; // XBL, for example, needs scripts and styles
>+  }

Hmm.. ideally we should keep the load-command unchanged an just teach all
neccesary code to deal with "loadAsInteractiveData". Would you mind filing a
followup bug on taking care of this?

assuming the loadgroup stuff won't land, r=sicking

Comment 24

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: v1.3

This patch no longer has the loadgroup stuff in here. Regarding the comment
Jonas made, in a typical usage pattern calling this->GetLoadGroup() would not
make sense since this was created in JS and is empty; it does not have a load
group.

Fixed the one instance of wrong loadAsData change.

Should we change all the code to understand loadAsInteractiveData? I don't like
it, although I see your point. We'll have to think about it, and I'll try to
file a bug.

Comment 25

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 102266 [details] [diff] [review]
v1.3

Moving review flags forward.

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 102266 [details] [diff] [review]
v1.3

a=asa for checkin to 1.2beta (on behalf of drivers)

Comment 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Checked in.