300255 - hint, help, alert and message need to generate xforms-link-error

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

Hint, help, alert and message don't generate xforms-link-error events when their
@src points to a bad, bad place.

Comment 1

[aaronr]

Attachment: testcase

Comment 2

[J.P.Klippel]

Attachment: corrected incorrect use of </xforms:input> input close tag instead of</xforms:label> in original testcase

I guess that shouldn't be part of the test.

Comment 3

[aaronr]

Attachment: better testcase

better testcase.  Took out error for hint since that gets hit too often.  Added
trigger to kick off alert.  So loading the form, pressing F1 in the input
field, and clicking on the trigger should generate xforms-link-error messages.

Comment 4

[aaronr]

Attachment: proposed fix

Comment 5

[Allan Beaufour]

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

Index: nsXFormsMessageElement.cpp
===================================================================
+  nsresult TestExternalFile(const nsAString& aSrc);
+  void ReportError(const PRUnichar *url[], PRBool securityError);

PRUnichar* aUrl[] (or aURL) and aSecurityError

and documentation please

 nsXFormsMessageElement::HandleAction(nsIDOMEvent* aEvent, 
				      nsIXFormsActionElement *aParentAction)
 {
   if (!mElement)
     return NS_OK;

+  // first, let's see if any external resources are required.	If there are
and
+  // they won't load, then might as well stop there.  We don't want to be

Can there be multiple resources defined and if there's a "first", where's the
"second"?

+  // popping up empty windows or windows that will just end up showing 404
+  // messages.
+  nsAutoString src;
+  mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
+  PRBool hasSrc = !src.IsEmpty();
+
+  if (hasSrc) {

Why save it in a variable, you only use it once?

+    nsresult rv = TestExternalFile(src);
+    if (NS_FAILED(rv)) {
+      // we couldn't get to the file we're supposed to link to.  Better throw

And now the ressource is a file.

+nsresult
+nsXFormsMessageElement::TestExternalFile(const nsAString& aSrc)
+{

Change this function to use "early returns" instead of this nested-if-business.

+  nsCOMPtr<nsIDOMDocument> domDoc;
+  mElement->GetOwnerDocument(getter_AddRefs(domDoc));
+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));
+  if (doc) {
+    nsCOMPtr<nsIURI> uri;
+    NS_NewURI(getter_AddRefs(uri), aSrc, doc->GetDocumentCharacterSet().get(),
+	       doc->GetDocumentURI());
+    if (uri) {
+      // XXX Passing |mElement| as |aContext| param to ReportError leads
+      //     to an infinite loop.  Avoid for now.

Are you sure about that? We (smaug) changed the serialization some time ago.

+      const nsPromiseFlatString& flat = PromiseFlatString(aSrc);
+      const PRUnichar *strings[] = { flat.get() };
+
+      if (nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), uri)) {
+	 nsresult rv;
+	 nsCOMPtr<nsIChannel> channel;

nit, move channel down just before it is used.

+	 nsCOMPtr<nsIIOService> ios = do_GetIOService(&rv);
+	 NS_ENSURE_SUCCESS(rv, rv);

Are you sure that ios is always valid? (I do not know, and have not checked...)

+	 rv = ios->NewChannelFromURI(uri, getter_AddRefs(channel));
+	 NS_ENSURE_SUCCESS(rv, rv);
+	 NS_ENSURE_STATE(channel);

Same question as above.

+	 // See if it's an http channel, which needs special treatment:
merge with comment below iff "special treatment" == "save time, just try to get
head"

+	 nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel);
+	 if (httpChannel) {
+	   // save time, just try to get the head

Do we really save time? The document needs to be loaded anyway, and loading it
here will cache it if possible. Of course, if it's not possible to cache, we do
save time by only doing a HEAD... hmmm, what's the common case?

+	   PRBool isReallyHTTP = PR_FALSE;
+	   uri->SchemeIs("http", &isReallyHTTP);
+	   if (!isReallyHTTP)
+	       uri->SchemeIs("https", &isReallyHTTP);
+	   if (isReallyHTTP) {
+	       httpChannel->SetRequestMethod(NS_LITERAL_CSTRING("HEAD"));
+	   }

Lack of consequence in use of curly brackets and if. I'm pretty sure most
XForms code always uses curly brackets

+	 }
+
+	 // using synchronous Open since a lot of logic in 

kill whitespace at end.

+	 // nsXFormsMessageElement depends on values from the generating event
+	 // (like in HandleAction()).  We can't let this thread return and
+	 // free up the event, which would happen if we did this
+	 // asynchronously.  But this may screw up interacting with the browser
+	 // UI.
+	 // XXX check out if this screws up interacting with the browser UI.

*ahem* that would be a good one to check before requesting review ;-)

I'm not sure you mean by "screw up interacting with the browser" though?

+	 nsCOMPtr<nsIInputStream> inputStream;
+	 rv = channel->Open(getter_AddRefs(inputStream));
+	 if (NS_FAILED(rv)) {
+	   // URI doesn't exist; report error.
+	   ReportError(strings, PR_FALSE);
+	   return rv;
+	 } else {

No need for an 'else', it's never used.

+	   PRUint32 responseStatus;
+	   rv = httpChannel->GetResponseStatus(&responseStatus);

+	   if (NS_FAILED(rv)) {
+	     ReportError(strings, PR_FALSE);
+	     return rv;
+	   }

kill above four lines, and:

+	   // If it's between 200-299, it's valid:
+	   if (responseStatus / 100 == 2) {
if (NS_SUCCEEDED(rv) && responseStatus / 100 == 2) {
+	     return NS_OK;
+	   }
+
+	   // URI doesn't exist; report error.
+	   ReportError(strings, PR_FALSE);
+	   return NS_ERROR_FAILURE;
+	 }
+      } else {
+	 ReportError(strings, PR_TRUE);
+	 return NS_ERROR_FAILURE;
+      }
+    }
+  }
+

+void
+nsXFormsMessageElement::ReportError(const PRUnichar *aURL[], 

kill whitespace at end

+				     PRBool aSecurityError)
+{

Use the generic error message I have proposed below, and eliminate one of the
switches.

+  if (aSecurityError) {
+    switch (mType) {
+      case eType_Normal:
+	 nsXFormsUtils::ReportError(NS_LITERAL_STRING("messageLinkLoadOrigin"),
+				    aURL, 1, mElement, nsnull);
+	 break;
+      case eType_Hint:
+	 nsXFormsUtils::ReportError(NS_LITERAL_STRING("hintLinkLoadOrigin"),
+				    aURL, 1, mElement, nsnull);
+	 break;
+      case eType_Help:
+	 nsXFormsUtils::ReportError(NS_LITERAL_STRING("helpLinkLoadOrigin"),
+				    aURL, 1, mElement, nsnull);
+	 break;
+      case eType_Alert:
+	 nsXFormsUtils::ReportError(NS_LITERAL_STRING("alertLinkLoadOrigin"),
+				    aURL, 1, mElement, nsnull);
+	 break;
+    }
+  } else {
+    switch (mType) {
+      case eType_Normal:
+	 nsXFormsUtils::ReportError(NS_LITERAL_STRING("messageLink1Error"),
+				    aURL, 1, mElement, nsnull);
+	 break;
+      case eType_Hint:
+	 nsXFormsUtils::ReportError(NS_LITERAL_STRING("hintLink1Error"),
+				    aURL, 1, mElement, nsnull);
+	 break;
+      case eType_Help:
+	 nsXFormsUtils::ReportError(NS_LITERAL_STRING("helpLink1Error"),
+				    aURL, 1, mElement, nsnull);
+	 break;
+      case eType_Alert:
+	 nsXFormsUtils::ReportError(NS_LITERAL_STRING("alertLink1Error"),
+				    aURL, 1, mElement, nsnull);
+	 break;
+    }
+  }
+}

