126258 - Need error message for failures in file saving and publishing

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[sujay]

using 2/18 build of netscape

1) launch netscape
2) launch composer
3) new blank page
4) add some text
5) File | Publish 
6) enter invalid publish URL ( "dhkjhlsdhsd" )
7) Publish


nothing happens....expecting a dialog/panel to come up
telling you that the URL is invalid or something like this.

Comment 1

[Kathleen :Brade]

-->cmanske

Comment 2

[Charles Manske]

Yes, we don't have any error feedback yet. I think fixing bug 27609 will take 
care of most of the standard error conditions.

Comment 3

[Charles Manske]

Generalizing the summary to cover all error checking feedback.

Comment 4

[Charles Manske]

*** Bug 126243 has been marked as a duplicate of this bug. ***

Comment 5

[Bill Law]

We should see alerts for all errors occurring while reading data or writing to 
disk.  I'm not sure about the "publishing" aspect.  But the mechanism is in 
place now, I think.

If there is an issue in the publishing scenario, then this can be reopened (and 
assigned to somebody else :-).

Comment 6

[Charles Manske]

Since we use nsIWebBrowserPersist for publishing, notifying via the 
nsIWebProgressListener::alert's should work just fine!
Look forward to testing this.

Comment 7

[sujay]

still not working, here is an example:

1) launch netscape
2) launch composer
3) new blank page
4) enter some text
5) File | Publish
6) give it an invalid publish URL(for example, "lksjf" )
7) click OK on publish panel

nothing happens, I'm expecting an error panel to pop up telling me
that the URL is invalid

Comment 8

[sujay]

additional error checking:

check for page title

Comment 9

[Kathleen :Brade]

I don't understand "check for page title."
We are not planning to prompt the user for a page title (the presence of the
field should be sufficient).  We will output a title tag (unlike 4.x which only
wrote the title if there was text for it) so the document won't be invalid due
to that issue.

Comment 10

[sujay]

okay I understand the page title field not needing a title.

Comment 11

[sujay]

nominating EDITORBASE.....already marked nsbeta1+

Comment 12

[Charles Manske]

We pass through the error messages given to use via nsIPrompt::Alert notificaions.
I'm not sure if there's any other errors we should trap. 
The major problem in this area is bug 132320: we don't get error status when 
FTP login fails. We get an alert, but no status value indicating an error

Comment 13

[Charles Manske]

Attachment: Patch v1

@@ -852,6 +859,40 @@ : Added dump of aStatus values during onStateChange method

of the progress listener (copied from existin "onStatus" dumps). This is done
only if debug flag is set.

@@ -883,48 +924,70 @@ : Less change than it looks: Do processing of non-zero
aStatus value during any STATE_STOP notification instead of just when 
STATE_IS_NETWORK is also set. This is intended to abort publishing quicker when

there's an error instead of piling up repeated errors such as when trying to
publish > 1 images to the same bad directory.
Also adds specific error handling when aStatus indicates the "Directory or
file doesn't exist" error, which primarily happens when trying to FTP publish
to a directory that doesn't exist on the server.

@@ -1074,23 +1142,9 @@ and following blocks : Carefull cleanup of all the 
nsIPrompt an nsIAuthPrompt methods to clarify when params are "inout" (use 
"...Obj" notation to indicate that). Make sure appropriate methods are called
(e.g., UpdateUsernamePasswordFromPrompt() after prompting for a username and/or

password) after prompt dialog is called.

Comment 14

[robinf]

Charley, can you list the user-visible error message text strings? I'd like to
review them. Thanks.

Comment 15

[Charles Manske]

*** Bug 134713 has been marked as a duplicate of this bug. ***

Comment 16

[Charles Manske]

This bug is critical to Publishing feature

Comment 17

[andreww]

Comment on attachment 77000 [details] [diff] [review]
Patch v1

comments:
1) should the error codes be in a .properties file ?
2) why the need for a setTimeout for the cancelpublishing() call?
3) be sure to get a test build for qa to try out.
4) not sure why we need all those dump statements (aStatus)- arent they just
for debugging?

Comment 18

[Charles Manske]

Answers to your questions:
1) should the error codes be in a .properties file ?
They SHOULD be defined in an IDL file. But because of the way we generate them
from macros, that is very difficult. I talked to Bill Law about this and there's 
a plan to fix this problem in the future. I think putting them in a properties
file would only confuse I18N. They are numbers, not strings, anyway.
2) why the need for a setTimeout for the cancelpublishing() call?
I must return to the network code before calling it to stop the network 
activities by: gPersistObj.cancelSave() in CancelPublishing()
3) be sure to get a test build for qa to try out.
yes, of course! Doing that right now
4) not sure why we need all those dump statements (aStatus)- arent they just
for debugging?
Yes they are. There's a const global set so they show up only when we need them.
Before, they were only in onStatusChange. The place we really need them is in
onStateChange

Comment 19

[andreww]

Comment on attachment 77000 [details] [diff] [review]
Patch v1

ok r=andreww

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 77000 [details] [diff] [review]
Patch v1

