299170 - Invalid <model> "functions=" attribute not caught

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Stephen Pride]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050509 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050509 Firefox/1.0.4

Using an invalid <model> functions="" attribute does not cause a problem.

I will attach a testcase to demonstrate.

Reproducible: Always

Comment 1

[Stephen Pride]

Attachment: XForms testcase

Comment 2

[aaronr]

That is right, we don't support the functions attribute on the model, yet. 
We'll need to come up with a way to ennumerate through all of the functions that
xpath supports and check them against the values of the functions attribute.

Comment 3

[Steve Speicher]

TestCase: 3.3.1.b
still valid

Comment 4

[Allan Beaufour]

(In reply to comment #2)
> That is right, we don't support the functions attribute on the model, yet. 
> We'll need to come up with a way to ennumerate through all of the functions that
> xpath supports and check them against the values of the functions attribute.

The attribute should only list needed "extension functions". Do Mozilla have support for _any_ extension functions right now, or is it only standard XPath 1.0?

If it is the latter, we should just (for now): if (model.getAttribute("functions") != "") handleFatalError()

Comment 5

[aaronr]

Well, XPath will be able to support all kinds of extension functions once bug 278981 goes in.  I don't know how close to 'final' that bug is, though, or whether it is targeted at FF 2.0, though.

Copying sicking to see what he thinks or if he knows of any non-standard xpath functions that they support.

Comment 6

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

We currently don't have any extension functions no (other then the xforms specific ones when you use xpath in xforms). I'm sure we could add support for querying for the existance of a function once we add support for extension functions.

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> We currently don't have any extension functions no (other then the xforms
> specific ones when you use xpath in xforms). I'm sure we could add support for
> querying for the existance of a function once we add support for extension
> functions.

Ok, so here's my plan: We check in a simple patch now that barfs if @function is non-empty, and create a follow up bug that depends on bug 278981. Ok?

Comment 8

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

Well, once you check for non-empty @function there is no more bug for now so technically none needs to be filed.

Even with bug 278981 fixed there would be no extension functions since that will only add the standard xforms functions, no?

Or am I missunderstanding which functions you're supposed to look for?

Comment 9

[Allan Beaufour]

