User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.124 Safari/537.36 Steps to reproduce: Generated sourcemaps using the Webpack bundling tool, as seen in this repo: https://github.com/Stuk/webpack-sourcemaps-firefox I tried this in FF 38.05 and FF Developer Edition 40.0a2. Actual results: Although debugger statement was hit, Firefox devtools debugger did not show the original code. Expected results: Expected debugger statement to be hit, and be able to step through the original code as specified by the source maps. Chrome and Safari work as expected. I loaded the generated source and sourcemap into this tool (http://sokra.github.io/source-map-visualization/) and that understands the sourcemap as well.

Confirmed that the visualiser indicates that this source map is fine. When I load it in Nightly, I get this on stdout. fitzgen says it's relevant: ************************* A coding exception was thrown in a Promise resolution callback. See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise Full message: TypeError: originalLocation.originalSourceActor is null Full stack: ThreadActor.prototype.onFrames/promise<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:1274:13 Handler.prototype.process@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:937:23 this.PromiseWalker.walkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:816:7 Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:747:11 this.PromiseWalker.schedulePromise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:779:7 Promise.prototype.then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:454:5 TabSources.prototype.getOriginalLocation@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/utils/TabSources.js:605:12 ThreadActor.prototype.onFrames@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:1269:21 DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1643:15 ChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:742:5 EventLoop.prototype.enter@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:348:5 ThreadActor.prototype._pushThreadPause@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:542:5 ThreadActor.prototype._pauseAndRespond@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:751:7 ThreadActor.prototype.onDebuggerStatement@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:1823:9 @file:///home/jorendorff/dev/webpack-sourcemaps-firefox/bundle.js:48:2 __webpack_require__@file:///home/jorendorff/dev/webpack-sourcemaps-firefox/bundle.js:20:12 @file:///home/jorendorff/dev/webpack-sourcemaps-firefox/bundle.js:40:18 @file:///home/jorendorff/dev/webpack-sourcemaps-firefox/bundle.js:1:11 ************************* Building the browser now, to do some good old-fashioned console.log() debugging...

Something strange is going on, I'm even noticing odd behavior with normal breakpoints. This looks like we have a real regression and we should fix it asap. I'm looking into it too.

Whenever use a location, we need to check if it actually got sourcemapped. When it doesn't, it returns `{ originalSourceActor: null, originalLine: null, ...}`. In this scenario, the bottom-most frame was unable to be sourcemapped for some reason, so we need to skip it instead of erroring out completely. I also noticed a regression in `_addSource` which made it so top-level breakpoints weren't working correctly either. Eddy let me know what you think. I'd love to include tests but honestly I'm not sure what tests to write. These are hard edge cases and I don't know how to trigger these conditions. I suppose I could try a mochitest that sets a breakpoint on top-level code.

(In reply to James Long (:jlongster) from comment #3) > I'd love to include tests but honestly I'm not sure what tests to write. > These are hard edge cases and I don't know how to trigger these conditions. > I suppose I could try a mochitest that sets a breakpoint on top-level code. Have a frame whose location maps to a null mapping on the stack when calling a debugger statement? // Set up debuggee code. const a = new SourceNode("foo.js", 1, 1, "function a() { b(); }"); const b = new SourceNode(null, null, null, "function b() { c(); }"); const c = new SourceNode("foo.js", 1, 1, "function c() { debugger; }"); const { map, code } = (new SourceNode(null, null, null [a,b,c])).toStringWithSourceMap({ file: "bar.js", sourceRoot: "root", }); debuggee.eval(code + "//# sourceMappingURL=data:text/json;base64," + btoa(map.toString())); // Trigger it. debuggee.eval("a()");

Aah that looks great, thanks!

With a test!

Above run didn't run xpcshell tests https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc94e5c190d0

Comment on attachment 8730911 [details] [diff] [review] 1177329.patch Review of attachment 8730911 [details] [diff] [review]: ----------------------------------------------------------------- Not much to add. Patch looks good to me. ::: devtools/server/actors/script.js @@ -1935,5 @@ > > let sourceActor = this.sources.createNonSourceMappedActor(aSource); > > // Set any stored breakpoints. > - let bpActors = this.breakpointActorMap.findActors(); How did this ever work?

[Tracking Requested - why for this release]: Per bug 1238734 comment 8, marking Dev. Ed as affected for potential uplift.

Attempting to improve the summary. Let me know if this wasn't helpful and I'll learn for next time.

That helps a lot, thanks :)