For the next patch please use more context, makes reviewing a lot
easier. -9 or -10 is a good default choice for reviews; the default
-3 is not enough for any but the most most trivial of fixes.

>+        // XXX We don't have error codes in IDL !!!
>+        // Give better error messages for cases we know about:
>+        if (aStatus == 2152398868)  // Bad directory

Sure we do, look at the uses of const in nsIPrefBranch.idl, nsIDOMEvent.idl
and others. Then you point some var at the iface:

   var myclass = Components.interfaces.nsIMyClass;
   if (aStatus == myclass.BAD_DIRECTORY) ...

The code would be a lot more readable (at the cost of a slightly larger
.xpt file/memory use). Or you could even define the consts locally
in this .js file if that made more sense. Either way, 215298868 is
way too ugly for words.

Defining these in hex might be less error prone, by the way.
Fewer characters, easier to eyeball.

Comment 21

[Charles Manske]

Dan: I don't understand the relevance of your examples. Of course I know how to
define const values in an idl file. But the values I need are already defined in
.h files like this:
NS_ERROR_FILE_UNRECOGNIZED_PATH        
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 1)
NS_ERROR_FILE_UNRESOLVABLE_SYMLINK     
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 8)
NS_ERROR_FILE_INVALID_PATH             
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 9)
NS_ERROR_FILE_DISK_FULL                
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 10)
NS_ERROR_FILE_CORRUPTED                
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 11)
NS_ERROR_FILE_NOT_DIRECTORY            
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 12)
NS_ERROR_FILE_IS_DIRECTORY             
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 13)
NS_ERROR_FILE_IS_LOCKED                
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 14)
NS_ERROR_FILE_TOO_BIG                  
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 15)
NS_ERROR_FILE_NO_DEVICE_SPACE          
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 16)
NS_ERROR_FILE_NAME_TOO_LONG            
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 17)
NS_ERROR_FILE_NOT_FOUND                
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 18)
NS_ERROR_FILE_READ_ONLY                
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 19)
NS_ERROR_FILE_DIR_NOT_EMPTY            
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 20)
NS_ERROR_FILE_ACCESS_DENIED            
NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 21)

Are you suggesting that I map the current value to some JS const like this:
const long ACCESS_DENIED  = 2152398868;
?
If "NS_ERROR_MODULE_FILES" happens to change, then this value will be wrong.
While that is more readable in the JS, doesn't that just obscure the problem of
getting at the internal the error codes defined in macros?
I felt that by putting the "hard coded" value in JS and commenting on the 
problem would keep the basic problem more visible.
I agree that hex values would be more readable :-)
Bill Law: Care to comment?

Comment 22

[Daniel Veditz [:dveditz]]

> Are you suggesting that I map the current value to some JS const like this:
> const long ACCESS_DENIED  = 2152398868;
> ?
> If "NS_ERROR_MODULE_FILES" happens to change, then this value will be wrong.

w/out the "long" for js, yes. If the real NS_ERROR_MODULES_FILES "happens" to
change (which will break an unknown number of 3rd-party components) you're toast
whether you define a const or use the raw number; I vote for readability.

If those standard file errors are in fact the ones you're using then they may
already be available hanging off Components.results, see
http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpc.msg#131

But ok, sr=dveditz either way.

Comment 23

[Bill Law]

>Bill Law: Care to comment?

Yes.  The whole mechanism of using C++ macros to define error codes and not
being able to define mnemonics for them in JS is badly broken.  I'm not sure I
quite agree with the claim that "there's a plan to fix this problem in the
future."  We threw out some vague ideas in passing, but that wouldn't constitute
a "plan."

Nailing it down (and presuming no fundamental shift in the current mechanism),
what we need are to:

1. Make the "module IDs" available to JS somehow.
2. Move the low-order bits of the error code (the last argument on the
NS_ERROR_GENERATE_FAILURE macro) to idl constants.  I.e., they end up looking
like: NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, nsIFile::BAD_DIRECTORY);

That would permit JS code to re-constitute the actual values (the same way
NS_ERROR_GENERATE_FAILURE does).  Components.errors could assist, perhaps (maybe
by exposing the module id values).

Probably step 2. is sufficient in the vast majority of the cases, since the
module can be safely ignored.

Comment 24

[Charles Manske]

Note to ADT:
As discussed at the 4/2 ADT meeting, this bug fix isn't 100% completed, but we'd 
like to checkin what is there as an interim because the current patch it fixes 
basic problems in error prompt dialogs and the specific case when the directory 
we try to publish to doesn't exist. We need to get this part in so we can get more 
thorough testing started ASAP.

Comment 25

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for approval for checkin for the current patch. Pls
ask for approval, by changing adt1.0.0+ to a nomination adt1.0.0.

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 77000 [details] [diff] [review]
Patch v1

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 27

[Charles Manske]

Comment on attachment 77000 [details] [diff] [review]
Patch v1

