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

[CP] [Stable] Cherry pick request for 0a20707e3f1084b85bcdcfb4035bdd150b90756f #54059

Closed
a-siva opened this issue Nov 15, 2023 · 4 comments
Closed

[CP] [Stable] Cherry pick request for 0a20707e3f1084b85bcdcfb4035bdd150b90756f #54059

a-siva opened this issue Nov 15, 2023 · 4 comments
Assignees
@vsmenon @whesse @itsjustkevin @mit-mit @athomas
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable

Comments

@a-siva
Copy link
Contributor

a-siva commented Nov 15, 2023

Commit(s) to merge

0a20707

Target

stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/336603

Issue Description

Fix JSON array parsing bug that causes seg fault when --coverage is used.

What is the fix

The fix changes the definition of end to point 1 past the end of the element, rather than the end of the element, which avoids the need for buggy look-ahead style lookups

Why cherry-pick

Flutter customers have hit this issue when using test coverage, the sympton of this bug is a hang in flutter test making it hard for the developers to figure out what went wrong.

Risk

low

Issue link(s)

flutter/flutter#124145

Extra Info

No response
@a-siva a-siva added the cherry-pick-review Issue that need cherry pick triage to approve label Nov 15, 2023
@vsmenon
Copy link
Member

vsmenon commented Nov 16, 2023

lgtm

@itsjustkevin
Copy link
Contributor

Approved. @a-siva please address the issues called out by @sortie in the CL, then we can merge.

@itsjustkevin itsjustkevin added merge-to-stable cherry-pick-approved Label for approved cherrypick request labels Nov 16, 2023
@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 Nov 17, 2023
@a-siva
Copy link
Contributor Author

a-siva commented Nov 17, 2023

Approved. @a-siva please address the issues called out by @sortie in the CL, then we can merge.

Addressed.

copybara-service bot pushed a commit that referenced this issue Nov 20, 2023
@a-siva
[stable] [vm/service] Fix JSON array parsing bug that causes seg fault
09b3b74 
Changed the definition of `end` to point 1 past the end of the element,
rather than the end of the element, which avoids the need for buggy
look-ahead style lookups.

I think this must have been a long standing bug, and we just never sent
empty JSON arrays to the service until now, because I didn't change this
logic when I refactored it.

TEST=Added Service_ParseJSONArray

Bug: flutter/flutter#124145 #53990
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/335980
Cherry-pick-request: #54059
Change-Id: I995e136881d08627cc6a7f9e60a57e76db55024c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/336603
Reviewed-by: Liam Appelbe <liama@google.com>
Commit-Queue: Siva Annamalai <asiva@google.com>
@athomas athomas closed this as completed Nov 22, 2023
@athomas
Copy link
Member

athomas commented Nov 22, 2023

Released in 3.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vsmenon vsmenon

@whesse whesse

@itsjustkevin itsjustkevin

@mit-mit mit-mit

@athomas athomas

Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@vsmenon @whesse @a-siva @itsjustkevin @mit-mit @athomas