283737 - controls not binding due to timing of instance loading, location of model

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

This is a catch-all bug to address two problems that we still see dealing with
control binding:

1) what if the model is after the control trying to bind to it?
2) what if the control is added to the model's list of controls before the
instance data is loaded and xforms-model-construct-done fires?  How does it
finally get bound to the instance data?

I think that we can solve this problem by maintaining lists of controls that
fail binding based on the model id string (or 'default' if it uses the default
model) and run through this list during nsXFormsModelElement::ConstructDone to
give the controls one last chance to bind.  

We could actually go one step farther and not allow any control that wants to be
bound to really bind or refresh until the model-construct-done is fired.  That
would save a lot of processing time we spend trying to bind/refresh/validate
nodes that can't bind, yet.  It might be simpler to do that in a seperate bug,
though.

Comment 1

[aaronr]

I've got a good couple of testcases.  I'll work on this one.

Comment 2

[Leigh L. Klotz, Jr.]

Attachment: Testcase showing that trigger without model should not display trigger

Comment 3

[Leigh L. Klotz, Jr.]

Attachment: Testcase showing that trigger with model but no instance should display trigger

Comment 4

[Leigh L. Klotz, Jr.]

Attachment: Trigger with small instance (somewhat boring default case)

Comment 5

[Leigh L. Klotz, Jr.]

>We could actually go one step farther and not allow any control that wants to
be bound to really bind or refresh until the model-construct-done is fired. 

This is what XForms 1.0 says to do.  You can tell that that Mozilla isn't doing
this because a page with no model still gets UI elements, even trigger which has
no required bindings.  I've attached a model-free page and and instance-free
page to help test this.

If you see the trigger in the model-free page, then you know that the processor
is not doing the event sequencing for initialization as defined in XFOrms 1.0.

See "The xforms-model-construct Event" [1], which is where xforms-rebuild comes
from.  If there is no model, then there is no xforms-model-construct event, and
the xforms-rebuild and other events never get sent.

I've attached three test cases, one with no model, one with a model but no
instance, and one with a model and a small instance, just to show that the
reason the trigger doesn't show is that it was never initialized, and NOT
because it is assumed to have ref='.' making it irrelevant if there is no root
node (which would be wrong).

[1] http://www.w3.org/TR/xforms/index-all.html#evt-modelConstruct

Comment 6

[aaronr]

Thanks for you comments, Leigh.  We are certainly headed in that direction.  I
don't think that we have any of that lazy-authoring done yet, either.

But I don't see anywhere in the spec that says that controls cannot be shown w/o
a model.  I can't show a xforms:output in a XHTML or SVG document without a model?

Comment 7

[aaronr]

Attachment: first try at a fix

Here is my first try at a fix.	Basically this is what I'm doing.  Right after
we dispatch the model-construct-done, I set a property on the document that
says that the document is ready for binding.  If we try to bind before this
(like when the attributes are set on a control during initialization) I fixed
it so that we'll add this control to a list of deferred bindings and then skip
out of ProcessNodeBinding.  After I set the property to say that binding is
allowed, then  I Bind() and Refresh() all of the elements on the deferred list.


I think that I really need to use a hashtable instead of a nsVoidArray to make
sure that no element is added to the list more than once.  No sense binding and
refreshing the same element multiple times.  That should also help more with
performance.

I still think that we should be able to show a trigger, output, label, etc.
even if there is no model in the document as long as the trigger doesn't have a
binding attribute.  So even with this fix I'll fail Leigh's testcase where a
trigger shouldn't show if there is no model.

Comment 8

[aaronr]

Attachment: using hashtable in fix

Using nsDataHashtable instead of a nsVoidArray to ensure that we don't have
duplicates on the list.

Comment 9

[Allan Beaufour]

Comment on attachment 182815 [details] [diff] [review]
using hashtable in fix

Index: nsXFormsModelElement.cpp
===================================================================
> @@ -1268,16 +1268,18 @@ nsXFormsModelElement::MaybeNotifyComplet
>    }
...
> +
> +  nsXFormsUtils::ProcessDeferredBinds(domDoc);

Why is this function not a function on the model?