Index: resources/locale/en-US/xforms.properties
===================================================================
+messageLinkLoadOrigin = XForms Error (21): Security check failed! Trying to
load message data from a different domain than document
+helpLinkLoadOrigin    = XForms Error (22): Security check failed! Trying to
load help data from a different domain than document
+hintLinkLoadOrigin    = XForms Error (23): Security check failed! Trying to
load hint data from a different domain than document
+alertLinkLoadOrigin   = XForms Error (24): Security check failed! Trying to
load alert data from a different domain than document
+messageLink1Error     = XForms Error (25): External file (%S) for Message
element not found
+helpLink1Error        = XForms Error (26): External file (%S) for Help element
not found
+hintLink1Error        = XForms Error (27): External file (%S) for Hint element
not found
+alertLink1Error       = XForms Error (28): External file (%S) for Alert
element not found

Please make labelLink1Error and labelLink2Error more generic and use those
instead, i.e.:
linkSecurityError      = XForms Error (XX): External file (%S) for %S element
not found
linkLoadOriginError    = XForms Error (XX): Security check failed! Trying to
load %S data from a different domain than document

kill instanceLoadOrigin too.

Comment 6

[aaronr]

Attachment: second try

fixed Allan's comments.  Also fixed a precendence bug where we were taking
value of @src above the single node binding.  That was a quick one line fix

Comment 7

[aaronr]

Attachment: same test, add precedence

same testcase as before, but added a precedence test

Comment 8

[Allan Beaufour]

Comment on attachment 189382 [details] [diff] [review]
second try

