WebIDE new project wizard silently fails / gets stuck in endless loop if target directory not writable

RESOLVED FIXED in Firefox 41

Status

DevTools
WebIDE
RESOLVED FIXED
3 years ago
5 days ago

People

(Reporter: callahad, Assigned: waffles, Mentored)

Tracking

({DevAdvocacy})

Trunk
Firefox 41
DevAdvocacy

Firefox Tracking Flags

(firefox40 affected, firefox41 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Steps to Reproduce:

1. Open the WebIDE
2. Choose New App, HelloWorld Example, give it the name "hello"
3. Click "OK"
4. In the File Picker that opens, navigate to a folder that you do *not* have write access to, like /System on Mac OS X.
5. Click "OK" to select that folder

What Should Happen:

1. An error appears, informing me that the project directory could not be created.

What Actually Happens:

1. The directory picker disappears, and I'm back at the wizard dialog from Step 3.
(Reporter)

Comment 1

3 years ago
I don't see any helpful diagnostic messages when I launch Firefox from the terminal, either.

This happened in a real-world workshop when an attendee accidentally did a `sudo mkdir` rather than `mkdir` when setting up a parent for his FxOS projects.
Yes, let's add an error message for this.  Marking as a good first bug.
Mentor: jryans
Whiteboard: [good first bug][lang=js]
(Assignee)

Comment 3

3 years ago
Hey Ryan, I'd like to work on this bug. Can you get me started? BTW, I've done some 2 or 3 bugs. So, we don't have to worry about setting things up. You can get directly get me into the bug. :)
Flags: needinfo?(jryans)
Great!  The failing line is from newapp.js[1] when it tries to create the new folder.

Instead of just calling console.error, I think we should call the main WebIDE window's reportError facility, with something like:

window.parent.UI.reportError(...)

and then close the new app dialog.

Or, alternatively, we could make a space in the new app dialog for showing errors, so that we don't have to close the dialog and lose the selections the user was making.

Let me know if you have further questions!

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/newapp.js#135
Flags: needinfo?(jryans)
(Assignee)

Comment 5

3 years ago
Created attachment 8605204 [details] [diff] [review]
webIDE

I tried building Firefox replacing that line. But well, that dialog box "disappearance" can't be fixed. I mean, it doesn't show any effect even after rebuilding the browser. You can have a look at my patch.
Flags: needinfo?(jryans)
Comment on attachment 8605204 [details] [diff] [review]
webIDE

Review of attachment 8605204 [details] [diff] [review]:
-----------------------------------------------------------------

For the next patch, it's fine to use feedback? or review? on the patch instead of needinfo?.  Just as long as some flag gets set, so I don't forget to reply. :)

::: browser/devtools/webide/content/newapp.js
@@ +133,5 @@
>  
>    try {
>      folder.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
>    } catch(e) {
> +    window.parent.UI.reportError(e);

Ah, you are right, my mistake!

In this case, you can use:

let win = Services.wm.getMostRecentWindow("devtools:webide");

to get the main WebIDE window.

from there, to report the error:

win.UI.reportError(<SOME STRING ID>);

You'll need to add a new string in webide.properties for the error message.

It will appear behind the dialog.  We could either close the current dialog, or just let it be behind the open dialog.
Flags: needinfo?(jryans)
(Assignee)

Comment 7

3 years ago
Created attachment 8605356 [details] [diff] [review]
webIDE

Okay, that took care of the error. Now, a nice dropdown thing just shows up indicating an error. BTW, what do you think of my <error variable> and its string? (I admit, I'm not that good with naming stuff). And, I think it might be better to close the dialog, because it's possible that the user might miss the warning, as it occurs in the inactive window behind. Well, I can remove it if you want. :)
Attachment #8605204 - Attachment is obsolete: true
Attachment #8605356 - Flags: review?(jryans)
Comment on attachment 8605356 [details] [diff] [review]
webIDE

Review of attachment 8605356 [details] [diff] [review]:
-----------------------------------------------------------------

The changes look good to me!  But, something is wrong with your patch's header, it seems to include an error message or something:

message. HG: -- HG: user: Ravi Shankar <wafflespeanut@gmail.com> HG: branch 'default' HG: changed
browser/devtools/webide/content/newapp.js

If you do not yet have try access, I can submit your patch once there's a fixed version attached.  (You should consider applying for access!)
Attachment #8605356 - Flags: review?(jryans) → review+
(Assignee)

Comment 9

3 years ago
Created attachment 8606601 [details] [diff] [review]
webIDE

Yeah, I didn't notice it until now. I've cleared it now, and I've also pushed it to the try-server for testing it on all platforms (https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e478bc12e10). I've got a question now. Should this test be ran using xpcshell or mochitest? I wasn't sure, and so I left it.
Attachment #8605356 - Attachment is obsolete: true
Attachment #8606601 - Flags: review?(jryans)
Comment on attachment 8606601 [details] [diff] [review]
webIDE

Review of attachment 8606601 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, the cleaned up patch header looks good!

Typically for most front end DevTools work (like WebIDE), I'll use the following try syntax:

-b do -p linux64,macosx64,win32 -u mochitest-dt,mochitest-o,mochitest-e10s-devtools-chrome -t none
Attachment #8606601 - Flags: review?(jryans) → review+
(Assignee)

Comment 11

3 years ago
Oh, I didn't know that. I've pushed that to the try server now :)
(https://treeherder.mozilla.org/#/jobs?repo=try&revision=b077ec1d4860)
(Assignee)

Comment 12

3 years ago
The try-server builds are successful :)
Flags: needinfo?(jryans)
Great, let's land it! :)
Flags: needinfo?(jryans)
Keywords: checkin-needed
Assignee: nobody → wafflespeanut
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/d7d2abe89060
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(Assignee)

Comment 16

3 years ago
Awesome! Thanks for mentoring me Ryan :)
Flags: needinfo?(jryans)
(In reply to Ravi Shankar [:waffles] from comment #16)
> Awesome! Thanks for mentoring me Ryan :)

Sure, it was good working with you on this!  I know you've already looked at a few other bugs, so perhaps you've already found some others to work on, but feel free to attempt whatever sounds good.  You can also ask in #devtools for more specific advice.
Flags: needinfo?(jryans)
(Assignee)

Comment 18

3 years ago
Thanks again! And yeah, I've been working on a few bugs. Currently, I've equipped myself with 3 other bugs :)

Updated

5 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.