315712 - Events thrown in the model need to be deferred.

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Doron Rosenberg (IBM)]

Spun off bug 303926.

Events in the model can happen before DOMContentLoaded, which is when XML Events are attached.

We thus need a event queue probabl.

Per model?

Comment 1

[aaronr]

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

Comment 2

[aaronr]

We need to remember any event that passes through the model to a child, too, not necessarily just those targeting the model.  Like binding exceptions that get generated during nsXFormsModelElement::ProcessBind.  Bug 330579 has two testcases that should work once this bug is fixed.

Comment 3

[Doron Rosenberg (IBM)]

Any comments?  Should we do this?

Comment 4

[aaronr]

first step is to identify the events that this would involve.  xforms-binding-exception seems to be the only one right now that is failing a testcase (4.2.1.e in the testsuite).  I purposely moved xforms-model-construct to after document load for this very reason.  xforms-binding-exception is pretty much just a notification so doesn't really need to be in any order relative to other events so queing it up should be easy.  xforms-compute-exception is in the same boat.  xforms-link-exception and xforms-link-error potentially.  Again, really just notifications.

These are the only ones that I came up with.  Any others?

Comment 5

[Allan Beaufour]

(In reply to comment #4)
> first step is to identify the events that this would involve. 
> xforms-binding-exception seems to be the only one right now that is failing a
> testcase (4.2.1.e in the testsuite).  I purposely moved xforms-model-construct
> to after document load for this very reason.  xforms-binding-exception is
> pretty much just a notification so doesn't really need to be in any order
> relative to other events so queing it up should be easy. 
> xforms-compute-exception is in the same boat.  xforms-link-exception and
> xforms-link-error potentially.  Again, really just notifications.
> 
> These are the only ones that I came up with.  Any others?

No, only the "error indication" events make sense to deferred. But yes, they do make sense. Let's do it.

Comment 6

[Doron Rosenberg (IBM)]

Well, if only error events are to be deferred, don't we need to just defer one?  Since it would be a fatal error?

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> Well, if only error events are to be deferred, don't we need to just defer one?
>  Since it would be a fatal error?

Heh. Good point. Three of them are actually fatal, so yes. But xforms-link-error is only a notification though.

Comment 8

[aaronr]

taking bug from Doron.  Schema stuff higher priority on his list for now.

Comment 9

[aaronr]

Attachment: testcase

Comment 10

[aaronr]

Attachment: testcase 2 (run locally)

Comment 11

[aaronr]

Attachment: testcase 3

Comment 12

[aaronr]

Attachment: testcase 4 (run locally)

Comment 13

[aaronr]

Attachment: testcase 5

This one is a doozy.  What if we need to dispatch an error (like xforms-link-error) to a model that doesn't exist, yet?  So not only do we have to defer it, but we don't even have an object to target.

In light of this scenario, I'll probably create a property on the document rather than on any models.

Comment 14

[aaronr]

Attachment: testcase

looking at the other implementations and squinting just right at the spec, it looks like a bind with @nodeset not pointing to a valid node is NOT a xforms-binding-exception (or even a xforms-compute-exception).  So replacing a testcase for that with a testcase for a normal xforms-binding-exception to make sure I don't break normal event dispatching.

Comment 15

[aaronr]

Attachment: regression testcase

previous regression testcase had an error in it.

Comment 16

[aaronr]

I probably should have mentioned that testcase 2, 4, and 5 need to be run locally to expose this bug.

Comment 17

[aaronr]

Attachment: proposed fix

I fixed this by making it so that xforms-link-error, xforms-link-exception, xforms-compute-exception, which are all targeted at a model, check to make sure that the model has handled DOMContentLoaded before dispatching.  If the model hasn't, then I put the events on a list that is stored as a property on the document.  For these 3 events I also had to be cautious because the model that is targeted might not be parsed, yet, so I also defer in that case.  I also added deferring of the event for xforms-binding-exceptions targeted at a bind to make sure that the model that contains the bind has gotten DOMContentLoaded and if not, defer the event dispatch.

In addition, I also added code to call HandleFatalError (the fatal error dialog) for xforms-compute-exceptions and for various xforms-binding-exceptions that aren't targeted at nsIXFormsControls (like the action handlers).

Comment 18

[Doron Rosenberg (IBM)]

Comment on attachment 221009 [details] [diff] [review]
proposed fix

>? resources/content/nsxformsmessagelement.cpp
>Index: nsIModelElementPrivate.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIModelElementPrivate.idl,v
>retrieving revision 1.19
>diff -u -8 -p -r1.19 nsIModelElementPrivate.idl
>--- nsIModelElementPrivate.idl	29 Mar 2006 10:49:27 -0000	1.19
>+++ nsIModelElementPrivate.idl	5 May 2006 22:59:05 -0000
>@@ -165,9 +165,14 @@ interface nsIModelElementPrivate : nsIXF
>                        out AString aNSUri);
> 
>   /**
>    * Notification that all of the external message loads have finished loading.
>    * Model contstruction cannot complete until all of the external messages
>    * have loaded their data.
>    */
>   void messageLoadFinished();
>+
>+  /**
>+   * This attribute is set when the model handled DOMContentLoaded event
>+   */
>+  boolean isDOMLoaded();

perhaps name this isInitialDOMLoaded or hasDOMContentFired to make clear this is about the initial load.

>Index: nsXFormsUtils.cpp
>===================================================================
>+
>+nsresult
>+DeferDispatchEvent(nsIDOMNode* aTarget, nsXFormsEvent aEvent,
>+                   nsIDOMElement *aModelFinder)

ModelFinder is the element that caused the event, right?  Why not call it aSrcElement?

Rest looks fine.

Comment 19

[aaronr]