This patch has been checked in. The issues fixed are better prompt dialog
handling and the specific case of publishing to a directory that does not
exist. You should see the aler dialog first, then after Ok there, the publish
progress dialog should show the message "xxxx/ doesn't exist at this site"

Comment 28

[Kathleen :Brade]

Attachment: proposed patch

I don't guarantee this fixes all of the issues in this bug but it does fix some
regressions I am seeing this week.

Comment 29

[Charles Manske]

Attachment: Patch v2

Exactly the same fix as brade, with a few addtional changes:
1. Turn off debug flags to show debug dump
2. Add dump of status message after checking for HTTP error
3. In publish progress: change default string to "PublishCompleted". It will
be changed to one of 2 error messages whenever a non-zero aStatus value is
passed into SetProgressFinished()

Comment 30

[Charles Manske]

Oops! This is the change to showdebug flags I mentioned:

@@ -822,9 +822,9 @@
   return promptService;
 }
 
-const gShowDebugOutputStateChange = true;
+const gShowDebugOutputStateChange = false;
 const gShowDebugOutputProgress = false;
-const gShowDebugOutputStatusChange = true;
+const gShowDebugOutputStatusChange = false;
 
 const gShowDebugOutputLocationChange = false;
 const gShowDebugOutputSecurityChange = false;

Comment 31

[Charles Manske]

Comment on attachment 77918 [details] [diff] [review]
Patch v2

previous patch is simpler and actually works fine. Be sure to add changes to
turn off debug flags.

Comment 32

[Charles Manske]

Comment on attachment 77916 [details] [diff] [review]
proposed patch

r=cmanske, with addition of turning off debug flags in ComposerCommands.js

Comment 33

[Kathleen :Brade]

cc kin for sr= of small patch

Comment 34

[kinmoz]

Comment on attachment 77916 [details] [diff] [review]
proposed patch

sr=[e-mail]

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 77916 [details] [diff] [review]
proposed patch

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 36

[Kathleen :Brade]

resolving this bug as fixed (checked in on both branch and trunk)

please verify this bug and all duplicates

Comment 37

[sujay]

still not fixed in 4/15 trunk build.

see original steps..

when I enter invalid URL and hit puiblish, nothing happens.
should get error/warning.

Comment 38

[michael wendell]

I am also still seeing the original problem, both on the 04-15 trunk, and the
04-13 1.0.0 branch build.

Comment 39

[Kathleen :Brade]

-->brade so it's on my radar and doesn't get lost
(my apologies to Michael and Sujay for not checking that case as well)

Comment 40

[Kathleen :Brade]

Attachment: patch to fix original problem described in bug

Display error message if publish fails.

I have tested these things:
 * successfully publish ftp html/images in subdirectory (success)
 * publish to http yahoo (failure)
 * publish to peoplestage bogus subdirectory (failure)

I am noticing that failure to publish (with this patch) is still causing the
location to reset when it shouldn't (as well as for document to be not dirty). 
Should I investigate that in this bug or can we put that issue in a new bug?

Comment 41

[Charles Manske]

Comment on attachment 79368 [details] [diff] [review]
patch to fix original problem described in bug

I don't think this patch is necessary. The error handling should be confined to
the Pulbish dialog "Validate()" method and in the status monitoring during
publishing and Publish Progress dialogs.

Comment 42

[Charles Manske]

Comment on attachment 79368 [details] [diff] [review]
patch to fix original problem described in bug

I don't think this patch is necessary. The error handling should be confined to
the Publish dialog "Validate()" method and in the status monitoring during
publishing and Publish Progress dialogs.

Comment 43

[Kathleen :Brade]

I disagree, we need to handle the fix in Publish function.  I'm testing a new
patch...

Comment 44

[Charles Manske]

*** Bug 136432 has been marked as a duplicate of this bug. ***

Comment 45

[Kathleen :Brade]

Attachment: better patch to fix original bug description

Comment 46

[Charles Manske]

Comment on attachment 79471 [details] [diff] [review]
better patch to fix original bug description

r=cmanske

Comment 47

[Kathleen :Brade]

kin, alecf: can one of you sr= the latest patch in this bug?  It's very small
(adding 3 lines of which two lines are just curly braces) :-)

Comment 48

[kinmoz]

Comment on attachment 79471 [details] [diff] [review]
better patch to fix original bug description

sr=[e-mail]

Comment 49

[sujay]

This is from bug 138157, we need to make sure we chekc this case
also:

"First time I tried I put
"fpt://..." instead of "ftp://.." in the settings. That caused mozilla to simply
do nothing - no warning or anything, just closed the dialog like nothing
happened."

Comment 50

[Kathleen :Brade]

This has been fixed on the trunk.

I'd like to land this tiny (and very safe) patch on the branch if it's not too late.

Comment 51

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this in to
the 1.0 branch today, then add the fixed1.0.0 keyword.

Comment 52

[sujay]

verified in 4/19 trunk.

Although I think we could have popped up a better dialog than "Publishing
Failed!" when the user enters an invalid URL.

Comment 53

[Kathleen :Brade]

checked into branch

Comment 54

[sujay]

verified in 4/23 branch build.