357569 - relative binding expressions in repeats have problems

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

I don't know when this started.  Noticed by Leigh.

When we go through the initial binding of the form controls, we expect that we are doing so in document order.  And we make an effort to do that for the content in the form.  But when repeats are in the form, they are 'unrolling' their content by cloning their content templates and then inserting them into the anonymous content's insertion point.  When this chain of cloned content is inserted into the anonymous content, we have no control over the order of which nodes get the 'document changed' notification from mozilla.  Turns out that the nodes are notified in reverse order (from the leaf back to the insertion root).  Which means that we add these new form controls into the model's form control list in reverse order, which means they will bind in reverse order.  Not to mention that the mFormControl list won't have the correct structure.

So looks like we probably need to make sure that the cloned content of a repeat will be inserted into the model list in order rather than hoping it will be done magically in the right order.  Might be able to do this in nsXFormsRepeatElement::CloneNode.

Comment 1

[aaronr]

Attachment: testcase

Comment 2

[aaronr]

Ok, the real problem causer is nsXTFElementWrapper::BindToTree and its recursion.  If a node with children is inserted into appended under an existing node, like in this bug's scenario, BindToTree will send call the xtf element's WillChangeDocument then WillChangeParent.  Then it calls BindToTree for each child.  Upon return, it will call DocumentChanged and then ParentChanged.  So basically xtf will call WillChangeDocument and WillChangeParent from the top-most node down to the bottom-most child and then work its way back up the chain calling DocumentChanged and ParentChanged along the way.

Our code assumes that every bind (which includes insertion into the model control list) will either happen in document order or that the insertion is a 'one off', just a single node at a time.

We could work around this problem by having repeat insert the nodes as it clones them rather than doing just one insertion.  But we'll still have a problem should anyone else decide to insert a chunk of xforms nodes into the DOM by appending the root of a subtree into the DOM.  So the workaround is out.  We really need to figure out how to fix this right.  And I don't think that we can move any of the bind stuff to WillChangeDocument because we really need to be in the DOM otherwise our FindParentContext stuff won't work right.

Ugh, this sux.

Comment 3

[aaronr]

Man, this could be gruesome.  Basically, we can't assume what order ParentChanged or DocumentChanged occurs in.  The normal order, when a node is added one at a time to the DOM like the parser does it, it is as I mentioned before:

- WillChangeDocument -> WillChangeParent -> DocumentChanged -> ParentChanged.

So we usually won't even try to bind a control until ParentChanged has been called.

However, in the case where we are cloning a template and inserting it all as one big chunk under the context container, ParentChanged will be called as the nodes are cloned and are inserted underneath their parents into the chunk one by one.  All of these ParentChanged notifications will happen from leaf back up the parent branch since we use recursion when cloning the templates.  Once the chunk of cloned nodes is built, it is inserted under the context container.  This is where the DocumentChanged notifications will happen, again from the leaf back up the branch of parent nodes.  In the end will be the DocumentChanged then ParentChanged notification of the direct children of the context container.

So if we want to be able to support this level of dynamic insertion of controls into the DOM, we basically have to be flexible enough to handle a call to Bind() no matter if it was caused by the DocumentChanged notification or the ParentChanged notification.  And when the Bind() process finds a parent context that doesn't have a bound node, we need to be smart enough to know whether that is because it tried to bind and couldn't find one (which is what we assume now) or if it is because the parent hasn't had a chance to try to bind, yet.

My preliminary thinking is that we'll need to change the mHasParent PRBool that is currently used to remember if we've been through ParentChanged with a new parent, yet, to be a tri-state flag so that we can also keep track if we have gone through DocumentChanged with a new document.  And not try to Bind until both have happened.  Then when we do bind and start looking in FindParentContext for parent contexts, that if we find a context container parent that doesn't have both of those set, then we need to put ourselves on some list that that parent has and bow out gracefully.  When the parent acquires the document or parent it was waiting for, it will then try to bind.  If it can do so without running into the same problem with one of its parent contexts, then it'll process its list of child controls that are awaiting bind.  If not, it'll put itself on its parent context's deferring list.  And so on.