> Index: nsXFormsInstanceElement.cpp
> ===================================================================
>    if (!nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), newURI)) {
> -    nsXFormsUtils::ReportError(NS_LITERAL_STRING("instanceLoadOrigin"), domDoc);
> +    nsAutoString tagName;
> +    mElement->GetLocalName(tagName);

When is tagName != "instance"?

> Index: nsXFormsLabelElement.cpp
> ===================================================================

>  nsXFormsLabelElement::LoadExternalLabel(const nsAString& aSrc)
...
> -            // XXX Passing |mElement| as |aContext| param to ReportError leads
> -            //     to an infinite loop.  Avoid for now.
>              const nsPromiseFlatString& flat = PromiseFlatString(aSrc);
> -            const PRUnichar *strings[] = { flat.get() };
> -            nsXFormsUtils::ReportError(NS_LITERAL_STRING("labelLink1Error"),
> -                                       strings, 1, mElement, nsnull);
> +            nsAutoString tagName;
> +            mElement->GetLocalName(tagName);

When is tagName != "label"?

> Index: nsXFormsMessageElement.cpp
> ===================================================================
> +  /**
> +   * Test to see if this message element could get held up later by linking to
> +   * an external resource.  This function will fail if the address in the src
> +   * attribute couldn't be reached or if an http request returned a non 2xx 
> +   * status code.  Will return successful if the src attribute isn't present,
> +   * if single node binding is present so that the src attribut should be

attribute


> +  // If TestExternalFile fails, then there was an external link that we needed
> +  // to use that we can't reach right now.  If it won't load, then might as 

present tense? "is an external link that we need"

> +nsXFormsMessageElement::TestExternalFile()
> +{
> +  // Let's see if checking for any external resources is even necessary.  Single
> +  // node binding trumps linking attributes in order of precendence.  If we
> +  // find single node binding in evidence, then return NS_OK to show that
> +  // this message element has access to the info that it needs.
> +  nsAutoString snb;
> +  mElement->GetAttribute(NS_LITERAL_STRING("bind"), snb);
> +  NS_ENSURE_TRUE(snb.IsEmpty(), NS_OK);
> +  mElement->GetAttribute(NS_LITERAL_STRING("ref"), snb);
> +  NS_ENSURE_TRUE(snb.IsEmpty(), NS_OK);

Do not use NS_ENSURE_TRUE for this as it will print errors on the console, and
that's not what you want.

Can you use mBindAttrsCount for this?

> +  const PRUnichar *strings[2];

Hmmm, space taken even if no errors. Not good.

> +  if (nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), uri)) {

I'm fond of early returns...

> +    // See if it's an http channel.  We'll look at the http status code more
> +    // closely and only request the "HEAD" to  keep the response small.
> +    // Especially since we are requesting it synchronously.
> +    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel);
> +    if (httpChannel) {
> +      PRBool isReallyHTTP = PR_FALSE;
> +      uri->SchemeIs("http", &isReallyHTTP);
> +      if (!isReallyHTTP) {
> +          uri->SchemeIs("https", &isReallyHTTP);
> +      }
> +      if (isReallyHTTP) {
> +          httpChannel->SetRequestMethod(NS_LITERAL_CSTRING("HEAD"));
> +      }
> +    }

See (and answer) comment 5.

> +    // set up the error strings in case we need them
> +    nsAutoString tagName;
> +    mElement->GetLocalName(tagName);
> +    strings[0] = src.get();
> +    strings[1] = tagName.get();

Same as above. Do not use space or time on stuff we do not need.

(btw, I like the idea of using GetLocalName() here instead of the switch).

> +    // using synchronous Open since a lot of logic in nsXFormsMessageElement
> +    // depends on values from the generating event (like in HandleAction()).
> +    // We can't let this thread return and free up the event, which would
> +    // happen if we did this asynchronously.  But this may screw up
> +    // interacting with the browser UI.

See (and answer) comment 5.

> +    nsCOMPtr<nsIInputStream> inputStream;
> +    rv = channel->Open(getter_AddRefs(inputStream));
> +    if (NS_FAILED(rv)) {
> +      // URI doesn't exist; report error.
> +      nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink1Error"),
> +                                 strings, 2, mElement, nsnull);
> +      return rv;
> +    }
> +
> +    PRUint32 responseStatus;
> +    rv = httpChannel->GetResponseStatus(&responseStatus);
> +
> +    // If it's between 200-299, it's valid:
> +    if (NS_SUCCEEDED(rv) && (responseStatus / 100 == 2)) {
> +      return NS_OK;
> +    }
> +
> +    // URI doesn't exist; report error.
> +    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink1Error"),
> +                               strings, 2, mElement, nsnull);
> +    return NS_ERROR_FAILURE;
> +  } else {

Still no need for this else (comment 5 again....), but see next comment

> +    nsAutoString tagName;
> +    mElement->GetLocalName(tagName);
> +    strings[0] = tagName.get();
> +    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLinkLoadOrigin"),
> +                               strings, 1, domDoc, domDoc);
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  return NS_ERROR_FAILURE;
> +}

I do not like all those returns. In the above code you can actually kill the
first two, and obtain the same.

I'm not too fond of the two identical ReportError-lines either, I'm pretty sure
you can kill one of them.

Comment 9

[aaronr]

(In reply to comment #8)
> (From update of attachment 189382 [details] [diff] [review] [edit])
> > Index: nsXFormsInstanceElement.cpp
> > ===================================================================
> >    if (!nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), newURI)) {
> > -    nsXFormsUtils::ReportError(NS_LITERAL_STRING("instanceLoadOrigin"),
domDoc);
> > +    nsAutoString tagName;
> > +    mElement->GetLocalName(tagName);
> 
> When is tagName != "instance"?
> 

fixed.  Using literal string "instance"

> > Index: nsXFormsLabelElement.cpp
> > ===================================================================
> 
> >  nsXFormsLabelElement::LoadExternalLabel(const nsAString& aSrc)
> ...
> > -            // XXX Passing |mElement| as |aContext| param to ReportError leads
> > -            //     to an infinite loop.  Avoid for now.
> >              const nsPromiseFlatString& flat = PromiseFlatString(aSrc);
> > -            const PRUnichar *strings[] = { flat.get() };
> > -            nsXFormsUtils::ReportError(NS_LITERAL_STRING("labelLink1Error"),
> > -                                       strings, 1, mElement, nsnull);
> > +            nsAutoString tagName;
> > +            mElement->GetLocalName(tagName);
> 
> When is tagName != "label"?

fixed.  Using literal string "label"

> 
> > Index: nsXFormsMessageElement.cpp
> > ===================================================================
> > +  /**
> > +   * Test to see if this message element could get held up later by linking to
> > +   * an external resource.  This function will fail if the address in the src
> > +   * attribute couldn't be reached or if an http request returned a non 2xx 
> > +   * status code.  Will return successful if the src attribute isn't present,
> > +   * if single node binding is present so that the src attribut should be
> 
> attribute
> 
> 
> > +  // If TestExternalFile fails, then there was an external link that we needed
> > +  // to use that we can't reach right now.  If it won't load, then might as 
> 
> present tense? "is an external link that we need"

fixed.

> 
> > +nsXFormsMessageElement::TestExternalFile()
> > +{
> > +  // Let's see if checking for any external resources is even necessary. 
Single
> > +  // node binding trumps linking attributes in order of precendence.  If we
> > +  // find single node binding in evidence, then return NS_OK to show that
> > +  // this message element has access to the info that it needs.
> > +  nsAutoString snb;
> > +  mElement->GetAttribute(NS_LITERAL_STRING("bind"), snb);
> > +  NS_ENSURE_TRUE(snb.IsEmpty(), NS_OK);
> > +  mElement->GetAttribute(NS_LITERAL_STRING("ref"), snb);
> > +  NS_ENSURE_TRUE(snb.IsEmpty(), NS_OK);
> 
> Do not use NS_ENSURE_TRUE for this as it will print errors on the console, and
> that's not what you want.
> 
> Can you use mBindAttrsCount for this?

stopped using NS_ENSURE_TRUE with a return of NS_OK.  But I can't use
mBindAttrsCount since nsXFormsMessageElement doesn't inherit from
nsXFormsControlStub.

> 
> > +  const PRUnichar *strings[2];
> 
> Hmmm, space taken even if no errors. Not good.

I figured easier reading to have array declared once in the function rather than
 a couple of different times in just the error cases.  But changed to do that.

> 
> > +  if (nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), uri)) {
> 
> I'm fond of early returns...

changed.

> 
> > +    // See if it's an http channel.  We'll look at the http status code more
> > +    // closely and only request the "HEAD" to  keep the response small.
> > +    // Especially since we are requesting it synchronously.
> > +    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel);
> > +    if (httpChannel) {
> > +      PRBool isReallyHTTP = PR_FALSE;
> > +      uri->SchemeIs("http", &isReallyHTTP);
> > +      if (!isReallyHTTP) {
> > +          uri->SchemeIs("https", &isReallyHTTP);
> > +      }
> > +      if (isReallyHTTP) {
> > +          httpChannel->SetRequestMethod(NS_LITERAL_CSTRING("HEAD"));
> > +      }
> > +    }
> 
> See (and answer) comment 5.

I don't see caching as terribly useful except maybe in the case of "hint" since
message, help, and alert aren't likely to be called again very often if they
were already triggered by the user once.  Or do you mean to do a full load here
and if it works to keep the stream around until we need to use it?  Looking into it.

> 
> > +    // set up the error strings in case we need them
> > +    nsAutoString tagName;
> > +    mElement->GetLocalName(tagName);
> > +    strings[0] = src.get();
> > +    strings[1] = tagName.get();
> 
> Same as above. Do not use space or time on stuff we do not need.
> 

Fixed.

> (btw, I like the idea of using GetLocalName() here instead of the switch).
> 
> > +    // using synchronous Open since a lot of logic in nsXFormsMessageElement
> > +    // depends on values from the generating event (like in HandleAction()).
> > +    // We can't let this thread return and free up the event, which would
> > +    // happen if we did this asynchronously.  But this may screw up
> > +    // interacting with the browser UI.
> 
> See (and answer) comment 5.

What specifically?  We have to do this synchronously as I mentioned in the
comment.  We have to have the event context information and can't allow the
event that caused us to execute to propagate up the chain to other handlers if
we haven't executed yet or the whole sequence will be off.  It is a minor UI
impact to the form even on a 28.8 line and the browser can still be interacted
with, at least on the forms that I tested with.  But the form I tested with was
small, simple plain text being served up over a phone line.  I didn't try a real
HTML document served as a help document, for example.  And of course on a
high-speed network (which most XForms users will likely have) it tests out just
fine.

I will ask bryner, though, if there is a way to pause event handling in this way
to do asynchronous requests.

> 
> > +    nsCOMPtr<nsIInputStream> inputStream;
> > +    rv = channel->Open(getter_AddRefs(inputStream));
> > +    if (NS_FAILED(rv)) {
> > +      // URI doesn't exist; report error.
> > +      nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink1Error"),
> > +                                 strings, 2, mElement, nsnull);
> > +      return rv;
> > +    }
> > +
> > +    PRUint32 responseStatus;
> > +    rv = httpChannel->GetResponseStatus(&responseStatus);
> > +
> > +    // If it's between 200-299, it's valid:
> > +    if (NS_SUCCEEDED(rv) && (responseStatus / 100 == 2)) {
> > +      return NS_OK;
> > +    }
> > +
> > +    // URI doesn't exist; report error.
> > +    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink1Error"),
> > +                               strings, 2, mElement, nsnull);
> > +    return NS_ERROR_FAILURE;
> > +  } else {
> 
> Still no need for this else (comment 5 again....), but see next comment
> 
> > +    nsAutoString tagName;
> > +    mElement->GetLocalName(tagName);
> > +    strings[0] = tagName.get();
> > +    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLinkLoadOrigin"),
> > +                               strings, 1, domDoc, domDoc);
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  return NS_ERROR_FAILURE;
> > +}
> 
> I do not like all those returns. In the above code you can actually kill the
> first two, and obtain the same.
> 
> I'm not too fond of the two identical ReportError-lines either, I'm pretty sure
> you can kill one of them.
> 

fixed.

My next patch will have the fixes mentioned above and I follow up about sync vs.
async and caching the load.

Comment 10

[aaronr]

The only external resource that we should probably go ahead with a full request
instead of just a "head" request would be for an ephemeral message.  Otherwise
we are just passing off the URL to other services to put up the display window.
  Since we don't currently handle any external resources for ephemeral messages
right now (https://bugzilla.mozilla.org/show_bug.cgi?id=300870), it should
probably be part of that work so that they can test that it works properly. 
I'll add a comment to that bug to that effect.

Comment 11

[aaronr]

Attachment: next attempt

patch with all of the changes mentioned in comment #9.	No caching for reason
given in comment #10.  Still waiting to hear from bryner to see if he has any
ideas on another way to do this other than synchronously and not cause problems
with events.

Comment 12

[aaronr]

comment from bryner:

DOM event dispatch is non-interruptable, and is pretty likely to stay
that way (think about script executing, etc, you would have to be able
to suspend the entire state of the JS engine and restore it later when
the data came back).

To me this seems like a major shortcoming in the XForms spec, that it
assumes it's acceptable for the src to load synchronously.   The only
thing I can think of that might help is to preload the src so that
it's ready to go should the message need to be displayed.

Comment 13

[aaronr]

comment from darin:

nsIChannel::open also does not guarantee that events won't
be processed.  In fact, the nsIInputStream returned by the
HTTP implementation of nsIChannel::open, will process pending
PLEvents when its Available or Read method is called.

I recommend not implementing the synchronous way of loading
resources via XForms.  It's a poorly designed specification if it
believes that synchronously loading resources in a browser (or
any other GUI application) is a good thing.

We have the same problem with document.load and the sync
way of loading via XMLHttpRequest.  Neither is implemented
well in Mozilla, and UI responsiveness generally suffers
whenever people use the APIs.

Comment 14

[aaronr]

Attachment: nother attempt

smaug, could you look at this patch to see what I am doing wrong?  I am adding
the URI loads to the document's load group, but the DOMContentLoaded event will
fire before the OnStopRequests are called on the message elements.  That
shouldn't happen, should it?

Comment 15

[aaronr]

(In reply to comment #14)
> Created an attachment (id=190651) [edit]
> nother attempt
> 
> smaug, could you look at this patch to see what I am doing wrong?  I am adding
> the URI loads to the document's load group, but the DOMContentLoaded event will
> fire before the OnStopRequests are called on the message elements.  That
> shouldn't happen, should it?

nevermind.  Looks like DOMContentLoaded is just to notify that parsing has
completed.  Since we aren't looking for the load event to fire xforms-ready, I
guess that I'll have to hook message testing in with the model like the instance
elements do and not allow xforms-ready to fire before all of our message tests
are done.

Does anyone remember why we aren't looking for load anymore and are looking for
DOMContentLoaded instead?

Comment 16

[aaronr]

Attachment: new attempt

in addition to the fixed comments, I've also changed patch to use AsyncOpen.  I
also added code to message and modal to hold off xforms-ready until all of the
external messages have been tested.

Comment 17

[Allan Beaufour]

Comment on attachment 191646 [details] [diff] [review]
new attempt

Please attend to:
http://lab.cph.novell.com/~abeaufour/jst-review/?patch=191646

Comment 18

[Allan Beaufour]

Comment on attachment 191646 [details] [diff] [review]
new attempt

> Index: nsXFormsInstanceElement.cpp
> ===================================================================

>    if (!nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), newURI)) {
> -    nsXFormsUtils::ReportError(NS_LITERAL_STRING("instanceLoadOrigin"), domDoc);
> +    const PRUnichar *strings[] = { NS_LITERAL_STRING("instance").get() };
> +    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLinkLoadOrigin"),
> +                               strings, 1, domDoc, domDoc);

Why not mElement, mElement?

> Index: nsXFormsLabelElement.cpp
> ===================================================================
> -            // XXX Passing |mElement| as |aContext| param to ReportError leads
> -            //     to an infinite loop.  Avoid for now.
>              const nsPromiseFlatString& flat = PromiseFlatString(aSrc);
> -            const PRUnichar *strings[] = { flat.get() };
> -            nsXFormsUtils::ReportError(NS_LITERAL_STRING("labelLink1Error"),
> -                                       strings, 1, mElement, nsnull);
> +            const PRUnichar *strings[] = { flat.get(),
> +                                           NS_LITERAL_STRING("label").get() };
> +            nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink1Error"),
> +                                       strings, 2, mElement, nsnull);

mElement as aContext?

>              nsCOMPtr<nsIModelElementPrivate> modelPriv =
>                                                nsXFormsUtils::GetModel(mElement);
>              nsCOMPtr<nsIDOMNode> model = do_QueryInterface(modelPriv);
>              nsXFormsUtils::DispatchEvent(model, eEvent_LinkError);
>            }
>          }
>        } else {
> -        nsXFormsUtils::ReportError(NS_LITERAL_STRING("labelLinkLoadOrigin"),
> -                                   domDoc);
> +        const PRUnichar *strings[] = { NS_LITERAL_STRING("label").get() };
> +        nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLinkLoadOrigin"),
> +                                   strings, 1, domDoc, domDoc);

Why not mElement, mElement?

> -    // XXX Passing |mElement| as |aContext| param to ReportError leads
> -    //     to an infinite loop.  Avoid for now.
>      nsAutoString src;
>      mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
> -    const PRUnichar *strings[] = { src.get() };
> -    nsXFormsUtils::ReportError(NS_LITERAL_STRING("labelLink2Error"),
> -                               strings, 1, mElement, nsnull);
> +    const PRUnichar *strings[] = { NS_LITERAL_STRING("label").get(), src.get() };
> +    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink2Error"),
> +                               strings, 2, mElement, nsnull);

Your editor is aparently not intelligent enough to understand the implications
of removing the comment :)

> Index: nsXFormsMessageElement.cpp
> ===================================================================

> +NS_IMETHODIMP
>  nsXFormsMessageElement::HandleAction(nsIDOMEvent* aEvent, 
>                                       nsIXFormsActionElement *aParentAction)
>  {
>    if (!mElement)
>      return NS_OK;

Shouldn't you check what type of event you handle here?

> +  // If there is still a channel, then someone must have changed the value of
> +  // the src attribute since the document finished loading and we haven't yet
> +  // determined whether the new link is valid or not.  For now we'll assume
> +  // that the value is good rather than returning a link error.  Let's show a
> +  // warning as a clue to the developer since they may see undesired behavior
> +  // (like an empty message or a message with a 404 error in it).  Also
> +  // canceling the channel since it is too late now.
> +  if (mChannel) {
> +    mChannel->Cancel(NS_BINDING_ABORTED);
> +    NS_WARN_IF_FALSE(!mChannel, "displaying a XForms message without testing link first\n");

Will Cancel() be handled synchroneously? If not, the test will always be false.

> +nsresult
> +nsXFormsMessageElement::TestExternalFile()
> +{
> +  // Let's see if checking for any external resources is even necessary.  Single
> +  // node binding trumps linking attributes in order of precendence.  If we
> +  // find single node binding in evidence, then return NS_OK to show that
> +  // this message element has access to the info that it needs.
> +  nsAutoString snb;
> +  mElement->GetAttribute(NS_LITERAL_STRING("bind"), snb);
> +  if (!snb.IsEmpty()) {
> +    return NS_OK;
> +  }
> +  mElement->GetAttribute(NS_LITERAL_STRING("ref"), snb);
> +  if (!snb.IsEmpty()) {
> +    return NS_OK;
> +  }
> +
> +  // if no external linking to check, we can also let the caller know that we
> +  // haven't encountered any problems.

Weird comment, or at least at the wrong place? Should be at the return?

> +  nsAutoString src;
> +  mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
> +  if (src.IsEmpty()) {
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIDOMDocument> domDoc;
> +  mElement->GetOwnerDocument(getter_AddRefs(domDoc));
> +  nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));
> +  NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);

Use NS_ENSURE_STATE

> +  nsCOMPtr<nsIURI> uri;
> +  NS_NewURI(getter_AddRefs(uri), src, doc->GetDocumentCharacterSet().get(),
> +            doc->GetDocumentURI());

save doc->GetDocumentURI() and use it in the if statement

> +  NS_ENSURE_TRUE(uri, NS_ERROR_FAILURE);

Use NS_ENSURE_STATE

> +
> +  if (!nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), uri)) {
> +    nsAutoString tagName;
> +    mElement->GetLocalName(tagName);
> +    const PRUnichar *strings[] = { tagName.get() };
> +    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLinkLoadOrigin"),
> +                               strings, 1, domDoc, domDoc);

Why not mElement?

> +    // Keep the the dialog from popping up.  Won't be able to reach the
> +    // resource anyhow.
> +    mStopType = eStopType_Security;
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsILoadGroup> loadGroup = doc->GetDocumentLoadGroup();
> +  NS_WARN_IF_FALSE(loadGroup, "No load group!");

Are you sure you just want to warn? Execution continues after the warning.
NS_ASSERTION or NS_ENSURE_STATE?

> +
> +  // Using the same load group as the main document and creating
> +  // the channel with LOAD_NORMAL flag delays the dispatching of
> +  // the 'load' event until message data document has been loaded.
> +  nsresult rv = NS_NewChannel(getter_AddRefs(mChannel), uri, nsnull, loadGroup,
> +                              this, nsIRequest::LOAD_NORMAL);
> +  NS_ENSURE_TRUE(mChannel, rv);

NS_ENSURE_STATE

> +  rv = mChannel->AsyncOpen(this, nsnull);
> +  if (NS_FAILED(rv)) {
> +    mChannel = nsnull;
> +  
> +    // URI doesn't exist; report error.
> +    // set up the error strings
> +    nsAutoString tagName;
> +    mElement->GetLocalName(tagName);
> +    const PRUnichar *strings[] = { src.get(), tagName.get() };
> +    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink1Error"),
> +                               strings, 2, mElement, nsnull);

Why not use mElement as context?

> +    mStopType = eStopType_LinkError;
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // channel should be running along smoothly, increment the count
> +  AddRemoveExternalResource(PR_TRUE);
> +  return NS_OK;
> +}
> +
> +// nsIInterfaceRequestor

Why do we need this?

> +NS_IMETHODIMP
> +nsXFormsMessageElement::OnStopRequest(nsIRequest *aRequest,
> +                                      nsISupports *aContext,
> +                                      nsresult aStatusCode)
> +{
> +
> +  // We are done with the load request, so make sure that we null out mChannel
> +  // before we return!
> +
> +  if (NS_FAILED(aStatusCode)) {
> +    AddRemoveExternalResource(PR_FALSE);

Start by doing this?

> +  PRUint32 responseStatus;
> +  nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);
> +  if (httpChannel) {
> +    nsresult rv = httpChannel->GetResponseStatus(&responseStatus);
> +    
> +    // If responseStatus is 4xx, it is an error.  2xx is success.  3xx (various
> +    // flavors of redirection) COULD be successful.  Can't really follow those
> +    // to conclusion so we'll assume they were successful.
> +    if (NS_FAILED(rv) || (responseStatus / 100 == 4)) {

5xx codes are also errors.

> +nsXFormsMessageElement::AddRemoveExternalResource(PRBool aAdd)
> +{
> +  // if this message doesn't have a channel established already or it has
> +  // already returned, then no since bumping the counter.

s/since/sense in/?

> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> +  PRUint32 loadingMessages =
> +    (PRUint32)doc->GetProperty(nsXFormsAtoms::externalMessagesProperty);

Use NS_STATIC_CAST

> +  if(!loadingMessages) {
> +    // no outstanding loads left, let the model in the document know in case
> +    // the models are waiting to send out the xforms-ready event
> +
> +    nsCOMPtr<nsIModelElementPrivate> modelPriv =
> +      nsXFormsUtils::GetModel(mElement);
> +    nsCOMPtr<nsIDOMDocumentEvent> docEvent = do_QueryInterface(doc);
> +    nsCOMPtr<nsIDOMEvent> event;
> +    docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
> +    event->InitEvent(NS_LITERAL_STRING(NS_XFORMS_EXTMSG_READY), PR_FALSE,
> +                     PR_FALSE);
> +  
> +    nsCOMPtr<nsIDOMEventTarget> targetEl = do_QueryInterface(modelPriv);
> +    if (targetEl) {
> +      nsCOMPtr<nsIDOMNode> el = do_QueryInterface(targetEl);
> +      nsXFormsUtils::SetEventTrusted(event, el);
> +      PRBool defaultActionEnabled;
> +
> +      // if there are no more messages loading then it is probably the case
> +      // that my mChannel is going to get nulled out as soon as this function
> +      // returns.  If model is waiting for this event, then it may kick off
> +      // the message right away and we should probably ensure that mChannel
> +      // is gone before HandleAction is called.  So...if the channel isn't
> +      // pending, let's null it out right here.
> +      PRBool isPending = PR_TRUE;
> +      mChannel->IsPending(&isPending);
> +      if(!isPending) {
> +        mChannel = nsnull;
> +      }
> +      targetEl->DispatchEvent(event, &defaultActionEnabled);
> +    }

What's the reason for not just calling a method on the model?

> Index: nsXFormsModelElement.cpp
> ===================================================================
> @@ -460,20 +462,52 @@ nsXFormsModelElement::HandleDefault(nsID
>    } else if (type.EqualsASCII(sXFormsEventsEntries[eEvent_Revalidate].name)) {
>      rv = Revalidate();
>    } else if (type.EqualsASCII(sXFormsEventsEntries[eEvent_Recalculate].name)) {
>      rv = Recalculate();
>    } else if (type.EqualsASCII(sXFormsEventsEntries[eEvent_Rebuild].name)) {
>      rv = Rebuild();
>    } else if (type.EqualsASCII(sXFormsEventsEntries[eEvent_ModelConstructDone].name)) {
>      rv = ConstructDone();
> +    mDone = PR_TRUE;
>    } else if (type.EqualsASCII(sXFormsEventsEntries[eEvent_Ready].name)) {
>      Ready();
>    } else if (type.EqualsASCII(sXFormsEventsEntries[eEvent_Reset].name)) {
>      Reset();
> +  } else if(type.EqualsASCII(NS_XFORMS_EXTMSG_READY)) {
> +    // currently no external messages remaining to finish test loading.  If

Cryptic comment. I know what you mean, but could you rephrase it?

> +    // we were waiting for this to send out xforms-ready, then now is the time.
> +
> +    nsCOMPtr<nsIDOMDocument> domDoc;
> +    mElement->GetOwnerDocument(getter_AddRefs(domDoc));
> +    const nsVoidArray *models = GetModelList(domDoc);
> +    nsCOMPtr<nsIDocument>doc = do_QueryInterface(domDoc);
> +    nsCOMArray<nsIXFormsControlBase> *deferredBindList =
> +      NS_STATIC_CAST(nsCOMArray<nsIXFormsControlBase> *,
> +                     doc->GetProperty(nsXFormsAtoms::deferredBindListProperty));
> +
> +    // if this document hasn't process xforms-model-construct-done, yet (which
> +    // must precede xforms-ready), or hasn't finished processing all of the
> +    // deferred binds, yet, then we'll be able to send xforms-ready out as
> +    // usual.  If we've already become ready, then this event was probably
> +    // generated by a change in the src attribute on the message element.
> +    // Ignore it in that case.

That's a lot of work before checking for the two booleans. Please check those
before checking the deferredBindList.

>  nsXFormsModelElement::Ready()
>  {
>    BackupOrRestoreInstanceData(PR_FALSE);
> +  mReady = PR_TRUE;

Why set this here, and mDone in HandleDefault()? I have no preference to where,
just do it consistently.

>  nsXFormsModelElement::BackupOrRestoreInstanceData(PRBool restore)
>  {
> @@ -1402,16 +1438,27 @@ nsXFormsModelElement::MaybeNotifyComplet
>    for (i = 0; i < models->Count(); ++i) {
>      nsXFormsModelElement *model =
>          NS_STATIC_CAST(nsXFormsModelElement *, models->ElementAt(i));
>      nsXFormsUtils::DispatchEvent(model->mElement, eEvent_ModelConstructDone);
>    }

>    nsXFormsModelElement::ProcessDeferredBinds(domDoc);

> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);

Possibly not the most important var. to null-check, but still.

> +  PRUint32 loadingMessages =
> +    (PRUint32)doc->GetProperty(nsXFormsAtoms::externalMessagesProperty);

NS_STATIC_CAST

> Index: nsXFormsModelElement.h
> ===================================================================
> +
> +  /**
> +   * Indicates whether the model's handled the xforms-model-construct-done
> +   * event already
> +   */

Please include that in the variable name too then. mConstructDoneHandled?

> +  PRBool mDone;
> +
> +  /**
> +   * Indicates whether the model's handled the xforms-ready event already
> +   */
> +  PRBool mReady;

Same here.

Comment 19

[Allan Beaufour]

Could you please create a testcase that tests whether you handle the
xforms-ready business correctly btw.?

Comment 20

[aaronr]

(In reply to comment #19)
> Could you please create a testcase that tests whether you handle the
> xforms-ready business correctly btw.?

Like what kind of testcase?  The attached testcase has a message that has an
invalid external link that fires on xforms-ready.  If you see an
xforms-link-error message when this testcase loads, then that means that by the
time that xforms-ready is processed by the event handler (and thus the message's
::HandleAction is called) the determination has been made that the link is no good.

Comment 21

[aaronr]

(In reply to comment #18)
> (From update of attachment 191646 [details] [diff] [review] [edit])
> > Index: nsXFormsInstanceElement.cpp
> > ===================================================================
> 
> >    if (!nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), newURI)) {
> > -    nsXFormsUtils::ReportError(NS_LITERAL_STRING("instanceLoadOrigin"),
domDoc);
> > +    const PRUnichar *strings[] = { NS_LITERAL_STRING("instance").get() };
> > +    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLinkLoadOrigin"),
> > +                               strings, 1, domDoc, domDoc);
> 
> Why not mElement, mElement?

For this and the other similar comments mentioned above, all I was changing was
the error message and the strings used to build them.  I didn't change anything
else that was getting passed to ReportError.  But you are right, using domDoc as
the context doesn't really fit now that smaug fixed using the contextNode
problem that we had before with ReportError.  I've changed this and the other
similar ReportErrors to use mElement instead of domDoc.
 
> > Index: nsXFormsMessageElement.cpp
> > ===================================================================
> 
> > +NS_IMETHODIMP
> >  nsXFormsMessageElement::HandleAction(nsIDOMEvent* aEvent, 
> >                                       nsIXFormsActionElement *aParentAction)
> >  {
> >    if (!mElement)
> >      return NS_OK;
> 
> Shouldn't you check what type of event you handle here?

This is like the other action elements' HandleAction, not a control's typical
HandleEvent.  A message doesn't care much what event caused it.  Just properties
of the event like whether to allow handle default and propagation.

> 
> > +  // If there is still a channel, then someone must have changed the value of
> > +  // the src attribute since the document finished loading and we haven't yet
> > +  // determined whether the new link is valid or not.  For now we'll assume
> > +  // that the value is good rather than returning a link error.  Let's show a
> > +  // warning as a clue to the developer since they may see undesired behavior
> > +  // (like an empty message or a message with a 404 error in it).  Also
> > +  // canceling the channel since it is too late now.
> > +  if (mChannel) {
> > +    mChannel->Cancel(NS_BINDING_ABORTED);
> > +    NS_WARN_IF_FALSE(!mChannel, "displaying a XForms message without
testing link first\n");
> 
> Will Cancel() be handled synchroneously? If not, the test will always be false.

We get the NS_BINDING_ABORTED from Cancel() asynchronously. Yeah, I meant to
remove this 'warn if false' and forgot.  Thanks for the catch.

> 
> > +nsresult
> > +nsXFormsMessageElement::TestExternalFile()
> > +{
> > +  // Let's see if checking for any external resources is even necessary. 
Single
> > +  // node binding trumps linking attributes in order of precendence.  If we
> > +  // find single node binding in evidence, then return NS_OK to show that
> > +  // this message element has access to the info that it needs.
> > +  nsAutoString snb;
> > +  mElement->GetAttribute(NS_LITERAL_STRING("bind"), snb);
> > +  if (!snb.IsEmpty()) {
> > +    return NS_OK;
> > +  }
> > +  mElement->GetAttribute(NS_LITERAL_STRING("ref"), snb);
> > +  if (!snb.IsEmpty()) {
> > +    return NS_OK;
> > +  }
> > +
> > +  // if no external linking to check, we can also let the caller know that we
> > +  // haven't encountered any problems.
> 
> Weird comment, or at least at the wrong place? Should be at the return?

Poor wording to be sure.  Changed to, "if no linking attribute, no need to go on"

> 
> > +  nsAutoString src;
> > +  mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
> > +  if (src.IsEmpty()) {
> > +    return NS_OK;
> > +  }
> > +
> > +  nsCOMPtr<nsIDOMDocument> domDoc;
> > +  mElement->GetOwnerDocument(getter_AddRefs(domDoc));
> > +  nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));
> > +  NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
> 
> Use NS_ENSURE_STATE

I personally like NS_ENSURE_TRUE because it is immediately apparent what the
return code will be without thinking about it plus if I see a function returned
NS_ERROR_FAILURE to me, it is easy to open up the function and look for
NS_ERROR_FAILURE to see why it might have been returned.  But since
NS_ENSURE_STATE is the style already used in the file, I will change mine to
follow suit.

 
> > +    // Keep the the dialog from popping up.  Won't be able to reach the
> > +    // resource anyhow.
> > +    mStopType = eStopType_Security;
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  nsCOMPtr<nsILoadGroup> loadGroup = doc->GetDocumentLoadGroup();
> > +  NS_WARN_IF_FALSE(loadGroup, "No load group!");
> 
> Are you sure you just want to warn? Execution continues after the warning.
> NS_ASSERTION or NS_ENSURE_STATE?

though it is probably not a good sign that the loadgroup is null (and thus the
warning), from what I can tell by skimming the http channel code (and a couple
of other channels' code), it isn't essential for a channel to have a loadgroup


> > +    mStopType = eStopType_LinkError;
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  // channel should be running along smoothly, increment the count
> > +  AddRemoveExternalResource(PR_TRUE);
> > +  return NS_OK;
> > +}
> > +
> > +// nsIInterfaceRequestor
> 
> Why do we need this?

The channel's notification callbacks (set on the channel to be 'this' via
NS_NewChannel) needs to implement this interface. 

> 
> > +NS_IMETHODIMP
> > +nsXFormsMessageElement::OnStopRequest(nsIRequest *aRequest,
> > +                                      nsISupports *aContext,
> > +                                      nsresult aStatusCode)
> > +{
> > +
> > +  // We are done with the load request, so make sure that we null out mChannel
> > +  // before we return!
> > +
> > +  if (NS_FAILED(aStatusCode)) {
> > +    AddRemoveExternalResource(PR_FALSE);
> 
> Start by doing this?

Can't.  Since AddRemoveExternalResource may result in xforms-ready firing right
away and causing ::HandleAction to be called if there is a message element
acting as an xforms-ready event handler, then we need to make sure that the
mStopType member data is set to the correct value before calling
AddRemoveExternalResource.  I've changed the code to include a comment
explaining this and making sure that this is followed throughout the
OnStopRequest function.

> 
> > +  PRUint32 responseStatus;
> > +  nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);
> > +  if (httpChannel) {
> > +    nsresult rv = httpChannel->GetResponseStatus(&responseStatus);
> > +    
> > +    // If responseStatus is 4xx, it is an error.  2xx is success.  3xx (various
> > +    // flavors of redirection) COULD be successful.  Can't really follow those
> > +    // to conclusion so we'll assume they were successful.
> > +    if (NS_FAILED(rv) || (responseStatus / 100 == 4)) {
> 
> 5xx codes are also errors.

Dang, missed them.  Ok.  Fixed.

> 
> > +nsXFormsMessageElement::AddRemoveExternalResource(PRBool aAdd)
> > +{
> > +  // if this message doesn't have a channel established already or it has
> > +  // already returned, then no since bumping the counter.
> 
> s/since/sense in/?
> 
> > +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> > +  PRUint32 loadingMessages =
> > +    (PRUint32)doc->GetProperty(nsXFormsAtoms::externalMessagesProperty);
> 
> Use NS_STATIC_CAST

That gave me errors which is why I went the direct route.  Guess that I should
use NS_REINTERPRET_CAST.

> 
> > +  if(!loadingMessages) {
> > +    // no outstanding loads left, let the model in the document know in case
> > +    // the models are waiting to send out the xforms-ready event
> > +
> > +    nsCOMPtr<nsIModelElementPrivate> modelPriv =
> > +      nsXFormsUtils::GetModel(mElement);
> > +    nsCOMPtr<nsIDOMDocumentEvent> docEvent = do_QueryInterface(doc);
> > +    nsCOMPtr<nsIDOMEvent> event;
> > +    docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
> > +    event->InitEvent(NS_LITERAL_STRING(NS_XFORMS_EXTMSG_READY), PR_FALSE,
> > +                     PR_FALSE);
> > +  
> > +    nsCOMPtr<nsIDOMEventTarget> targetEl = do_QueryInterface(modelPriv);
> > +    if (targetEl) {
> > +      nsCOMPtr<nsIDOMNode> el = do_QueryInterface(targetEl);
> > +      nsXFormsUtils::SetEventTrusted(event, el);
> > +      PRBool defaultActionEnabled;
> > +
> > +      // if there are no more messages loading then it is probably the case
> > +      // that my mChannel is going to get nulled out as soon as this function
> > +      // returns.  If model is waiting for this event, then it may kick off
> > +      // the message right away and we should probably ensure that mChannel
> > +      // is gone before HandleAction is called.  So...if the channel isn't
> > +      // pending, let's null it out right here.
> > +      PRBool isPending = PR_TRUE;
> > +      mChannel->IsPending(&isPending);
> > +      if(!isPending) {
> > +        mChannel = nsnull;
> > +      }
> > +      targetEl->DispatchEvent(event, &defaultActionEnabled);
> > +    }
> 
> What's the reason for not just calling a method on the model?

I didn't think that it was worth adding another interface method just for
this especially when no one other than message will use it.

> 
> > Index: nsXFormsModelElement.cpp
> > ===================================================================
> > @@ -460,20 +462,52 @@ nsXFormsModelElement::HandleDefault(nsID
> >    } else if (type.EqualsASCII(sXFormsEventsEntries[eEvent_Revalidate].name)) {
> >      rv = Revalidate();
> >    } else if (type.EqualsASCII(sXFormsEventsEntries[eEvent_Recalculate].name)) {
> >      rv = Recalculate();
> >    } else if (type.EqualsASCII(sXFormsEventsEntries[eEvent_Rebuild].name)) {
> >      rv = Rebuild();
> >    } else if
(type.EqualsASCII(sXFormsEventsEntries[eEvent_ModelConstructDone].name)) {
> >      rv = ConstructDone();
> > +    mDone = PR_TRUE;
> >    } else if (type.EqualsASCII(sXFormsEventsEntries[eEvent_Ready].name)) {
> >      Ready();
> >    } else if (type.EqualsASCII(sXFormsEventsEntries[eEvent_Reset].name)) {
> >      Reset();
> > +  } else if(type.EqualsASCII(NS_XFORMS_EXTMSG_READY)) {
> > +    // currently no external messages remaining to finish test loading.  If
> 
> Cryptic comment. I know what you mean, but could you rephrase it?
> 
> > +    // we were waiting for this to send out xforms-ready, then now is the time.
> > +
> > +    nsCOMPtr<nsIDOMDocument> domDoc;
> > +    mElement->GetOwnerDocument(getter_AddRefs(domDoc));
> > +    const nsVoidArray *models = GetModelList(domDoc);
> > +    nsCOMPtr<nsIDocument>doc = do_QueryInterface(domDoc);
> > +    nsCOMArray<nsIXFormsControlBase> *deferredBindList =
> > +      NS_STATIC_CAST(nsCOMArray<nsIXFormsControlBase> *,
> > +                    
doc->GetProperty(nsXFormsAtoms::deferredBindListProperty));
> > +
> > +    // if this document hasn't process xforms-model-construct-done, yet (which
> > +    // must precede xforms-ready), or hasn't finished processing all of the
> > +    // deferred binds, yet, then we'll be able to send xforms-ready out as
> > +    // usual.  If we've already become ready, then this event was probably
> > +    // generated by a change in the src attribute on the message element.
> > +    // Ignore it in that case.
> 
> That's a lot of work before checking for the two booleans. Please check those
> before checking the deferredBindList.

done

> 
> >  nsXFormsModelElement::Ready()
> >  {
> >    BackupOrRestoreInstanceData(PR_FALSE);
> > +  mReady = PR_TRUE;
> 
> Why set this here, and mDone in HandleDefault()? I have no preference to where,
> just do it consistently.

I moved mReady out, then.  Didn't put mDone inside ConstructDone since I would
have had to put it between InitializeControls() and the NS_ENSURE_SUCCESS that
follows it which didn't look right.

Comment 22

[aaronr]

Attachment: fixed comments

Comment 23

[aaronr]

Attachment: regression testcase

testcase that may show a problem due to this patch.  I wonder if it might not
be because we are trying to run this dialog code before the document is loaded?

Comment 24

[aaronr]

(In reply to comment #23)
> Created an attachment (id=192047) [edit]
> regression testcase
> 
> testcase that may show a problem due to this patch.  I wonder if it might not
> be because we are trying to run this dialog code before the document is loaded?

testcase should show a dialog with another bugzilla testcase inside it. 
However, it never seems to stop loading.  Smaug, you have any idea what might be
going wrong?  I never see in the console where the chrome dialog that we are
trying to load is loading successfully.  When I tried to debug it, I see the
nsHttpChannel::AsyncOpen getting called during nsXULWindow::ShowModal stuff, but
I never see the nsHttpChannel::OnStopRequest.  Could it be because we are trying
to do show this dialog before the main xforms document is loaded?  Or because we
are doing in on a different thread than the main XForms thread?  I guess that I
could try to use a timer to get it back to executing on the xforms thread?

Comment 25

[Allan Beaufour]

(In reply to comment #21)
> (In reply to comment #18)
> > (From update of attachment 191646 [details] [diff] [review] [edit] [edit])
> > > Index: nsXFormsInstanceElement.cpp
> > > ===================================================================
> > 
> > >    if (!nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), newURI)) {
> > > -    nsXFormsUtils::ReportError(NS_LITERAL_STRING("instanceLoadOrigin"),
> domDoc);
> > > +    const PRUnichar *strings[] = { NS_LITERAL_STRING("instance").get() };
> > > +    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLinkLoadOrigin"),
> > > +                               strings, 1, domDoc, domDoc);
> > 
> > Why not mElement, mElement?
> 
> For this and the other similar comments mentioned above, all I was changing was
> the error message and the strings used to build them.  I didn't change anything
> else that was getting passed to ReportError.  But you are right, using domDoc as
> the context doesn't really fit now that smaug fixed using the contextNode
> problem that we had before with ReportError.  I've changed this and the other
> similar ReportErrors to use mElement instead of domDoc.

Ah, didn't think of that. Trying to get you to fix more than you should I guess,
sorry.
  
> > > Index: nsXFormsMessageElement.cpp
> > > ===================================================================
> > 
> > > +NS_IMETHODIMP
> > >  nsXFormsMessageElement::HandleAction(nsIDOMEvent* aEvent, 
> > >                                       nsIXFormsActionElement *aParentAction)
> > >  {
> > >    if (!mElement)
> > >      return NS_OK;
> > 
> > Shouldn't you check what type of event you handle here?
> 
> This is like the other action elements' HandleAction, not a control's typical
> HandleEvent.  A message doesn't care much what event caused it.  Just properties
> of the event like whether to allow handle default and propagation.

Ok, I can see that you just follow the previous code. I still think it's weird
to trigger on any random event received by the control though.
 
> > > +    // Keep the the dialog from popping up.  Won't be able to reach the
> > > +    // resource anyhow.
> > > +    mStopType = eStopType_Security;
> > > +    return NS_ERROR_FAILURE;
> > > +  }
> > > +
> > > +  nsCOMPtr<nsILoadGroup> loadGroup = doc->GetDocumentLoadGroup();
> > > +  NS_WARN_IF_FALSE(loadGroup, "No load group!");
> > 
> > Are you sure you just want to warn? Execution continues after the warning.
> > NS_ASSERTION or NS_ENSURE_STATE?
> 
> though it is probably not a good sign that the loadgroup is null (and thus the
> warning), from what I can tell by skimming the http channel code (and a couple
> of other channels' code), it isn't essential for a channel to have a loadgroup

Ah, didn't know that it wasn't essential, hence my comment.
 
> > > +  if(!loadingMessages) {
> > > +    // no outstanding loads left, let the model in the document know in case
> > > +    // the models are waiting to send out the xforms-ready event
> > > +
> > > +    nsCOMPtr<nsIModelElementPrivate> modelPriv =
> > > +      nsXFormsUtils::GetModel(mElement);
> > > +    nsCOMPtr<nsIDOMDocumentEvent> docEvent = do_QueryInterface(doc);
> > > +    nsCOMPtr<nsIDOMEvent> event;
> > > +    docEvent->CreateEvent(NS_LITERAL_STRING("Events"),
getter_AddRefs(event));
> > > +    event->InitEvent(NS_LITERAL_STRING(NS_XFORMS_EXTMSG_READY), PR_FALSE,
> > > +                     PR_FALSE);
> > > +  
> > > +    nsCOMPtr<nsIDOMEventTarget> targetEl = do_QueryInterface(modelPriv);
> > > +    if (targetEl) {
> > > +      nsCOMPtr<nsIDOMNode> el = do_QueryInterface(targetEl);
> > > +      nsXFormsUtils::SetEventTrusted(event, el);
> > > +      PRBool defaultActionEnabled;
> > > +
> > > +      // if there are no more messages loading then it is probably the case
> > > +      // that my mChannel is going to get nulled out as soon as this function
> > > +      // returns.  If model is waiting for this event, then it may kick off
> > > +      // the message right away and we should probably ensure that mChannel
> > > +      // is gone before HandleAction is called.  So...if the channel isn't
> > > +      // pending, let's null it out right here.
> > > +      PRBool isPending = PR_TRUE;
> > > +      mChannel->IsPending(&isPending);
> > > +      if(!isPending) {
> > > +        mChannel = nsnull;
> > > +      }
> > > +      targetEl->DispatchEvent(event, &defaultActionEnabled);
> > > +    }
> > 
> > What's the reason for not just calling a method on the model?
> 
> I didn't think that it was worth adding another interface method just for
> this especially when no one other than message will use it.

It's a lot more expensive to send and handle an event, especially when there a
1-1 correspondance between sender and receiver -- noone else than message and
model cares about the event, right? I would go for an interface method.

Comment 26

[Allan Beaufour]

(In reply to comment #20)
> (In reply to comment #19)
> > Could you please create a testcase that tests whether you handle the
> > xforms-ready business correctly btw.?
> 
> Like what kind of testcase?  The attached testcase has a message that has an
> invalid external link that fires on xforms-ready.  If you see an
> xforms-link-error message when this testcase loads, then that means that by the
> time that xforms-ready is processed by the event handler (and thus the message's
> ::HandleAction is called) the determination has been made that the link is no
good.

Ok. I might buy that, but it is far from evident, and is not explained in the
testcase.

Comment 27

[aaronr]

Attachment: corrected testcase

Comment 28

[aaronr]

Attachment: added xforms-ready test to testcase

Comment 29

[aaronr]

Attachment: updated to trunk

updated to trunk.  Changed from using event to calling a xforms model private interface function to let the model know that external message loads have finished.

Comment 30

[aaronr]

Attachment: fixed some comments

should have all of Allan's comments fixed.

Comment 31

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

Comment on attachment 204714 [details] [diff] [review]
fixed some comments


> class nsXFormsMessageElement : public nsXFormsXMLVisualStub,
>                                public nsIDOMEventListener,
>-                               public nsIXFormsActionModuleElement
>+                               public nsIXFormsActionModuleElement,
>+                               public nsIStreamListener,
>+                               public nsIInterfaceRequestor


I think we want to handle also redirects properly, 
same origin check is needed in OnChannelRedirect().
So nsXFormsMessageElement should extends 
nsIChannelEventSink, like nsXFormsInstanceElement does.

Comment 32

[aaronr]

(In reply to comment #31)
> (From update of attachment 204714 [details] [diff] [review] [edit])
> 
> > class nsXFormsMessageElement : public nsXFormsXMLVisualStub,
> >                                public nsIDOMEventListener,
> >-                               public nsIXFormsActionModuleElement
> >+                               public nsIXFormsActionModuleElement,
> >+                               public nsIStreamListener,
> >+                               public nsIInterfaceRequestor
> 
> 
> I think we want to handle also redirects properly, 
> same origin check is needed in OnChannelRedirect().
> So nsXFormsMessageElement should extends 
> nsIChannelEventSink, like nsXFormsInstanceElement does.
> 


good catch!  will fix this in next patch.

Comment 33

[aaronr]

Attachment: fixed OnRedirect comment

added OnRedirect handling per smaug's comment

Comment 34

[Allan Beaufour]

Comment on attachment 207781 [details] [diff] [review]
fixed OnRedirect comment

(does not apply cleanly)

> Index: nsIModelElementPrivate.idl
> ===================================================================
> +
> +  /**
> +   * Notification that all of the external message loads have finished loading.
> +   * Model contstruction cannot complete until all of the external messages

typo: construction

> Index: nsXFormsMessageElement.cpp
> ===================================================================
> +NS_IMETHODIMP
>  nsXFormsMessageElement::HandleAction(nsIDOMEvent* aEvent, 
>                                       nsIXFormsActionElement *aParentAction)
>  {
>    if (!mElement)
>      return NS_OK;
 
> +  // If TestExternalFile fails, then there is an external link that we need
> +  // to use that we can't reach right now.  If it won't load, then might as
> +  // well stop here.  We don't want to be popping up empty windows
> +  // or windows that will just end up showing 404 messages.
> +  if (mStopType == eStopType_LinkError) {
> +    // we couldn't successfully link to our external resource.  Better throw
> +    // the xforms-link-error event
> +    nsCOMPtr<nsIModelElementPrivate> modelPriv =
> +      nsXFormsUtils::GetModel(mElement);
> +    nsCOMPtr<nsIDOMNode> model = do_QueryInterface(modelPriv);
> +    nsXFormsUtils::DispatchEvent(model, eEvent_LinkError);
> +    return NS_OK;
> +  }

This means we send a link-error for each activation of the message does it not? Is that what we want?

And what if the resource is available later? As far as I can read we only try to load a ressource once, and if it fails we give up forever. I'm not sure I like that. It's an edge case, but still. Do file a follow-up bug on this at least. We can then discuss the problem there, or just agree that it's "INVALID".

> +nsXFormsMessageElement::OnStopRequest(nsIRequest *aRequest,
> +                                      nsISupports *aContext,
> +                                      nsresult aStatusCode)
> +{
> +
> +  // We are done with the load request, whatever the result, so make sure to
> +  // null out mChannel before we return.  Keep in mind that if this is the last
> +  // message channel to be loaded for the xforms document then when
> +  // AddRemoveExternalResource is called, it may result in xforms-ready firing.
> +  // Should there be a message acting as a handler for xforms-ready, it will
> +  // start the logic to display itself (HandleAction()).  So we can't call
> +  // AddRemoveExternalResource to remove this channel from the count until we've
> +  // set the mStopType to be the proper value.

And then in the following you do exactly that: Calling ARER() without setting mStopType. (I know it is set in AttributeSet(), but just reading along you get confused...

Moreover, I guess it's only important to set mStopType if something's wrong with the link? It should always be None when this is called, right? If not, it should be initialized here, because you only set it on errors.

> +
> +  if (NS_FAILED(aStatusCode)) {
> +    // If we received NS_BINDING_ABORTED, then we were cancelled by a later
> +    // AttributeSet() call.  Don't do anything and return.
> +    if (aStatusCode == NS_BINDING_ABORTED) {
> +      AddRemoveExternalResource(PR_FALSE);
> +      mChannel = nsnull;
> +    
> +      return NS_OK;
> +    }

How about killing the above, and doing this:

// NS_BINDING_ABORTED means that we have been cancelled by a later AttributeSet() call(which sets the mStopType accordingly)
if (aStatusCode != NS_BINDING_ABORTED) {
> +
> +    nsAutoString src, tagName;
> +    mElement->GetLocalName(tagName);
> +    mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
> +    const PRUnichar *strings[] = { tagName.get(), src.get() };
> +    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink2Error"),
> +                               strings, 2, mElement, mElement);
> +    mStopType = eStopType_LinkError;
}

> +    AddRemoveExternalResource(PR_FALSE);
> +    mChannel = nsnull;
> +  
> +    return NS_OK;
> +  }

> +++ nsXFormsModelElement.h	7 Jan 2006 00:06:40 -0000
> +
> +  /**
> +   * Indicates whether the model's handled the xforms-model-construct-done
> +   * event already
> +   */

skip "already", and is "model's" correct English (says the non-native English speaker...)

> +  PRBool mConstructDoneHandled;
> +
> +  /**
> +   * Indicates whether the model's handled the xforms-ready event already
> +   */

do.

With the above nits fixed, and a follow up bug for the external resource handling, r=me.

Comment 35

[aaronr]

Attachment: patch for checkin

This patch is updated and ready for checkin whenever the tree opens up again.  Handled beaufour's nits and opened a followup bug.

Comment 36

[aaronr]

checked into trunk

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This caused build breakage on 64-bit gcc. See bug 331412.