The default bug view has changed. See this FAQ.

Invalid <model> "functions=" attribute not caught

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
8 months ago

People

(Reporter: Stephen Pride, Assigned: Stephen Pride)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
fixed1.8.0.5, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

614 bytes, application/xhtml+xml
Details
3.72 KB, patch
Doron Rosenberg (IBM)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
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
(Assignee)

Comment 1

12 years ago
Created attachment 187692 [details]
XForms testcase
(Assignee)

Updated

12 years ago
Attachment #187692 - Attachment description: Demonstrates invalid <model> functions="" attribute not causing a problem. → XForms testcase

Comment 2

12 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

11 years ago
Blocks: 322255

Updated

11 years ago
Blocks: 326372

Updated

11 years ago
Blocks: 326373

Comment 3

11 years ago
TestCase: 3.3.1.b
still valid

Comment 4

11 years ago
(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

11 years ago
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.
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

11 years ago
(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?
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

11 years ago
(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?

Updated

11 years ago
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

11 years ago
(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

11 years ago
(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.
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

11 years ago
(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

11 years ago
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 != "".
Whiteboard: [good first bug]
> 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)
(Assignee)

Comment 17

11 years ago
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

11 years ago
(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

11 years ago
> 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

11 years ago
(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

11 years ago
(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

11 years ago
(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.)
(Assignee)

Comment 23

11 years ago
Created attachment 221457 [details] [diff] [review]
proposed patch

proposed fix

Comment 24

11 years ago
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.
Attachment #221457 - Flags: review+
(Assignee)

Comment 25

11 years ago
Created attachment 221595 [details] [diff] [review]
proposed patch

Doron - can you do a second review when you have time?  Thanks!
Attachment #221457 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #221595 - Attachment description: Fixes Allan's review coments. → proposed patch
Attachment #221595 - Flags: review?(doronr)

Updated

11 years ago
Attachment #221595 - Flags: review?(doronr) → review+

Updated

11 years ago
Assignee: aaronr → stpride

Comment 26

11 years ago
checked into trunk for stpride
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug] → xf-to-branch

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.0.5

Updated

11 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.