278981 - Extension mechanism for XPath extension functions

Status: RESOLVED FIXED

(Core :: XSLT, defect, P3)

Description

[Peter Van der Beken [:peterv]]

Here's my proposal:

We provide two entry points for registering functions.
1) One or two methods on nsIXPathEvaluatorInternal taking a namespace URI and
either an object (nsISupports*) or a contract ID.
2) Through the category manager, with a namespace URI and a contract ID. We
might want to disallow an empty namespace URI here?

The object that the contract ID refers to or that we get passed in implements at
least two interfaces, one containing the extension functions and nsIClassInfo.
Using nsIClassInfo, we can dynamically figure out the names and arguments
(number and type) of the extension functions that are defined on the other
interface. Using XPTCall we can then call those functions directly. This makes
it rather simple to implement extension functions, both in C++ and in JS.
I have prototyped this approach, and it works nicely. I'd like to discuss it and
there's some choices that we need to make.

How do we pass nodesets? Axel proposed a wrapper implementing nsIDOMNodeList, I
wonder if it wouldn't be better to have our own interface for that wrapper,
something like:

interface txINodeSet : nsISupports
{
  nsIDOMNode item(in unsigned long index);
  boolean itemAsBoolean(in unsigned long index);
  boolean itemAsNumber(in unsigned long index);
  boolean itemString(in unsigned long index);
  readonly attribute unsigned long        length;
};

I added the other item* functions because it seems like a common pattern to
convert every item of a nodeset to a different type and then do something with
it (max, min, count-non-empty, ...).

What about the context? We can create a wrapper and pass that as one of the
arguments to the function call (we can dynamically detect whether a function
needs it through the IDL). I don't know if this is urgent, or if we can postpone
this till we have concrete usecases to figure it out.

Do we want both:

NS_IMETHOD RegisterExtensionFunctionsHelper(const nsAString &aNamespaceURI,
                                            const nsACString &aContractID);

and

NS_IMETHOD RegisterExtensionFunctionsHelper(const nsAString &aNamespaceURI,
                                            nsISupports *aHelper);
?

My thinking was that we the second one is handy to keep state, create the helper
object yourself, set state on it and then pass it before evaluating. I included
the first because state isn't always necessary. Maybe I'm overengineering. Thoughts?

XPIDL doesn't allow minus signs in the method names. There are a number of
functions with minus signs in XForms, EXSLT, ... I opted to remove the minus
signs and uppercase the following letter (foo-bar becomes fooBar) and
disallowing uppercase letters in function names (to avoid both foo-bar and
fooBar matching fooBar, where we only want foo-bar). I don't think uppercase
letters in function names are common, so it shouldn't be a big problem.

Just as an example, here's how you'd define the XForms boolean-from-string function:

interface nsITestExtension : nsISupports
{
    boolean booleanFromString(in DOMString aString);
};

and

class nsTestExtension : public nsITestExtension
{
public:
  NS_DECL_ISUPPORTS
  NS_DECL_NSITESTEXTENSION
};

NS_IMPL_ISUPPORTS1_CI(nsTestExtension, nsITestExtension)

NS_IMETHODIMP
nsTestExtension::BooleanFromString(const nsAString & aString, PRBool *aResult)
{
    *aResult = aString.EqualsLiteral("1") ||
               aString.LowerCaseEqualsLiteral("true");

    return NS_OK;
}

Comment 1

[Axel Hecht]

> We provide two entry points for registering functions.
> 1) One or two methods on nsIXPathEvaluatorInternal taking a namespace URI and
> either an object (nsISupports*) or a contract ID.
> 2) Through the category manager, with a namespace URI and a contract ID. We
> might want to disallow an empty namespace URI here?
> 
> The object that the contract ID refers to or that we get passed in implements at
> least two interfaces, one containing the extension functions and nsIClassInfo.

Make that three? txIExtensionFunctionProvider, too? That should implement
boolean implements(DOMString aLocalName, DOMString aNameSpace);

That would simplify function-available() a good deal, and resolve the 
ambiguity below.

<...>
> 
> How do we pass nodesets? Axel proposed a wrapper implementing nsIDOMNodeList, I
> wonder if it wouldn't be better to have our own interface for that wrapper,
> something like:
> 
> interface txINodeSet : nsISupports
> {
>   nsIDOMNode item(in unsigned long index);
>   boolean itemAsBoolean(in unsigned long index);
>   boolean itemAsNumber(in unsigned long index);
double ;-)
>   boolean itemString(in unsigned long index);
DOMString, I suppose.
>   readonly attribute unsigned long        length;
> };
> 
> I added the other item* functions because it seems like a common pattern to
> convert every item of a nodeset to a different type and then do something with
> it (max, min, count-non-empty, ...).

