884484 - Don't show "Loading..." forever if loading a source fails

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Instead, we should tell the user that loading that source failed.

Comment 1

[Victor Porof [:vporof][:vp]]

We could use the yellow notification thing. "Keep trying" and "Discard source" could be useful options in this case.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

As a first pass, we could just set the editor text to "Error loading source: " + error + "\n" + error.stack

This would improve our lives a bit, because then people filing bugs (one example among many is bug 905213) would be able to give us more info to work with, and we wouldn't have to tell them to go load the browser console, and they would know there was a bug, rather than things being REALLY slow.

Would be a pretty simple change here:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-view.js#282

See a couple lines above that for how to set the source editor's text content.

Comment 3

[Hubert B Manilla]

I'm a first timer, Can I work on this?

Comment 4

[Hubert B Manilla]

I'm a first timer, Can I work on this?

Comment 5

[Hubert B Manilla]

I'm a first timer, Can I work on this?

Comment 6

[Hubert B Manilla]

I'm a first timer, Can I work on this?

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hey Hubert! Yes, you may work on it. Check out our get involved page for instructions on how to get up and running, and comment 2 for details about this specific bug. Join us in irc and say hello :)

https://wiki.mozilla.org/DevTools/GetInvolved

(PS: careful with clicking that submit button multiple times! :))

Comment 8

[Hubert B Manilla]

Hi nick, thanks.
:) Cheers, newbie zeal..

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hubert, I've seen you ping me in irc a couple times now, but when I get back to you, you seem to have left the channel. We must be in different time zones :)

If you have any questions and I'm not around, feel free to ask in the channel for general help, and if that fails, comment here so I can answer when I get back online.

Comment 10

[Hubert B Manilla]

Hi, Nick.
Yeah...different timezones, i'm in london.

i'll be adding a patch later today.

Comment 11

[Hubert B Manilla]

Attachment: Improves the error output for the debbugger

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 792554 [details] [diff] [review]
Improves the error output for the debbugger

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

Looks good! Fix up the things I mention below, and then I will push to try server to test the patch on all of our platforms. Assuming all goes well, I'll land it in fx-team, which will then merge to mozilla-central after a few hours (up to a day), and then it will be in the next Nightly!

::: browser/devtools/debugger/debugger-view.js
@@ +278,5 @@
>        // Notify that we've shown a source file.
>        window.dispatchEvent(document, "Debugger:SourceShown", aSource);
>      },
>      ([, aError]) => {
>        // Rejected. TODO: Bug 884484.

Please remove this TODO, since we are fixing it now :)

@@ +279,5 @@
>        window.dispatchEvent(document, "Debugger:SourceShown", aSource);
>      },
>      ([, aError]) => {
>        // Rejected. TODO: Bug 884484.
> +      let msg = "Error loading source: " + aError + "\n" + aError.stack;

I should have mentioned this before, but we should use

  DevToolsUtils.safeErrorString(aError)

instead of just coercing it with the + operator. If you want to rebase on top of my patch in bug 906795, you won't need to include the stack anymore either. If you don't want to, I can rebase myself and remove the stack here in that patch.

I don't think DevToolsUtils is used in this jsm yet, so you may have to import it with:

  Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Also, please add a small little test for this change. You will need to add a test file "browser/devtools/debugger/tests/browser_dbg_script-switching-03.js", and set |DebuggerView.editorSource| with a bogus source (say { url: "http://example.com/fake.js", actor: "fake.actor" }), and then verify that the editor displays the error message like we expect it to.

You'll need to add your new file here as well: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/Makefile.in#76

See also, https://wiki.mozilla.org/DevTools/Hacking#Browser_Mochitests

Comment 14

[Victor Porof [:vporof][:vp]]

Comment on attachment 792554 [details] [diff] [review]
Improves the error output for the debbugger

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

::: browser/devtools/debugger/debugger-view.js
@@ +279,5 @@
>        window.dispatchEvent(document, "Debugger:SourceShown", aSource);
>      },
>      ([, aError]) => {
>        // Rejected. TODO: Bug 884484.
> +      let msg = "Error loading source: " + aError + "\n" + aError.stack;

You'll also want to localize this string.

Comment 15

[Hubert B Manilla]

Thanks guys.
i'll make the changes.
Nick, i'll rebase on top your patch

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

> You'll also want to localize this string.

And you get localized strings via |L10N.getStr|, and define them in http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/debugger.properties

Comment 17

[Hubert B Manilla]

Attachment: name.patch

Comment 18

[Hubert B Manilla]

Attachment: Changes to improve error output for source loading

Added test, localized the error string

Comment 19

[Hubert B Manilla]

Attachment: Changes to improve error output for the debugger when source loading fails

Also localized the error string, added test

Comment 20

[Hubert B Manilla]

hi guys,
Sorry for the number of patch update attachments. i noticed i had left out the test file i created.
But its all sorted in this last patch.
Thanks

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 798220 [details] [diff] [review]
Changes to improve error output for the debugger when source loading fails

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

Thanks for the path Hubert! This is really close, but there are just a couple things we need to fix before this can land, which I mention inline below.

::: browser/devtools/debugger/debugger-view.js
@@ +282,5 @@
>      },
>      ([, aError]) => {
> +      // Rejected.
> +      let msg = L10N.getStr("errorLoadingText") + DevToolsUtils.safeErrorString(aError);
> +      this.editor.setText(msg);

I think we should dispatch a new event Debugger:SourceErrorShown here that the test can hook into.

::: browser/devtools/debugger/test/browser_dbg_scripts-switching-03.js
@@ +15,5 @@
> +var gL10N = null;
> +
> +function test(){
> +
> +	debug_tab_pane(TAB_URL, function(aTab, aDebuggee, aPane){

Small style nit: please use 2 space indents in this file (not tabs) like the other files do.

@@ +31,5 @@
> +
> +function onScriptShown(aEvent){
> +	gView.editorSource = { url: "http://example.com/fake.js", actor: "fake.actor" };
> +	gDebugger.removeEventListener("Debugger:SourceShown", onScriptShown);
> +	executeSoon(testDebuggerLoadingError);

Using executeSoon here is kinda hacky, and will most likely lead to new intermittent errors when RDP requests take longer than normal. Instead, we should just create an event we can listen for and know that the error has been loaded in the editor when the event is fired, as I mention above.

@@ +38,5 @@
> +function testDebuggerLoadingError(){
> +
> +	ok(gDebugger.editor.getText().search(new RegExp(gL10N.getStr("errorLoadingText")) != -1), 
> +		"The valid error loading message is displayed.");
> +   			

Small style nit: please remove the trailing whitespace on this line, and the one a bit above. In emacs you can highlight trailing whitespace with

  (setq-default show-trailing-whitespace t)

Most other editors have config options to do the same.

Comment 22

[Hubert B Manilla]

Attachment: Changes to improve error output for the debugger when source loading fails: updates based of latest feedback

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 799825 [details] [diff] [review]
Changes to improve error output for the debugger when source loading fails: updates based of latest feedback

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

Thanks a bunch! This looks great :) Assuming the try push comes back positive, I'll land this in fx-team.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c3c951c41cdd

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Pushed to fx-team!

https://hg.mozilla.org/integration/fx-team/rev/fb3a60e0feda

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb3a60e0feda