248025 - cannot add a DOM Node as an xsl:param using XSLTProcessor.setParameter()

Status: RESOLVED FIXED

(Core :: XSLT, enhancement)

Description

[Jan Willem]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113

I don't know if this is really a bug or a feature, but here it is:

I use the JavaScript interface to XSLT for transforming my docs. This all works
fine until I start passing parameters to the processor that are DOM Nodes. Since
the XSLT specs say (in chapter 11) that an xsl:param can be of any type (ie.
string, nodesets, etc.), I would expect this to be possible. But it only seems
to work for strings at the moment.

Reproducible: Always
Steps to Reproduce:
0a. Create an xsl template with a top-level <xsl:param name="my-node"/>
0b. And use it in a template, like <xsl:for-each
select="$my-node/books/title">... etc.
1. The rest is in JavaScript, so set up an html-page with a script tag
2. processor = new XSLTProcessor();
3. doc = [some DOM Document]
4. paramnode = [some other DOM Document or Node]
5. template = [an xsl template, in DOM form]
6. processor.importStylesheet(template);
7. processor.setParameter("", "my-node", paramnode);
8. processor.transformToDocument(doc);

Actual Results:  
The transformation stops (an Exception is thrown). The JavaScript Console gives
me this:

Error: uncaught exception: [Exception... "Component returned failure code:
0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXSLTProcessor.transformToDocument]" 
nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
http://localhost/js/XmlUtils.js :: anonymous :: line 215"  data: no]

The linenr 215 corresponds to step 8 above.

If I remove both step 7. and the for-each code in the xsl template that uses
param $my-node, the whole thing works again.

Expected Results:  
I expect the for-each statement to loop through a list of book titles, as it
would with nodes from the context document.

Comment 1

[Peter Van der Beken [:peterv]]

I have a patch that does this (and nodelists and arrays of nodes), but it still
needs some work.

Comment 2

[William J. Edney]

This would be really great to have.

I'm missing a whole chunk of functionality that I use in IE that I cannot do in
Mozilla because of this.

Cheers,

- Bill

Comment 3

[Axel Hecht]

... it'd be cool if we could add a XPathResult, too. That way, folks can create
nodesets with xpath statements, like requested in bug 265410.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: WIP

Absolutely, and my patch does that. I'll try to pick this up again in the
coming weeks.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 7

[Axel Hecht]

Comment on attachment 191209 [details] [diff] [review]
v1.1


>Index: extensions/transformiix/source/xpath/nsXPathResult.h
>===================================================================
<...>
>@@ -55,8 +54,33 @@ class nsIXPathResult : public nsISupport
> {
> public:
>     NS_DEFINE_STATIC_IID_ACCESSOR(NS_IXPATHRESULT_IID)
>-    NS_IMETHOD SetExprResult(txAExprResult* aExprResult,
>-                             PRUint16 aResultType) = 0;
>+    virtual nsresult SetExprResult(txAExprResult *aExprResult,
>+                                   PRUint16 aResultType) = 0;
>+    virtual nsresult GetExprResult(txAExprResult **aExprResult) = 0;
>+};

Why did you change away from NS_IMETHOD?

<...>

>Index: extensions/transformiix/source/xpath/txXPathTreeWalker.h
<...>
>@@ -162,6 +162,17 @@ public:
>     static nsresult getNode(const txXPathNode& aNode, nsIDOMNode** aResult);
>     static nsIContent* getContent(const txXPathNode& aNode);
>     static nsIDocument* getDocument(const txXPathNode& aNode);
>+    static void addRef(const txXPathNode& aNode)
>+    {
>+        // Hopefully it's ok to access mContent through mDocument.
>+        NS_ADDREF(aNode.mDocument);
>+    }
>+    static void release(const txXPathNode& aNode)
>+    {
>+        // Hopefully it's ok to access mContent through mDocument.
>+        nsISupports *node = aNode.mDocument;
>+        NS_RELEASE(node);
>+    }
> };

Why do you cast down to nsISupports on release and not on addref?

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to comment #7)

> Why did you change away from NS_IMETHOD?

Internal interface, so no need for binary compatibility.

> Why do you cast down to nsISupports on release and not on addref?

I didn't really want to cast down, just to circumvent NS_RELEASE setting that
pointer to 0. It's not so important here, but iirc I copied this from another
patch where it is.

Also note that I changed

+            if (type != nsIDataType::VTYPE_INTERFACE ||
+                type != nsIDataType::VTYPE_INTERFACE_IS) {

to

+            if (type != nsIDataType::VTYPE_INTERFACE &&
+                type != nsIDataType::VTYPE_INTERFACE_IS) {

which is saner :-).

Comment 9

[Axel Hecht]

Comment on attachment 191209 [details] [diff] [review]
v1.1

r=me then

Comment 10

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

You need to add secrurity checks to make sure that the added is one that the
caller is allowed to access. See

http://lxr.mozilla.org/mozilla/source/extensions/transformiix/source/xpath/nsXPathExpression.cpp#94

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Testcase

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: Testcase

Added security testcase.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Added security checks in setParameter. I also had to make setParameter clone
the XPathResult object (and the variant) because XPathResult objects could be
mutated by reusing them in a call to XPathEvaluator.evaluate and I'm not
entirely sure we can prevent them coming from a document that the caller can't
access. I don't think other value types need to be cloned, they can't be
mutated in that way afaik. setParameter does all the checking now about whether
we can handle the type of the value for the parameter and whether the caller is
allowed to access it.
Jonas: if you could take a quick look to verify that there should be no
security problems with this patch? Thanks.

Comment 14

[Brian Baker]

What is the likelihood that this patch will get into the upcoming beta?

Comment 15

[Axel Hecht]

Comment on attachment 191489 [details] [diff] [review]
v1.2

looks good to me, and peterv has folks who are testing this stuff in their own
builds.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 191489 [details] [diff] [review]
v1.2

sr=jst

Comment 17

[Peter Van der Beken [:peterv]]

Comment on attachment 191489 [details] [diff] [review]
v1.2

Checked in on trunk. Asking for branch approval. This is a feature that IE
already has and we have at least one user that would need this fairly soon for
their app, they have tested the patch and reported that it solves the issue for
them. The patch is limited to XSLT code.

Comment 18

[William J. Edney]

Peter -

Does this also fix #278467 as stated in comment #4 of that bug?

Thanks for this fix!!

Cheers,

- Bill

Comment 19

[Peter Van der Beken [:peterv]]

Checked in on trunk and branch (with the one-liner from bug 305326 to fix that
regression).