report source fetch failures to the web console

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Developer Tools
P3
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
In bug 1345533, I'm changing the source map service to report source map
errors to the user on the web console.
However, I think at least failures to fetch the original source will still
not be properly reported -- that requires another wrapper.
(Assignee)

Comment 1

2 months ago
This is going to need a small tweak in the debugger, because right now a missing source
causes an uncaught promise rejection, causing the new test to fail.
Comment hidden (mozreview-request)

Comment 3

2 months ago
mozreview-review
Comment on attachment 8893957 [details]
Bug 1387477 - report source map errors due to missing sources;

https://reviewboard.mozilla.org/r/165020/#review170700

Thanks for the patch Tom!

Have a look at my suggestion about the Proxy, and let me know if you want to discuss that.
You might have already agreed with debugger folks that this was the right approach, but I prefer to check.

::: devtools/client/framework/toolbox.js:572
(Diff revision 1)
> +                .catch(text => {
> +                  let message = L10N.getFormatStr("toolbox.sourceMapSourceFailure",
> +                                                  text, originalSource.url);
> +                  this.target.logErrorInPage(message, "source map");
> +                  // Also replace the result with the error text.
> +                  return {

It feel it would be better to let the consumer (here the debugger) deal with the failure in order to display whatever they want in their UI.

Here we swallow the exception and the debugger has no way to act upon this. What about adding the catch, but still returning the original promise?

> let p = target.getOriginalSourceText(originalSource);
> p.catch(/** catch code **/);
> return p;

Otherwise we need to add comments here to say that {text, contentType} should remain in sync with the return value structure of getOriginalSourceText on GitHub, and vice versa.

On a side node, maybe we should apply the same treatment to the other Proxy for getOriginalURLs, which is swallowing exceptions as well at the moment.

::: devtools/client/locales/en-US/toolbox.properties:188
(Diff revision 1)
>  # The text of the error: %1$S
>  # The URL that caused DevTools to try to fetch a source map: %2$S
>  # The URL of the source map itself: %3$S
>  toolbox.sourceMapFailure=Source map error: %1$S\nResource URL: %2$S\nSource Map URL: %3$S
> +
> +# LOCALIZATION NOTE (toolbox.sourceMapSoruceFailure): This is shown in

sourceMapSo[ru]ceFailure > sourceMapSo[ur]ceFailure

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sourcemap_nosource.js:40
(Diff revision 1)
> +  yield testOpenInDebugger(hud, toolbox, "here");
> +
> +  info("Selecting the console again");
> +  yield toolbox.selectTool("webconsole");
> +
> +  const node2 = yield waitFor(() => findMessage(hud, "original source"));

nit: there is no node1, node2 -> node?

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:208
(Diff revision 1)
>      });
>      observer.observe(node, observeConfig);
>    });
>  }
> +
> +function* testOpenInDebugger(hud, toolbox, text) {

Small JSDoc here, mainly to explain that text will be used to identify the message that is supposed to contain the debugger link.

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:209
(Diff revision 1)
>      observer.observe(node, observeConfig);
>    });
>  }
> +
> +function* testOpenInDebugger(hud, toolbox, text) {
> +  info(`Testing message with text "${text}"`);

Would be nice to have a general info statement that explains that this will assess clicking on a link that should open the debugger.
Attachment #8893957 - Flags: review?(jdescottes)
(Assignee)

Comment 4

2 months ago
mozreview-review-reply
Comment on attachment 8893957 [details]
Bug 1387477 - report source map errors due to missing sources;

https://reviewboard.mozilla.org/r/165020/#review170700

> It feel it would be better to let the consumer (here the debugger) deal with the failure in order to display whatever they want in their UI.
> 
> Here we swallow the exception and the debugger has no way to act upon this. What about adding the catch, but still returning the original promise?
> 
> > let p = target.getOriginalSourceText(originalSource);
> > p.catch(/** catch code **/);
> > return p;
> 
> Otherwise we need to add comments here to say that {text, contentType} should remain in sync with the return value structure of getOriginalSourceText on GitHub, and vice versa.
> 
> On a side node, maybe we should apply the same treatment to the other Proxy for getOriginalURLs, which is swallowing exceptions as well at the moment.

In this case, propagating the failure will cause the new test to fail, because the debugger doesn't check for this.  This doesn't matter in the debugger, because it doesn't have a test for failed source fetching, and because it doesn't have the mochitest behavior of automatic failure on an uncaught rejected promise.

I brought this up on #debugger on slack and Jason said:

> up to you. I don’t care about showing the error at the moment

So I went forward with this approach because it is simpler and because it puts all the source map error handling in one spot.

I will add some comments.

As for getOriginalURLs, it's always ok to swallow exceptions there, because a null result means no source map was found.
Comment hidden (mozreview-request)

Comment 6

2 months ago
mozreview-review
Comment on attachment 8893957 [details]
Bug 1387477 - report source map errors due to missing sources;

https://reviewboard.mozilla.org/r/165020/#review170778

Thanks for the update.
Attachment #8893957 - Flags: review?(jdescottes) → review+
(Assignee)

Comment 7

2 months ago
I think the test just needed to wait for the source map to be applied.
I'll upload the new version shortly.
Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95d5606107451c30adb31c9fa51eb5b3d72868f3
Comment hidden (mozreview-request)

Comment 9

2 months ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb95d04c5948
report source map errors due to missing sources; r=jdescottes

Comment 10

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eb95d04c5948
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1228496
You need to log in before you can comment on or make changes to this bug.