714983 - avoid exceptions in file-utils.js when dismissing file picker dialog

Status: VERIFIED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[:aceman]

When managing feeds subscription, there is Import and Export button. Both of them open a file picker dialog. When that dialog is canceled, there is an exception thrown. I think that is not useful as it is a normal operation so it should be handled gracefully.

The exceptions are:
Error: uncaught exception: [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIFilePicker.file]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: chrome://messenger-newsblog/content/file-utils.js :: pickOpen :: line 207"  data: no]

Error: uncaught exception: [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIFilePicker.file]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: chrome://messenger-newsblog/content/file-utils.js :: pickSaveAs :: line 189"  data: no]

Comment 1

[:aceman]

Attachment: proposed patch

I propose this change. In case the dialog is dismissed it seems picker.file is undefined and throws exception. So do not touch that field and do not return it. The parent calling functions properly check rv.reason and do not touch rv.file if the dialog was cancelled. I also do not see why .picker is returned in case of success, I do not see any usage of it. But I didn't change that for now.
If it is better to return all 3 members in all cases, picker.file can be replaced by null in case of Cancel.

Comment 2

[Ian Neal]

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


>+
>+    if (rv != PICK_CANCEL) {
>         futils.lastSaveAsDir = picker.file.parent;
>+        return {reason: rv, file: picker.file, picker: picker};
>+    } else {
>+        return {reason: rv};
>+    }
> }
>+
>+    if (rv != PICK_CANCEL) {
>         futils.lastOpenDir = picker.file.parent;
>+        return {reason: rv, file: picker.file, picker: picker};
>+    } else {
>+        return {reason: rv};
>+    }
> }
Just as a drive by, in both of the above you don't need to put the second return in the else statement.

Comment 3

[:aceman]

Yes, I should have already learned this :)

Comment 4

[:aceman]

Attachment: patch v2

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 585819 [details] [diff] [review]
patch v2

Looks good, works as expected, r=myk!

Comment 6

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/70fa39beb291