Last Comment Bug 321876 - Handle empty instances (they are invalid)
: Handle empty instances (they are invalid)
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Allan Beaufour
: Stephen Pride
:
Mentors:
Depends on:
Blocks: 338263
  Show dependency treegraph
 
Reported: 2005-12-29 22:38 PST by alexander :surkov
Modified: 2016-07-15 14:46 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Crashes trunk (3.45 KB, application/xhtml+xml)
2006-04-27 08:51 PDT, Allan Beaufour
no flags Details
Patch (8.37 KB, patch)
2006-05-15 09:33 PDT, Allan Beaufour
aaronr: review+
Details | Diff | Splinter Review
Patch v2 (8.34 KB, patch)
2006-05-17 00:40 PDT, Allan Beaufour
bugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2005-12-29 22:38:40 PST
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] 2005-12-30 07:42:07 PST
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 2006-01-03 00:23:20 PST
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 2006-01-04 17:01:26 PST
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 2006-01-04 17:19:17 PST
Changing summary since an empty instance isn't really part of lazy authoring.
Comment 5 Allan Beaufour 2006-04-20 08:37:00 PDT
(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 2006-04-27 08:25:10 PDT
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 2006-04-27 08:26:49 PDT
(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 2006-04-27 08:51:09 PDT
Created attachment 220015 [details]
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 2006-04-27 11:04:30 PDT
(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 2006-05-15 09:33:15 PDT
Created attachment 222047 [details] [diff] [review]
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 2006-05-16 11:55:46 PDT
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 2006-05-17 00:34:55 PDT
(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 2006-05-17 00:40:19 PDT
Created attachment 222317 [details] [diff] [review]
Patch v2
Comment 14 Olli Pettay [:smaug] 2006-05-17 00:50:14 PDT
Comment on attachment 222317 [details] [diff] [review]
Patch v2

looks good
Comment 15 alexander :surkov 2006-05-17 00:58:14 PDT
There is a question. If I insert dynamic instance without nodes then I'll get error too?
Comment 16 Allan Beaufour 2006-05-17 01:22:00 PDT
(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 2006-05-17 01:24:56 PDT
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 2006-05-17 01:43:21 PDT
(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 2006-05-17 01:48:23 PDT
(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 2006-05-17 02:00:04 PDT
(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 2006-05-17 02:03:16 PDT
(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 2006-05-17 02:10:53 PDT
(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 :).

Note You need to log in before you can comment on or make changes to this bug.