To be sure this will be slow and cause some aborted attempts at binding, but the only other alternative is to not allow branches of children to be inserted under a node in the DOM.  And I don't know how we could possibly enforce that.

I'll work on a patch for this when I get a bit more time.

Comment 4

[aaronr]

Attachment: more challenging testcase

Comment 5

[aaronr]

Attachment: patch

basically did what I mentioned earlier.  If parentChanged or documentChanged then I won't allow binding until both mHasDoc and mHasParent have a value.  If that happens and we go through the binding process and call FindParentContext and if a parent context control doesn't have mHasDoc or mHasParent, then the control trying to bind will be added to that parent context controls mAbortedBindList and then we'll return NS_OK_XFORMS_NOTREADY all the way out to ResetBoundNode().  So when the context control DOES have both a parent and a document and it is able to bind successfully, it will process its mAbortedBindList prior to refreshing.  I'm defaulting the size of the mAbortedBindList to 10 just as a guess.  I'm certainly open to suggestions there.

Comment 6

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

Comment on attachment 259380 [details] [diff] [review]
patch


>-  return ForceModelDetach(mHasParent && aNewDocument);
>+  return ForceModelDetach(mHasParent && mHasDoc && aNewDocument);

Isn't |return ForceModelDetach(mHasParent && mHasDoc);| enough.