I like the nodelist approach, because it let's you use the scriptable helper.
Thus, js folks could just do nodelist[2].
I wonder if we should have a XPathUtils object for the DOM-> xpath conversions.
Or will the schema validation code provide that? (I do hope to get something 
like that, as I'd love to use it for RDF.)

> What about the context? We can create a wrapper and pass that as one of the
> arguments to the function call (we can dynamically detect whether a function
> needs it through the IDL). I don't know if this is urgent, or if we can postpone
> this till we have concrete usecases to figure it out.

I'm happy with a phase 2 for this. It would seem to be 100% backwards compatible
if we add it later.

> Do we want both:
> 
> NS_IMETHOD RegisterExtensionFunctionsHelper(const nsAString &aNamespaceURI,
>                                             const nsACString &aContractID);
> 
> and
> 
> NS_IMETHOD RegisterExtensionFunctionsHelper(const nsAString &aNamespaceURI,
>                                             nsISupports *aHelper);
> ?

The helper should actually be a nsIFactory or nsIModule. It serves the same
state functionality, but we agreed to have an object per function call expr,
right? Plus the singleton classinfo stuff? Smells like nsIModule.

> XPIDL doesn't allow minus signs in the method names. There are a number of
> functions with minus signs in XForms, EXSLT, ... I opted to remove the minus
> signs and uppercase the following letter (foo-bar becomes fooBar) and
> disallowing uppercase letters in function names (to avoid both foo-bar and
> fooBar matching fooBar, where we only want foo-bar). I don't think uppercase
> letters in function names are common, so it shouldn't be a big problem.

Together with the implements function proposed above, I'd just leave the 
ambiguity. We can check for both and error if it happens. And it seems to be
much easier to say "this is our name mangling, conflicts are verboten" compared
to "this is our name mangling, and we may introduce uppercase, but you must not"

Comment 2

[Peter Van der Beken [:peterv]]

(In reply to comment #1)
> Make that three? txIExtensionFunctionProvider, too? That should implement
> boolean implements(DOMString aLocalName, DOMString aNameSpace);
> 
> That would simplify function-available() a good deal, and resolve the 
> ambiguity below.

function-available is fairly trivial already. How about, if it doesn't implement
txIExtensionFunctionProvider we disallow uppercase letters? That way it stays
simple for most cases?

> > I added the other item* functions because it seems like a common pattern to
> > convert every item of a nodeset to a different type and then do something with
> > it (max, min, count-non-empty, ...).
> 
> I like the nodelist approach, because it let's you use the scriptable helper.
> Thus, js folks could just do nodelist[2].

We can do that too with my interface using DOMCI. We need to implement DOMCI
even for nsIDOMNodeList to make that work (and it's non-trivial so I'd rather
postpone it).

> I wonder if we should have a XPathUtils object for the DOM-> xpath conversions.
> Or will the schema validation code provide that? (I do hope to get something 
> like that, as I'd love to use it for RDF.)

This could live on the context at some point?

> I'm happy with a phase 2 for this. It would seem to be 100% backwards compatible
> if we add it later.

Indeed.

> > NS_IMETHOD RegisterExtensionFunctionsHelper(const nsAString &aNamespaceURI,
> >                                             nsISupports *aHelper);
> > ?
> 
> The helper should actually be a nsIFactory or nsIModule. It serves the same
> state functionality, but we agreed to have an object per function call expr,
> right? Plus the singleton classinfo stuff? Smells like nsIModule.

Hmm, I don't see what that buys us over contractID? The idea behind nsISupports
*aHelper is instead of us instantiating the object when we need it, the caller
instantiates it and sets some state on it. Setting state on a nsIFactory or a
nsIModule doesn't really solve anything I think.

Comment 3

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

While it would be nice to use the scripthelper for nsIDOMNodeList I think it'd
be better to use our own interface. First of all nsIDOMNodeList is supposed to
be some sort of live object that is updated while the DOM under it changes. IIRC
that is the reason DOM-XPath used their own interface rather then
nsIDOMNodeList. Second, the methods that peterv provided on txINodeSet seems
usefull (possibly except for itemAsBoolean, how do you define it?).

I like the idea of being able to pass the factory as an object so that you can
set state in it. At least I think I do. There is a certain risk of abuse.

Both xforms and xslt has the need to set state in the created functioncall
objects. xslt sets baseURI for document() and xforms sets element the expression
appeared on for index(). How will that be done with this? Do you have to set up
a new factory with state every time createExpresssion is called?

Xforms needs to be able to reach the contextnode so we can't get rid of the
xforms code until that is done.

How will we know which function on which interface to call for the
extensionfunction? It would be very good if you could implement several
extensionfunctions in the same class. So that implementors doesn't have to
implement an interface+classinfo+class for each extensionfunction.

Comment 4

[Peter Van der Beken [:peterv]]

So do we want to allow random webpages to define extension functions? It's
something that this proposal doesn't allow, while Axel's (attachment 170942 [details] [diff] [review])
would be adaptable to allow it.
I'm not entirely convinced that we want to expose them, but if we do, I might be
over-engineering and Axel's proposal is better. We'd need to switch it to use
nsIVariant though.

Comment 5

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

I was thinking about that on my way home (taking the subway the wrong direction
late at night == long walk home). It is defenetly a problem that if we allow
random script to run during xpath evaluation it is very possible for that script
to mutate the tree so that parts of it is recycled and we're left with dangling
pointers.

However I think that allowing scripts to run during evaluation is a very usefull
feature and it'd be great if we could come up with a way.

I see a few alternatives:
A) Let treewalkers and xpathnodes hold owning references to nodes. This would
   increase the amount of refcounting we do a lot, but it's hard to say how big
   effect this would have on overall performance.
B) Not expose nsIDOMNodes directly to the script. Either expose a wrapper around
   txXPathTreeWalker, or around nsIDOMNodes that simply didn't allow mutating
   the DOM.
C) Add the capability to the mozilla DOM to be locked down, so that it couldn't
   be edited. We'd then lock the tree down during xpath evaluation and xslt
   execution.

