Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555223004:
Fix behavior causing frames lacking a source map to always be omitted. Add `includeUnmappedFrames` … (Closed)
DescriptionSupport extended source maps.
Pass stack frame uris to source map `Mapping` fixing bug where source
map lookups occurred using the wrong file when using a extended source map
bundle that specified file information.
R=nweiz@google.com
Committed: https://github.com/dart-lang/source_map_stack_trace/commit/f4bbbda4b32fb704d7eeec1d43b1948c8ebeed26
Patch Set 1 #Patch Set 2 : Fix behavior causing frames lacking a source map to always be omitted. Add `includeUnmappedFrames` … #Patch Set 3 : Fix behavior so frames that do not match the source map are still be emitted if the new `includeUnm… #
Total comments: 20
Patch Set 4 : Support extended source maps. #Patch Set 5 : Support extended source maps. #Patch Set 6 : Support extended source maps. #
Total comments: 13
Patch Set 7 : Support extended source maps. #Patch Set 8 : Support extended source maps. #Patch Set 9 : Support extended source maps. #
Total comments: 3
Patch Set 10 : Support extended source maps. #Patch Set 11 : Add second dart file to test. #
Messages
Total messages: 18 (3 generated)
jacobr@google.com changed reviewers: + nweiz@google.com
Fix behavior so frames that do not match the source map are still be emitted if the new `includeUnmappedFrames` named parameter is set. Pass stack frame uris to source map `Mapping` fixing bug where source map lookups occurred using the wrong file.
Can you provide some more context for this? What JS files are we seeing that have incomplete source maps? Why do we want to see additional stack trace lines for those files? https://codereview.chromium.org/2555223004/diff/40001/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/2555223004/diff/40001/CHANGELOG.md#newcode1 CHANGELOG.md:1: ## 1.1.3+1 Build numbers ("+x") are only used for packages with a major version of 0. This should be 1.1.4. https://codereview.chromium.org/2555223004/diff/40001/CHANGELOG.md#newcode5 CHANGELOG.md:5: frames in a backwards compatible manner. This is a bit of a run-on sentence. Maybe: "Add an `includeUnmappedFrames` parameter to `mapStackTrace`. If `true`, frames that aren't covered by the source map are included in the resulting trace." https://codereview.chromium.org/2555223004/diff/40001/CHANGELOG.md#newcode8 CHANGELOG.md:8: map lookups occurred using the wrong file. Changelog entries should just describe the user-visible behavior, not the underlying implementation. Under what circumstances would a user have seen the bug in previous versions? Why was the behavior wrong? What does it do now? https://codereview.chromium.org/2555223004/diff/40001/lib/source_map_stack_tr... File lib/source_map_stack_trace.dart (left): https://codereview.chromium.org/2555223004/diff/40001/lib/source_map_stack_tr... lib/source_map_stack_trace.dart:68: // internal that the user doesn't care about. This comment should still exist, maybe reworded a bit to fit the new code. https://codereview.chromium.org/2555223004/diff/40001/lib/source_map_stack_tr... File lib/source_map_stack_trace.dart (right): https://codereview.chromium.org/2555223004/diff/40001/lib/source_map_stack_tr... lib/source_map_stack_trace.dart:24: /// [sourceMap] should be included in the mapped stack trace. What happens to those frames if it's `false`? https://codereview.chromium.org/2555223004/diff/40001/lib/source_map_stack_tr... lib/source_map_stack_trace.dart:87: from: packageResolver.packageRoot.toString()); Please don't include unrelated formatting changes. https://codereview.chromium.org/2555223004/diff/40001/test/source_map_stack_t... File test/source_map_stack_trace_test.dart (right): https://codereview.chromium.org/2555223004/diff/40001/test/source_map_stack_t... test/source_map_stack_trace_test.dart:6: import 'package:path/path.dart' as path; as p https://codereview.chromium.org/2555223004/diff/40001/test/source_map_stack_t... test/source_map_stack_trace_test.dart:62: test("shows JS frames without corresponding spans", () { Put these in a "with includeUnmappedFrames" group. https://codereview.chromium.org/2555223004/diff/40001/test/source_map_stack_t... test/source_map_stack_trace_test.dart:99: packageRoot: "packages/", includeUnmappedFrames: true) packageRoot is deprecated; use packageResolver instead. https://codereview.chromium.org/2555223004/diff/40001/test/source_map_stack_t... test/source_map_stack_trace_test.dart:108: expect(frame.line, equals(10)); Add some empty lines here, this is difficult to read.
Also, please follow the commit message best practices: https://github.com/dart-lang/sdk/wiki/5-Commit-Message-Best-Practices
Description was changed from ========== Fix behavior causing frames lacking a source map to always be omitted. Add `includeUnmappedFrames` named parameter enabling showing unmapped frames in a backwards compatible manner. Pass stack frame uris to source map `Mapping` fixing bug where source map lookups occurred using the wrong file. BUG= ========== to ========== Support extended source maps. Pass stack frame uris to source map `Mapping` fixing bug where source map lookups occurred using the wrong file when using a extended source map bundle that specified file information. ==========
https://codereview.chromium.org/2555223004/diff/40001/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/2555223004/diff/40001/CHANGELOG.md#newcode1 CHANGELOG.md:1: ## 1.1.3+1 On 2016/12/08 23:34:31, nweiz wrote: > Build numbers ("+x") are only used for packages with a major version of 0. This > should be 1.1.4. Done. https://codereview.chromium.org/2555223004/diff/40001/CHANGELOG.md#newcode5 CHANGELOG.md:5: frames in a backwards compatible manner. On 2016/12/08 23:34:31, nweiz wrote: > This is a bit of a run-on sentence. Maybe: > > "Add an `includeUnmappedFrames` parameter to `mapStackTrace`. If `true`, frames > that aren't covered by the source map are included in the resulting trace." Done. https://codereview.chromium.org/2555223004/diff/40001/CHANGELOG.md#newcode8 CHANGELOG.md:8: map lookups occurred using the wrong file. On 2016/12/08 23:34:31, nweiz wrote: > Changelog entries should just describe the user-visible behavior, not the > underlying implementation. Under what circumstances would a user have seen the > bug in previous versions? Why was the behavior wrong? What does it do now? just removed. not worth mentioning. https://codereview.chromium.org/2555223004/diff/40001/lib/source_map_stack_tr... File lib/source_map_stack_trace.dart (right): https://codereview.chromium.org/2555223004/diff/40001/lib/source_map_stack_tr... lib/source_map_stack_trace.dart:24: /// [sourceMap] should be included in the mapped stack trace. On 2016/12/08 23:34:31, nweiz wrote: > What happens to those frames if it's `false`? if it is false they are not included. https://codereview.chromium.org/2555223004/diff/40001/lib/source_map_stack_tr... lib/source_map_stack_trace.dart:87: from: packageResolver.packageRoot.toString()); On 2016/12/08 23:34:31, nweiz wrote: > Please don't include unrelated formatting changes. I'll send a followup cl that runs dartfm. would be nice if using a dart editor that automatically runs dartfmt didn't mess up this file. https://codereview.chromium.org/2555223004/diff/40001/test/source_map_stack_t... File test/source_map_stack_trace_test.dart (right): https://codereview.chromium.org/2555223004/diff/40001/test/source_map_stack_t... test/source_map_stack_trace_test.dart:6: import 'package:path/path.dart' as path; On 2016/12/08 23:34:31, nweiz wrote: > as p Done. https://codereview.chromium.org/2555223004/diff/40001/test/source_map_stack_t... test/source_map_stack_trace_test.dart:62: test("shows JS frames without corresponding spans", () { On 2016/12/08 23:34:31, nweiz wrote: > Put these in a "with includeUnmappedFrames" group. obsolete as all but one test doesn't matter given offline suggestions. https://codereview.chromium.org/2555223004/diff/40001/test/source_map_stack_t... test/source_map_stack_trace_test.dart:99: packageRoot: "packages/", includeUnmappedFrames: true) On 2016/12/08 23:34:31, nweiz wrote: > packageRoot is deprecated; use packageResolver instead. Done. https://codereview.chromium.org/2555223004/diff/40001/test/source_map_stack_t... test/source_map_stack_trace_test.dart:108: expect(frame.line, equals(10)); On 2016/12/08 23:34:31, nweiz wrote: > Add some empty lines here, this is difficult to read. Done.
https://codereview.chromium.org/2555223004/diff/40001/lib/source_map_stack_tr... File lib/source_map_stack_trace.dart (left): https://codereview.chromium.org/2555223004/diff/40001/lib/source_map_stack_tr... lib/source_map_stack_trace.dart:68: // internal that the user doesn't care about. On 2016/12/08 23:34:31, nweiz wrote: > This comment should still exist, maybe reworded a bit to fit the new code. obsolete.
https://codereview.chromium.org/2555223004/diff/100001/lib/source_map_stack_t... File lib/source_map_stack_trace.dart (right): https://codereview.chromium.org/2555223004/diff/100001/lib/source_map_stack_t... lib/source_map_stack_trace.dart:66: sourceMap.spanFor(frame.line - 1, column - 1, uri: frame.uri?.toString()); Long line. You're using a new feature in the source_maps package, which means you need to bump the dependency on source_maps to ensure that we only ever get versions that have the new feature. https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... File test/source_map_stack_trace_test.dart (left): https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... test/source_map_stack_trace_test.dart:24: .build("foo.dart.js.map")); Please remove unrelated formatting changes. https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... File test/source_map_stack_trace_test.dart (right): https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... test/source_map_stack_trace_test.dart:62: test("include JS frames from external libraries with source map bundle", () { "includes" I'm not sure what "external" means here. What makes a library external vs internal? https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... test/source_map_stack_trace_test.dart:95: Also test a source map bundle contains multiple source maps for multiple different target files.
https://codereview.chromium.org/2555223004/diff/100001/lib/source_map_stack_t... File lib/source_map_stack_trace.dart (right): https://codereview.chromium.org/2555223004/diff/100001/lib/source_map_stack_t... lib/source_map_stack_trace.dart:66: sourceMap.spanFor(frame.line - 1, column - 1, uri: frame.uri?.toString()); On 2016/12/12 22:04:56, nweiz wrote: > Long line. > > You're using a new feature in the source_maps package, which means you need to > bump the dependency on source_maps to ensure that we only ever get versions that > have the new feature. Done. https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... File test/source_map_stack_trace_test.dart (left): https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... test/source_map_stack_trace_test.dart:24: .build("foo.dart.js.map")); On 2016/12/12 22:04:56, nweiz wrote: > Please remove unrelated formatting changes. Done. https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... File test/source_map_stack_trace_test.dart (right): https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... test/source_map_stack_trace_test.dart:62: test("include JS frames from external libraries with source map bundle", () { On 2016/12/12 22:04:56, nweiz wrote: > "includes" > > I'm not sure what "external" means here. What makes a library external vs > internal? jquery.js is external library foo.dart.js is internal. https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... test/source_map_stack_trace_test.dart:95: On 2016/12/12 22:04:56, nweiz wrote: > Also test a source map bundle contains multiple source maps for multiple > different target files. that is the sort of test that belongs in the source maps package. tests in this package just need to make sure we are actually passing in the uris so that the source map package can do its thing. To verify that, having jquery.js and foo.dart.js is sufficient. The tests in the source_maps package are the right place for tests that verify in detail that edge cases resolving source map bundles work.
https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... File test/source_map_stack_trace_test.dart (right): https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... test/source_map_stack_trace_test.dart:62: test("include JS frames from external libraries with source map bundle", () { On 2016/12/13 18:01:54, Jacob wrote: > On 2016/12/12 22:04:56, nweiz wrote: > > "includes" > > > > I'm not sure what "external" means here. What makes a library external vs > > internal? > jquery.js is external library > foo.dart.js is internal. Maybe "JS files not covered by the source map bundle" https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... test/source_map_stack_trace_test.dart:95: On 2016/12/13 18:01:54, Jacob wrote: > On 2016/12/12 22:04:56, nweiz wrote: > > Also test a source map bundle contains multiple source maps for multiple > > different target files. > > that is the sort of test that belongs in the source maps package. tests in this > package just need to make sure we are actually passing in the uris so that the > source map package can do its thing. > > To verify that, having jquery.js and foo.dart.js is sufficient. > The tests in the source_maps package are the right place for tests that verify > in detail that edge cases resolving source map bundles work. This CL concretely changes the behavior with respect to bundles, because it's now passing in the URL from the frame. That's the most important user-facing change here, and we should test it. https://codereview.chromium.org/2555223004/diff/160001/lib/source_map_stack_t... File lib/source_map_stack_trace.dart (right): https://codereview.chromium.org/2555223004/diff/160001/lib/source_map_stack_t... lib/source_map_stack_trace.dart:66: sourceMap.spanFor(frame.line - 1, column - 1, uri: frame.uri?.toString()); Still a long line. https://codereview.chromium.org/2555223004/diff/160001/pubspec.yaml File pubspec.yaml (right): https://codereview.chromium.org/2555223004/diff/160001/pubspec.yaml#newcode12 pubspec.yaml:12: source_maps: "^0.10.1" The url parameter for spanFrom() wasn't added in 0.10.1, it was added in 0.10.1+3.
https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... File test/source_map_stack_trace_test.dart (right): https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... test/source_map_stack_trace_test.dart:62: test("include JS frames from external libraries with source map bundle", () { On 2016/12/13 22:05:46, nweiz wrote: > On 2016/12/13 18:01:54, Jacob wrote: > > On 2016/12/12 22:04:56, nweiz wrote: > > > "includes" > > > > > > I'm not sure what "external" means here. What makes a library external vs > > > internal? > > jquery.js is external library > > foo.dart.js is internal. > > Maybe "JS files not covered by the source map bundle" Done. https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... test/source_map_stack_trace_test.dart:95: On 2016/12/13 22:05:46, nweiz wrote: > On 2016/12/13 18:01:54, Jacob wrote: > > On 2016/12/12 22:04:56, nweiz wrote: > > > Also test a source map bundle contains multiple source maps for multiple > > > different target files. > > > > that is the sort of test that belongs in the source maps package. tests in > this > > package just need to make sure we are actually passing in the uris so that the > > source map package can do its thing. > > > > To verify that, having jquery.js and foo.dart.js is sufficient. > > The tests in the source_maps package are the right place for tests that verify > > in detail that edge cases resolving source map bundles work. > > This CL concretely changes the behavior with respect to bundles, because it's > now passing in the URL from the frame. That's the most important user-facing > change here, and we should test it. Integration level testing of that behavior exists in this file. notice that behavior for the line in foo.dart and and jquery.js are different only because the file names are passed in. If we had a bug and omitted the uri then behavior for the jquery.js line would change. That is the appropriate level of granularity to test in this package. In source_maps you should and I do have tests that verify edge cases within the source_maps package. https://codereview.chromium.org/2555223004/diff/160001/pubspec.yaml File pubspec.yaml (right): https://codereview.chromium.org/2555223004/diff/160001/pubspec.yaml#newcode12 pubspec.yaml:12: source_maps: "^0.10.1" On 2016/12/13 22:05:46, nweiz wrote: > The url parameter for spanFrom() wasn't added in 0.10.1, it was added in > 0.10.1+3. Done.
https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... File test/source_map_stack_trace_test.dart (right): https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... test/source_map_stack_trace_test.dart:95: On 2016/12/13 23:13:05, Jacob wrote: > On 2016/12/13 22:05:46, nweiz wrote: > > On 2016/12/13 18:01:54, Jacob wrote: > > > On 2016/12/12 22:04:56, nweiz wrote: > > > > Also test a source map bundle contains multiple source maps for multiple > > > > different target files. > > > > > > that is the sort of test that belongs in the source maps package. tests in > > this > > > package just need to make sure we are actually passing in the uris so that > the > > > source map package can do its thing. > > > > > > To verify that, having jquery.js and foo.dart.js is sufficient. > > > The tests in the source_maps package are the right place for tests that > verify > > > in detail that edge cases resolving source map bundles work. > > > > This CL concretely changes the behavior with respect to bundles, because it's > > now passing in the URL from the frame. That's the most important user-facing > > change here, and we should test it. > Integration level testing of that behavior exists in this file. notice that > behavior for the line in foo.dart and and jquery.js are different only because > the file names are passed in. If we had a bug and omitted the uri then behavior > for the jquery.js line would change. > That is the appropriate level of granularity to test in this package. > In source_maps you should and I do have tests that verify edge cases within the > source_maps package. That's not clear from a black-box perspective. There's a big semantic difference between "different files that appear in the source map" and "a file that doesn't appear in the source map at all", and it's reasonable to test both cases.
On 2016/12/13 23:43:48, nweiz wrote: > https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... > File test/source_map_stack_trace_test.dart (right): > > https://codereview.chromium.org/2555223004/diff/100001/test/source_map_stack_... > test/source_map_stack_trace_test.dart:95: > On 2016/12/13 23:13:05, Jacob wrote: > > On 2016/12/13 22:05:46, nweiz wrote: > > > On 2016/12/13 18:01:54, Jacob wrote: > > > > On 2016/12/12 22:04:56, nweiz wrote: > > > > > Also test a source map bundle contains multiple source maps for multiple > > > > > different target files. > > > > > > > > that is the sort of test that belongs in the source maps package. tests in > > > this > > > > package just need to make sure we are actually passing in the uris so that > > the > > > > source map package can do its thing. > > > > > > > > To verify that, having jquery.js and foo.dart.js is sufficient. > > > > The tests in the source_maps package are the right place for tests that > > verify > > > > in detail that edge cases resolving source map bundles work. > > > > > > This CL concretely changes the behavior with respect to bundles, because > it's > > > now passing in the URL from the frame. That's the most important user-facing > > > change here, and we should test it. > > Integration level testing of that behavior exists in this file. notice that > > behavior for the line in foo.dart and and jquery.js are different only because > > the file names are passed in. If we had a bug and omitted the uri then > behavior > > for the jquery.js line would change. > > That is the appropriate level of granularity to test in this package. > > In source_maps you should and I do have tests that verify edge cases within > the > > source_maps package. > > That's not clear from a black-box perspective. There's a big semantic difference > between "different files that appear in the source map" and "a file that doesn't > appear in the source map at all", and it's reasonable to test both cases. Tests in this package do not need to verify that the source map package is correct. They need to verify that we are passing the correct inputs to the source maps package. The tests here fully verify that we are passing the correct inputs to the source map package. The query.js line would resolve against the source map for the .dart file if the source map lookup was not considering the uri. I can copy and paste the more detailed tests in the source maps package here to cover the case you are suggesting which is a good case to test in the source maps package but I am still at a loss to see why that duplication of test cases is a good thing.
On 2016/12/14 02:11:46, Jacob wrote: > Tests in this package do not need to verify that the source map package is > correct. They need to verify that we are passing the correct inputs to the > source maps package. The tests here fully verify that we are passing the correct > inputs to the source map package. The query.js line would resolve against the > source map for the .dart file if the source map lookup was not considering the > uri. > I can copy and paste the more detailed tests in the source maps package here to > cover the case you are suggesting which is a good case to test in the source > maps package but I am still at a loss to see why that duplication of test cases > is a good thing. Please just add the test case. I've explained why I think it's important to the best of my ability, but if that's not enough please just take my word for it.
On 2016/12/14 02:20:25, nweiz wrote: > On 2016/12/14 02:11:46, Jacob wrote: > > Tests in this package do not need to verify that the source map package is > > correct. They need to verify that we are passing the correct inputs to the > > source maps package. The tests here fully verify that we are passing the > correct > > inputs to the source map package. The query.js line would resolve against the > > source map for the .dart file if the source map lookup was not considering the > > uri. > > I can copy and paste the more detailed tests in the source maps package here > to > > cover the case you are suggesting which is a good case to test in the source > > maps package but I am still at a loss to see why that duplication of test > cases > > is a good thing. > > Please just add the test case. I've explained why I think it's important to the > best of my ability, but if that's not enough please just take my word for it. Added a second dart file to the test. PTAL
lgtm
Description was changed from ========== Support extended source maps. Pass stack frame uris to source map `Mapping` fixing bug where source map lookups occurred using the wrong file when using a extended source map bundle that specified file information. ========== to ========== Support extended source maps. Pass stack frame uris to source map `Mapping` fixing bug where source map lookups occurred using the wrong file when using a extended source map bundle that specified file information. R=nweiz@google.com Committed: https://github.com/dart-lang/source_map_stack_trace/commit/f4bbbda4b32fb704d7... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) manually as f4bbbda4b32fb704d7eeec1d43b1948c8ebeed26 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
