failing to generate xforms-submit-error

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
11 months ago

People

(Reporter: aaronr, Assigned: Allan Beaufour)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
x86
All
fixed1.8.0.5, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

1.04 KB, application/xhtml+xml
Details
3.04 KB, patch
smaug
: review+
Doron Rosenberg (IBM)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 197577 [details]
testcase

testcase that shows the problem
(Reporter)

Comment 2

12 years ago
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.
Attachment #197577 - Attachment is obsolete: true
(Reporter)

Comment 3

12 years ago
Created attachment 197580 [details]
another try at the testcase
Attachment #197579 - Attachment is obsolete: true

Comment 4

12 years ago
Created attachment 197582 [details] [diff] [review]
le patch

Updated

12 years ago
Assignee: aaronr → doronr
Status: NEW → ASSIGNED
Attachment #197582 - Flags: review?(aaronr)

Comment 5

12 years ago
Submitting to a 404 sounds like a good error case.  Ask the WG? :)
(Reporter)

Comment 6

12 years ago
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.
Attachment #197582 - Flags: review?(aaronr) → review-
(Reporter)

Comment 7

12 years ago
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.
(Reporter)

Updated

12 years ago
Blocks: 278448
(Reporter)

Updated

12 years ago
Blocks: 326372
(Reporter)

Updated

12 years ago
Blocks: 326373
(Assignee)

Comment 8

11 years ago
(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
(Assignee)

Comment 9

11 years ago
http://lists.w3.org/Archives/Public/www-forms/2006Apr/0009.html
(Assignee)

Comment 10

11 years ago
(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.
(Assignee)

Comment 11

11 years ago
Created attachment 223685 [details] [diff] [review]
Patch
Assignee: doronr → allan
Attachment #197582 - Attachment is obsolete: true
Attachment #223685 - Flags: review?(Olli.Pettay)

Updated

11 years ago
Attachment #223685 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

11 years ago
Attachment #223685 - Flags: review?(doronr)

Updated

11 years ago
Attachment #223685 - Flags: review?(doronr) → review+
(Assignee)

Comment 12

11 years ago
Fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.5
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1
(Assignee)

Updated

11 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.