A seems like the easiest way. We should measure performance impact of course. I
tend to think that there are better ways to gain performance (like better
patternmatching, optimized xpath and optimized xslt). Though of course every
optimization count.

B just isn't possible in DOM-XPath as the script is able to reach nsIDOMNodes
before the expression is started, so it'd be easy to pass in references to the
nodes. Also, it feels like a lot of work to create our own DOM api, not to
mention that authors probably wouldn't be too exited to learn another api.

C is a very interesting idea that might be simpler then it sounds. We only need
to lock down operations that move nodes around in the tree. Setting text and
attribute values would be ok.

Comment 6

[Axel Hecht]

I don't like the idea of web-code running inside XSLT. Mainly, I don't see an 
excellent reason to do it. And I don't want to do the security audit.

I do buy the liveness nsIDOMNodeList argument. So let's go for a completely new 
interface. Sadly, I don't see any way to reuse the nodelist scriptable helper. But
it's not that much code, anyway. And not that much convenience to do [] in the
end. So I'd be ok with not doing that for now.

Registering a impl of nsIModule against a contractID (I'm actually favoring a 
classID, as we're not really having a contract with the impl.) is that we can
use a factory per transform, feeding state between multiple instances of the
same transform.
I envision something along the lines of calling into the observer service with
a topic "tx-xpath-start" (and "tx-xslt-start") and the txIExtensionHook (?) we
implement the RegisterMethods on as subject. That way we can expose some level
of "state per transform" that we need for stuff like the document() function.

I wonder if we need some kind of hook for the "element" scope of state. index()
smells like we do.

Hrm. Seems like we need the following scopes of state-handling:
- global/none
- transform/stylesheet/source
- stylesheet-element

Any clues on to how to do that right? We could provide some interface like 
txIAboutExtension or txIExtensionInfo, as I don't think that class info will
be enough to deal with this.
We probably should think about how to handle extension elements as well, at 
least with respect to state.

Comment 7

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

What are the security concerns? The only code I can think of in transformiix
that could be dangerous is the document-loading code, but that should be
possible to securityreview.

Well, that and if we use the generic-xpcom-factory-calling code peterv had in
mind. But if we wanted scriptability then we obviously couldn't use that approach.

Other then that the only thing I can think of they can do is expose bugs leading
to crashes, but that's true no matter how they exercise the code.

Comment 8

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

Didn't mean to sound so sure of that we should allow webscripts to play around.
There might defenetly be security concerns, so we should defenetly discuss this.
I just can't think of any off the top of my head.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v1

Not really sure anymore if this is the right way to go. :-/

Some functions need to return NaN, I added a getter for that on the context
with the end result that plenty of functions need the context now. An
alternative would be a nsresult succes value signifying NaN, but that seems
like such a hack.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: XForms functions

Here is the current implementation of the XForms functions, ported to the new
extension mechanism. Since I don't build XForms myself, everything is still in
extensions/transformiix but it can just be in extensions/xforms. The definition
of the if function doesn't define its behaviour very well, I assumed it's ok to
evaluate the two results before evaluating the if.

Comment 11

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

NaN is a normal double (well, family of doubles really), so I think it'd be fine
to leave it up to the implementors to create that value themselvs. It doesn't
require a lot of code. Alternativly we could maybe create a macro that returns
NaN. Hmm.. though I wonder why we didn't use that approach in Double.cpp

Comment 12

[Peter Van der Beken [:peterv]]

(In reply to comment #11)
> NaN is a normal double (well, family of doubles really), so I think it'd be fine
> to leave it up to the implementors to create that value themselvs.

Ah, right. I removed GetNaN and let XForms define its own NaN constant.

Comment 13

[Axel Hecht]

Comment on attachment 172117 [details] [diff] [review]
XForms functions

note, this xforms patch looks way odd, some way not like a patch at all, and it
seems to include stuff generated by xpidl (without an idl, though). I'll
comment over in bug 258472.

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Comment 16

[Axel Hecht]

Is it intentional that one can register multiple extension function helper for 
one namespace?

Should the copyrights of the new files go to 2005?

I'm a bit confused about 0 or 1 based indexing in txNodeSetAdaptor.
txINodeSet should probably tell, but I guess the out-of-bounds checks aren't
right either way.

I'm a bit lost with the xptcall stuff, esp with respect to ownership et al.
I think you leak the iidArray, for example. I'd welcome if someone more
xptcall-savvy could look at that part.

Comment 17

[aaronr]

I'm starting to play around with this along with XForms and noticed two build
errors compiling the patch on Windows:

nsXPathEvaluator.cpp - build break because
nsXPathEvaluator::ParseContextImpl::resolveFunctionCall is trying to call
nsXPathEvaluator::resolveFunctionCall which it can't do because that is a
private method.

txXPCOMExtensionFunction.cpp - build break in
txParamArrayCleaner::~txParamArrayCleaner trying to do
NS_RELEASE((nsISupports*)variant.val.p) because you have a cast on the left side
of the expression when NS_RELEASE tries to set the pointer to 0.

Comment 18

[aaronr]

Two things:

Looks like the interface changes to
content/base/public/nsIXPathEvaluatorInternal.h are missing after version 1 of
the patch.  Everyone will need access to RegisterExtensionFunctionsHelper and
SetState functions.

Also, have you given thought to what might happen if the W3C comes up with more
xpath functions without a namespace via other specs?  For example, if the fooXML
spec does this, then if a fooXML extension is written and comes in and registers
its functions interface using the EmptyString() namespace (just like xforms
does), then evaluations that the fooXML extension does wouldn't have access to
the xforms functions, correct?

A more concrete example are the XForms 1.1 functions.  Since XForms 1.1 is in a
different namespace and changes some of the behaviors of (non xpath)elements
defined in 1.0, then it is conceivable that a user could have a 1.0 extension
and a 1.1 extension installed (as potentially troublesome as this is).  So I'd
now have to have the 1.0 xpath functions also defined in the 1.1 extension since
they are both in the default namespace.  Ugh, my head hurts just thinking about
this.

Comment 19

[aaronr]

nsXPathEvaluator::SetState has an infinite loop in it.  Never breaks out of the
while(helper) loop when it finds one with the right id.

Comment 20

[aaronr]

Attachment: works for me

Here is the patch of my system after I got all of these pieces to play nicely
together.  Includes moving the xforms extension functions into the xforms.dll,
removing the need for nsXFormsUtilityService.* and fixing the issues that I
noted in this bug.  Also fixes a problem where the return buffer for a return
type of string was a little off.

But this still requires a decision on how to handle a returned nodeset.  Right
now if we run the xforms instance() function that returns a nodeset with one
node in it, xpath won't do anything with the returned txINodeSet and takes an
exception later on.

Comment 21

[Axel Hecht]

Peter, which of these patches should I review, attachement 173777 or 
attachement 178531?

Comment 22

[Peter Van der Beken [:peterv]]

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

None for now, new one coming up.

Comment 23

[Peter Van der Beken [:peterv]]

Attachment: Support aggregation of nsXPathEvaluator v1

Comment 24

[Peter Van der Beken [:peterv]]

Attachment: Support aggregation of nsXPathEvaluator v1.1

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 197704 [details] [diff] [review]
Support aggregation of nsXPathEvaluator v1.1

r+sr=jst

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It would be good to see the interface for extension mechanisms document the invariants that our codebase assumes XPath extension functions maintain.  For example, is this code designed such that XPath extension functions could select based on layout of objects, or would that cause bugs when computed style queries cause layout flushes in the middle of XPath processing?  I've heard of some pretty nasty XPath extension functions in some contexts (I think somebody rewrote css3-mediaqueries as a set of XPath extension functions), so it seems like it's important to document at least our best understanding of what extensions are legal in our architecture.

Comment 27

[Peter Van der Beken [:peterv]]

Attachment: v1.3

Comment 28

[Peter Van der Beken [:peterv]]

Attachment: Files to remove

Comment 29

[Peter Van der Beken [:peterv]]

Attachment: v1.3

Attached the wrong patch.

Comment 30

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

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

>Index: content/xslt/public/txDouble.h
...
>+//A trick to handle IEEE floating point exceptions on FreeBSD - E.D.
>+#ifdef __FreeBSD__
>+#include <ieeefp.h>
>+#ifdef __alpha__
>+fp_except_t allmask = FP_X_INV|FP_X_OFL|FP_X_UFL|FP_X_DZ|FP_X_IMP;
>+#else
>+fp_except_t allmask = FP_X_INV|FP_X_OFL|FP_X_UFL|FP_X_DZ|FP_X_IMP|FP_X_DNML;
>+#endif
>+fp_except_t oldmask = fpsetmask(~allmask);
>+#endif

I'm a little bit worried about having this defined in 'random' files. Would be nice to get input from someone that understands this better if it's an issue or not.

>+interface txINodeSet : nsISupports
>+{
>+    nsIDOMNode item(in unsigned long index);
>+    double itemAsNumber(in unsigned long index);
>+    DOMString itemAsString(in unsigned long index);
>+    readonly attribute unsigned long length;
>+    void add(in nsIDOMNode node);
>+    [noscript] txAExprResultPtr getTxNodeSet();
>+};

Do we need itemAsBool, that returns true if itemAsString would have returned a non-empty string?

>Index: content/xslt/src/xpath/nsXPathEvaluator.cpp

>+nsXPathEvaluator::SetState(const nsAString &aNamespaceURI, nsISupports *aState)
>+{
>+    PRInt32 nsId;
>+    nsresult rv =
>+        nsContentUtils::NameSpaceManager()->RegisterNameSpace(aNamespaceURI,
>+                                                              nsId);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = NS_ERROR_XPATH_UNKNOWN_FUNCTION;
>+
>+    txExtensionsHelper *helper = mExtensionHelper;
>+    while (helper) {
>+        if (helper->mNamespaceID == nsId) {
>+            helper->mState = aState;
>+            rv = NS_OK;

Just return NS_OK here.

>+        }
>+        helper = helper->mNext;
>+    }
>+
>+    return rv;
>+}

And return NS_ERROR_XPATH_UNKNOWN_FUNCTION here.

>+nsresult
>+nsXPathEvaluator::resolveFunctionCall(nsIAtom* aName, PRInt32 aID,
>+                                      FunctionCall*& aFunction)
>+{
>+    nsresult rv = NS_ERROR_XPATH_UNKNOWN_FUNCTION;
>+    txExtensionsHelper *helper = mExtensionHelper;
>+    while (helper) {
>+        if (helper->mNamespaceID == aID) {
>+            rv = TX_ResolveFunctionCallXPCOM(helper->mContractID, aID, aName,
>+                                             helper->mState, aFunction);
>+            if (NS_SUCCEEDED(rv)) {
>+                break;

return directly here.

>+            }
>+        }
>+        helper = helper->mNext;
>+    }
>+
>+    return rv;
>+}

And return NS_ERROR_XPATH_UNKNOWN_FUNCTION here.

>Index: content/xslt/src/xpath/txNodeSetAdaptor.cpp

>+txNodeSetAdaptor::txNodeSetAdaptor() : mWritable(PR_TRUE)
>+{
>+}
>+
>+txNodeSetAdaptor::txNodeSetAdaptor(txNodeSet* aNodeSet) : mNodeSet(aNodeSet),
>+                                                          mWritable(PR_FALSE)
>+{
>+    NS_ASSERTION(aNodeSet,
>+                 "Don't create an adaptor if you don't have a txNodeSet");
>+}

Please put the initiator list on a new line (in both ctors).

>+txNodeSetAdaptor::~txNodeSetAdaptor()
>+{
>+}

Just nuke this?

>+NS_IMETHODIMP
>+txNodeSetAdaptor::ItemAsString(PRUint32 aIndex, nsAString &aResult)
>+{
>+    if (aIndex > (PRUint32)mNodeSet->size()) {
>+        return NS_ERROR_ILLEGAL_VALUE;
>+    }
>+
>+    nsAutoString result;
>+    txXPathNodeUtils::appendNodeValue(mNodeSet->get(aIndex), aResult);
>+
>+    return NS_OK;

|result| is unused.

>+NS_IMETHODIMP
>+txNodeSetAdaptor::GetTxNodeSet(txAExprResult **aResult)
>+{
>+    NS_ADDREF(*aResult = mNodeSet);
>+
>+    return NS_OK;
>+}

If you mark this function as [notxpcom] we could get a deCOMtaminated syntax here I think. Don't have a strong oppinion either way.

>Index: extensions/xforms/nsXFormsModelElement.cpp
>@@ -1292,10 +1294,10 @@ nsXFormsModelElement::ProcessBindElement
>   nsCOMPtr<nsIDOMElement> firstInstanceRoot;
>   firstInstanceDoc->GetDocumentElement(getter_AddRefs(firstInstanceRoot));
> 
>-  nsresult rv;
>-  nsCOMPtr<nsIXFormsXPathEvaluator> xpath = 
>-           do_CreateInstance("@mozilla.org/dom/xforms-xpath-evaluator;1", &rv);
>-  NS_ENSURE_TRUE(xpath, rv);
>+  nsCOMPtr<nsIDOMXPathEvaluator> xpath = do_QueryInterface(firstInstanceDoc);
>+  nsCOMPtr<nsIXPathEvaluatorInternal> xpathInternal = do_QueryInterface(xpath);
>+  xpathInternal->RegisterExtensionFunctionsHelper(EmptyString(),
>+                                                  NS_LITERAL_CSTRING("@mozilla.org/xforms-xpath-functions;1"));

Can |firstInstanceDoc| ever be a document exposed to the user? If so I don't think we want to register extension functions on its evaluator. Seems like the safest is to always create a new evaluator.

This is something that we in general should try to prevent from accidentally happening. Maybe we should disable the ability to register extension functions on evaluators tied to a document. Extensions that want to add additional XPath functions should probably register in som global extensionfunction list that is shared across all evaluators.


>-nsXFormsModelElement::ProcessBind(nsIXFormsXPathEvaluator *aEvaluator,
>-                                  nsIDOMNode              *aContextNode,
>-                                  PRInt32                 aContextPosition,
>-                                  PRInt32                 aContextSize,
>-                                  nsIDOMElement           *aBindElement)
>+nsXFormsModelElement::ProcessBind(nsIDOMXPathEvaluator *aEvaluator,
>+                                  nsIDOMNode           *aContextNode,
>+                                  PRInt32              aContextPosition,
>+                                  PRInt32              aContextSize,
>+                                  nsIDOMElement        *aBindElement)
> {
>   // Get the model item properties specified by this \<bind\>.
>-  nsCOMPtr<nsIDOMNSXPathExpression> props[eModel__count];
>+  nsCOMPtr<nsIDOMXPathExpression> props[eModel__count];
>   nsAutoString propStrings[eModel__count];
>   nsresult rv;
>   nsAutoString attrStr;
> 
>+  nsCOMPtr<nsIDOMXPathNSResolver> resolver;
>+  aEvaluator->CreateNSResolver(aBindElement, getter_AddRefs(resolver));
>+
>+  nsCOMPtr<nsIXPathEvaluatorInternal> eval = do_QueryInterface(aEvaluator);
>+  eval->SetState(EmptyString(), aBindElement);
>+

Check the return value of these two calls?


reviewed up to: extensions/xforms/nsXFormsUtils.cpp

Comment 31

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

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

I think that it might be a good idea to assert if RegisterExtensionFunctionsHelper is called with a namespace and contract that already exists. Also, SetState doesn't deal too well with two users registring helpers for the same namespace. I wonder if it'd be a good idea to either pass in a contract+namespace (or just contract) rather then a namespace when setting state. Or disable support for multiple helpers for the same namespace entierly.

>Index: extensions/xforms/nsXFormsUtils.cpp
>@@ -395,13 +401,26 @@ nsXFormsUtils::EvaluateXPath(const nsASt
>   aContextNode->GetOwnerDocument(getter_AddRefs(doc));
>   NS_ENSURE_TRUE(doc, nsnull);
> 
>-  nsCOMPtr<nsIXFormsXPathEvaluator> eval = 
>-           do_CreateInstance("@mozilla.org/dom/xforms-xpath-evaluator;1");
>+  nsCOMPtr<nsIDOMXPathEvaluator> eval = do_QueryInterface(doc);
>   NS_ENSURE_TRUE(eval, nsnull);

Same thing here, you should probably create a new evaluator rather then registring extension functions on the doc. Also, isn't there a problem that you'll register the extension functions with the same evaluator multiple times otherwise?

>+  rv = evalInternal->SetState(EmptyString(), aResolverNode);
>+  NS_ENSURE_SUCCESS(rv, nsnull);

As things are now |aResolverNode| will never be released until the document dies which seems bad. That problem will go away if you instantiate a new evaluator though.

>+nsXFormsUtils::EvaluateXPath(nsIDOMXPathEvaluator   *aEvaluator,
...
>+  rv = nsExpression->EvaluateWithContext(aContextNode, aContextPosition,
>+                                         aContextSize, aResultType,
>+                                         aInResult, getter_AddRefs(supResult));
>+  if (supResult) {
>+    rv = CallQueryInterface(supResult, aResult);
>+  }
>+
>+  return rv;

Don't use supResult if rv is a failure code. Just do

  rv = nsExpression->EvaluateWithContext(...);
  NS_ENSURE_SUCCESS(rv, rv);

  return CallQueryInterface(supResult, aResult);

>+nsXFormsUtils::GetInstanceDocumentRoot(const nsAString &aID,
...
>+  if (element) {
>+    NS_IF_ADDREF(*aInstanceRoot = element);
>+  }

You can remove either the explicit |if| or make that NS_ADDREF.

I take it the remaining functions in this file are just moved from nsXFormsUtilityService.


Still looking at the TX_ResolveFunctionCallXPCOM stuff, but other then that it looks fine.

Comment 32

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

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

It'd be nice to support optional arguments by searching for a function with the right number of arguments, but I guess that is tricky since XPIDL doesn't allow multiple functions with the same name.

Another way to do optional arguments would be to stick an argumentCount on txIFunctionEvaluationContext.

Anyhow, it's probably unnecessary to worry about it until we need it.

Another thing I was thinking about is if it's worth caching the type data in the functioncall to avoid having to call into nsIInterfaceInfo all the time. Usually there are very few arguments so we wouldn't waste a lot of memory. Something to consider if performance comes up.

>Index: content/xslt/src/xpath/txXPCOMExtensionFunction.cpp

>+class txXPCOMExtensionFunctionCall : public FunctionCall
>+{
>+public:
>+    txXPCOMExtensionFunctionCall(nsISupports *aHelper, const nsIID &aIID,
>+                                 PRUint16 aMethodIndex,
>+#ifdef TX_TO_STRING
>+                                 PRInt32 aNamespaceID, nsIAtom *aName,
>+#endif

aNamespaceID isn't used. We can't really serialize prefixed functions currently :( Unless you want to stick the whole name (including prefix and colon) in the aName atom?

>+LookupFunction(const char *aContractID, nsIAtom* aName, nsIID &aIID,
...
>+    while ((letter = *name)) {
>+        if (letter == '-') {
>+            upperNext = PR_TRUE;
>+        }
>+        else {
>+            methodName.Append(upperNext ? nsCRT::ToUpper(letter) : letter);
>+            upperNext = PR_FALSE;
>+        }
>+        ++name;
>+    }

You should probably make sure that all characters in the name are ascii characters. If there is an UTF8 char in there you could end up calling ToUpper on the first byte in a multibyte character.


>+TX_HasFunctionCallXPCOM(const nsCString &aContractID, nsIAtom* aName,

This function seems unused

>+txXPCOMExtensionFunctionCall::evaluate(txIEvalContext* aContext,
...
>+    PRUint8 paramCount = methodInfo->GetParamCount();
>+    NS_ENSURE_FALSE(paramCount == 0, NS_ERROR_FAILURE);
>+
>+    // Assume we always need at least a return value and that retval is last
>+    // arg (the xpidl compiler ensures the latter).
>+    PRUint8 inArgs = paramCount - 1;
>+    if (paramCount == 0 || !methodInfo->GetParam(inArgs).IsRetval()) {
>+        return NS_ERROR_FAILURE;
>+    }

This part could be done in LookupFunction. You're also checking for |paramCount == 0| twice.

>+    txFunctionEvaluationContext *context;
>+    PRUint8 i = 0;
>+    if (type == CONTEXT) {
>+        // Create context wrapper.
>+        context = new txFunctionEvaluationContext(aContext, mState);
>+        if (!context) {
>+            return NS_ERROR_OUT_OF_MEMORY;
>+        }
>+
>+        nsXPTCVariant &invokeParam = invokeParams[0];
>+        invokeParam.type = paramInfo.GetType();
>+        invokeParam.SetValIsInterface();
>+        void *val = &invokeParam.val;
>+        NS_ADDREF(*((txIFunctionEvaluationContext**)val) = context);

How about this instead:

  invokeParam.val.p = (txIFunctionEvaluationContext*)context;
  NS_ADDREF(context);

Or, if it works:

  NS_ADDREF((txIFunctionEvaluationContext*&)invokeParam.val.p = context);

>+    txListIterator iter(&params);
>+    for (; i < inArgs; ++i) {
>+        if (!iter.hasNext()) {
>+            // XXX Missing a required argument.
>+            return NS_ERROR_FAILURE;
>+        }

This test isn't needed since you've already call requireParams.

>+        const nsXPTParamInfo &paramInfo = methodInfo->GetParam(i);
>+        if (!GetTag(paramInfo, info, type)) {
>+            return NS_ERROR_FAILURE;
>+        }
>+
>+        nsXPTCVariant &invokeParam = invokeParams[i];
>+        if (paramInfo.IsOut()) {
>+            // We don't support out values.
>+            return NS_ERROR_FAILURE;
>+        }

This test is missing when you're checking for a context argument.

>+        invokeParam.type = paramInfo.GetType();
>+        void *val = &invokeParam.val;
>+        switch (type) {
>+            case NODESET:
>+            {
>+                nsRefPtr<txNodeSet> nodes;
>+                rv = evaluateToNodeSet(expr, aContext, getter_AddRefs(nodes));
>+                NS_ENSURE_SUCCESS(rv, rv);
>+
>+                txNodeSetAdaptor *adaptor = new txNodeSetAdaptor(nodes);
>+                if (!adaptor) {
>+                    return NS_ERROR_OUT_OF_MEMORY;
>+                }
>+
>+                nsCOMPtr<txINodeSet> nodeSet = adaptor;
>+                rv = adaptor->Init();
>+                NS_ENSURE_SUCCESS(rv, rv);
>+
>+                invokeParam.SetValIsInterface();
>+                nodeSet.swap(*((txINodeSet**)val));
>+                break;

Wha, more casting evilness! Would the following work:

nodeSet.swap((txINodeSet*&)invokeParam.val.p);

>+            case BOOLEAN:
>+            {
>+                *((PRBool*)val) = evaluateToBoolean(expr, aContext);

invokeParam.val.b = evaluateToBoolean(expr, aContext);

>+                break;
>+            }
>+            case NUMBER:
>+            {
>+                *((double*)val) = evaluateToNumber(expr, aContext);

invokeParam.val.d = evaluateToNumber(expr, aContext);

>+                break;
>+            }
>+            case STRING:
>+            {
>+                nsString *value = new nsString();
>+                if (!value) {
>+                    return NS_ERROR_OUT_OF_MEMORY;
>+                }
>+
>+                evaluateToString(expr, aContext, *value);
>+
>+                invokeParam.SetValIsDOMString();
>+                *((const nsAString**)val) = value;

invokeParam.val.p = (nsAString*)value;

>+                break;
>+            }
>+            case RESULT_TREE_FRAGMENT:
>+            {
>+                return NS_ERROR_NOT_IMPLEMENTED;
>+            }

Just kill IMHO.

>+            case CONTEXT:
>+            default:
>+            {
>+                // We only support passing the context as the *first* argument.
>+                return NS_ERROR_FAILURE;
>+            }

You should be able to leave out the |default|. Would be nice to get a warning if we add a type to the enum but not to this switch.

>+    const nsXPTParamInfo &returnInfo = methodInfo->GetParam(inArgs);
>+    PRUint8 returnType;
>+    if (!GetTag(returnInfo, info, returnType)) {
>+        return NS_ERROR_FAILURE;
>+    }

I guess this call to GetTag makes it impossible to add an IsOut test in GetTag?

>+    nsXPTCVariant &returnParam = invokeParams[inArgs];
>+    returnParam.type = returnInfo.GetType();
>+    if (returnType == STRING) {
>+        nsString *value = new nsString();
>+        if (!value) {
>+            return NS_ERROR_FAILURE;
>+        }

Alternativly, you could instantiate a txStringResult and use its mValue directly. Though that'd complicate the txParamArrayHolder dtor.

>+        returnParam.SetValIsDOMString();
>+        returnParam.val.p = value;

Does this not need casting to nsAString?

>+    }
>+    else {
>+        returnParam.SetPtrIsData();
>+        returnParam.ptr = &returnParam.val;
>+    }
>+
>+    rv = XPTC_InvokeByIndex(mHelper, mMethodIndex, paramCount, invokeParams);

Might as well call ClearContext right away here so that we can early-return safly.

>+            case NUMBER:
>+            {
>+                aContext->recycler()->getNumberResult(returnParam.val.d,
>+                                                      aResult);
>+                break;
>+            }

You should check the return value from getNumberResult. You could return right here.

>+            case STRING:
>+            {
>+                nsString *returned = NS_STATIC_CAST(nsString*,
>+                                                    returnParam.val.p);
>+                aContext->recycler()->getStringResult(*returned, aResult);
>+                break;
>+            }

Same here.

>+enum txArgumentType {
>+    BOOLEAN = nsXPTType::T_BOOL,
>+    NUMBER = nsXPTType::T_DOUBLE,
>+    STRING = nsXPTType::T_DOMSTRING,
>+    NODESET,
>+    RESULT_TREE_FRAGMENT,
>+    CONTEXT
>+};

RTF seems unused and can't really happen until we support this stuff in XSLT. So just remove for now IMHO. If you stick UNKNOWN in there you could make GetTag return the type rather then use an outparam.

>+PRBool
>+txXPCOMExtensionFunctionCall::GetTag(const nsXPTParamInfo &aParam,
>+                                     nsIInterfaceInfo *aInfo, PRUint8 &aType)

I'd prefer not to rely on that the nsXPTType enum never grows beyond 8 bits. Especially since this is stack based.

Also, the name is not very descriptive for someone not familiar with XPT terms. How about GetParamType?


r=me

Comment 33

[Peter Van der Beken [:peterv]]

I wonder if it would make more sense to drop RegisterExtensionFunctionsHelper/SetState and instead pass in the namespace/contractid/state in a createExpression call. It would add a second createExpression type function, which I don't really like, but it would do away with the "persistence" of RegisterExtensionFunctionsHelper on the evaluator.

Comment 34

[Peter Van der Beken [:peterv]]

Attachment: v2

This implements the CreateExpression approach. I think it's nicer, for one the connection between an extension function component and its state is strong (one could have multiple components for the same namespace all having their own state). I made CreateExpression take arrays, let me know if you think that's overkill.

I also handled the other comments. I'm rerequesting review mainly because of the CreateExpression changes. I'm not sure what to do about the txDouble stuff :-/.

> You should probably make sure that all characters in the name are ascii
> characters. If there is an UTF8 char in there you could end up calling ToUpper
> on the first byte in a multibyte character.

AFAIK (XP)IDL only allows ASCII for the method name.

Comment 35

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

So what has changed in this patch compared to the last? Just the parsing bits and review comments?

Comment 36

[Peter Van der Beken [:peterv]]

(In reply to comment #35)
> So what has changed in this patch compared to the last? Just the parsing bits
> and review comments?

Yes. The interesting changes are in content/base/public/nsIXPathEvaluatorInternal.h and content/xslt/src/xpath/nsXPathEvaluator.cpp (and .h). Take a look at: https://bugzilla.mozilla.org/attachment.cgi?oldid=202531&action=interdiff&newid=219139&headers=1 ;-)

Comment 37

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

Comment on attachment 219139 [details] [diff] [review]
v2

Looks great! Just rename mState to mStates.

r=sicking

Comment 38

[Johnny Stenback (:jst)]

Comment on attachment 219139 [details] [diff] [review]
v2

sr=jst

Comment 39

[Alex Vincent [:WeirdAl]]

I see a checkin for this; can someone please write up a testcase / demo so I can see how I might use this?

Comment 40

[Peter Van der Beken [:peterv]]

(In reply to comment #39)
> I see a checkin for this; can someone please write up a testcase / demo so I
> can see how I might use this?

Look at the XForms code (see nsXFormsXPathFunctions and nsIXFormsXPathFunctions). I very much doubt that you'd need this.

Comment 41

[melkor]

I take it that this was never exposed to javascript.

I really wish it had been, I use XPath in Greasemonkey scripts to fix webpages and it would be handy to define a regexp match function (because you can do things with XPath you just can't do with Selectors) and this would make it all the more powerful.