Index: nsXFormsUtils.cpp
===================================================================
> +DeleteHashtable(void    *aObject,
...
> +    deferredBindList = NULL;

nsnull

> +BindElement(nsISupports *aKey, int &aData, void *aClosure)
...
> +  return PL_DHASH_REMOVE;

My guess is that this is rather ineffective, as the hashtable has to do checks
and housekeeping just so you can delete it right afterwards.

> +/* static */ void
> +nsXFormsUtils::DeferElementBind(nsIDOMDocument *aDoc, nsIDOMElement *aElement)

This should return nsresult, so errors can be passed back to the control.

And why do you store nsIDOMElements and not nsIXFormsControls? The bind can
just pass 'this', and you would save the QI from the XTF element to the XF
control.

> +    if (!deferredBindList) {
> +      return;
> +    } else {

The 'else' has not purpose, it's right after a 'return'.

> +        deferredBindList = NULL;

nsnull

> +  deferredBindList->Put(aElement, 1);

You should use a hashset instead. The integer is only there because of
laziness I guess :)

I'm not sure about the hash-map/set in the first place. mFormControls on the
model is a nsVoidArray, and is used pretty much for the same purpose. My logic
tells me that either you or the model should change. Not too sure which...

> +nsXFormsUtils::ProcessDeferredBinds(nsIDOMDocument *aDoc)
...
>+    deferredBindList->Clear();

no need to Clear() it, you do that in the unset-handler.

> +    doc->UnsetProperty(nsXFormsAtoms::deferredBindListProperty);

Index: nsXFormsUtils.h
===================================================================
> +  /**
> +   * Stores an element on a list of elements waiting to get bound when the
> +   * models are ready.

How about: The models are not ready for binding, so ddefer the binding of the
control by
storing it as a property on the document. The models will run through this
when they are ready for binding.

> +   * @param aDoc              Document that contains aElement
> +   * @param aElement          Element waiting to be bound
> +   */
> +  static NS_HIDDEN_(void) DeferElementBind(nsIDOMDocument *aDoc,
> +                                           nsIDOMElement  *aElement);
> +
> +  /**
> +   * Sends the deferred elements through bind processing.

How about: Call Bind() and Refresh() on controls, that was defered because the
model was not ready.

> +   *
> +   * @param aDoc              Document that contains aElement
> +   */
> +  static NS_HIDDEN_(void) ProcessDeferredBinds(nsIDOMDocument *aDoc);
> +

Comment 10

[aaronr]

Yeah, HashSets would be perfect except that I couldn't figure out how to
ennumerate through them.  Is that possible?

Comment 11

[aaronr]

Attachment: fixes comments

fixes comments except for HashSet since I don't know how to ennumerate them.  

Also, a nsVoidArray for the modellist is fine because models are guarenteed to
be added to the list only once when the model is inserted into the document. 
So no duplicate models really possible on the list.  Since there are multiple
things that can cause a bind (parent changed, document changed, attribute set,
etc.), I was concerned that there could be possibilities where duplicates could
find their way onto the list. Right now I don't see any duplications, but a
couple of weeks ago before a fix or two went in I would have.  Using a
hashtable guarentees that we'll only have the control on the list once and so
we won't have to worry if future changes might cause new bind requests before
models are ready.  That is my thinking.

Comment 12

[Allan Beaufour]

Comment on attachment 183095 [details] [diff] [review]
fixes comments

something is not working with this patch. Testcase coming

Comment 13

[Allan Beaufour]

Attachment: Testcase that fails

attachment 183095 [details] [diff] [review] makes this testcase fail. It should always display |bind:
Title|, but its more or less random for me whether it does or not. Try hitting
(shift-) reload a couple of times.

Comment 14

[aaronr]

That was a good catch.  The randomness has to do with me using a hashtable
combined with the fact that the xf:output is contained in a xf:group and uses a
relative xpath and not FQ xpath.  As I enumerate through the deferred binds, the
controls come out in the order that the hashtable gives them to me, not
necessarily in document order.  So if I go to bind the output before the group
has been bound, the context node for the output's xpath evaluation isn't the
group's bound node (since it doesn't have one yet) but rather the root element
in the instance document since that is the default context.

Ugh.  Ok, my options are to use a nsVoidArray and always append to the end and
hope that there aren't duplicates (multiple rebinds on the same element would
hurt performance) or write a class that uses a hashset to store the key values
combined with a nsVoidArray.  So that if the element is in the hashset already,
it isn't added to the nsVoidArray.