(In reply to comment #8)
> Well, once you check for non-empty @function there is no more bug for now so
> technically none needs to be filed.
> 
> Even with bug 278981 fixed there would be no extension functions since that
> will only add the standard xforms functions, no?
> Or am I missunderstanding which functions you're supposed to look for?

"Extension functions". And will bug 278981 not allow you to add arbitrary extension functions?

Comment 10

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

Sure, it will allow it, but are you guys going to do that? Or are you just going to add the standard xforms functions?

Comment 11

[Allan Beaufour]

(In reply to comment #10)
> Sure, it will allow it, but are you guys going to do that? Or are you just
> going to add the standard xforms functions?

You never know. We're quite crazy you know!! :)

No, but would it be possible to define extension functions directly in the page, which could be used in XPath expressions in same?

Comment 12

[aaronr]

(In reply to comment #11)
> (In reply to comment #10)
> > Sure, it will allow it, but are you guys going to do that? Or are you just
> > going to add the standard xforms functions?
> 
> You never know. We're quite crazy you know!! :)
> 
> No, but would it be possible to define extension functions directly in the
> page, which could be used in XPath expressions in same?
> 


What I'd see as a more likely scenario is that say we upgrade to 1.1 and then XForms 1.2's spec comes out with some new XForms XPath functions.  Not wanting to wait for us, someone will write an extension to XPath providing these new functions and then people will want to use them on their pages.

Another possibility is that a paying customer (no, not just an IBM customer :=) wants some custom XPath functions as part of a web-based software solution involving XForms and Firefox.  So their consulting company builds an XPath extension for them and ties their internal XForms to the extension using @function.  This one is important to consider because the Mozilla XForms extension folks may never even know about the existance of these extension functions.

Comment 13

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

A little bit more then bug 278981 is going to be needed for stuff like that to work. Bug 278981 will do the bulk of the work, but if you guys want the ability to plug in additional functions when XForms XPath is executed you're going to have to add the hooks for that.

On the transformiix side we won't (yet) have the ability to plug in functions that are generally available, but that shouldn't be too hard to add.

Comment 14

[Allan Beaufour]

(In reply to comment #13)
> A little bit more then bug 278981 is going to be needed for stuff like that to
> work. Bug 278981 will do the bulk of the work, but if you guys want the ability
> to plug in additional functions when XForms XPath is executed you're going to
> have to add the hooks for that.
> 
> On the transformiix side we won't (yet) have the ability to plug in functions
> that are generally available, but that shouldn't be too hard to add.

Ok. Thanks for all the info Jonas.

Conclusion is, that for now there is _no_ way of adding plug in functions that are generally available -- without Transformiix or XForms does some magic. This means that we can add what I suggested in comment 4, and close this bug until the magic arrives :)

Comment 15

[Allan Beaufour]

Besides that this might depend on bug 315712, it should be fairly easy then. DispatchError() and HandleFatalError(), possibly in 
nsXFormsModelElement::MaybeNotifyCompletion(), if any model has a function attribute != "".

Comment 16

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

> Conclusion is, that for now there is _no_ way of adding plug in functions that
> are generally available -- without Transformiix or XForms does some magic. This
> means that we can add what I suggested in comment 4, and close this bug until
> the magic arrives :)

Yup. When we do add that magic to transformiix we'll be sure to include the hooks for you guys (we'll need them for xslt too since it has a similar feature)

Comment 17

[Stephen Pride]

Could someone critic the code below to see if I'm on the right track with this bug?  From Allan's comment #15, it sounds like we just need to check if the functions attribute, if it exists in the model element, has a value.  If the value is empty then throw an error.  Note: This is my first attempt at fixing a Mozilla bug, so don't hold back on the comments. :-)

   // Nothing to be done if any model is incomplete or hasn't seen
   // DOMContentLoaded.
   for (i = 0; i < models->Count(); ++i) {
     nsXFormsModelElement *model =
         NS_STATIC_CAST(nsXFormsModelElement *, models->ElementAt(i));
     if (!model->mDocumentLoaded || !model->IsComplete())
       return;
+
+    // XXX: (stp) barf if model function attribute exists but is emtpy
+    PRBool tmp;
+    nsresult rv = mElement->HasAttribute(NS_LITERAL_STRING("functions"), &tmp);
+    // XXX: (stp) should we report some fatal error like the following if rv fails,
+    //            or just return "silently"?
+    //
+    //      nsXFormsUtils::ReportError(NS_LITERAL_STRING("getAttributeError"), mElement);
+    //      nsXFormsUtils::DispatchEvent(mElement, eEvent_LinkException);
+    if (NS_FAILED(rv))
+      return;
+    // XXX: (stp) if "functions" exists, check if it is empty
+    if ( tmp ) {
+      nsAutoString list;
+      nsIDOMElement* tElement = model->mElement;
+      tElement->GetAttribute(NS_LITERAL_STRING("functions"), list);
+      if (list.IsEmpty()) {
+         // XXX: (stp) not sure what should be reported (or even if the process is right)
+         nsXFormsUtils::ReportError(NS_LITERAL_STRING("attributeMissingValueError"), mElement);
+         nsXFormsUtils::DispatchEvent(mElement, eEvent_LinkException);
+         if (!nsXFormsUtils::HandleFatalError(mElement,
+                                              NS_LITERAL_STRING("XFormsLinkException"))) {
+             return;
+         }
+      }
+    }
   }
 
   // Okay, dispatch xforms-model-construct-done and xforms-ready events!
   for (i = 0; i < models->Count(); ++i) {
     nsXFormsModelElement *model =
         NS_STATIC_CAST(nsXFormsModelElement *, models->ElementAt(i));
     nsXFormsUtils::DispatchEvent(model->mElement, eEvent_ModelConstructDone);
   }

Comment 18

[Allan Beaufour]

(In reply to comment #17)
> Could someone critic the code below to see if I'm on the right track with this
> bug?  From Allan's comment #15, it sounds like we just need to check if the
> functions attribute, if it exists in the model element, has a value.  If the
> value is empty then throw an error.

No, the other way around. I'm not even sure that "" is a valid xsd:list, so maybe we should fail just if the attribute is present. And it is a fatal error.

> +    // XXX: (stp) barf if model function attribute exists but is emtpy

Use XXX for stuff to do or bugs.

> +    PRBool tmp;

Use a more descriptive name than "tmp"

And please attach a patch instead of cut'n'paste. Then we can use all the facilities that Bugzilla offers for that, and apply the patch, etc.

Comment 19

[aaronr]

> I'm not even sure that "" is a valid xsd:list, so
> maybe we should fail just if the attribute is present. And it is a fatal error.

In 7.12 of the spec it says, "XForms Processors perform this check at the time the document is loaded, and stop processing by signaling an exception (4.5.4 The xforms-compute-exception Event) if the XForms document declares an extension function for which the processor does not have an implementation."

So I'd think that we don't throw the fatal error unless @function exists and also has a non-empty value.  If this is what we go with, should we still put an error in the JS console if @functions exists and is empty?

Comment 20

[Allan Beaufour]

(In reply to comment #19)
> > I'm not even sure that "" is a valid xsd:list, so
> > maybe we should fail just if the attribute is present. And it is a fatal error.
> 
> In 7.12 of the spec it says, "XForms Processors perform this check at the time
> the document is loaded, and stop processing by signaling an exception (4.5.4
> The xforms-compute-exception Event) if the XForms document declares an
> extension function for which the processor does not have an implementation."
> 
> So I'd think that we don't throw the fatal error unless @function exists and
> also has a non-empty value.  If this is what we go with, should we still put an
> error in the JS console if @functions exists and is empty?

If "" is an invalid xsd:list imho, we should bail if it exists at all. If "" is valid, I think we should do nothing until != "".

Comment 21

[aaronr]

(In reply to comment #20)
> (In reply to comment #19)
> > > I'm not even sure that "" is a valid xsd:list, so
> > > maybe we should fail just if the attribute is present. And it is a fatal error.
> > 
> > In 7.12 of the spec it says, "XForms Processors perform this check at the time
> > the document is loaded, and stop processing by signaling an exception (4.5.4
> > The xforms-compute-exception Event) if the XForms document declares an
> > extension function for which the processor does not have an implementation."
> > 
> > So I'd think that we don't throw the fatal error unless @function exists and
> > also has a non-empty value.  If this is what we go with, should we still put an
> > error in the JS console if @functions exists and is empty?
> 
> If "" is an invalid xsd:list imho, we should bail if it exists at all. If "" is
> valid, I think we should do nothing until != "".
> 


looking at 2.5.1 in the schema spec (http://www.w3.org/TR/xmlschema-2/#dt-list), "[Definition:]  List datatypes are those having values each of which consists of a finite-length (possibly empty) sequence of values of an ·atomic· datatype."  So it looks like a list can be empty.

Comment 22

[Allan Beaufour]

(In reply to comment #21)
> looking at 2.5.1 in the schema spec
> (http://www.w3.org/TR/xmlschema-2/#dt-list), "[Definition:]  List datatypes are
> those having values each of which consists of a finite-length (possibly empty)
> sequence of values of an ·atomic· datatype."  So it looks like a list can be
> empty.

Ok, So GetAttribute() != EmptyString() -> bail. (thanks for finding the ref.)

Comment 23

[Stephen Pride]

Attachment: proposed patch

proposed fix

Comment 24

[Allan Beaufour]

Comment on attachment 221457 [details] [diff] [review]
proposed patch

>+    // Check validity of |functions=| attribute, if it exists.  Since we
>+    // don't support ANY extension functions currently, the existance of
>+    // value(s) in the attribute is an error.  Note: If attribute exists,
>+    // but is empty, it IS a valid operation.
>+    PRBool hasAttribute = PR_FALSE;
>+    nsCOMPtr<nsIDOMElement> tElement = model->mElement;
>+    nsresult rv = tElement->HasAttribute(NS_LITERAL_STRING("functions"),
>+                                         &hasAttribute);
>+    if (NS_FAILED(rv))
>+      return;
>+    if (hasAttribute) {

You do not need to call HasAttribute() first. GetAttribute() returns an empty string if the attribute does not exist.

>+      nsAutoString list;

nit: could you call it something else than "list"? It is a string and not actually a list.

>+      tElement->GetAttribute(NS_LITERAL_STRING("functions"), list);
>+      if (!list.IsEmpty()) {
>+        nsXFormsUtils::ReportError(NS_LITERAL_STRING("invalidExtFunction"),
>+                                   tElement);
>+        nsXFormsUtils::DispatchEvent(tElement, eEvent_ComputeException);
>+        nsXFormsUtils::HandleFatalError(tElement,
>+                                        NS_LITERAL_STRING("XFormsComputeException"));
>+        return;
>+      }
>+    }

With those two fixes r=me.

Comment 25

[Stephen Pride]

Attachment: proposed patch

Doron - can you do a second review when you have time?  Thanks!

Comment 26

[aaronr]

checked into trunk for stpride