Closed Bug 884484 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Debugger, defect, P3)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: fitzgen, Assigned: b4bomsy)

Details

(Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js])

Attachments

(1 file, 4 obsolete files)

Instead, we should tell the user that loading that source failed.
We could use the yellow notification thing. "Keep trying" and "Discard source" could be useful options in this case.
Priority: -- → P3
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.
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
I'm a first timer, Can I work on this?
I'm a first timer, Can I work on this?
I'm a first timer, Can I work on this?
I'm a first timer, Can I work on this?
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! :))
Assignee: nobody → b4bomsy
Hi nick, thanks.
:) Cheers, newbie zeal..
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.
Hi, Nick.
Yeah...different timezones, i'm in london.

i'll be adding a patch later today.
Attachment #792554 - Flags: review?(nfitzgerald)
Attachment #792554 - Flags: feedback?(nfitzgerald)
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");
Attachment #792554 - Flags: review?(nfitzgerald)
Attachment #792554 - Flags: feedback?(nfitzgerald)
Attachment #792554 - Flags: feedback+
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 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.
Thanks guys.
i'll make the changes.
Nick, i'll rebase on top your patch
> 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
Attached patch name.patch (obsolete) — Splinter Review
Attachment #792554 - Attachment is obsolete: true
Attachment #798095 - Flags: review?(nfitzgerald)
Attachment #798095 - Flags: feedback?(nfitzgerald)
Added test, localized the error string
Attachment #798095 - Attachment is obsolete: true
Attachment #798095 - Flags: review?(nfitzgerald)
Attachment #798095 - Flags: feedback?(nfitzgerald)
Attachment #798202 - Flags: review?(nfitzgerald)
Attachment #798202 - Flags: feedback?(nfitzgerald)
Also localized the error string, added test
Attachment #798202 - Attachment is obsolete: true
Attachment #798202 - Flags: review?(nfitzgerald)
Attachment #798202 - Flags: feedback?(nfitzgerald)
Attachment #798220 - Flags: review?(nfitzgerald)
Attachment #798220 - Flags: feedback?(nfitzgerald)
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 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.
Attachment #798220 - Flags: review?(nfitzgerald)
Attachment #798220 - Flags: feedback?(nfitzgerald)
Attachment #798220 - Attachment is obsolete: true
Attachment #799825 - Flags: review?(nfitzgerald)
Attachment #799825 - Flags: feedback?(nfitzgerald)
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
Attachment #799825 - Flags: review?(nfitzgerald)
Attachment #799825 - Flags: review+
Attachment #799825 - Flags: feedback?(nfitzgerald)
Pushed to fx-team!

https://hg.mozilla.org/integration/fx-team/rev/fb3a60e0feda
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/fb3a60e0feda
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][fixed-in-fx-team] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.