(In reply to Eddy Bruel [:ejpbruel] from comment #9) > Comment on attachment 8730911 [details] [diff] [review] > 1177329.patch > > Review of attachment 8730911 [details] [diff] [review]: > ----------------------------------------------------------------- > > Not much to add. Patch looks good to me. > > ::: devtools/server/actors/script.js > @@ -1935,5 @@ > > > > let sourceActor = this.sources.createNonSourceMappedActor(aSource); > > > > // Set any stored breakpoints. > > - let bpActors = this.breakpointActorMap.findActors(); > > How did this ever work? It's a regression. Don't have the commit on hand but I think it's been broken for a little while. I think the result was the sometimes top-level breakpoints with sourcemaps didn't work sometimes, which is unfortunate. I filed a bug to write a test for that case: https://bugzilla.mozilla.org/show_bug.cgi?id=1258006 I'd like to go ahead and land this though. It's a pretty good low-hanging fix.

Comment on attachment 8730911 [details] [diff] [review] 1177329.patch Review of attachment 8730911 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/script.js @@ +1277,5 @@ > + line: originalLocation.originalLine, > + column: originalLocation.originalColumn > + }; > + form.source = sourceForm; > + frames.push(form); Can the getOriginalLocation promises complete out of order, causing the frames to be pushed out of order? Does it matter? This looked suspicious to me, being unfamiliar with the code. Regrets if it's obvious to everyone else why this is OK.

