Last Comment Bug 315712 - Events thrown in the model need to be deferred.
: Events thrown in the model need to be deferred.
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: aaronr
: Stephen Pride
:
Mentors:
: 324719 (view as bug list)
Depends on:
Blocks: 306846 322255 331209
  Show dependency treegraph
 
Reported: 2005-11-09 07:37 PST by Doron Rosenberg (IBM)
Modified: 2016-07-15 14:46 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase (1.59 KB, application/xhtml+xml)
2006-05-01 14:20 PDT, aaronr
no flags Details
testcase 2 (run locally) (1.02 KB, application/xhtml+xml)
2006-05-01 14:26 PDT, aaronr
no flags Details
testcase 3 (1.14 KB, application/xhtml+xml)
2006-05-01 14:36 PDT, aaronr
no flags Details
testcase 4 (run locally) (955 bytes, application/xhtml+xml)
2006-05-01 14:49 PDT, aaronr
no flags Details
testcase 5 (1.12 KB, application/xhtml+xml)
2006-05-01 15:29 PDT, aaronr
no flags Details
testcase (1.88 KB, application/xhtml+xml)
2006-05-03 17:32 PDT, aaronr
no flags Details
regression testcase (1.88 KB, application/xhtml+xml)
2006-05-03 17:36 PDT, aaronr
no flags Details
proposed fix (27.11 KB, patch)
2006-05-05 16:27 PDT, aaronr
doronr: review+
Details | Diff | Splinter Review
testcase 5 (run locally) (1.14 KB, application/xhtml+xml)
2006-05-08 14:02 PDT, aaronr
no flags Details
patch with doron's comments fixed (27.12 KB, patch)
2006-05-08 14:05 PDT, aaronr
no flags Details | Diff | Splinter Review
fixed nits (27.16 KB, patch)
2006-05-09 12:38 PDT, aaronr
no flags Details | Diff | Splinter Review
fixed other nits (27.20 KB, patch)
2006-05-11 15:28 PDT, aaronr
no flags Details | Diff | Splinter Review
fix with handlefatalerror moved (30.02 KB, patch)
2006-05-16 17:30 PDT, aaronr
allan: review+
Details | Diff | Splinter Review
patch for checkin (28.89 KB, patch)
2006-05-17 12:22 PDT, aaronr
no flags Details | Diff | Splinter Review

Description Doron Rosenberg (IBM) 2005-11-09 07:37:54 PST
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 2006-01-25 17:10:16 PST
*** Bug 324719 has been marked as a duplicate of this bug. ***
Comment 2 aaronr 2006-03-15 14:37:34 PST
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) 2006-03-17 13:14:05 PST
Any comments?  Should we do this?
Comment 4 aaronr 2006-03-17 13:46:39 PST
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 2006-03-20 02:54:09 PST
(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) 2006-03-20 15:27:47 PST
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 2006-03-21 00:21:19 PST
(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 2006-05-01 14:15:23 PDT
taking bug from Doron.  Schema stuff higher priority on his list for now.
Comment 9 aaronr 2006-05-01 14:20:23 PDT
Created attachment 220433 [details]
testcase
Comment 10 aaronr 2006-05-01 14:26:03 PDT
Created attachment 220434 [details]
testcase 2 (run locally)
Comment 11 aaronr 2006-05-01 14:36:22 PDT
Created attachment 220436 [details]
testcase 3
Comment 12 aaronr 2006-05-01 14:49:40 PDT
Created attachment 220441 [details]
testcase 4 (run locally)
Comment 13 aaronr 2006-05-01 15:29:41 PDT
Created attachment 220448 [details]
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 2006-05-03 17:32:19 PDT
Created attachment 220724 [details]
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 2006-05-03 17:36:22 PDT
Created attachment 220725 [details]
regression testcase

previous regression testcase had an error in it.
Comment 16 aaronr 2006-05-05 16:20:48 PDT
I probably should have mentioned that testcase 2, 4, and 5 need to be run locally to expose this bug.
Comment 17 aaronr 2006-05-05 16:27:10 PDT
Created attachment 221009 [details] [diff] [review]
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) 2006-05-08 12:30:18 PDT
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 2006-05-08 14:02:55 PDT
Created attachment 221355 [details]
testcase 5 (run locally)

I forgot...labels don't automatically pick up model from context node.  Fixed testcase.
Comment 20 aaronr 2006-05-08 14:05:04 PDT
Created attachment 221356 [details] [diff] [review]
patch with doron's comments fixed
Comment 21 Allan Beaufour 2006-05-09 05:32:11 PDT
http://beaufour.dk/jst-review/?patch=221356
Comment 22 aaronr 2006-05-09 12:38:16 PDT
Created attachment 221482 [details] [diff] [review]
fixed nits

doh!  Sorry.  Forgot to run the JST tool.
Comment 23 Allan Beaufour 2006-05-11 08:22:08 PDT
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 2006-05-11 15:10:10 PDT
(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 2006-05-11 15:28:30 PDT
Created attachment 221754 [details] [diff] [review]
fixed other nits
Comment 26 Allan Beaufour 2006-05-15 04:30:43 PDT
(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 2006-05-16 16:47:40 PDT
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 2006-05-16 17:30:41 PDT
Created attachment 222279 [details] [diff] [review]
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 2006-05-17 01:45:25 PDT
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 2006-05-17 12:22:00 PDT
Created attachment 222396 [details] [diff] [review]
patch for checkin
Comment 31 aaronr 2006-05-17 12:31:21 PDT
checked into trunk

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