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.

Sign up for GitHub

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

Jump to bottom

VMScript URI does not uniquely identify a script #194

Closed
cbracken opened this issue Oct 3, 2017 · 2 comments
Closed

VMScript URI does not uniquely identify a script #194

cbracken opened this issue Oct 3, 2017 · 2 comments
Assignees
@cbracken
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@cbracken
Copy link
Member

cbracken commented Oct 3, 2017

In at least one place in the code, we use VMScript.uri as a proxy for the ID under the assumption that they map 1:1.

This is not a safe assumption.
@cbracken cbracken added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Oct 3, 2017
@cbracken cbracken self-assigned this Oct 3, 2017
@cbracken
Copy link
Member Author

cbracken commented Oct 3, 2017

@rmacnak-google @jason-simmons

whesse pushed a commit to dart-lang/sdk that referenced this issue Oct 5, 2017
@aam
Prefix scripts from core builtin libraries so the URIs are unique.
d822bdb 
After https://dart-review.googlesource.com/c/sdk/+/7588 we ended up with two crypto.dart files in the core prebuilt libraries(io and _http), which caused problem with coverage tool that relied on the fact that script uris can uniquely identify files. dart-lang/coverage#194 was filed to handle non-unique uris.
This CL ensures that core builtin libraries URIs stays unique by prefixing them with 'dart:' library prefix. This makes core builtin libraries scripts have names similarly structured to script names for other core libraries.

Bug:
Change-Id: I79960f4f24e6e958836df866365355584c28df27
Reviewed-on: https://dart-review.googlesource.com/11140
Commit-Queue: Alexander Aprelev <aam@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
@amirh amirh mentioned this issue Jan 5, 2018
Merged
amirh added a commit to flutter/flutter that referenced this issue Jan 5, 2018
@amirh
Don't use flutter_test in animated_icons_private_test. (#13917)
9ed652d 
This broke the coverage tool, as material/animated_icons/animated_icons.dart
was loaded twice as a part, once directly animated_icons_private_test, and one
through animated_icons_private_test->flutter_tester->...->material

The coverage package assumes a 1:1 mapping between VM scripts and URIs due to a
limitation in the underlying vm_service_client package, which currently doesn't
provide a unique identifier for VM scripts.

The underlying issue is tracked by dart-lang/coverage#194.
@amirh
Copy link

amirh commented Jan 5, 2018

This was failing the coverage tool when running on animated_icons_private_test, stack trace was:

