430810 - illegal ref for xf:group crashes Firefox

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Ken Yap]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1.13) Gecko/20080316 SUSE/2.0.0.13-0.1 Firefox/2.0.0.13
Build Identifier: Firefox 2.0.0.13, Mozilla XForms 0.8.5

In an attempt to get shading on alternate rows, I entered this illegal XPath expression for the ref of an xf:group.

            <xf:group ref="(position() mod 2) = 0">
              <tr>
                <th><xf:output value="text" /></th>
                <td>
                  <xf:select1 appearance="full" ref="value">
                    <xf:itemset class="horiz" nodeset="instance('lists')/treatme
nts/treatment">
                      <xf:label ref="label" />
                      <xf:value ref="value" />
                    </xf:itemset>
                  </xf:select1>
                </td>
              </tr>
            </xf:group>

When this XHTML was rendered the browser exited.


Reproducible: Always

Steps to Reproduce:
1. Embed the posted XHTML in an XForm
2. Navigate to page with Firefox

Actual Results:  
Browser exited

Expected Results:  
Error report in Error console and no exit

Comment 1

[aaronr]

Please attach a standalone testcase that recreates the problem.  My attempt at creating a testcase from your code snippet didn't crash.

Comment 2

[Ken Yap]

Attachment: form with illegal group ref that crashes Firefox

Comment 3

[aaronr]

The problem causing the crash is that somehow the itemset is getting added to the model's control list before the select1 that contains it.  This is badness.  Things have to be added to the control list in document order.  I don't have time to look at this now.

Comment 4

[Sergey Reymerov]

nsXFormsModelElement::InitializeControls calls ~nsPostRefresh. This destructor calls nsXFormsControlListItem::AddControl. nsXFormsControlListItem::AddControl says "Parent not found?!" and Firefox crashes.
I think select1 refresh (it in group refresh) forget garbage that break Firefox later.

Comment 5

[Sergey Reymerov]

