321876 - Handle empty instances (they are invalid)

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051219 SeaMonkey/1.0b
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051219 SeaMonkey/1.0b

<xf:model id="model">
	<xf:instance id="data"/>
</xf:model>

<script>
function onload()
{
  var model = document.getElementById('model');
  var doc = model.getInstanceDocument('data');
  alert(doc.documentElement);  // it is null
}

I guess instance should create document with root element "instanceData".
</script>

Reproducible: Always

Comment 1

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

Hmm, should it?
Because <model> contains as <instance> it is not exactly 'lazy authored'.
'Lazy authored' is something where <model> doesn't have <instance> as a child element.

Also
"If the instance referenced on the form control did not exist when the first form control for the same instance was processed:
For the first reference to an instance a default instance is created by following the rules described below.
 1. A root instanceData element is created. ..."

Marking INVALID, but please re-open if you think there is a reason to create
the documentElement.
What does other XForms implementations do?

Comment 2

[alexander :surkov (:asurkov)]

I think it's not correct behaviour because document is created for such "lazy authored" instance but documentElement is null. It means I cannot append new elements and threafore the document is dead. I reopen the bug. If you think that we shouldn't create "good" documentElement or if the document can be restored to life then mark as invalid.

Comment 3

[aaronr]

It would be my opinion that having an empy instance element would be invalid XForms.  Here is my reasoning:

In http://www.w3.org/TR/2005/PER-xforms-20051006/index-all.html#structure-model-instance, the spec says that "If the initial instance data is given by inline content, then instance data is obtained by first creating a detached copy of the inline content (including namespaces inherited from the enveloping ancestors), then creating an XPath data model over the detached copy. The detached copy must consist of content that would be well-formed XML if it existed in a separate document. Note that this restricts the element content of instance to a single child element."

Looking at the W3C XML spec: http://www.w3.org/TR/REC-xml/#sec-documents, it looks to me that a XML document requires a root element to be considered 'well formed'.  Which means that any xforms instance document must contain at least a root element.

So I would think in the example given, XForms processing should halt with a xforms-link-exception.  Which is, indeed, the error that the Novell processor gives.  XSmiles also won't process my test form, though it displays its own processing error message.  formsPlayer seems to let it slide, though.  We currently don't do this, but it should probably be done inside nsXFormsInstanceElement::CloneInlineInstance(), I'd think.  Or have CloneInlineInstance return an error and have ::Initialize() do it.

I'll ask in the news group to see if the formsPlayer guys have a reason that they don't generate the processing error.

Comment 4

[aaronr]

Changing summary since an empty instance isn't really part of lazy authoring.

Comment 5

[Allan Beaufour]

