Closed Bug 1154606 Opened 9 years ago Closed 9 years ago

Should have a better error message when source map does not exist

Categories

(DevTools :: Debugger, defect, P2)

37 Branch
x86
macOS
defect

Tracking

(firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: jsantell, Assigned: jlong)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(4 files, 1 obsolete file)

When loading source maps from this page, I get an error that's displayed in codemirror. If the source map is invalid, then the original source should be shown.

https://rollbar.com/docs/source-maps/
Fx39 and Fx40
This site as well, some of the source maps here have the same error: http://lisperator.net/uglifyjs/
try utils.js, parse.js
STR:

* Go to https://rollbar.com/docs/source-maps/
* Open debugger
* Select "jquery.autosize.js" in the sources pane

ER:

Loads the source contents

AR:

Error loading source, following in the browser console:

> Got an exception during SA_onSource: "https://d37gvrvc0wt4s1.cloudfront.net/static/js/jquery.autosize.js" is not in the SourceMap.:
> SourceMapConsumer_sourceContentFor@resource://gre/modules/devtools/SourceMap.jsm:622:15
> SourceActor.prototype._getSourceText/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:2408:24
> Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
> this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
> this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
> Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:5
> this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:709:7
> Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:417:5
> SourceActor.prototype._getSourceText@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:2406:12
> resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
> then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
> then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
> SourceActor.prototype.onSource@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:2506:12
> DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1459:15
> LocalDebuggerTransport.prototype.send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:561:11
> makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
> makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
Whiteboard: [devedition-40][difficulty=medium]
Priority: -- → P2
I don't see "jquery.autosize.js" in the sources listing anymore and I can't reproduce. Can you all check to see if it's there for you? I have sourcemaps enabled.

We do show the raw source if the sourcemap is invalid, but this looks like there might be another check we need to do about validitiy
James, try this site: http://lisperator.net/uglifyjs/ Almost everything in sources is a source map I believe
Thanks I see it, looking into it now
Ah interesting, the URL in the sourcemap points to file:///home/mishoo/work/my/uglifyjs2/lib/parse.js.

This might be really hard. Right now we make a sourcemapped SourceActor when it has a sourcemap and it was correctly fetched and parsed. But here we also want to check if the URL of the source in the sourcemap can be downloaded.

Obviously we don't want to do that when the SourceActor is created (eagerly download contents). So we want to... "unsourcemap" a source actor if you click on it and it can't get the contents? That sounds super hard. "load source error" here honestly is correct.

I hate sourcemaps.
I think we just need a better error message. Like "couldn't download the contents of this URL: ...".

Can we change the bug to be that? If you can get me a different test case of something that should show up, I can look at that.
Assignee: nobody → jlong
Attached patch 1154606.patch (obsolete) — Splinter Review
What do you think about this for now? This should clear up a lot of confusion when it happens. I think most of the time it's a valid error, and showing the URL to the user that we tried to fetch will really help show that.
Attachment #8603525 - Flags: review?(jsantell)
Comment on attachment 8603525 [details] [diff] [review]
1154606.patch

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

Ah didnt know these were pointing to file:/// URIs. Not much we can do in that case. Chrome just shows an empty text editor, so atleast this shows what the problem is (files that do not exist). With the localization change, I think this is a solid fix!

::: browser/locales/en-US/chrome/browser/devtools/debugger.properties
@@ +215,5 @@
>  loadingText=Loading\u2026
>  
>  # LOCALIZATION NOTE (errorLoadingText): The text that is displayed in the debugger
>  # viewer when there is an error loading a file
> +errorLoadingText=Error loading this URL:

L10N rules require updating the key -- so this needs to be changed to `errorLoadingText2` unfortunately (and where it's consumed in debugger-view.js as well).

Can also use `L10N.getFormatStr("errorLoadingText2", url)` and change the localization to "Error loading this URL: %S"
Attachment #8603525 - Flags: review?(jsantell) → review+
Summary: Error loading source: loadSourceError → Should have a better error message when source map does not exist
Attached patch 1154606.patchSplinter Review
patch with comments addressed
Attachment #8603525 - Attachment is obsolete: true
We need to make sure to land this today for devedition-40. Can't push to try because tree is closed, but I don't think this needs a try push.
Status: NEW → ASSIGNED
Unfortunately this bug also needs strings, and will land for devedition-40.1 :/ Following the same process I did for bug 1131756, RyanVM approved landing just the strings for this even though tree is closed. Looping RyanVM because I added a=RyanVM here too.
Flags: needinfo?(ryanvm)
R+ just landing string changes, have been doing that a lot myself.

Also, looking at it again, and being nitpicky, maybe we should get rid of the "this" in the string. Something about that sounds weird. "Error loading /this/ URL: %S". It makes devtools sound a little too human. Again, super nit.

I can keep an eye out for a green tree as well to try and land the string if it gets late, too, just ni? me here and I'll be vigilant.
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #14)
> Also, looking at it again, and being nitpicky, maybe we should get rid of
> the "this" in the string. Something about that sounds weird. "Error loading
> /this/ URL: %S". It makes devtools sound a little too human. Again, super
> nit.

Too late for the minor string change - that'll have to ship in 41.  Landed on m-c
Flags: needinfo?(ryanvm)
Yup saw in IRC, not a prob!
rebased patch for aurora without string changes

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: bad debugger error messages when sources can't be loaded
[Describe test coverage new/current, TreeHerder]: on m-c
[Risks and why]: not much risk, only in debugger devtool
[String/UUID change made/needed]:
Attachment #8605472 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c4280fa46aa5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Can we get the uplift approved soon? This needs to sit on aurora for a little bit before June 2
Flags: needinfo?(lhenry)
sorry for the spam, I think this is the right contact
Flags: needinfo?(lhenry) → needinfo?(lmandel)
Comment on attachment 8605472 [details] [diff] [review]
1164483-aurora.patch

Approved for uplift to aurora. Fix for dev tools so nice to have it in aurora.
Flags: needinfo?(lmandel)
Attachment #8605472 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [devedition-40][difficulty=medium] → [polish-backlog][difficulty=medium]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.