unhandled error during test:
/b/build/slave/Linux/build/flutter/packages/flutter/test/material/animated_icons_private_test.dart
Bad state: Couldn't find line and column for token 246 in package:flutter/src/material/animated_icons/animated_icons.dart.
05:45 +860 ~2 -1: loading /b/build/slave/Linux/build/flutter/packages/flutter/test/material/animated_icons_private_test.dart [E]
  Bad state: Couldn't find line and column for token 246 in package:flutter/src/material/animated_icons/animated_icons.dart.
  package:flutter_tools/src/test/flutter_platform.dart 399   _FlutterPlatform._startTest
  ===== asynchronous gap ===========================
  dart:async/future_impl.dart 22                             _Completer.completeError
  package:flutter_tools/src/test/flutter_platform.dart 403   _FlutterPlatform._startTest
  ===== asynchronous gap ===========================
  dart:async/zone.dart 1045                                  _CustomZone.registerUnaryCallback
  dart:async-patch/async_patch.dart 30                       _asyncThenWrapperHelper
  package:flutter_tools/src/test/flutter_platform.dart 141   _FlutterPlatform._startTest
  package:flutter_tools/src/test/flutter_platform.dart 134   _FlutterPlatform.loadChannel
  package:test/src/runner/plugin/platform.dart 62            PlatformPlugin.load
  ===== asynchronous gap ===========================
  dart:async/zone.dart 1034                                  _CustomZone.registerCallback
  dart:async/zone.dart 924                                   _CustomZone.bindCallback
  dart:async/schedule_microtask.dart 148                     scheduleMicrotask
  dart:async/future.dart 198                                 new Future.microtask
  package:test/src/runner/plugin/platform.dart 59            PlatformPlugin.load
  package:test/src/runner/loader.dart 253                    Loader.loadFile.<fn>
  ===== asynchronous gap ===========================
  dart:async/zone.dart 1045                                  _CustomZone.registerUnaryCallback
  dart:async-patch/async_patch.dart 30                       _asyncThenWrapperHelper
  package:test/src/runner/loader.dart 247                    Loader.loadFile.<fn>
  package:test/src/runner/load_suite.dart 79                 new LoadSuite.<fn>.<fn>
  ===== asynchronous gap ===========================
  dart:async/zone.dart 1034                                  _CustomZone.registerCallback
  dart:async/zone.dart 924                                   _CustomZone.bindCallback
  dart:async/schedule_microtask.dart 148                     scheduleMicrotask
  dart:async/future.dart 198                                 new Future.microtask
  package:test/src/runner/load_suite.dart 77                 new LoadSuite.<fn>.<fn>
  package:test/src/utils.dart 329                            invoke
  package:test/src/runner/load_suite.dart 77                 new LoadSuite.<fn>
  package:test/src/backend/invoker.dart 351                  Invoker._onRun.<fn>.<fn>.<fn>
  ===== asynchronous gap ===========================
  dart:async/zone.dart 1034                                  _CustomZone.registerCallback
  dart:async/zone.dart 924                                   _CustomZone.bindCallback
  dart:async/schedule_microtask.dart 148                     scheduleMicrotask
  dart:async/future.dart 198                                 new Future.microtask
  package:test/src/backend/invoker.dart 350                  Invoker._onRun.<fn>.<fn>.<fn>
  dart:async/future.dart 174                                 new Future.<fn>
  package:stack_trace/src/stack_zone_specification.dart 209  StackZoneSpecification._run
  package:stack_trace/src/stack_zone_specification.dart 119  StackZoneSpecification._registerCallback.<fn>
  dart:async/zone.dart 1116                                  _rootRun
  dart:async/zone.dart 1001                                  _CustomZone.run
  dart:async/zone.dart 901                                   _CustomZone.runGuarded
  dart:async/zone.dart 926                                   _CustomZone.bindCallback.<fn>
  package:stack_trace/src/stack_zone_specification.dart 209  StackZoneSpecification._run
  package:stack_trace/src/stack_zone_specification.dart 119  StackZoneSpecification._registerCallback.<fn>
  dart:async/zone.dart 1120                                  _rootRun
  dart:async/zone.dart 1001                                  _CustomZone.run
  dart:async/zone.dart 901                                   _CustomZone.runGuarded
  dart:async/zone.dart 926                                   _CustomZone.bindCallback.<fn>
  dart:async-patch/timer_patch.dart 21                       Timer._createTimer.<fn>
  dart:isolate-patch/timer_impl.dart 366                     _Timer._runTimers
  dart:isolate-patch/timer_impl.dart 394                     _Timer._handleMessage
  dart:isolate-patch/isolate_patch.dart 151                  _RawReceivePortImpl._handleMessage
  ===== asynchronous gap ===========================
  dart:async/zone.dart 1034                                  _CustomZone.registerCallback
  dart:async/zone.dart 924                                   _CustomZone.bindCallback
  dart:async/timer.dart 52                                   new Timer
  dart:async/timer.dart 90                                   Timer.run
  dart:async/future.dart 172                                 new Future
  package:test/src/backend/invoker.dart 350                  Invoker._onRun.<fn>.<fn>
  ===== asynchronous gap ===========================
  dart:async/zone.dart 1034                                  _CustomZone.registerCallback
  dart:async/zone.dart 924                                   _CustomZone.bindCallback
  dart:async/schedule_microtask.dart 148                     scheduleMicrotask
  dart:async/future.dart 198                                 new Future.microtask
  package:test/src/backend/invoker.dart 337                  Invoker._onRun.<fn>.<fn>
  dart:async/zone.dart 1120                                  _rootRun
  dart:async/zone.dart 1001                                  _CustomZone.run
  dart:async/zone.dart 1467                                  runZoned
  package:test/src/backend/invoker.dart 337                  Invoker._onRun.<fn>
  package:stack_trace/src/chain.dart 101                     Chain.capture.<fn>
  dart:async/zone.dart 1120                                  _rootRun
  dart:async/zone.dart 1001                                  _CustomZone.run
  dart:async/zone.dart 1467                                  runZoned
  package:stack_trace/src/chain.dart 99                      Chain.capture
  package:test/src/backend/invoker.dart 336                  Invoker._onRun
  package:test/src/backend/live_test_controller.dart 188     LiveTestController._run
  package:test/src/backend/live_test_controller.dart 40      _LiveTest.run
  dart:async/future.dart 200                                 new Future.microtask.<fn>
  dart:async/zone.dart 1120                                  _rootRun
  dart:async/zone.dart 1001                                  _CustomZone.run
  dart:async/zone.dart 928                                   _CustomZone.bindCallback.<fn>
  dart:async/schedule_microtask.dart 41                      _microtaskLoop
  dart:async/schedule_microtask.dart 50                      _startMicrotaskLoop
  dart:isolate-patch/isolate_patch.dart 99                   _runPendingImmediateCallback
  dart:isolate-patch/isolate_patch.dart 152                  _RawReceivePortImpl._handleMessage

