Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for parsing Wasm stack frames of Chrome (V8), Firefox, Safari #159

Merged
merged 25 commits into from
Sep 19, 2024

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Sep 17, 2024

Instead of updating existing regexes to handle new formats, this adds new
regexes for Wasm frames.

Parser functions are updated to try to parse the frame with Wasm regexes.

Column numbers can be used by the source_map_stack_trace package to map Wasm
frames to source locations.

Tests added for parsing all combinations of:

  • Frames with and without function names
  • In Chrome, Firefox, Safari formats

@osa1 osa1 marked this pull request as draft September 17, 2024 09:51
@osa1
Copy link
Member Author

osa1 commented Sep 17, 2024

This is not ready yet, we need to parse code offsets to be able to convert them to source, line, column info in source_map_stack_trace. I'll update. Done.

Copy link
Member

@mkustermann mkustermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osa1 Do we have actual tests that throw in browser and use package:stack_trace to decode it?

We could roll this change in package:stack_trace (via DEPS) into Dart SDK and add the tests there.

lib/src/frame.dart Outdated Show resolved Hide resolved
@osa1 osa1 marked this pull request as ready for review September 17, 2024 10:28
@osa1
Copy link
Member Author

osa1 commented Sep 17, 2024

@osa1 Do we have actual tests that throw in browser and use package:stack_trace to decode it?

We could roll this change in package:stack_trace (via DEPS) into Dart SDK and add the tests there.

Good idea, but it should be tested in this package instead of in SDK.

Is there a reason to not test this in this package?

@mkustermann
Copy link
Member

Good idea, but it should be tested in this package instead of in SDK.

Ideally yes.

Is there a reason to not test this in this package?

May be possible - I don't know. The github tests would need to run with various browsers. Currently it seems from .github/workflows/test-package.yml it only runs ubuntu. So it e.g. couldn't test Safari.

Copy link
Member

@lrhn lrhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by comment.
I saw RegExps, so I had to say something.