I trying fix this bug. 
Crash occurs in ItemSet refreshing when "Parent not found?!" in nsXFormsControlListItem::AddControl (mControlListHash doesn't contain element of control).
Should be refresh be actuated when errors occur in bind (ref) relations?

Comment 6

[Sergey Reymerov]

Attachment: solution

I just added examine if parent found for element or not. If not found then break adding control.
Error message
> Error: XForms Error (15): Could not bind control to instance data
> Source File: file:///home/sergey/test.xhtml
> Line: 0
> Source Code: <xf:group ref="(position() mod 2) = 0"/>
prints twice because it makes xf:repeat.

Comment 7

[Sergey Reymerov]

Attachment: solution

Comment 8

[aaronr]

Olli knows this area of the code better than me, but it looks like this patch is just working around the assertion.  The real problem is why is the itemset getting added to the control list before the select1 control.  XForms controls should be added to the list in document order.  Is the select1 control going through AddControl and bouncing out for some reason without being added to the list?

Comment 9

[Sergey Reymerov]

I don't think that there is wrong order of adding controls. With patch I've get page with normal <select1> with 3 items (for each instance('lists')/treatments/treatment).

Comment 10

[Sergey Reymerov]

To show order of adding I add to ::AddControl output of tagname of processing aControl.
> AddControl xf:repeat
> AddControl contextcontainer
> AddControl xf:itemset
> AddControl item
> AddControl xf:label
> AddControl item
> AddControl xf:label
> AddControl item
> AddControl xf:label
> AddControl xf:itemset
> AddControl item
> AddControl xf:label
> AddControl item
> AddControl xf:label
> AddControl item
> AddControl xf:label

Comment 11

[Sergey Reymerov]

Creating and refreshing of Repeat, Group, Select1 and ItemSet elements view like next calls:
> Repeat NEW
> Group NEW
> Select1 NEW
> ItemSet NEW
> ItemSet Refresh()
> Repeat NEW
> Repeat Refresh()
> Group NEW
> Select1 NEW
> Select1 Refresh()
> ItemSet NEW
> ItemSet Refresh()
> ItemSet Refresh()
> ItemSet Refresh()
> ItemSet Refresh()
> ItemSet Refresh()
> ItemSet Refresh()
> Select1 Refresh()
> Select1 Refresh()
> Group NEW
> Select1 NEW
> Select1 Refresh()
> ItemSet NEW
> ItemSet Refresh()
> ItemSet Refresh()
> ItemSet Refresh()
> ItemSet Refresh()
> ItemSet Refresh()
> ItemSet Refresh()
> Select1 Refresh()
> Select1 Refresh()
> Select1 Refresh()
> ItemSet Refresh()
-- crash --

Comment 12

[Sergey Reymerov]

Unfortunately I confused.Aaron could you explain where select1 added to control list? Select1 doesn't process in AddControl.

Comment 13

[aaronr]

Controls are added to the controls list contained by the model when the binding on the control is resolved.

How things normally happen is that nsXFormsControlStub::ProcessNodeBinding calls nsXFormsControlStub::MaybeAddToModel which should call AddFormControl on its model which should call AddControl on the list.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Uh, this is so Allan's code. I haven't looked at this code for ages.

Sergey, could you explain why parentControl is null in this case?

Comment 8 sounds still valid. Especially the last sentence.

Comment 15

[Sergey Reymerov]

I walk throw calls of AddControl and I've found that error occurs in nsXFormsControlStub::ProcessNodeBinding:
> WARNING NS_ENUSRE_SUCCESS(rv, rv) failed with result 0x805B0034: file ../nsXFormsControlStub.cpp, line 402
> WARNING NS_ENUSRE_SUCCESS(rv, rv) failed with result 0x805B0034: file ../nsXFormsUtils.cpp, line 789
> WARNING NS_ENUSRE_SUCCESS(rv, rv) failed with result 0x805B0034: file ../nsXPathExpression.cpp, line 182
Callstack for clarify:
> nsXFormsControlStub::ProcessNodeBinding line 402
> nsXFormsUtils::EvaluateNodeBinding line 789
> nsXPathExpression::EvaluateWithContext line 182
With normal ref of xf:group NS_ENSURE_SUCCES doesn't fail but with ref from example it fails.

MaybeAddToModel doesn't process because ProcessNodeBinding fails.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Could you actually upload full stack (as an attachment).

Comment 17

[Sergey Reymerov]

Attachment: Callstack for NS_ENSURE_SUCCESS

Comment 18

[Sergey Reymerov]

When I write "abrakadabra" to ref (not valid xpath expression) group doesn't render (it skips from tree), crash doesn't occur and Error Console is empty (looks like another or even this bug). 

Aaron, Olli: What must be with group element (with bad ref): render or not?

Comment 19

[aaronr]

Read this: http://www.w3.org/TR/xforms11/#ui-group

If the xpath expression resolves to an empty nodeset, the group is considered non-relevant and thus all of its contents are considered non-relevant.  Non-relevant elements are hidden by default.  So a non-relevant group not showing up is not a bug.

Comment 20

[Sergey Reymerov]

Attachment: solution

I've made another patch that solves problem. Firefox doesn't crash and group doesn't render.

Comment 21

[Sergey Reymerov]

Comment on attachment 447704 [details] [diff] [review]
solution

This is an aproximate patch. (I forgot remove some tracking output)

Comment 22

[aaronr]

Comment on attachment 447704 [details] [diff] [review]
solution

I don't think that this is the proper behavior, but you'll have to double check.  With this patch, you'll be throwing a xforms-compute-exception for this expression.  But I don't think that the expression meets the criteria for it listed here: http://www.w3.org/TR/xforms11/#expr.  I believe that the expression is syntactically correct, the namespaces are in scope and the functions and variables are defined.

Comment 23

[Sergey Reymerov]

I think that XPath from testcase is not syntactically correct. 
"(position() mod 2) = 0" doesn't correspond to XPath specification. 
So, I think that it is must be broke with xforms-compute-exception event.

Comment 24

[aaronr]

I'm sorry, I don't see what is wrong with the xpath expression.  Can you please explain where you think that it is incorrect?  position() returns a number.  mod returns a number.  And the result is tested against a number.  The test seems ok as defined in the xpath spec -> "When neither object to be compared is a node-set and the operator is = or !=, then the objects are compared by converting them to a common type as follows and then comparing them. If at least one object to be compared is a boolean, then each object to be compared is converted to a boolean as if by applying the boolean  function. Otherwise, if at least one object to be compared is a number, then each object to be compared is converted to a number as if by applying the number function."  So this expression seems ok to me.

And remember, this isn't just about the expression in the testcase.  With this patch, any time xpath returned NS_ERROR_DOM_TYPE_ERR, a xforms-compute-exception will be thrown.  I'm sure that NS_ERROR_DOM_TYPE_ERR can be returned in more situations than just those attributed to the xforms-compute-exception.

Comment 25

[Alexey Gaidukov]

According section 9.1.1 the group Element can have Single-Node Binding attribute. In the section 3.2.3 - "When ref is used, the node is determined by evaluating the XPath expression with the evaluation context described in Section 7.2 Evaluation Context". In section 7.2 - "A binding expression attribute contains an XPath expression that references zero or more nodes of instance data."

Expression "(position() mod 2) = 0" can't reference nodes because it has boolean type. 

This expression is correct XPath expression but it is not a binding expression.

Section "4.5.1 The xforms-binding-exception Event" - "Dispatched as an indication of: an illegal binding expression". In the testcase we have illegal binding expression. So I guess the exception xforms-binding-exception should be fired.

Comment 26

[aaronr]

I can't remember if this is considered an 'illegal binding expression' or not.  I don't think it is but I wrote to the w3c to get a clarification of what should happen if a binding expression is used that doesn't return a node-set.

Comment 27

[Sergey Reymerov]

Attachment: another patch

Similar patch for previous. I made this patch just to share my thoughts. 
I've read http://lists.w3.org/Archives/Public/www-forms-editor/2007Apr/0059.html (link from http://lists.w3.org/Archives/Public/www-forms/2010Jun/thread.html) and decide that xforms-binding-exception is acceptable behaviour. 
So, we must catch case when result type differ from type node (result type must be nodeset and resulted node type is boolean).
I think that such expression is dynamic error (definition in link above). So, we must run EvaluateWithContext that breaks with NS_ERROR_DOM_TYPE_ERR.
What next?! Group could add to control list (such in this patch). Or Group must skip control list and in this case we must don't add all childrens of group.
What behaviour of runtime and processing when I fire DispatchEvent(model, eEvent_BindingException,...) ? Is it only alert that appeared xforms-binding-exception or something more?

Comment 28

[aaronr]

Comment on attachment 451949 [details] [diff] [review]
another patch

I'm on the fence.  I don't see anything wrong with generating a xforms-binding-exception in the case where the result type is different than nodeset, however xforms-binding-exception is a fatal exception and form processing will stop.  Which seems kinda drastic, but I guess it is better than letting the form developer maybe continue on not knowing anything is wrong with his form.  I'd like Olli and Alex to sign off on this new behavior before we commit it.

As far as the control list, I don't know that it matters since form processing should halt.  It should be consistent with what happens in the other situations where we generate a xforms-binding-exception.

When DispatchEvent for xforms-binding-exception happens, the popup alert should show.  Also, the event should be dispatched to the proper element (the element containing the binding attribute with the bad expression).  And form processing should halt like it happens with all other fatal exceptions.  Please verify that this happens with your patch even if the expression is in a xf:bind element as well as a control.

Also please verify that NS_ERROR_DOM_TYPE_ERR is only returned because we asked for a result type different than what we got and that it can't be generated under other circumstances.  If it can be returned for other reasons, we'll have to tighten our code to test specifically for this case.

Please verify these things and then, if the patch doesn't need changing, re-ask for review.  Thanks.

Comment 29

[Sergey Reymerov]

Attachment: another patch 2

NS_ERROR_DOM_TYPE_ERR returns only in case when resulted type is different than result type that needed. 
So test for NS_ERROR_DOM_TYPE_ERR is acceptable to catch binding exception but I think that this check is not for nsXFormsUtils::EvaluateXPath, for example if aResultType is number and expression refer to string then EvaluateWithContext breaks with NS_ERROR_DOM_TYPE_ERR too and EvaluateXPath fire xforms-binding-exception that are wrong.
Well I've removed check&fire to EvaluateNodeBinding that calls EvaluateXPath.
Now I have some trubles with it that (I skip it before).
Alert message is empty (form with alert doesn't contain error message). 
Error console contains four messages: "Error: XForms Error (11): Error evaluating XPath expression: (position() mod 2) = 0".
Such expression in bind makes normal exception (alert with text and normal text in error console).

Comment 30

[aaronr]

Comment on attachment 452214 [details] [diff] [review]
another patch 2

>diff -r f51a343e8228 nsXFormsUtils.cpp
>--- a/nsXFormsUtils.cpp	Tue Apr 27 21:11:07 2010 +0900
>+++ b/nsXFormsUtils.cpp	Fri Jun 18 16:45:43 2010 +0900

>+      nsCOMPtr<nsXFormsContextInfo> contextInfo =
>+        new nsXFormsContextInfo(aElement);
>+      NS_ENSURE_TRUE(contextInfo, NS_ERROR_OUT_OF_MEMORY);
>+      contextInfo->SetStringValue("error-message", errorMsg);
>+      nsCOMArray<nsIXFormsContextInfo> contextInfoArray;
>+      contextInfoArray.AppendObject(contextInfo);
>+      DispatchEvent(model, eEvent_BindingException, nsnull, aElement,
>+                    &contextInfoArray);
>+

DispatchEvent here is incorrect.  The event should target the element that the binding expression is on, not the model.

With this fixed, r=me

Comment 31

[Sergey Reymerov]

I've change it to:
DispatchEvent(aElement, eEvent_BindingException);
And it still doesn't work correctly.

Comment 32

[Sergey Reymerov]

I've understood why alert's blank. 
Alert processing breaks with next:
###!!! ASSERTION: This is unsafe: 'nsContentUtils::IsSafeToRunScript()', file .../layout/base/nsDocumentViewer.cpp, line 1111

Comment 33

[Sergey Reymerov]

DispatchEvent calls below are normally displayed. And how I could fix my DispatchEvent?

Comment 34

[Sergey Reymerov]

Attachment: patch 3

Solution with normal alert. Now DispatchEvent calls async and IsSafeToRunScript doesn't curse.

Comment 35

[aaronr]

Comment on attachment 454501 [details] [diff] [review]
patch 3

>diff -r f51a343e8228 nsXFormsUtils.cpp
>--- a/nsXFormsUtils.cpp	Tue Apr 27 21:11:07 2010 +0900
>+++ b/nsXFormsUtils.cpp	Mon Jun 28 22:06:08 2010 +0900

>@@ -786,7 +800,28 @@ nsXFormsUtils::EvaluateNodeBinding(nsIDO
>   rv  = EvaluateXPath(expr, contextNode, aElement, aResultType,
>                       getter_AddRefs(res), contextPosition, contextSize,
>                       aDeps, aIndexesUsed);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  if (NS_FAILED(rv)) {
>+    // Case when bind expression doesn't have rational result type.
>+    if (rv == NS_ERROR_DOM_TYPE_ERR) {
>+      const nsString& flat = PromiseFlatString(expr);
>+      const PRUnichar *strings[] = { flat.get() };
>+      nsXFormsUtils::ReportError(NS_LITERAL_STRING("exprEvaluateError"),
>+                                strings, 1, aElement, nsnull);
>+
>+      class AsyncBindingExceptionDispatcher : public nsRunnable {
>+      public:
>+        AsyncBindingExceptionDispatcher(nsIDOMNode* aTarget):aTarget(aTarget) {}
>+        NS_IMETHOD Run() {
>+          return nsXFormsUtils::DispatchEvent(aTarget, eEvent_BindingException);
>+        }
>+      private:
>+        nsIDOMNode* aTarget;
>+      };
>+      nsCOMPtr<nsIRunnable> ev = new AsyncBindingExceptionDispatcher(aElement);
>+      return NS_DispatchToCurrentThread(ev);
>+    }
>+    return rv;
>+  }
> 

I'm guessing that you didn't mean to leave in this inline class since you've already defined AsyncBindingExceptionDispatcher?

I think that I'd also like it better if you generalized this class so that it could potentially be used for other events than just binding exception.  Maybe call it AsyncEventDispatcher and give it an event member?

Could you explain why we need this exception dispatched asynchronously?  Do we need to do this with any of our other events?

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> Could you explain why we need this exception dispatched asynchronously?  Do we
> need to do this with any of our other events?

If IsSafeRunScript return false then it means it might be a security problem. If we are dispatching events to script then it should be done asynchroniously to avoid this. I think Olli can give more detailed information.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #36)
> (In reply to comment #35)
> > Could you explain why we need this exception dispatched asynchronously?  Do we
> > need to do this with any of our other events?
> 
> If IsSafeRunScript return false then it means it might be a security problem.
> If we are dispatching events to script then it should be done asynchroniously
> to avoid this. I think Olli can give more detailed information.

Technically if we're notified by Gecko at unsafe time then everything that can be handled by script should be done async.

Comment 38

[aaronr]

Ok, then I'd suggest as part of this bug fix that DispatchEvent is made smarter and that if it tries to dispatch an event such that IsSafeRunScript fails as unsafe, then it'll dispatch it async instead.  Or it could be done in a separate bug.  But I think that would be better than special casing it just for this particular binding exception.

Comment 39

[Sergey Reymerov]

Attachment: patch

I've found in nsXFormsUtils::DispatchEvent testing:
> modelPriv->GetHasDOMContentFired(&safeToSendEvent);
And if (!safeToSendEvent) then it calls DeferDispatchEvent else it calls original DispatchXFormsEvent,
In this bug DeferDispatchEvent is called (it's rightfully). But HandleFatalError calls OpenDialog that breaks (HandleFatalError is called from DeferDispatchEvent).
I think that OpenDialog could be asynchronous everytime. Another processing in HandleFatalError and DeferDispatchEvent are normal worked. So I've made this patch with just asynchronous OpenDialog.

Comment 40

[aaronr]

Comment on attachment 454798 [details] [diff] [review]
patch

looks ok to me

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 454798 [details] [diff] [review]
patch


>+// Class that opens popup dialog with fatal error. It is used in HandleFatalError
>+// for asynchronously opening dialog window.
>+class AsyncErrorPopupOpener : public nsRunnable {
{ should be in the next line



>+public:
>+  AsyncErrorPopupOpener(nsIDOMWindowInternal* aInternal,
>+                        nsIDOMWindow* aWindow,
>+                        const nsAString &aName) {
Same here

>+    mName = aName;
>+    mInternal = aInternal;
>+    mWindow = aWindow;
>+  }
>+  NS_IMETHOD Run() {
>+    return mInternal->OpenDialog(NS_LITERAL_STRING("chrome://xforms/content/bindingex.xul"),
>+                                mName,
>+                                NS_LITERAL_STRING("modal,dialog,chrome,dependent"),
>+                                nsnull,
>+                                getter_AddRefs(mWindow));
Align parameters properly

Comment 42

[Sergey Reymerov]

Attachment: styled patch

Comment 43

[alexander :surkov (:asurkov)]

landed - http://hg.mozilla.org/xforms/rev/998ab396abb6, unfortunately I landed it under my name, sorry for that.