>@@ -769,39 +816,62 @@ nsXFormsControlStub::GetContext(nsAStrin
>   *aContextPosition = 1;
>   *aContextSize = 1;
> 
>   if (aContextNode) {
>     if (mBoundNode) {
>       // We are bound to a node: This is the context node
>       NS_ADDREF(*aContextNode = mBoundNode);
>     } else if (mModel) {
>+      // If there is no bound node, it could be because everything isn't ready,
>+      // yet.  If mHasDoc or mHasParent are false, then we haven't had a chance
>+      // to bind.  This could happen if the nodes in this chain are being added
>+      // via DOM manipulation rather than via the parser.
>+      if (HasBindingAttribute() && (!mHasDoc || !mHasParent)) {
>+        return NS_OK_XFORMS_NOTREADY;
>+      }
>+

Could you assign *aContextNode = nsnull in the beginning of the method.


> 
>   /**
>+   * List of XForms elements contained by this control who tried to bind
>+   * and couldn't because this control wasn't ready to bind, yet.  This means
>+   * that these contained nodes couldn't bind because they could be potentially
>+   * using this control's bound node as a context node.  When this control
>+   * is ready to bind and successfully binds, then this list will be processed.
>+   * And in turn, if these controls contain controls on THEIR mAbortedBindList,
>+   * then they will be similarly processed.
>+   */
>+  nsVoidArray *mAbortedBindList;

Why this is nsVoidArray? Couldn't it be nsCOMArray<nsIXFormsControl>?
In that case there shouldn't be cycles in DOM (like there might be if it was
nsCOMArray<nsIDOMElement>).
nsVoidArray looks a bit dangerous. What if an element is somehow removed from the container and 
then deleted (I *think* that is possible). The array would point to a deleted object.

With the changes, r=me

Comment 7

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

21:35 < Cartman> I guess that I should keep on the control also a ref to the abortedbindlist that the control is in, too, right?  Then if it is removed, I
                 should remove it from the list
21:36 < Cartman> (if a control is removed from its parent or its document, I should remove it from any abortedBindList that it is on)

Comment 8

[aaronr]

Attachment: patch2

Added handling to remove control from a context control's aborted binding list if the control is changing parents or changing documents.  Leaving the control's aborted binding list intact since I assume we should force them to rebind to reflect this control's new home.

Olli, please review these changes to make sure I didn't screw something up.  Thanks.

Comment 9

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

Comment on attachment 259460 [details] [diff] [review]
patch2


>+   * List of XForms elements contained by this control who tried to bind
>+   * and couldn't because this control wasn't ready to bind, yet.  This means
>+   * that these contained nodes couldn't bind because they could be potentially
>+   * using this control's bound node as a context node.  When this control
>+   * is ready to bind and successfully binds, then this list will be processed.
>+   * And in turn, if these controls contain controls on THEIR mAbortedBindList,
>+   * then they will be similarly processed.
>+   */
>+  nsCOMArray<nsIXFormsControl> *mAbortedBindList;
Could this be just nsCOMArray<nsIXFormsControl> mAbortedBindList
or if you want to really create it using |new|, does
nsAutoPtr<nsCOMArray<nsIXFormsControl> > work? That would ensure that
the list gets deleted eventually.

>+
>+  /**
>+   * The aborted binding list that contains this control.  It should never
>+   * be on more than one list at a time.
>+   */
>+  nsCOMArray<nsIXFormsControl> *mContainingAbortedBindList;
>+

Would it be simpler (and maybe safer) to have
nsCOMPtr<nsIXFormsControl> abortedBindContainer;
That would contain value only when the control is set to some controls mAbortedBindList.
And definitely set nsnull when OnDestroyed is called.

Comment 10

[aaronr]

(In reply to comment #9)
> (From update of attachment 259460 [details] [diff] [review])
> 
> >+   * List of XForms elements contained by this control who tried to bind
> >+   * and couldn't because this control wasn't ready to bind, yet.  This means
> >+   * that these contained nodes couldn't bind because they could be potentially
> >+   * using this control's bound node as a context node.  When this control
> >+   * is ready to bind and successfully binds, then this list will be processed.
> >+   * And in turn, if these controls contain controls on THEIR mAbortedBindList,
> >+   * then they will be similarly processed.
> >+   */
> >+  nsCOMArray<nsIXFormsControl> *mAbortedBindList;
> Could this be just nsCOMArray<nsIXFormsControl> mAbortedBindList
> or if you want to really create it using |new|, does
> nsAutoPtr<nsCOMArray<nsIXFormsControl> > work? That would ensure that
> the list gets deleted eventually.
> 

I just figured that a nsCOMArray was bigger than a pointer so I didn't want to take up the extra room on every control for something that isn't likely to affect most controls in a form.  But I can see your point about it being better to be safe than sorry.

> >+
> >+  /**
> >+   * The aborted binding list that contains this control.  It should never
> >+   * be on more than one list at a time.
> >+   */
> >+  nsCOMArray<nsIXFormsControl> *mContainingAbortedBindList;
> >+
> 
> Would it be simpler (and maybe safer) to have
> nsCOMPtr<nsIXFormsControl> abortedBindContainer;
> That would contain value only when the control is set to some controls
> mAbortedBindList.
> And definitely set nsnull when OnDestroyed is called.
> 

ok.

Comment 11

[aaronr]

Ugh, there are just too many possible holes.  For instance what if we had:

<xf:repeat nodeset="foo">
  <p>
    <xf:group ref=".">
      <div>
        <span>
          <xf:output ref="."/>
        </span>
      </div>
    </xf:group>
  </p>
</xf:repeat>

So we'll end up in the situation that this bug is addressing.  So with my patch as it is now, output would go on group's aborted bind list and output will also contain a link to the list that it was added to.  Now what if, during this DOM mutation, some mutation handler decides to move the span up under the context container and moves the paragraph to be the span's nextSibling?  Output won't be aware that anything happened so it won't know to remove itself from group's list.  Group also won't know that anything happened so it can't do anything, either.  I know that this is really far out edge case stuff, but it shows that this list approach isn't fool proof.

I think we are just going to have to live with this.  The worst that can happen is that some controls could possibly be rebound and refreshed an extra time.  They will still get their new, correct context during the bind process so it isn't like their data will be wrong.

Another scenario is that the span be moved up under the context container and the paragraph removed.  I guess I should handle that, so that if WillChangeDocument sees that the control has an aborted bind list, it should control by control in the list and remove their link back to the list.  Ugh, what a mess.

Comment 12

[aaronr]

Attachment: patch3

Addresses smaug's comments and my last comment by fixing a couple more potential holes where the list and a control's link to the list might have had problems if someone was stirring the DOM.

Comment 13

[aaronr]

Attachment: patch4

removed extraneous stoopid whitespace character that crept into the patch.

Comment 14

[aaronr]

checked into trunk

Comment 15

[aaronr]

checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16