cbracken added a commit to cbracken/coverage that referenced this issue Jan 5, 2018
@cbracken
Use VMScriptRef to identify VM scripts, not URI
b029e34 
Previously, coverage collection assumed that a script could be uniquely
identified by URI, which is not a valid assumption. For example, when a
part is loaded via two libraries, both of which are loaded by an
isolate, the VM will track these are two scripts that map to the same
URI.

During collection, we now track each script by its (unique)
VMScriptRef. This ensures we lookup the correct script when computing
the affected line for each hit token. The hitmap remains URI based,
since in the end, we want a single, unified set of line->hitCount
mappings per script.

Fixes dart-lang#194
@cbracken cbracken mentioned this issue Jan 5, 2018
Merged
@amirh amirh mentioned this issue Jan 5, 2018
Closed
DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this issue May 14, 2018
@amirh
Don't use flutter_test in animated_icons_private_test. (flutter#13917)
d7a2f5f 
This broke the coverage tool, as material/animated_icons/animated_icons.dart
was loaded twice as a part, once directly animated_icons_private_test, and one
through animated_icons_private_test->flutter_tester->...->material

The coverage package assumes a 1:1 mapping between VM scripts and URIs due to a
limitation in the underlying vm_service_client package, which currently doesn't
provide a unique identifier for VM scripts.

The underlying issue is tracked by dart-lang/coverage#194.
@cbracken cbracken mentioned this issue Oct 18, 2018
Closed
cbracken added a commit to cbracken/coverage that referenced this issue Oct 19, 2018
@cbracken
Use VMScriptRef to identify VM scripts, not URI
2423f86 
Previously, coverage collection assumed that a script could be uniquely
identified by URI, which is not a valid assumption. For example, when a
part is loaded via two libraries, both of which are loaded by an
isolate, the VM will track these are two scripts that map to the same
URI.

During collection, we now track each script by its (unique)
VMScriptRef. This ensures we lookup the correct script when computing
the affected line for each hit token. The hitmap remains URI based,
since in the end, we want a single, unified set of line->hitCount
mappings per script.

Fixes dart-lang#194
cbracken added a commit that referenced this issue Oct 19, 2018
@cbracken
Use VMScriptRef to identify VM scripts, not URI (#216)
8ca3f6e 
Previously, coverage collection assumed that a script could be uniquely
identified by URI, which is not a valid assumption. For example, when a
part is loaded via two libraries, both of which are loaded by an
isolate, the VM will track these are two scripts that map to the same
URI.

During collection, we now track each script by its (unique)
VMScriptRef. This ensures we lookup the correct script when computing
the affected line for each hit token. The hitmap remains URI based,
since in the end, we want a single, unified set of line->hitCount
mappings per script.

Fixes #194
@cbracken cbracken mentioned this issue Oct 19, 2018
Merged
cbracken added a commit to flutter/flutter that referenced this issue Oct 19, 2018
@cbracken
Update coverage, linter to latest releases (#23304)
8007e1b 
Coverage 0.12.3 includes a fix for dart-lang/coverage#194, which was
causing errors on the flutter build bots.

Linter was updated automatically as a side effect of running
`flutter update-packages --force-upgrade`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cbracken cbracken

Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@cbracken @amirh