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

Taking snapshot fails with RangeError: Offset (1163457) must be a multiple of BYTES_PER_ELEMENT #55475

Closed
polina-c opened this issue Apr 15, 2024 · 41 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. cherry-pick-candidate Candidates to be cherry-picked P0 A serious issue requiring immediate resolution pkg-vm-service triaged Issue has been triaged by sub team

Comments

@polina-c
Copy link
Contributor

Tried for macos and for android emulator. I have mac arm64.

[ERROR]: [PlatformDispatcher]: RangeError: Offset (1163457) must be a multiple of BYTES_PER_ELEMENT (2)
[ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: RangeError: Offset (1163457) must be a multiple of BYTES_PER_ELEMENT (2)
#0      _offsetAlignmentCheck (dart:typed_data-patch/typed_data_patch.dart:5124:5)
#1      _ByteBuffer.asUint16List (dart:typed_data-patch/typed_data_patch.dart:1952:5)
#2      new Uint16List.sublistView (dart:typed_data:1300:24)
#3      ReadStream.readUtf16 (package:vm_service/src/_stream_helpers.dart:311:30)
#4      _getNonReferenceData (package:vm_service/src/snapshot_graph.dart:629:26)
#5      new HeapSnapshotObject._read (package:vm_service/src/snapshot_graph.dart:200:18)
#6      HeapSnapshotGraph._readObjects (package:vm_service/src/snapshot_graph.dart:462:39)
#7      new HeapSnapshotGraph.fromChunks (package:vm_service/src/snapshot_graph.dart:400:5)
#8      HeapSnapshotGraph.getSnapshot.<anonymous closure> (package:vm_service/src/snapshot_graph.dart:352:46)
<asynchronous suspension>
Flutter 3.22.0-11.0.pre.9 • channel [user-branch] • unknown source
Framework • revision 8c12ba8470 (33 minutes ago) • 2024-04-15 16:09:07 -0400
Engine • revision 07ae93c9b7
Tools • Dart 3.5.0 (build 3.5.0-56.0.dev) • DevTools 2.35.0-dev.8
@polina-c
Copy link
Contributor Author

polina-c commented Apr 15, 2024

Repro:

  1. Build DevTools: https://github.com/flutter/devtools/blob/master/BETA_TESTING.md
  2. Connect to counter app on macos or android emulator
  3. Open Memory > Diff
  4. Click recording:
Screenshot 2024-04-15 at 2 21 34 PM
  1. Find error printed on DevTools console

@polina-c
Copy link
Contributor Author

@bkonyi @derekxu16 @a-siva

@polina-c polina-c added the P0 A serious issue requiring immediate resolution label Apr 15, 2024
@a-siva a-siva added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Apr 15, 2024
@a-siva a-siva added triaged Issue has been triaged by sub team pkg-vm-service labels Apr 15, 2024
@bkonyi
Copy link
Contributor

bkonyi commented Apr 16, 2024

I'm not able to reproduce on Flutter master and the latest DevTools:

Flutter 3.22.0-11.0.pre.18 • channel main • git@github.com:flutter/flutter.git
Framework • revision cb719db814 (65 minutes ago) • 2024-04-16 09:40:19 -0400
Engine • revision 71ab8854f1
Tools • Dart 3.5.0 (build 3.5.0-65.0.dev) • DevTools 2.35.0-dev.8

I have a x64 Mac, so maybe that has something to do with it. @derekxu16, can you give this a try?

@lrhn
Copy link
Member

lrhn commented Apr 16, 2024

The error should not depend on the CPU alignment, it's an intrinsic requirement of the typed-data classes that they start at the correct place in a buffer.
I guess OS differences could affect the incoming data being parsed, or the chunking of it.

The current version of readUtf16 does seem to check for alignment, but it relies on the original incoming buffer to start at an even offset.

The line

    if (_currentChunkHasBytes(len * 2) && _byteIndex & 1 == 0) {

should be

    if (_currentChunkHasBytes(len * 2) && (_currentChunk.offsetInBytes + _byteIndex) & 1 == 0) {

@osa1 Seems to be your code from https://dart-review.googlesource.com/c/sdk/+/340182

@polina-c polina-c changed the title Taking screenshot fails with RangeError: Offset (1163457) must be a multiple of BYTES_PER_ELEMENT Taking snapshot fails with RangeError: Offset (1163457) must be a multiple of BYTES_PER_ELEMENT Apr 16, 2024
@polina-c
Copy link
Contributor Author

polina-c commented Apr 16, 2024

Thank you!

If it helps, this is exact project for repro: https://github.com/polina-c/spikes/tree/master/counter

It is a modified counter. On each click it aggressively allocates memory into array, and there is additional button to clear the array.
But I repro the issue without any clicks, for just started application.

@osa1
Copy link
Member

osa1 commented Apr 16, 2024

Thanks for the ping. @lrhn found the bug, we should just make the change @lrhn shows. Let me know if you want me to do it.

@derekxu16
Copy link
Member

I was trying to reproduce the problem first to check if the patch fixes the issue, but I'm also unable to reproduce it. I'm making a CL with the patch now.

@polina-c
Copy link
Contributor Author

Thank you @derekxu16!
And then we want to make sure the issue does not squeeze to flutter stable...

copybara-service bot pushed a commit that referenced this issue Apr 16, 2024
Issue: #55475
Change-Id: Ib7b09e21096dd76cee5037c3120a7944a2b85340
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/363120
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Derek Xu <derekx@google.com>
@a-siva
Copy link
Contributor

a-siva commented Apr 16, 2024

Thank you @derekxu16! And then we want to make sure the issue does not squeeze to flutter stable...

Looks like the change made it to the beta branch and so will end up in stable, @derekxu16 we will probably need a cherry pick of your change

@polina-c
Copy link
Contributor Author

Linking fix here: https://dart-review.googlesource.com/c/sdk/+/363120

@a-siva a-siva added the cherry-pick-candidate Candidates to be cherry-picked label Apr 16, 2024
@polina-c polina-c reopened this Apr 16, 2024
@polina-c
Copy link
Contributor Author

The failing snapshot, after fromChanks with applied patch and then saved with toChanks.
We can try to use it to test cover the fix, if the issue reproduces with it:
main-1_2024-04-16_12:03:07.927.data.zip

@polina-c
Copy link
Contributor Author

polina-c commented Apr 16, 2024

14.2.1 of vm_service contains the fix
Preparing upgrade of DevTools

@kenzieschmoll , do we need to cherry pick the upgrade to make sure DevTools with the issue does not squeeze into Flutter master?
Or 14.2.1 will be auto picked up by DevTools, because vm_service is cherry picking the change?
If we need cherry pick, should it happen after vm service cherry pick?

@polina-c
Copy link
Contributor Author

Upgrade for DevTools: flutter/devtools#7597

@kenzieschmoll
Copy link
Contributor

What version of Flutter did this first start on? If this breakage is on the beta channel, then we need to cherry-pick the fix to beta. If the breakage is only on master, then no need for a cherry pick.

@polina-c
Copy link
Contributor Author

polina-c commented Apr 16, 2024

It is introduced by this PR: https://dart-review.googlesource.com/c/sdk/+/340182
@derekxu16 is cherry picking Flutter, because the issue is already on beta
Not sure how to detect exact version of flutter.

auto-submit bot pushed a commit to flutter/devtools that referenced this issue Apr 16, 2024
@kenzieschmoll
Copy link
Contributor

If DevTools fails on beta even with @derekxu16's cherry pick, then yes we will need a DevTools cherry pick (2.34.3) into the Dart SDK as well, and then into Flutter. @polina-c or @derekxu16 can you drive this?

@derekxu16
Copy link
Member

The fix is package:vm_service and not in the Dart SDK, and Flutter has an exact pin of package:vm_service, so cherry-picking the fix into Flutter beta would mean updating the version of package:vm_service pinned on the beta branch. Has something like this ever been done, @christopherfujino?

@derekxu16
Copy link
Member

Oh, I see flutter/flutter#125231. So when package:vm_service 14.2.1 rolls into Flutter, the commit performing the roll can just be cherry-picked? I was worried about some weirdness with the pubspec checksum.

copybara-service bot pushed a commit that referenced this issue Apr 17, 2024
TEST=confirmed that the added test case fails when undoing the changes
made in package:vm_service 14.2.1

Issue: #55475
Change-Id: I189db9dda4c730199157432522323e34a6f87c9c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/363441
Commit-Queue: Derek Xu <derekx@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
auto-submit bot pushed a commit to flutter/flutter that referenced this issue Apr 17, 2024
@kenzieschmoll
Copy link
Contributor

@derekxu16 have you verified that DevTools is WAI with your flutter cherry pick included? The change @polina-c made to bump the DevTools dependency on vm_service to 14.2.1 is not included in the Dart or Flutter SDKs, and won't be in the build your cherry-pick makes it into.

@polina-c
Copy link
Contributor Author

polina-c commented Apr 18, 2024

What should happen first: cherry pick of fixed DevTools into Dart SDK or cherry pick of fixed vm_service into Dart SDK? Or it does not matter?

@derekxu16
Copy link
Member

The change @polina-c made to bump the DevTools dependency on vm_service to 14.2.1 is not included in the Dart or Flutter SDKs, and won't be in the build your cherry-pick makes it into.

Since there's a prebuilt copy of DevTools in the Dart SDK, you're right that a new build of DevTools built with package:vm_service 14.2.1 needs to be cherry-picked into the Dart SDK.

@polina-c
Copy link
Contributor Author

polina-c commented Apr 18, 2024

What should happen first: cherry pick of fixed DevTools into Dart SDK or cherry pick of fixed vm_service into Dart SDK? Or it does not matter?

We want to build fix for DevTools with already cherry picked SDK. So we need cherry pick of vm_service to land first.
Please, correct me if I am wrong.

@derekxu16
Copy link
Member

We want to build fix for DevTools with already cherry picked SDK. So we need cherry pick of vm_service to land first. Please, correct me if I am wrong.

DevTools needs to be built against a version of the Flutter SDK that accepts package:vm_service 14.2.1; the version of the Dart SDK that it's built against does not matter. I'm not sure which branch you want to use to build the new version of DevTools. FWIW, this PR has landed: flutter/flutter#146896.

@polina-c
Copy link
Contributor Author

started putting together DevTools cherry pick.

@christopherfujino
Copy link
Member

Since there's a prebuilt copy of DevTools in the Dart SDK, you're right that a new build of DevTools built with package:vm_service 14.2.1 needs to be cherry-picked into the Dart SDK.

I'm a little confused as to what needed to match here, and it sounds like we're now doing the right thing. However, once we get all the right CPs in, this feels like a tricky situation we could very well mess up next time.

Is the issue that flutter_tools and dev_tools need to have matching vm_service versions? Or just we want to ensure that none of the tools we're shipping in the Flutter SDK have the affected (14.2.0) vm_service version? If it's the former, we could add a flutter/flutter test to verify this. If it's the latter, I've been playing around with a tool to make inspecting the transitive deps of a Flutter SDK more easily inspectable, that would probably help in situations like this.

@polina-c
Copy link
Contributor Author

I think we just want every tool not to have 14.2.0.

@derekxu16
Copy link
Member

The prebuilt version of DevTools in the current Dart SDK beta candidate was built with package:vm_service 14.2.0. My understanding is that package:vm_service 14.2.1 had to be rolled into the Flutter SDK beta branch because a new build of DevTools will be built using the Flutter SDK beta branch, and this build will be cherry-picked into the Dart SDK beta branch.

@polina-c
Copy link
Contributor Author

Since there's a prebuilt copy of DevTools in the Dart SDK, you're right that a new build of DevTools built with package:vm_service 14.2.1 needs to be cherry-picked into the Dart SDK.

I'm a little confused as to what needed to match here, and it sounds like we're now doing the right thing. However, once we get all the right CPs in, this feels like a tricky situation we could very well mess up next time.

Is the issue that flutter_tools and dev_tools need to have matching vm_service versions? Or just we want to ensure that none of the tools we're shipping in the Flutter SDK have the affected (14.2.0) vm_service version? If it's the former, we could add a flutter/flutter test to verify this. If it's the latter, I've been playing around with a tool to make inspecting the transitive deps of a Flutter SDK more easily inspectable, that would probably help in situations like this.

Easy way to understand what is built with what would be very helpful!

@polina-c
Copy link
Contributor Author

Okay, I did git checkout upstream/beta in flutter to get branch i want to cherry pick. Then I did flutter --version and got:

Flutter 3.22.0-0.1.pre • channel [user-branch] • unknown source
Framework • revision 29babcb32a (2 weeks ago) • 2024-04-03 17:17:04 -0500
Engine • revision 97550907b7
Tools • Dart 3.4.0 (build 3.4.0-282.1.beta) • DevTools 2.34.2

This means I want to cherry pick DevTools with tag 2.34.2 to Dart sdk 3.4.0-282.1.beta.

Sounds correct?

polina-c added a commit to polina-c/devtools that referenced this issue Apr 18, 2024
@kenzieschmoll
Copy link
Contributor

The prebuilt version of DevTools in the current Dart SDK beta candidate was built with package:vm_service 14.2.0. My understanding is that package:vm_service 14.2.1 had to be rolled into the Flutter SDK beta branch because a new build of DevTools will be built using the Flutter SDK beta branch, and this build will be cherry-picked into the Dart SDK beta branch.

Has anyone manually reproduced the failure with DevTools from a Flutter beta branch with Derek's fix included (@polina-c or @derekxu16)? If the bug was caused because of the VM service logic that is running on the device, then it is possible that cherry-picking Derek's change into Flutter is enough. Based on flutter/devtools#7597, it doesn't look like there are any logic changes in DevTools that came with the vm_service version bump, which would make me think that it's possible no cherry-pick is needed for DevTools.

Someone should confirm manually that the issue still exists before we move forward with a DevTools cherry-pick.

@polina-c
Copy link
Contributor Author

Are you saying DevTools will pickup 14.2.1 from sdk, even if it was built with 14.2.0?

The issue is flaky, so even if it does not repro, it does not mean it is fixed.

How does it sound?

@kenzieschmoll
Copy link
Contributor

I'm saying that if the crash happened because of the VM service running on the device, then it is possible that Derek's fix may be compatible with the existing DevTools on beta, and there would be no need to rebuild DevTools with 14.2.1. If 14.2.1 has implications that would change DevTools logic, then we'd need to CP DevTools too, but looking at your DevTools PR, it was just a patch version changes with no other changes. Which makes me think that the existing DevTools on beta may be compatible with the vm_service changes that Derek CP'ed into Flutter.

@derekxu16
Copy link
Member

The error did not happen because of the VM Service running on the device, it happened when DevTools called getHeapSnapshotGraph here.

@polina-c
Copy link
Contributor Author

Yes, the fix is not in vm itself, it is in the code of vm_service: https://dart-review.googlesource.com/c/sdk/+/363120

@polina-c
Copy link
Contributor Author

polina-c commented Apr 19, 2024

Cherry pick is waiting for @kenzieschmoll to decide if we are doing it: https://dart-review.googlesource.com/c/sdk/+/363642

@kenzieschmoll
Copy link
Contributor

Based on the two previous comments [1] and [2], it sounds like we do need the cherry pick, yes.

polina-c added a commit to polina-c/devtools that referenced this issue Apr 22, 2024
polina-c added a commit to polina-c/devtools that referenced this issue Apr 22, 2024
polina-c added a commit to polina-c/devtools that referenced this issue Apr 22, 2024
copybara-service bot pushed a commit that referenced this issue Apr 23, 2024
The [Cherry-pick] link below is fake. There is no real link yet, because DevTools with fix will release to main at normal schedule.

Bug: #55475
Cherry-pick: flutter/devtools#7597, flutter/devtools#7633
Cherry-pick-request: #55509
Change-Id: I6f9a90f49d4efb3c65147f9132866c7c5432410a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/363642
Commit-Queue: Kevin Chisholm <kevinjchisholm@google.com>
Reviewed-by: Elliott Brooks <elliottbrooks@google.com>
Reviewed-by: Jacob Richman <jacobr@google.com>
@polina-c
Copy link
Contributor Author

Cherry pick is merged. Thanks all for help and catches.

It was a significant and risky effort to sync the fix between 4 products (vm_service, DevTools, DartSDK, Flutter).

IMHO it would be great to prioritize test coverage for the fix, to avoid repeating this.
See #55475 (comment)

@bkonyi, @derekxu16, @osa1, @a-siva

@a-siva
Copy link
Contributor

a-siva commented Apr 24, 2024

IMHO it would be great to prioritize test coverage for the fix, to avoid repeating this. See #55475 (comment)

I believe Derek has added a test for this at
https://dart-review.googlesource.com/c/sdk/+/363441

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. cherry-pick-candidate Candidates to be cherry-picked P0 A serious issue requiring immediate resolution pkg-vm-service triaged Issue has been triaged by sub team
Projects
None yet
Development

No branches or pull requests

8 participants