Last Comment Bug 310138 - Setting a type on instance data makes rebuild() throw exception
: Setting a type on instance data makes rebuild() throw exception
Status: RESOLVED FIXED
: fixed1.8.0.2, fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: P1 normal with 1 vote (vote)
: ---
Assigned To: Allan Beaufour
: Stephen Pride
Mentors:
Depends on:
Blocks: 323593 326372 326373 326397 326555 332853
  Show dependency treegraph
 
Reported: 2005-09-26 21:20 PDT by alexander :surkov
Modified: 2006-04-07 04:39 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (2.32 KB, application/xhtml+xml)
2005-09-26 21:23 PDT, alexander :surkov
no flags Details
Patch using hashtables (20.18 KB, patch)
2006-02-13 04:27 PST, Allan Beaufour
no flags Details | Diff | Review
Patch v2 (20.39 KB, patch)
2006-02-14 02:23 PST, Allan Beaufour
aaronr: review-
Details | Diff | Review
Patch v3 (20.39 KB, patch)
2006-02-15 01:28 PST, Allan Beaufour
aaronr: review+
bugs: review+
Details | Diff | Review

Description alexander :surkov 2005-09-26 21:20:50 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

When 'bind' element has type then rebuild() method of model throws an exception:
Error: XForms Error (7): Multiple defined model item property on element

Reproducible: Always
Comment 1 alexander :surkov 2005-09-26 21:23:13 PDT
Created attachment 197511 [details]
testcase
Comment 2 Allan Beaufour 2005-09-27 06:31:58 PDT
(In reply to comment #0)
> When 'bind' element has type then rebuild() method of model throws an exception:
> Error: XForms Error (7): Multiple defined model item property on element

You are absolutely right.

We need to clear the nsXFormsAtoms::type on all instance nodes on rebuild. Ouch,
that's expensive :(
Comment 3 alexander :surkov 2005-09-27 21:52:43 PDT
(In reply to comment #2)
> 
> We need to clear the nsXFormsAtoms::type on all instance nodes on rebuild. Ouch,
> that's expensive :(

What means expensive?
Comment 4 Allan Beaufour 2005-09-28 04:48:43 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > 
> > We need to clear the nsXFormsAtoms::type on all instance nodes on rebuild. Ouch,
> > that's expensive :(
> 
> What means expensive?

We need to run through all instance nodes, that takes time. So expensive == time
consuming.
Comment 5 Victor Engmark 2005-12-05 08:27:57 PST
Confirmed on 2005-12-05-06-trunk. In addition, @type is required for file uploads, so I'm stuck!

Code fragments:
<xf:bind id="file-bind" nodeset="instance('file')/file" type="xsd:anyURI" constraint="string-length(instance('context')/slotId) &gt; 0 and string-length(instance('context')/jobDescription) &gt; 0"/>
...
<xf:upload bind="file-bind" mediatype="*">
  <xf:label>File</xf:label>
  <xf:filename ref="instance('jobsFile')/jobs:Jobs/jobs:Job[jobs:Description=instance('context')/jobDescription and jobs:ParentSlot/@id=instance('context')/slotId]/jobs:Files/jobs:File[last()]"/>
</xf:upload>

Error message:
Error: XForms Error (7): Multiple defined model item property on element
Source File: file:///C:/LIMO-dev/install/Mozilla/jobs.xhtml
Line: 0
Source Code:
<xf:bind id="file-bind" nodeset="instance('file')/file" type="xsd:anyURI"/>
Comment 6 Victor Engmark 2005-12-05 08:33:33 PST
Oh yeah, when removing the bind and using @ref and @type on the upload element directly, I get the following:
Error: XForms Error (24): Upload element not bound to valid datatype.  Must be bound to datatype 'xsd:anyURI', 'xsd:base64Binary', or 'xsd:hexBinary'.
Source File: file:///C:/LIMO-dev/install/Mozilla/jobs.xhtml
Line: 0
Source Code:
<xf:upload ref="instance('file')/file" mediatype="*" type="xsd:anyURI" type="http://www.w3.org/2001/XMLSchema#string"/>

Seems like @type is completely ignored on this element; i.e., I only specified the first one, but the second one is always appended, while the first is ignored.
Comment 7 aaronr 2005-12-05 13:01:45 PST
(In reply to comment #6)
> Oh yeah, when removing the bind and using @ref and @type on the upload element
> directly, I get the following:
> Error: XForms Error (24): Upload element not bound to valid datatype.  Must be
> bound to datatype 'xsd:anyURI', 'xsd:base64Binary', or 'xsd:hexBinary'.
> Source File: file:///C:/LIMO-dev/install/Mozilla/jobs.xhtml
> Line: 0
> Source Code:
> <xf:upload ref="instance('file')/file" mediatype="*" type="xsd:anyURI"
> type="http://www.w3.org/2001/XMLSchema#string"/>
> 
> Seems like @type is completely ignored on this element; i.e., I only specified
> the first one, but the second one is always appended, while the first is
> ignored.
> 


@type on a control isn't proper XForms.  XForms uses @type on xf:bind to describe the instance data that the control is bound to.  You can also put @type on the instance element itself (though in that case you would need to use @xsi:type).

So in the example that you cited, the two places that we look for typing (on the instance data and on the bind) turned up nothing so we are explaining that we didn't find the proper type for the control.
Comment 8 Victor Engmark 2005-12-06 01:07:44 PST
(In reply to comment #7)
> @type on a control isn't proper XForms.  XForms uses @type on xf:bind to
> describe the instance data that the control is bound to.  You can also put
> @type on the instance element itself (though in that case you would need to use
> @xsi:type).

Then the error message makes even less sense, since it indicates that the XForms engine inserts @type there, and expects the user to override it. The "natural" place to correct would be where the XForms engine points out the error...
Comment 9 Allan Beaufour 2005-12-06 18:15:51 PST
(In reply to comment #8)
> (In reply to comment #7)
> > @type on a control isn't proper XForms.  XForms uses @type on xf:bind to
> > describe the instance data that the control is bound to.  You can also put
> > @type on the instance element itself (though in that case you would need to use
> > @xsi:type).
> 
> Then the error message makes even less sense, since it indicates that the
> XForms engine inserts @type there, and expects the user to override it. The
> "natural" place to correct would be where the XForms engine points out the
> error...
> 

Hmmm, well it's telling you that this control is not _bound_ to the correct type. That's correct, and makes sense.

We could help further by writing out the node that it is bound to, and the type. I'll give you that, but that's a different bug.
Comment 10 Allan Beaufour 2005-12-06 18:21:32 PST
(In reply to comment #5)
> Confirmed on 2005-12-05-06-trunk. In addition, @type is required for file
> uploads, so I'm stuck!

Well, don't rebuild then :) No, seriously, I'm sorry for that. It's my doing, I really need to fix this.

When I implemented it, I thought it was smart to store the type on the instance data node -- I just forgot about rebuild.

Either we need to version/timestamp the type information, or we need to store it somewhere else where it's easy to clear. Right now we have to run through all nodes and kill the type info on a rebuild. Not good at all.
Comment 11 aaronr 2005-12-07 10:10:25 PST
(In reply to comment #10)
> (In reply to comment #5)
> > Confirmed on 2005-12-05-06-trunk. In addition, @type is required for file
> > uploads, so I'm stuck!
> 
> Well, don't rebuild then :) No, seriously, I'm sorry for that. It's my doing, I
> really need to fix this.
> 
> When I implemented it, I thought it was smart to store the type on the instance
> data node -- I just forgot about rebuild.
> 
> Either we need to version/timestamp the type information, or we need to store
> it somewhere else where it's easy to clear. Right now we have to run through
> all nodes and kill the type info on a rebuild. Not good at all.
> 


I started looking at fixing this and then got sidetracked with Minimo.  The problem with a side list is that while it is easy to clear, but you'll have to repopulate it on every rebuild to make sure that you have an accurate list since instance data could be changed by JS prior to the rebuild.  And the list would have to be keyed off of the pointer value or something to keep it as small a list as possible since we will have a list living in memory for every instance document that we have.  That's as far as I got in my thinking :=)
Comment 12 Allan Beaufour 2005-12-07 13:00:27 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #5)
> > > Confirmed on 2005-12-05-06-trunk. In addition, @type is required for file
> > > uploads, so I'm stuck!
> > 
> > Well, don't rebuild then :) No, seriously, I'm sorry for that. It's my doing, I
> > really need to fix this.
> > 
> > When I implemented it, I thought it was smart to store the type on the instance
> > data node -- I just forgot about rebuild.
> > 
> > Either we need to version/timestamp the type information, or we need to store
> > it somewhere else where it's easy to clear. Right now we have to run through
> > all nodes and kill the type info on a rebuild. Not good at all.
> > 
> 
> 
> I started looking at fixing this and then got sidetracked with Minimo.  The
> problem with a side list is that while it is easy to clear, but you'll have to
> repopulate it on every rebuild to make sure that you have an accurate list
> since instance data could be changed by JS prior to the rebuild.  And the list
> would have to be keyed off of the pointer value or something to keep it as
> small a list as possible since we will have a list living in memory for every
> instance document that we have.  That's as far as I got in my thinking :=)

Well, whether we save it in a list or store it as properties we use memory. The same goes for what we have to do on a rebuild, except that clearing a list is much quicker than traversing all instance nodes.
Comment 13 aaronr 2006-01-27 17:26:32 PST
<thinking out loud>
To fix this with code as it is today, on detection of rebuild:

for (each instance doc in model) {
  for (each node in instance doc) {
    if (node has type property) {
      if (node has @type) {
        set type property
      } else {
        delete type property
      }
    } else if (node has @type) {
      set type property
    }
    if (node has p3ptype property) {
      delete property
    }
  }
}
model->ProcessBindElements();
model->ApplyTypesFromSchemas(); /*yet to be implemented*/

Or if we use a side list: 

model->ClearTypeList();

for (each instance doc in model) {
  for (each node in instance doc) {
    if (node has @type) {
      model->AddTypeToList(node, type);
    }
  }
}

model->ProcessBindElements();
model->ApplyTypesFromSchemas(); /*yet to be implemented*/

So either way we'll have to walk every node in the instance document.  Either way we'll have to set the node type, whether it is on the node iteself or in the list.  What we are saving is a getProperty on every node for both type and p3ptype and a remove property on all p3ptype nodes as well as nodes that don't have @type set on them.  Also safeguards us from a JS author setting the properties themselves on the instance nodes and potentially screwing us up.

</thinking out loud>
Comment 14 Allan Beaufour 2006-02-03 05:05:36 PST
(In reply to comment #13)
> Or if we use a side list: 
> 
> model->ClearTypeList();
> 
> for (each instance doc in model) {
>   for (each node in instance doc) {
>     if (node has @type) {
>       model->AddTypeToList(node, type);
>     }
>   }
> }

No need for that. When we need to get the schema type or a node, we just first check the node directly, and if there's no xsi:type there we check the "type list". It's the same as we do today, just using a list instead of properties on each node.
Comment 15 Allan Beaufour 2006-02-08 04:18:19 PST
As it can bee seen, I'm for the list approach as I believe it will be the most efficient. I see two options:
1) Replace the property-handling with a list (hashmap) in each model

2) Use the existing "instance data node -> meta data" mapping that the MDG already contains.

Option 2 is where we should go, but each entry in the MDG is "heavy", and looking at it with fresh eyes I should redo the MDG datastructures a bit. There is some redundancy. Some optimizations would help quite a lot on memory consumption I think.

But, messing around with the MDG is always fun for me, so unless somebody objects loudly I'll do the "quick fix" option 1, and then follow up with a option 2 when the MDG is refactored.
Comment 16 Allan Beaufour 2006-02-13 04:27:11 PST
Created attachment 211721 [details] [diff] [review]
Patch using hashtables

Here's a patch taking the list (hashtables) approach.

I've moved ParseTypeFromNode() functionality from nsXFormsUtils to the model, as the data lives there. The old function is still there for when the model is not known by the caller.
Comment 17 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2006-02-13 12:14:52 PST
Comment on attachment 211721 [details] [diff] [review]
Patch using hashtables


Few comments:

>+  // Initialize hash tables
>+  NS_ENSURE_TRUE(mNodeToType.Init(), NS_ERROR_FAILURE);
>+  NS_ENSURE_TRUE(mNodeToP3PType.Init(), NS_ERROR_FAILURE);

I *think* this should return NS_ERROR_OUT_OF_MEMORY if not true.


>+
>+  if (separator == kNotFound) {
>+    // no namespace prefix, which is valid;
>+    prefix.AssignLiteral("");

I'd prefer prefix = EmptyString() or prefix.Assign(EmptyString());
(also elsewhere)


> 
>+        // Insert value
>+        NS_ENSURE_TRUE(table->Put(node, new nsString(propStrings[j])),
>+                       NS_ERROR_FAILURE);
>+

IIRC, also this should be NS_ERROR_OUT_OF_MEMORY, not NS_ERROR_FAILURE.
And please check whether new nsString(propStrings[j]) succeeds.
Comment 18 aaronr 2006-02-13 15:47:44 PST
Comment on attachment 211721 [details] [diff] [review]
Patch using hashtables

>@@ -635,16 +639,22 @@ nsXFormsModelElement::Rebuild()
>   printf("nsXFormsModelElement::Rebuild()\n");
> #endif
> 
>   // 1 . Clear graph
>   nsresult rv;
>   rv = mMDG.Clear();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // Clear any type information
>+  NS_ENSURE_TRUE(mNodeToType.IsInitialized() && mNodeToType.IsInitialized(),
>+                 NS_ERROR_FAILURE);
>+  mNodeToType.Clear();
>+  mNodeToP3PType.Clear();
>+

checking IsInitialized() twice on the same hashtable.  I assume one of these should be mNodeToP3PType.

Gotta run.  More feedback tomorrow.
Comment 19 Allan Beaufour 2006-02-14 02:23:28 PST
Created attachment 211838 [details] [diff] [review]
Patch v2
Comment 20 aaronr 2006-02-14 14:50:23 PST
Comment on attachment 211838 [details] [diff] [review]
Patch v2


>diff -X patch-excludes -uprN -U8 xforms/nsXFormsModelElement.cpp xforms.typefix/nsXFormsModelElement.cpp
>--- xforms/nsXFormsModelElement.cpp	2006-02-09 09:53:29.000000000 +0100
>+++ xforms.typefix/nsXFormsModelElement.cpp	2006-02-14 11:20:41.000000000 +0100

>@@ -1180,16 +1190,89 @@ nsXFormsModelElement::GetLazyAuthored(PR
> 
> NS_IMETHODIMP
> nsXFormsModelElement::GetIsReady(PRBool *aIsReady)
> {
>   *aIsReady = mReadyHandled;
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+nsXFormsModelElement::GetTypeFromNode(nsIDOMNode *aInstanceData,
>+                                      nsAString  &aType,
>+                                      nsAString  &aNSUri)
>+{
>+  // aInstanceData could be an instance data node or it could be an attribute
>+  // on an instance data node (basically the node that a control is bound to).
>+
>+  nsString *typeVal = nsnull;
>+
>+  // Get type stored directly on instance node
>+  nsString typeAttribute;
>+  nsCOMPtr<nsIDOMElement> nodeElem(do_QueryInterface(aInstanceData));
>+  if (nodeElem) {
>+    nodeElem->GetAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_XML_SCHEMA_INSTANCE),
>+                             NS_LITERAL_STRING("type"), typeAttribute);
>+    if (!typeAttribute.IsEmpty()) {
>+      typeVal = &typeAttribute;
>+    }
>+  }
>+

doesn't typeAttribute need to be an nsAutoString here?

Also, shouldn't we throw a binding exception (and report multiMIP to the console) if we have an instance node with a xsi:type on it and then we have a xf:bind pointing at the node and setting type="xxx" on it?  I don't see where that would happen in this code.  Though, if it is any consolation, wouldn't have happened in the previous code, either.

Otherwise code looks good to me.
Comment 21 Allan Beaufour 2006-02-15 00:27:05 PST
(In reply to comment #20)
> (From update of attachment 211838 [details] [diff] [review] [edit])
> >+  // Get type stored directly on instance node
> >+  nsString typeAttribute;
> >+  nsCOMPtr<nsIDOMElement> nodeElem(do_QueryInterface(aInstanceData));
> >+  if (nodeElem) {
> >+    nodeElem->GetAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_XML_SCHEMA_INSTANCE),
> >+                             NS_LITERAL_STRING("type"), typeAttribute);
> >+    if (!typeAttribute.IsEmpty()) {
> >+      typeVal = &typeAttribute;
> >+    }
> >+  }
> >+
> 
> doesn't typeAttribute need to be an nsAutoString here?

Well, it does not need to be nsAutoString, but there was real reason for me to use nsString there. Back to original.

> Also, shouldn't we throw a binding exception (and report multiMIP to the
> console) if we have an instance node with a xsi:type on it and then we have a
> xf:bind pointing at the node and setting type="xxx" on it?  I don't see where
> that would happen in this code.  Though, if it is any consolation, wouldn't
> have happened in the previous code, either.

This patch does not, as you also state, change behaviour. If you want changed behaviour: Create a bug for it :)
Comment 22 Allan Beaufour 2006-02-15 01:28:58 PST
Created attachment 211960 [details] [diff] [review]
Patch v3

nsString -> nsAutoString
Comment 23 aaronr 2006-02-15 09:01:51 PST
Comment on attachment 211960 [details] [diff] [review]
Patch v3

I can't figure out if it should be a binding exception or not.  fP throws a link exception in the testcase that I created (and I'm not linking to anything) so I assume that this is why, but I'm not sure.  I'll open another bug if I can get an answer back from the WG.
Comment 24 Allan Beaufour 2006-02-17 02:14:11 PST
Checked in on trunk
Comment 25 Allan Beaufour 2006-02-23 08:03:08 PST
Checked in on 1_8_0

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