The default bug view has changed. See this FAQ.

Setting a type on instance data makes rebuild() throw exception

RESOLVED FIXED

Status

Core Graveyard
XForms
P1
normal
RESOLVED FIXED
12 years ago
9 months ago

People

(Reporter: surkov, Assigned: Allan Beaufour)

Tracking

({fixed1.8.0.2, fixed1.8.0.4, fixed1.8.1})

Trunk
fixed1.8.0.2, fixed1.8.0.4, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

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

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

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

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

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

Comment 9

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

Comment 10

12 years ago
(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.
Assignee: aaronr → allan
OS: Windows 2000 → All
Priority: -- → P1
Hardware: PC → All
Summary: nsIXFormsModelElement::rebuild throw exception → Setting a type on instance data makes rebuild() throw exception

Comment 11

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

Comment 12

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

Updated

11 years ago
Blocks: 323593

Comment 13

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

Comment 14

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

Updated

11 years ago
Blocks: 326372

Updated

11 years ago
Blocks: 326373
(Assignee)

Comment 15

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

Updated

11 years ago
Blocks: 326397

Updated

11 years ago
Blocks: 326555
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 16

11 years ago
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.
Attachment #211721 - Flags: review?(aaronr)
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

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

Comment 19

11 years ago
Created attachment 211838 [details] [diff] [review]
Patch v2
Attachment #211721 - Attachment is obsolete: true
Attachment #211838 - Flags: review?(aaronr)
Attachment #211721 - Flags: review?(aaronr)
(Assignee)

Updated

11 years ago
Attachment #211838 - Flags: review?(smaug)

Comment 20

11 years ago
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.
Attachment #211838 - Flags: review?(aaronr) → review-
(Assignee)

Comment 21

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

Comment 22

11 years ago
Created attachment 211960 [details] [diff] [review]
Patch v3

nsString -> nsAutoString
Attachment #211838 - Attachment is obsolete: true
Attachment #211960 - Flags: review?(aaronr)
Attachment #211838 - Flags: review?(smaug)

Comment 23

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

Updated

11 years ago
Attachment #211960 - Flags: review?(smaug)
Attachment #211960 - Flags: review?(smaug) → review+
(Assignee)

Comment 24

11 years ago
Checked in on trunk
Whiteboard: xf-to-branch
(Assignee)

Comment 25

11 years ago
Checked in on 1_8_0
Keywords: fixed1.8.0.2
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Blocks: 332853
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.3, fixed1.8.1
(Assignee)

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.