(In reply to comment #3)
> So I would think in the example given, XForms processing should halt with a
> xforms-link-exception.

I agree with Aaron's reasoning. I've taken the liberty of changing the summary to reflect this.

Comment 6

[Allan Beaufour]

I now get crashes on controls not getting bound because of an invalid instance .... I think. I'm investigating.

I think this needs to be caugh before reaching the fix in ValidateDocument() (bug 333113).

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> I now get crashes on controls not getting bound because of an invalid instance
> .... I think. I'm investigating.
> 
> I think this needs to be caugh before reaching the fix in ValidateDocument()
> (bug 333113).

Or maybe it is actually a fatal error, and the processing should stop?

Comment 8

[Allan Beaufour]

Attachment: Crashes trunk

I'm not sure exactly what makes this crash, but it does. It's the same about the parent not being found, which is probably because some parent error'ed out because the instance() function is assuming something it should not:

WARNING: NS_ENSURE_TRUE(instanceRoot) failed: file txXFormsFunctionCall.cpp, line 328
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file txPathExpr.cpp, line 113
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file nsXPathExpression.cpp, line 138
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file nsXFormsUtils.cpp, line 581
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file nsXFormsControlStub.cpp, line 319
ERR: XForms Error (15): Could not bind control to instance data

Comment 9

[aaronr]

(In reply to comment #7)
> (In reply to comment #6)
> > I now get crashes on controls not getting bound because of an invalid instance
> > .... I think. I'm investigating.
> > 
> > I think this needs to be caugh before reaching the fix in ValidateDocument()
> > (bug 333113).
> 
> Or maybe it is actually a fatal error, and the processing should stop?
> 


from 4.2.1 and 3.3.2...if the instance data is not there or not well formed, we should send the xforms-link-exception event (fatal error).

Comment 10

[Allan Beaufour]

Attachment: Patch

Makes CloneInlineInstance() dispatch link-exception, and report and return an error if 1) there's no child element or 2) there are multiple child elements.
(Not tested in practice because of bug 337999)

We might need to check for the fatalError flag more often, in general. The model should refuse to do anything when a fatal error has occurred. That is a different bug though.

Comment 11

[aaronr]

Comment on attachment 222047 [details] [diff] [review]
Patch

>Index: nsXFormsModelElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v
>retrieving revision 1.115
>diff -u -p -U8 -r1.115 nsXFormsModelElement.cpp
>--- nsXFormsModelElement.cpp	15 May 2006 14:14:52 -0000	1.115
>+++ nsXFormsModelElement.cpp	15 May 2006 16:28:50 -0000
>@@ -756,24 +756,26 @@ nsXFormsModelElement::InitializeInstance
>   nsCOMPtr<nsIDOMNodeList> children;
>   mElement->GetChildNodes(getter_AddRefs(children));
> 
>   PRUint32 childCount = 0;
>   if (children) {
>     children->GetLength(&childCount);
>   }
> 
>+  nsresult rv;
>   for (PRUint32 i = 0; i < childCount; ++i) {
>     nsCOMPtr<nsIDOMNode> child;
>     children->Item(i, getter_AddRefs(child));
>     if (nsXFormsUtils::IsXFormsElement(child, NS_LITERAL_STRING("instance"))) {
>       nsCOMPtr<nsIInstanceElementPrivate> instance(do_QueryInterface(child));
>-      if (instance) {
>-        instance->Initialize();
>-      }
>+      NS_ASSERTION(instance,
>+                   "instance not implementing nsIInstanceElementPrivate?!");
>+      rv = instance->Initialize();
>+      NS_ENSURE_SUCCESS(rv, rv);

nit: NS_ASSERTION only exists on debug.  If this ever happens in a shipped processor, we'd bring the browser down.  I vote that you leave the 'if' test in, in addition to the assertion.

Comment 12

[Allan Beaufour]

(In reply to comment #11)
> >-      if (instance) {
> >-        instance->Initialize();
> >-      }
> >+      NS_ASSERTION(instance,
> >+                   "instance not implementing nsIInstanceElementPrivate?!");
> >+      rv = instance->Initialize();
> >+      NS_ENSURE_SUCCESS(rv, rv);
> 
> nit: NS_ASSERTION only exists on debug.  If this ever happens in a shipped
> processor, we'd bring the browser down.  I vote that you leave the 'if' test
> in, in addition to the assertion.

I know. But when will that assertion fail? An "xf:instance" not implementing nsIInstanceElementPrivate is a design error, and should better be caught while developing/debugging.

But for the sake of closing the bug, I'll do an ensure_state instead.

Comment 13

[Allan Beaufour]

Attachment: Patch v2

Comment 14

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

Comment on attachment 222317 [details] [diff] [review]
Patch v2

looks good

Comment 15

[alexander :surkov (:asurkov)]

There is a question. If I insert dynamic instance without nodes then I'll get error too?

Comment 16

[Allan Beaufour]

(In reply to comment #15)
> There is a question. If I insert dynamic instance without nodes then I'll get
> error too?

Eh, you and your dynamic stuff :)

Good point. Well, that depends. You will get a (JS) warning if you append <instance/> to a model. So you need to construct a valid instance first (that is, add a child or a @src), and then append it to the model. Which imho makes sense.

Comment 17

[Allan Beaufour]

Fixed on trunk.

Work is still needed though, as the instance() function needs to handle invalid instances. This is bug 338263.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> (In reply to comment #15)
> > There is a question. If I insert dynamic instance without nodes then I'll get
> > error too?
> 
> Eh, you and your dynamic stuff :)

Yep, give the world a dynamic xforms! :)

> Good point. Well, that depends. You will get a (JS) warning if you append
> <instance/> to a model. So you need to construct a valid instance first (that
> is, add a child or a @src), and then append it to the model. Which imho makes
> sense.
> 

Now I too prefer to construct a instance a wholy before inserting. Though I guess somebody can consider the behaviour as strange. Generally if I'm not sure what is right then I don't like it a lot. Just some thoughts.

Comment 19

[Allan Beaufour]

(In reply to comment #18)
> (In reply to comment #16)
> > Good point. Well, that depends. You will get a (JS) warning if you append
> > <instance/> to a model. So you need to construct a valid instance first (that
> > is, add a child or a @src), and then append it to the model. Which imho makes
> > sense.
> > 
> 
> Now I too prefer to construct a instance a wholy before inserting. Though I
> guess somebody can consider the behaviour as strange. Generally if I'm not sure
> what is right then I don't like it a lot. Just some thoughts.

Well, the model needs to know when to initialize the instance. We would have to watch for mutations if we were to handle additions after insertions, _and_ do a full rebuild each time. Not good.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)

> Well, the model needs to know when to initialize the instance. We would have to
> watch for mutations if we were to handle additions after insertions, _and_ do a
> full rebuild each time. Not good.
> 

If we will observe mutations then we should keep synchronized instance document and <instance/> chilren. From one side I guess it will slow down xforms, from other side it's not good because instance document nodes and instance child nodes are not the same nodes and it will confuse someone :). If we would use instance child nodes instead of instance document then I guess we will not have the problem. But probably we get'll some others.

Comment 21

[Allan Beaufour]

(In reply to comment #20)
> (In reply to comment #19)
> 
> > Well, the model needs to know when to initialize the instance. We would have to
> > watch for mutations if we were to handle additions after insertions, _and_ do a
> > full rebuild each time. Not good.
> > 
> 
> If we will observe mutations then we should keep synchronized instance document
> and <instance/> chilren. From one side I guess it will slow down xforms, from
> other side it's not good because instance document nodes and instance child
> nodes are not the same nodes and it will confuse someone :). If we would use
> instance child nodes instead of instance document then I guess we will not have
> the problem. But probably we get'll some others.

And we would not be spec. compliant :)

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > 
> > > Well, the model needs to know when to initialize the instance. We would have to
> > > watch for mutations if we were to handle additions after insertions, _and_ do a
> > > full rebuild each time. Not good.
> > > 
> > 
> > If we will observe mutations then we should keep synchronized instance document
> > and <instance/> chilren. From one side I guess it will slow down xforms, from
> > other side it's not good because instance document nodes and instance child
> > nodes are not the same nodes and it will confuse someone :). If we would use
> > instance child nodes instead of instance document then I guess we will not have
> > the problem. But probably we get'll some others.
> 
> And we would not be spec. compliant :)
> 

O, it's the one among others :).