@@ -17,12 +17,36 @@ final _vmFrame = RegExp(r'^#\d+\s+(\S.*) \((.+?)((?::\d+){0,2})\)$');
// at VW.call$0 (eval as fn
// (https://example.com/stuff.dart.js:560:28), efn:3:28)
// at https://example.com/stuff.dart.js:560:28
final _v8Frame =
final _v8JsFrame =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(While here, it may be worth saying that these regexps are applied to individual lines. That's why the ^ doesn't require multiLine: true and why it's not a problem that [^\]+ can match newlines. Helps the reader set expectations.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's clear from the use sites a few lines below that these parse a single "frame", as passed to factories like Frame.parseVM(String frame).

However the exact format of those "frame"s is not described by this library even though the Frame type and its factories are public.

It would be good to describe the format for the users who may want to parse a single frame.

Let's do it separately though.

lib/src/frame.dart Outdated Show resolved Hide resolved
lib/src/frame.dart Outdated Show resolved Hide resolved
lib/src/frame.dart Outdated Show resolved Hide resolved
lib/src/frame.dart Outdated Show resolved Hide resolved
lib/src/frame.dart Show resolved Hide resolved
lib/src/frame.dart Outdated Show resolved Hide resolved
test/frame_test.dart Show resolved Hide resolved
//
// Group 1: URI, required
// Group 2: line number, required
// Group 3: column number, optional
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If you are changing things anyway, consider using named capture groups here too, for consistency. Or not, can be done at any later time too.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do unrelated changes separately. It helps with debugging, bisecting and reverting.

@osa1
Copy link
Member Author

osa1 commented Sep 18, 2024

I'm now able to map parse stack traces with source maps using the source_map_stack_trace using the changes in this PR.

I'll now address the comments.

@osa1 osa1 merged commit 9b1ed4f into dart-lang:master Sep 19, 2024
4 checks passed
@osa1 osa1 deleted the wasm_frames branch September 19, 2024 08:42
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Sep 23, 2024
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/8100ccf..526dbd5):
  526dbd55  2024-09-15  Sam Rawlins  Strip the enclosing package's lower-case name from a library's dir name (dart-lang/dartdoc#3883)
  98dd9dad  2024-09-13  Sam Rawlins  Add output-size info to the '--stats' flag on various tasks. (dart-lang/dartdoc#3879)
  090b05ba  2024-09-13  Sam Rawlins  Bump to 8.2.0 (dart-lang/dartdoc#3880)
  0a529762  2024-09-12  Sam Rawlins  Fix search which is not using canonical libraries as enclosing elements (dart-lang/dartdoc#3877)
  d4dff9a4  2024-09-12  Konstantin Scheglov  Stop using 'augmented' from the element model. (dart-lang/dartdoc#3878)
  d6b865f9  2024-09-11  Sam Rawlins  Support multiple partial calls from a single template (dart-lang/dartdoc#3873)
  25c9edf3  2024-09-10  Parker Lougheed  Standardize to newer SDK APIs (dart-lang/dartdoc#3872)
  b61de2f2  2024-09-10  Sam Rawlins  Clean up some sorting, naming, and doc nits (dart-lang/dartdoc#3876)
  55ce1164  2024-09-10  Sam Rawlins  Use latest language version for DartFormatter (dart-lang/dartdoc#3875)
  2a262818  2024-09-10  Sam Rawlins  Comply with analyzer 6.9.0 APIs (dart-lang/dartdoc#3874)
  ca98b898  2024-09-09  Sam Rawlins  Include extension members on the extended type's page (dart-lang/dartdoc#3863)

mockito (https://github.com/dart-lang/mockito/compare/d0fda0c..b66be81):
  b66be81  2024-09-13  Googler  Migration for analyzer APIs.

stack_trace (https://github.com/dart-lang/stack_trace/compare/090d3d1..5b82965):
  5b82965  2024-09-19  Ömer Sinan Ağacan  Relax URI matching in V8 Wasm frame regex (dart-lang/stack_trace#161)
  9b1ed4f  2024-09-19  Ömer Sinan Ağacan  Add support for parsing Wasm stack frames of Chrome (V8), Firefox, Safari (dart-lang/stack_trace#159)
  d38eee8  2024-09-17  Ömer Sinan Ağacan  Fix analysis issues (dart-lang/stack_trace#160)

test (https://github.com/dart-lang/test/compare/9a2d155..22835e2):
  22835e2e  2024-09-13  Nate Bosch  Add support for multiple full paths on macos (dart-lang/test#2276)

web (https://github.com/dart-lang/web/compare/933a37d..d8549a3):
  d8549a3  2024-09-13  Srujan Gaddam   Add a generate-all flag to emit all bindings (dart-lang/web#302)

Change-Id: Ib07e4c0eda9c8ea180ef989c7d9cf20c390457e8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/386260
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
FMorschel pushed a commit to FMorschel/sdk that referenced this pull request Sep 25, 2024
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/8100ccf..526dbd5):
  526dbd55  2024-09-15  Sam Rawlins  Strip the enclosing package's lower-case name from a library's dir name (dart-lang/dartdoc#3883)
  98dd9dad  2024-09-13  Sam Rawlins  Add output-size info to the '--stats' flag on various tasks. (dart-lang/dartdoc#3879)
  090b05ba  2024-09-13  Sam Rawlins  Bump to 8.2.0 (dart-lang/dartdoc#3880)
  0a529762  2024-09-12  Sam Rawlins  Fix search which is not using canonical libraries as enclosing elements (dart-lang/dartdoc#3877)
  d4dff9a4  2024-09-12  Konstantin Scheglov  Stop using 'augmented' from the element model. (dart-lang/dartdoc#3878)
  d6b865f9  2024-09-11  Sam Rawlins  Support multiple partial calls from a single template (dart-lang/dartdoc#3873)
  25c9edf3  2024-09-10  Parker Lougheed  Standardize to newer SDK APIs (dart-lang/dartdoc#3872)
  b61de2f2  2024-09-10  Sam Rawlins  Clean up some sorting, naming, and doc nits (dart-lang/dartdoc#3876)
  55ce1164  2024-09-10  Sam Rawlins  Use latest language version for DartFormatter (dart-lang/dartdoc#3875)
  2a262818  2024-09-10  Sam Rawlins  Comply with analyzer 6.9.0 APIs (dart-lang/dartdoc#3874)
  ca98b898  2024-09-09  Sam Rawlins  Include extension members on the extended type's page (dart-lang/dartdoc#3863)

mockito (https://github.com/dart-lang/mockito/compare/d0fda0c..b66be81):
  b66be81  2024-09-13  Googler  Migration for analyzer APIs.

stack_trace (https://github.com/dart-lang/stack_trace/compare/090d3d1..5b82965):
  5b82965  2024-09-19  Ömer Sinan Ağacan  Relax URI matching in V8 Wasm frame regex (dart-lang/stack_trace#161)
  9b1ed4f  2024-09-19  Ömer Sinan Ağacan  Add support for parsing Wasm stack frames of Chrome (V8), Firefox, Safari (dart-lang/stack_trace#159)
  d38eee8  2024-09-17  Ömer Sinan Ağacan  Fix analysis issues (dart-lang/stack_trace#160)

test (https://github.com/dart-lang/test/compare/9a2d155..22835e2):
  22835e2e  2024-09-13  Nate Bosch  Add support for multiple full paths on macos (dart-lang/test#2276)

web (https://github.com/dart-lang/web/compare/933a37d..d8549a3):
  d8549a3  2024-09-13  Srujan Gaddam   Add a generate-all flag to emit all bindings (dart-lang/web#302)

Change-Id: Ib07e4c0eda9c8ea180ef989c7d9cf20c390457e8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/386260
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
mosuem pushed a commit to dart-lang/tools that referenced this pull request Dec 10, 2024
…fari (dart-lang/stack_trace#159)

Instead of updating existing regexes to handle new formats, this adds new
regexes for Wasm frames.

Parser functions are updated to try to parse the frame with Wasm regexes.

Column numbers can be used by the `source_map_stack_trace` package to map Wasm
frames to source locations.

Tests added for parsing all combinations of:

- Frames with and without function names
- In Chrome, Firefox, Safari formats
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants