249405 - NS_ERROR_FAILURE when cancelling the POSTDATA warning

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Jens Bannmann]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/20040628
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/20040628

When trying to reload a frame that is the response to a POST form, a warning
appears. If you click 'Cancel', an uncaught exception is shown in the JS
console. Note that this is not the case with the regular 'reload page' function.

Reproducible: Always
Steps to Reproduce:
1. Load up the attached test case
2. Submit the POST form
3. Choose context menu, this frame, reload frame
4. In the POSTDATA warning alert, click 'Cancel'

Actual Results:  
In the JS console, the following error appears:
---
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSLocation.reload]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://communicator/content/nsContextMenu.js :: anonymous :: line 517"  data: no]
---

Expected Results:  
No error should appear.

Comment 1

[Jens Bannmann]

Attachment: Test case (frameset with a POST form)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153229 [details] [diff] [review]
Proposed patch

darin, what do you think?  biesi was sort of in favor of creating an
nsDocShellErrors.h or something and having this return a NS_USER_ABORTED
success code instead of NS_OK, but that sounds like a lot of complication...

Comment 4

[Boris Zbarsky [:bzbarsky]]

No form submission involved (this stuff happens way before anything is submitted).

Thanks for the nice testcase, by the way!

Comment 5

[Darin Fisher]

Comment on attachment 153229 [details] [diff] [review]
Proposed patch

so, NS_BINDING_ABORTED is usually used to indicate a user aborted action.  i
realize that the idea here is to avoid the JS exception, but might some callers
not be interested in an actual meaningful return value?  returning NS_OK seems
like a lie, or am i missing something?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Like I said, I could create a success code (and I feel it should be a success
code) to indicate this condition.  User cancellation here is not an exceptional
circumstance (it's the common case) and hence should not trigger an error code.

If I do create such a code, where should it go?  Necko errors?  Or create an
nsDocshellErrors?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

you could put it in the .idl for this method

Comment 8

[Boris Zbarsky [:bzbarsky]]

I supposed I could, in a {%C++ section... darin?  Thoughts?

Comment 9

[Darin Fisher]

(In reply to comment #6)
> Like I said, I could create a success code (and I feel it should be a success
> code) to indicate this condition.  User cancellation here is not an exceptional
> circumstance (it's the common case) and hence should not trigger an error code.

I'm confused... didn't the caller ask the docshell to load an URL (reload,
repost, or whatever)?  and didn't the load fail?  isn't that what error codes
convey -- failure to do the primary function?  NS_BINDING_ABORTED makes sense to
me.  perhaps the caller should handle the exception in a better way.  i think we
are looking at this problem in two different ways.  from the point of view of
the user, it is not an error to have aborted the load.  but from the point of
view of a programming calling a docshell API, it seems like it should indeed be
an error.  what to do.. what to do.. can we fix the caller somehow to suppress
reporting the exception in the JS console?  like if the caller is JS code, then
can it not catch the exception?

Comment 10

[Boris Zbarsky [:bzbarsky]]

The caller is content JS.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Er, actually in this case it's chrome JS.  But it could just as easily be
content JS -- this is a public API....

The call is not direct to docshell but goes through nsLocation, and I suppose I
could suppress the error there.  Would that be preferable?

Comment 12

[Jens Bannmann]

Actally, catching the exception is what I do at the moment to work around this
bug when a frame-reload is triggered by Mouse Gestures. However, I'd not
recommend this as a permanent solution: it would silently catch *all* exceptions
(at least the way I did it), including potentially interesting or severe ones...

Comment 13

[Darin Fisher]

In an ideal world (IMO)...

The caller should be aware of the fact that NS_BINDING_ABORTED corresponds to
the user aborting the load.  If it sees this exception then it can choose to
suppress error reporting to the user.  Seems wrong to suppress error reporting
to the user at the docshell level since it doesn't know the context in which it
is being called.

Perhaps nsLocation should catch this exception for the purposes of hiding it
from content JS.  As for chrome JS, if it talks directly to the docshell, then
it should get full access to the error reporting that docshell can provide.  We
should document NS_BINDING_ABORTED as a possible error condition corresponding
to user intervention.  (That's already sort of documented in nsNetError.h: "The
async request failed because it was aborted by some user action.")

Am I just making more work for us?  Can someone make the case that there is
little point in going this route?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Darin, that's pretty much what I was thinking of -- catching NS_BINDING_ABORTED
min the location impl.

Jens, you could rethrow your exception if it's not the one you're specifically
trying to catch, you know...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch along those lines

Comment 16

[Darin Fisher]

Comment on attachment 154662 [details] [diff] [review]
Patch along those lines

>Index: docshell/base/nsIWebNavigation.idl

>+  * @throws NS_BINDING_ABORTED if the page is the result of a POST and the user
>+  *         cancels the reload.
>   */
>   void reload(in unsigned long reloadFlags);

Are you sure you want to be so specific in that comment?  It seems
to me that there could be other ways that a user might abort the
reload before it has a chance to start... even if we haven't 
invented any other such ways.


>Index: docshell/base/nsWebShell.cpp

>+            /* If the user pressed cancel in the dialog, return. Don't
>+             * try to load the page with out the post data.
>              */
>             if (!repost)
>               return NS_OK;

should this say "return NS_BINDING_ABORTED;" instead?


Otherwise, the patch looks good to me.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Are you sure you want to be so specific in that comment? 

I can make it just refer to the user cancelling, sure.

> should this say "return NS_BINDING_ABORTED;" instead?

I don't think so.  This is EndPageLoad().  The end of the page load was
successful, in fact.  We offered a user to try a different page load instead,
and the user said no.  That doesn't mean EndPageLoad() should throw...

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 154662 [details] [diff] [review]
Patch along those lines

sr=jst

Comment 19

[Darin Fisher]

Comment on attachment 154662 [details] [diff] [review]
Patch along those lines

r=darin

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checked in with that change.