(In reply to Matt McCutchen from comment #16) > Can the getOriginalLocation promises complete out of order, causing the > frames to be pushed out of order? Does it matter? > > This looked suspicious to me, being unfamiliar with the code. Regrets if > it's obvious to everyone else why this is OK. Yes, it totally can return out of order, and yes it totally does matter. Good catch!

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #17) > Yes, it totally can return out of order, and yes it totally does matter. > > Good catch! How will this be addressed now that the bug is marked fixed? Should I file a follow-up bug?

You don't need to file a follow up, thanks for offering. ni jlongster so that it doesn't slip under the radar. He can decide to do a follow up patch here or open another bug.

Yikes, good point. I'm definitely going to add a test make sure frames are ordered correctly. I'll back out and follow-up here (not right now, but soon).

Will post a follow-up patch

I think all the promises before should have run in order because the promises were created in order and the all blocked on the same promise (that is resolved when the sourcemap is fetched). But we definitely shouldn't rely on that. I wasn't able to reproduce any failures, but this patch makes it more deterministic. I also improved the test I added to actually check the frames. There were other tests that checked the order of the frames but they are all passing because the promises happened to be resolved correctly.

Comment on attachment 8732970 [details] [diff] [review] 1177329-promise-resolution.patch Review of attachment 8732970 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/script.js @@ +1283,5 @@ > form.source = sourceForm; > + } else { > + // Remove the frame. We can't show sourcemapped and > + // unsourcemapped sources at the same time yet. > + frames.splice(idx, 1); I believe the index needs to be "idx - start". A bigger problem is that when you splice out one frame, the indices shift, and later splices will not remove the frame you intended.

James, if it helps, here's a patch that I believe is correct, though I haven't actually tested it and you may prefer to use a different technique.

Ugh, I hate mutations. Thanks Matt. I'll take your patch but I may tweak it some more and make the promises just return a frame and then filter out nulls instead.

Definitely going to add a bunch more frames in the test as well

Alright this is a much better patch. Thanks for helping push this Matt.

Comment on attachment 8733033 [details] [diff] [review] 1177329-promise-resolution.patch Review of attachment 8733033 [details] [diff] [review]: ----------------------------------------------------------------- Yes, I like using the return value of the promise better too. ::: devtools/server/actors/script.js @@ +1263,3 @@ > let promises = []; > for (; frame && (!count || i < (start + count)); i++, frame=frame.older) { > + let idx = i; This is unused now.

Thanks a lot Matt! https://treeherder.mozilla.org/#/jobs?repo=try&revision=68733c2bcc34

Comment on attachment 8733033 [details] [diff] [review] 1177329-promise-resolution.patch Review of attachment 8733033 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! r=me Don't feel like you have to change to my preferred style, but I left comments on how I would write it nonetheless :) ::: devtools/server/actors/script.js @@ +1263,3 @@ > let promises = []; > for (; frame && (!count || i < (start + count)); i++, frame=frame.older) { > + let idx = i; Nice catch, Matt :) @@ +1283,2 @@ > } > + return null; Personally, I prefer early returns to deeper nesting: if (!originalLocation.originalSourceActor) { return null; } ... return form; @@ +1287,5 @@ > } > > + return all(promises).then(function (frames) { > + // Filter null values because sourcemapping may have failed. > + return { frames: frames.filter(x => x) }; I also tend to prefer being explicit: `x => x !== null` or even `x => !!x`. But I recognize, again, that this is somewhat personal, and it doesn't matter too much. FWIW, we have a `isNotNull` function in TabSources.js which is also used for filtering -- maybe we should move it to ThreadSafeDevToolsUtils.js and share it? ::: devtools/server/tests/unit/test_sourcemaps-17.js @@ +51,5 @@ > + // b.js should be skipped > + do_check_eq(frames[0].where.source.url, "http://example.com/www/root/e.js"); > + do_check_eq(frames[1].where.source.url, "http://example.com/www/root/c.js"); > + do_check_eq(frames[2].where.source.url, "http://example.com/www/root/a.js"); > + do_check_eq(frames[3].where.source.url, null); Nice

Thanks for the review! All tests pass so I'll land this. (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #31) > > @@ +1283,2 @@ > > } > > + return null; > > Personally, I prefer early returns to deeper nesting: Usually we agree on style, so I'm fine tweaking it! Made this change. > @@ +1287,5 @@ > > } > > > > + return all(promises).then(function (frames) { > > + // Filter null values because sourcemapping may have failed. > > + return { frames: frames.filter(x => x) }; > > I also tend to prefer being explicit: `x => x !== null` or even `x => !!x`. > But I recognize, again, that this is somewhat personal, and it doesn't > matter too much. > > FWIW, we have a `isNotNull` function in TabSources.js which is also used for > filtering -- maybe we should move it to ThreadSafeDevToolsUtils.js and share > it? I changed it to `x => !!x` which I think is good enough. Personally I don't think just checking for null deserves its own function, but that's just me. And here we just need a truthy value, so regardless `!!x` is ok.

Hi James, is this also an issue with Fx46 and needs to be uplifted to Beta?

Hello, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Yes, I believe beta is affected

Comment on attachment 8730911 [details] [diff] [review] 1177329.patch Approval Request Comment [Feature/regressing bug #]: NA [User impact if declined]: Debugger may break sometimes when using sourcemaps [Describe test coverage new/current, TreeHerder]: New tests have been added [Risks and why]: Not much risk. Small change and only for devtools [String/UUID change made/needed]:

Comment on attachment 8733033 [details] [diff] [review] 1177329-promise-resolution.patch Approval Request Comment [Feature/regressing bug #]: NA [User impact if declined]: Debugger may break sometimes when using sourcemaps [Describe test coverage new/current, TreeHerder]: New tests have been added [Risks and why]: Not much risk. Small change and only for devtools [String/UUID change made/needed]:

Comment on attachment 8730911 [details] [diff] [review] 1177329.patch It's an old bug but one worth fixing for webdevs, Aurora47+

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce278e1caab8 https://hg.mozilla.org/releases/mozilla-aurora/rev/e74741001244

I'm a bit late, but I can confirm that this has fixed the issue for me.

Thanks for letting us know!

Thank you all for fixing it!

(In reply to website from comment #42) > I'm a bit late, but I can confirm that this has fixed the issue for me. Great! Appreciate your follow up here.