Attachment: testcase 5 (run locally)

I forgot...labels don't automatically pick up model from context node.  Fixed testcase.

Comment 20

[aaronr]

Attachment: patch with doron's comments fixed

Comment 21

[Allan Beaufour]

http://beaufour.dk/jst-review/?patch=221356

Comment 22

[aaronr]

Attachment: fixed nits

doh!  Sorry.  Forgot to run the JST tool.

Comment 23

[Allan Beaufour]

Comment on attachment 221482 [details] [diff] [review]
fixed nits

Did I just dream that I actually reviewed this. I must have forgotten to actually submit the review :( So it was not my intention just to give link to jst-review. Sorry for that.

>+  /**
>+   * Returns true when the model has been notified that the DOMContentLoaded
>+   * event has been fired on the XForms document.
>+   */
>+  boolean hasDOMContentFired();

This is actually an attribute, so:
readonly attribute boolean hasDOMContentFired;

>       nsXFormsUtils::DispatchEvent(mElement, eEvent_ComputeException);
>+      nsXFormsUtils::HandleFatalError(
>+        mElement, NS_LITERAL_STRING("XFormsComputeException"));

Is there any case where we do not need to handleFatalError after these kinds of exceptions? Why not move the HandleFatalError into DispatchEvent()?

>Index: nsXFormsUtils.cpp
>===================================================================

>+  nsVoidArray *array = NS_STATIC_CAST(nsVoidArray *, aPropertyValue);
>+  PRInt32 count = array->Count();
>+  for (PRInt32 i=0; i < count; i++) {

nit (from hell): "i = 0"

Comment 24

[aaronr]

(In reply to comment #23)

fixing the other nits

> >       nsXFormsUtils::DispatchEvent(mElement, eEvent_ComputeException);
> >+      nsXFormsUtils::HandleFatalError(
> >+        mElement, NS_LITERAL_STRING("XFormsComputeException"));
> 
> Is there any case where we do not need to handleFatalError after these kinds of
> exceptions? Why not move the HandleFatalError into DispatchEvent()?
> 

That would work in 95% of the cases.  But there are two places in the code right now where we want to serialize a node to the JS Console error message that ISN'T the target of the fatal error event.  For instance, the node that has the bad xpath expression on it that causes the xforms-compute-exception to be dispatched to the model.  Better to serialize that node than the model element.  In the other case we are dispatching a xforms-binding-exception for the case where a MIP is defined more than once on a node.  Rather than serializing the bind element, we serialize the model that contains the conflicting MIP's.  I assume since it won't always be two xf:binds conflicting, but also potentially an instance node conflicting with a bind element.

So I guess we should leave these as they are?

Comment 25

[aaronr]

Attachment: fixed other nits

Comment 26

[Allan Beaufour]

(In reply to comment #24)
> (In reply to comment #23)
> > >       nsXFormsUtils::DispatchEvent(mElement, eEvent_ComputeException);
> > >+      nsXFormsUtils::HandleFatalError(
> > >+        mElement, NS_LITERAL_STRING("XFormsComputeException"));
> > 
> > Is there any case where we do not need to handleFatalError after these kinds of
> > exceptions? Why not move the HandleFatalError into DispatchEvent()?
> > 
> 
> That would work in 95% of the cases.  But there are two places in the code
> right now where we want to serialize a node to the JS Console error message
> that ISN'T the target of the fatal error event.  For instance, the node that
> has the bad xpath expression on it that causes the xforms-compute-exception to
> be dispatched to the model.  Better to serialize that node than the model
> element.  In the other case we are dispatching a xforms-binding-exception for
> the case where a MIP is defined more than once on a node.  Rather than
> serializing the bind element, we serialize the model that contains the
> conflicting MIP's.  I assume since it won't always be two xf:binds conflicting,
> but also potentially an instance node conflicting with a bind element.
> 
> So I guess we should leave these as they are?
> 

If we do one thing 95% percent of the time, then I'd rather make that the common case (ie. include it in DispatchEvent), and then have a parameter for the exception.

Comment 27

[aaronr]

I was full of crap.  Looks like the element that is passed in is used to get the owner document, nothing more.  So as long as the event target for fatal errors lives in the xforms document, than should work.  I can't think of any situation where that isn't the case.  Will make the change ASAP.

Comment 28

[aaronr]

Attachment: fix with handlefatalerror moved

moved all HandleFatalError's to DispatchXFormsEvent.  Please review and verify that I handle the removal from other locations properly.  A couple of places actually handled the exceptions to return a 'handled' boolean (so I always return true in those places now) and one place checked the return code from HandleFatalError though I never saw anyone ever use the returned return code.  So I just returned NS_OK in that situation.

Comment 29

[Allan Beaufour]

Comment on attachment 222279 [details] [diff] [review]
fix with handlefatalerror moved

>@@ -71,17 +71,19 @@ nsXFormsSendElement::HandleAction(nsIDOM
> 
>   nsCOMPtr<nsIDOMElement> el;
>   doc->GetElementById(submissionID, getter_AddRefs(el));
> 
>   if (!el || !nsXFormsUtils::IsXFormsElement(el, submission)) {
>     const PRUnichar *strings[] = { submissionID.get(), submission.get() };
>     nsXFormsUtils::ReportError(NS_LITERAL_STRING("idRefError"),
>                                strings, 2, mElement, mElement);
>-    return nsXFormsUtils::DispatchEvent(mElement, eEvent_BindingException);
>+    nsresult rv = nsXFormsUtils::DispatchEvent(mElement,
>+                                               eEvent_BindingException);
>+    return rv;
>   }

NOP change...

With that skipped, dandy, r=me

Comment 30

[aaronr]

Attachment: patch for checkin

Comment 31

[aaronr]

checked into trunk