Skip to content

Fix failure on getting a list with out of range offset #1947

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

Merged
merged 7 commits into from
Feb 10, 2023
Merged

Fix failure on getting a list with out of range offset #1947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3c0ffbb
Validate only needed summaries in expression_compiler_service
Jan 27, 2023
16a5596
Merge branch 'master' of github.com:dart-lang/webdev
Feb 1, 2023
c3a99a8
Merge branch 'master' of github.com:dart-lang/webdev
Feb 2, 2023
b7ca764
Merge branch 'master' of github.com:dart-lang/webdev
Feb 7, 2023
5427e42
Merge branch 'master' of github.com:dart-lang/webdev
Feb 9, 2023
e8276fe
Fix failure on getting a list with out of range offset
Feb 9, 2023
397419a
Optimize calls to chrome debugger if count is 0
Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dwds/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

- Cleanup `getObject` code for lists and maps.
- Now works with offset `0` and `null` count.
- Fix failures on edge cases.

## 17.0.0

Expand Down
10 changes: 10 additions & 0 deletions dwds/lib/src/debugging/debugger.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,13 @@ class Debugger extends Domain {
return null;
}

static bool _isEmptyRange({int? offset, int? count, int? length}) {
if (count == 0) return true;
if (length == null) return false;
if (offset == null) return false;
return offset >= length;
}

static bool _isSubRange({int? offset, int? count, int? length}) {
if (length == null) return false;
if (offset == 0 && count == null) return false;
Expand Down Expand Up @@ -495,6 +502,9 @@ class Debugger extends Domain {
int? length,
}) async {
String rangeId = objectId;
if (_isEmptyRange(offset: offset, count: count, length: length)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[Optional] Conceptually I think an empty range is a type of subrange. Would it make sense to have isSubRange return true if it's an empty range, and _subRange return []?

Copy link
Contributor Author

@annagrin annagrin Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

_subrange does not return a list but a remote object that refers to the sublist. I want to avoid unnecessary 2 roundtrips through the chrome debugger to get the empty list properties when we have all the information available statically.
Alternatively, we could make _subRange return null and test for it and return an empty list as a result but I think the current way is clearer...
Basically, It is a bug fix (for when offset >= length) but also is just an optimization for all empty cases we can detect without the runtime information.

Copy link
Contributor

Choose a reason for hiding this comment

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

SG

return [];
}
if (_isSubRange(offset: offset, count: count, length: length)) {
final range = await _subRange(objectId,
offset: offset ?? 0, count: count, length: length!);
Expand Down
64 changes: 64 additions & 0 deletions dwds/test/chrome_proxy_service_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,38 @@ void main() {
expect(only.valueAsString, '5');
});

test('Lists are truncated to empty if offset runs off the end',
() async {
final list = await createList();
final inst = await service.getObject(
isolate.id!,
list.objectId!,
count: 5,
offset: 1002,
) as Instance;
expect(inst.elements!.length, 0);
expect(inst.length, 1001);
expect(inst.offset, 1002);
expect(inst.count, 0);
expect(inst.elements!.length, 0);
});

test('Lists are truncated to empty with 0 count and null offset',
() async {
final list = await createList();
final inst = await service.getObject(
isolate.id!,
list.objectId!,
count: 0,
offset: null,
) as Instance;
expect(inst.elements!.length, 0);
expect(inst.length, 1001);
expect(inst.offset, null);
expect(inst.count, 0);
expect(inst.elements!.length, 0);
});

test('Maps with null offset/count are not truncated', () async {
final map = await createMap();
final inst = await service.getObject(
Expand Down Expand Up @@ -823,6 +855,38 @@ void main() {
expect(world.offset, 1);
});

test('Maps are truncated to empty if offset runs off the end',
() async {
final list = await createMap();
final inst = await service.getObject(
isolate.id!,
list.objectId!,
count: 5,
offset: 1002,
) as Instance;
expect(inst.associations!.length, 0);
expect(inst.length, 1001);
expect(inst.offset, 1002);
expect(inst.count, 0);
expect(inst.associations!.length, 0);
});

test('Maps are truncated to empty with 0 count and null offset',
() async {
final list = await createMap();
final inst = await service.getObject(
isolate.id!,
list.objectId!,
count: 0,
offset: null,
) as Instance;
expect(inst.associations!.length, 0);
expect(inst.length, 1001);
expect(inst.offset, null);
expect(inst.count, 0);
expect(inst.associations!.length, 0);
});

test(
'Strings are truncated to the end if offset/count runs off the end',
() async {
Expand Down