Last Comment Bug 310125 - failing to generate xforms-submit-error
: failing to generate xforms-submit-error
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: Allan Beaufour
: Stephen Pride
Mentors:
Depends on:
Blocks: 278448 326372 326373
  Show dependency treegraph
 
Reported: 2005-09-26 18:35 PDT by aaronr
Modified: 2016-07-15 14:46 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase (1.04 KB, application/xhtml+xml)
2005-09-27 09:44 PDT, aaronr
no flags Details
less confusing testcase (1.04 KB, application/xhtml+xml)
2005-09-27 09:48 PDT, aaronr
no flags Details
another try at the testcase (1.04 KB, application/xhtml+xml)
2005-09-27 09:50 PDT, aaronr
no flags Details
le patch (1.24 KB, patch)
2005-09-27 10:25 PDT, Doron Rosenberg (IBM)
aaronr: review-
Details | Diff | Splinter Review
Patch (3.04 KB, patch)
2006-05-29 04:53 PDT, Allan Beaufour
bugs: review+
doronr: review+
Details | Diff | Splinter Review

Description aaronr 2005-09-26 18:35:03 PDT
In the spec, at: http://www.w3.org/TR/xforms/index-all.html#submit-event under
point #5, it says, "For an error response nothing in the document is replaced,
and submit processing concludes after dispatching xforms-submit-error".

However, we don't seem to be doing that.  If I submit to
http://www.xformstest.org/cgi-bin/echo.shakakahaka, for example, I get a 404
message back.  We display the error page, but I don't get the
xforms-submit-error to popup.  None of the other processors display the popup,
either.  

It seems to me that in ::OnStopRequest we should be checking to see if the
channel is a HttpChannel and if it is, then we should be checking
channel->GetResponseStatus to see if we got a 4xx or not.  Not just looking at
the value of status coming in.
Comment 1 aaronr 2005-09-27 09:44:02 PDT
Created attachment 197577 [details]
testcase

testcase that shows the problem
Comment 2 aaronr 2005-09-27 09:48:14 PDT
Created attachment 197579 [details]
less confusing testcase

Here is a less confusing testcase.  The other obsolete testcase WOULD show the
xforms-submit-error but only if you didn't have bugzilla in your trusted list
for cross domain submission.  This testcase should show the anticipated
failure.
Comment 3 aaronr 2005-09-27 09:50:50 PDT
Created attachment 197580 [details]
another try at the testcase
Comment 4 Doron Rosenberg (IBM) 2005-09-27 10:25:31 PDT
Created attachment 197582 [details] [diff] [review]
le patch
Comment 5 Doron Rosenberg (IBM) 2005-09-27 11:26:59 PDT
Submitting to a 404 sounds like a good error case.  Ask the WG? :)
Comment 6 aaronr 2005-09-27 12:19:59 PDT
Comment on attachment 197582 [details] [diff] [review]
le patch

>Index: extensions/xforms/nsXFormsSubmissionElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSubmissionElement.cpp,v
>retrieving revision 1.40
>diff -u -1 -0 -r1.40 nsXFormsSubmissionElement.cpp
>--- extensions/xforms/nsXFormsSubmissionElement.cpp	15 Sep 2005 11:37:37 -0000	1.40
>+++ extensions/xforms/nsXFormsSubmissionElement.cpp	27 Sep 2005 17:25:04 -0000
>@@ -408,20 +408,31 @@
>   LOG(("xforms submission complete [status=%x]\n", status));
> 
>   if (!mElement) {
>     return NS_OK;
>   }
> 
>   nsCOMPtr<nsIChannel> channel = do_QueryInterface(request);
>   NS_ASSERTION(channel, "request should be a channel");
> 
>   PRBool succeeded = NS_SUCCEEDED(status);
>+
>+  // Check if the return was a 4xx/5xx, since status will be set to true
>+  // if a 404 (for example) with a message body was returend.
>+  if (succeeded) {
>+    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel);
>+
>+    if (httpChannel) {
>+      httpChannel->GetRequestSucceeded(&succeeded);
>+    }
>+  }
>+

I would say that GetRequestSucceeded isn't the right test, since it will return
false for anything that isn't a 2xx code.  We are supposed to special case "an
error response", not a 'non-success' response.	1xx and 3xx responses aren't
errors.  I would say that the test should be to check for 4xx and 5xx
responses.
Comment 7 aaronr 2005-09-27 12:38:34 PDT
Allan, I told Kevin Kelly about this, but could you follow up on the next XForms
call?  We really need to know what the proper handling should be.  In the case
of a 4xx or 5xx http response, should we show any error page coming back from
the server AND send the xforms-submit-error?  Or just dispatch the
xforms-submit-error event?  Currently, Novell, fP and XSmiles all just show the
response from the server and do NOT dispatch the error.
Comment 8 Allan Beaufour 2006-04-03 08:40:02 PDT
(In reply to comment #7)
> Allan, I told Kevin Kelly about this, but could you follow up on the next
> XForms
> call?  We really need to know what the proper handling should be.  In the case
> of a 4xx or 5xx http response, should we show any error page coming back from
> the server AND send the xforms-submit-error?  Or just dispatch the
> xforms-submit-error event?  Currently, Novell, fP and XSmiles all just show the
> response from the server and do NOT dispatch the error.

Re-reading this, and I must say that the spec. clearly says that on an "error response" nothing is done. By all means, a 404 is an error response. My interpretation is that, we should dispatch the error and do nothing else. I'll post to www-forms and ask though
Comment 10 Allan Beaufour 2006-05-26 03:04:13 PDT
(In reply to comment #7)
> Allan, I told Kevin Kelly about this, but could you follow up on the next
> XForms
> call?  We really need to know what the proper handling should be.  In the case
> of a 4xx or 5xx http response, should we show any error page coming back from
> the server AND send the xforms-submit-error?  Or just dispatch the
> xforms-submit-error event?  Currently, Novell, fP and XSmiles all just show the
> response from the server and do NOT dispatch the error.

Not a lot of direct replies, but there seems to be agreement on that that we should throw the error on 4xx and 5xx for 1.0. Needs to be "enhanced", but that's post 1.0 stuff.
Comment 11 Allan Beaufour 2006-05-29 04:53:53 PDT
Created attachment 223685 [details] [diff] [review]
Patch
Comment 12 Allan Beaufour 2006-05-30 07:57:07 PDT
Fixed on trunk

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