Comment 15

[aaronr]

Attachment: fixes failing testcase

Decided to go with the nsVoidArray.  Since the mHasParent check was added to
ResetBoundNode, we ignore all other requests to bind before document load
except when our element is inserted into the document under the appropriate
parent element.  So items will only be added to the deferred list once.  Should
this behavior change, we'll have to probably go to the nsVoidArray + HashSet
combo in order to keep things as efficient as possible.

Comment 16

[Allan Beaufour]

Comment on attachment 183207 [details] [diff] [review]
fixes failing testcase

Index: nsXFormsControlStub.cpp
===================================================================
>+  if(!test) {

Insert space before '('

Index: nsXFormsModelElement.cpp
===================================================================

>+nsXFormsModelElement::DeferElementBind(nsIDOMDocument *aDoc, nsIXFormsControl *aControl)

Long line, split it.

Index: nsXFormsModelElement.h
===================================================================
>+  /**
>+   * The models are not ready for binding, so defer the binding of the control
>+   * by storing it as a property on the document.  The models will run through
>+   * this list when they are ready for binding.
>+   *
>+   * @param aDoc              Document that contains aElement
aElement -> |aControl| or |the XForms control|

r=me with those nits

Comment 17

[aaronr]

Attachment: fixes nits

smaug, can you do second review please?

Comment 18

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

Comment on attachment 183310 [details] [diff] [review]
fixes nits


>+/* static */ nsresult
>+nsXFormsModelElement::DeferElementBind(nsIDOMDocument *aDoc, 
>+                                       nsIXFormsControl *aControl)
>+{
>+  nsCOMPtr<nsIDocument> doc = do_QueryInterface(aDoc);
>+
>+  if (!doc || !aControl) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  nsVoidArray *deferredBindList =
>+      NS_STATIC_CAST(nsVoidArray *,
>+                    doc->GetProperty(nsXFormsAtoms::deferredBindListProperty));
>+
>+  if (!deferredBindList) {
>+    deferredBindList = new nsVoidArray(16);
>+    if (!deferredBindList) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+
>+    doc->SetProperty(nsXFormsAtoms::deferredBindListProperty, deferredBindList,
>+                     DeleteBindList);
>+  }
>+
>+  // always append to the end of the list.  We need to keep the elements in
>+  // document order when we process the binds later.  Otherwise we have trouble
>+  // when an element is trying to bind and should use its parent as a context
>+  // for the xpath evaluation but the parent isn't bound yet.
>+  deferredBindList->AppendElement(aControl);
>+
>+  return NS_OK;
>+}
>+

I think you should ADDREF when adding an element to the deferredbindlist and
RELEASE when
processing/deleting the list. Otherwise something bad may happen if an element
is removed from
document before xforms-model-construct.

With those, r=me

Comment 19

[aaronr]

Attachment: fix with nsCOMArray

along smaug's suggestion, tor suggested changing to use nsCOMArray which will
do all of the addrefing, releasing automatically.  So that is the only change
that I made.  Guess I should go through the reviews process again.

Comment 20

[Allan Beaufour]

Checked in.

Comment 21

[Allan Beaufour]

Actually, this patch does not fix attachment 177412 [details]. Please create a bug for
that Aaron.

Comment 22

[aaronr]

(In reply to comment #21)
> Actually, this patch does not fix attachment 177412 [details] [edit]. Please create a
bug for
> that Aaron.

As I noted in comment #6 I don't see anything that says that a model HAS to
exist.  I can see scenarios where the user might want xforms:output's, for
example, especially with our whiz-bang skinning.  And a model isn't necessary
for output.  I'll send a note to some WG people and see what they say.

Comment 23

[Allan Beaufour]

(In reply to comment #22)
> (In reply to comment #21)
> > Actually, this patch does not fix attachment 177412 [details]. Please
> > create a bug for that Aaron.
> 
> As I noted in comment #6 I don't see anything that says that a model HAS to
> exist.  I can see scenarios where the user might want xforms:output's, for
> example, especially with our whiz-bang skinning.  And a model isn't necessary
> for output.  I'll send a note to some WG people and see what they say.

Ah, well, ok. I didn't read everything, I just assumed that the testcases were
valid. Sorry for that.

Comment 24

[aaronr]

*** Bug 275213 has been marked as a duplicate of this bug. ***