Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Support empty strings and vectors in standard codec #12974

Merged
franciscojma86 merged 4 commits into from
Oct 15, 2019
Merged

Support empty strings and vectors in standard codec #12974

franciscojma86 merged 4 commits into from
Oct 15, 2019

Conversation

@rokob
Copy link
Contributor

@rokob rokob commented Oct 7, 2019

Fixes #41993

Currently an empty string or vector will call through to WriteBytes
which asserts that the number of bytes it is being asked to write is
strictly positive. Instead we should not call WriteBytes if the length
is zero.

Similarly, when we read, we don't need to call out if the length is
zero.

@rokob
Support empty strings and vectors in standard codec
2f174c9 
Fixes flutter#41993

Currently an empty string or vector will call through to WriteBytes
which asserts that the number of bytes it is being asked to write is
strictly positive. Instead we should not call WriteBytes if the length
is zero.

Similarly, when we read, we don't need to call out if the length is
zero.
franciscojma86
franciscojma86 reviewed Oct 7, 2019
View reviewed changes
shell/platform/common/cpp/client_wrapper/standard_codec.cc
if (length == 0) {
return EncodableValue(list_value);
}
list_value.reserve(length);
Copy link
Contributor

@franciscojma86 franciscojma86 Oct 7, 2019

Choose a reason for hiding this comment

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

Was this causing a failure too? I think reserve(0) is just a no-op, and the for loop would be no-op as well.

Copy link
Contributor Author

@rokob rokob Oct 7, 2019

Choose a reason for hiding this comment

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

Good point, I was just being over cautious.

shell/platform/common/cpp/client_wrapper/standard_codec.cc
case EncodableValue::Type::kString: {
const auto& string_value = value.StringValue();
size_t size = string_value.size();
WriteSize(size, stream);
Copy link
Contributor

@franciscojma86 franciscojma86 Oct 7, 2019

Choose a reason for hiding this comment

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

Should this be included in the if statement below?

Copy link
Contributor Author

@rokob rokob Oct 7, 2019

Choose a reason for hiding this comment

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

No we always want to write the size because the format specifies type|size|contents and the reading code needs to read that zero to be able to decode an empty string.

Copy link
Contributor

@franciscojma86 franciscojma86 Oct 7, 2019

Choose a reason for hiding this comment

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

makes sense, thanks!

@rokob rokob mentioned this pull request Oct 7, 2019
Closed
@rokob
Copy link
Contributor Author

rokob commented Oct 14, 2019

Any update on when this will get merged?

@franciscojma86 franciscojma86 merged commit 81110bb into flutter:master Oct 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 16, 2019
@engine-flutter-autoroll
81110bb Support empty strings and vectors in standard codec (flutter/…
89f3a1d 
…engine#12974)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 16, 2019
Merged
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Oct 16, 2019
@engine-flutter-autoroll
Roll engine 540fc97..5e6c005 (33 commits) (#42853)
1dc09e4 
git@github.com:flutter/engine.git/compare/540fc977bb6b...5e6c005

git log 540fc97..5e6c005 --no-merges --oneline
2019-10-16 skia-flutter-autoroll@skia.org Roll src/third_party/skia 083a75d6762c..59e72b71b5cf (1 commits) (flutter/engine#13169)
2019-10-16 a-siva@users.noreply.github.com Roll src/third_party/dart 4131d3d7c4...41b65b27c2 (28 commits) (flutter/engine#13163)
2019-10-16 skia-flutter-autoroll@skia.org Roll src/third_party/skia 7274850f96f2..083a75d6762c (1 commits) (flutter/engine#13168)
2019-10-16 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from 5I4Iw... to oTVah... (flutter/engine#13167)
2019-10-16 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from qpzUe... to KVDL4... (flutter/engine#13166)
2019-10-16 skia-flutter-autoroll@skia.org Roll src/third_party/skia 634d15032d37..7274850f96f2 (3 commits) (flutter/engine#13165)
2019-10-16 yjbanov@google.com Move surface-based SceneBuilder implementation under surface/ (flutter/engine#13159)
2019-10-16 skia-flutter-autoroll@skia.org Roll src/third_party/skia ba8752f37dab..634d15032d37 (2 commits) (flutter/engine#13164)
2019-10-16 chinmaygarde@gmail.com Revert "Issue 13238: on iOS, force an orientation change when the current orientation is not allowed" (flutter/engine#13160)
2019-10-15 chinmaygarde@google.com Roll buildroot to pull in static thread safety analysis options. (flutter/engine#13155)
2019-10-15 skia-flutter-autoroll@skia.org Roll src/third_party/skia fb6a1abe4567..ba8752f37dab (8 commits) (flutter/engine#13156)
2019-10-15 chinmaygarde@google.com Make the Dart isolate constructor private. (flutter/engine#13153)
2019-10-15 iska.kaushik@gmail.com Revert "Upgrades the ICU version to 64.2 (#13123)" (flutter/engine#13146)
2019-10-15 1541038+josh-ksr@users.noreply.github.com Issue 13238: on iOS, force an orientation change when the current orientation is not allowed (flutter/engine#12295)
2019-10-15 chinmaygarde@google.com Allow embedders to specify a render task runner description. (flutter/engine#13124)
2019-10-15 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from Jv4XM... to 5I4Iw... (flutter/engine#13150)
2019-10-15 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from i5xD1... to qpzUe... (flutter/engine#13149)
2019-10-15 bkonyi@google.com Roll src/third_party/dart fc933312f7..4131d3d7c4 (3 commits)
2019-10-15 chinmaygarde@google.com Document //flutter/runtime/dart_vm (flutter/engine#13144)
2019-10-15 iska.kaushik@gmail.com Revert "Enable/tweak web sdk source maps (#13141)" (flutter/engine#13148)
2019-10-15 jason-simmons@users.noreply.github.com Merge the Fuchsia frontend_server build script into the new flutter_frontend_server target (flutter/engine#13145)
2019-10-15 iska.kaushik@gmail.com Add `flutter_tester` binary to the CIPD package (flutter/engine#13143)
2019-10-15 skia-flutter-autoroll@skia.org Roll src/third_party/skia f22c57ddcc8c..fb6a1abe4567 (2 commits) (flutter/engine#13142)
2019-10-15 vsm@google.com Enable/tweak web sdk source maps (flutter/engine#13141)
2019-10-15 filmil@gmail.com Upgrades the ICU version to 64.2 (flutter/engine#13123)
2019-10-15 rmacnak@google.com [frontend_server] Include bytecode generation in the training run. (flutter/engine#13126)
2019-10-15 wvvwwvw@gmail.com Support empty strings and vectors in standard codec (flutter/engine#12974)
2019-10-15 bkonyi@google.com Roll src/third_party/dart 50f7ae9c5d..fc933312f7 (2 commits)
2019-10-15 skia-flutter-autoroll@skia.org Roll src/third_party/skia 55f9cba6e2e7..f22c57ddcc8c (1 commits) (flutter/engine#13136)
2019-10-15 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from xRgq0... to Jv4XM... (flutter/engine#13135)
2019-10-15 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from Lk7iT... to i5xD1... (flutter/engine#13134)
2019-10-15 skia-flutter-autoroll@skia.org Roll src/third_party/skia 858cf233ef71..55f9cba6e2e7 (3 commits) (flutter/engine#13133)
2019-10-15 bkonyi@google.com Roll src/third_party/dart 70a7ef3f58..50f7ae9c5d (18 commits)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC franciscojma@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
...
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@engine-flutter-autoroll @Inconnu08
Roll engine 540fc97..5e6c005 (33 commits) (flutter#42853)
a686e4b 
git@github.com:flutter/engine.git/compare/540fc977bb6b...5e6c005

git log 540fc97..5e6c005 --no-merges --oneline
2019-10-16 skia-flutter-autoroll@skia.org Roll src/third_party/skia 083a75d6762c..59e72b71b5cf (1 commits) (flutter/engine#13169)
2019-10-16 a-siva@users.noreply.github.com Roll src/third_party/dart 4131d3d7c4...41b65b27c2 (28 commits) (flutter/engine#13163)
2019-10-16 skia-flutter-autoroll@skia.org Roll src/third_party/skia 7274850f96f2..083a75d6762c (1 commits) (flutter/engine#13168)
2019-10-16 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from 5I4Iw... to oTVah... (flutter/engine#13167)
2019-10-16 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from qpzUe... to KVDL4... (flutter/engine#13166)
2019-10-16 skia-flutter-autoroll@skia.org Roll src/third_party/skia 634d15032d37..7274850f96f2 (3 commits) (flutter/engine#13165)
2019-10-16 yjbanov@google.com Move surface-based SceneBuilder implementation under surface/ (flutter/engine#13159)
2019-10-16 skia-flutter-autoroll@skia.org Roll src/third_party/skia ba8752f37dab..634d15032d37 (2 commits) (flutter/engine#13164)
2019-10-16 chinmaygarde@gmail.com Revert "Issue 13238: on iOS, force an orientation change when the current orientation is not allowed" (flutter/engine#13160)
2019-10-15 chinmaygarde@google.com Roll buildroot to pull in static thread safety analysis options. (flutter/engine#13155)
2019-10-15 skia-flutter-autoroll@skia.org Roll src/third_party/skia fb6a1abe4567..ba8752f37dab (8 commits) (flutter/engine#13156)
2019-10-15 chinmaygarde@google.com Make the Dart isolate constructor private. (flutter/engine#13153)
2019-10-15 iska.kaushik@gmail.com Revert "Upgrades the ICU version to 64.2 (flutter#13123)" (flutter/engine#13146)
2019-10-15 1541038+josh-ksr@users.noreply.github.com Issue 13238: on iOS, force an orientation change when the current orientation is not allowed (flutter/engine#12295)
2019-10-15 chinmaygarde@google.com Allow embedders to specify a render task runner description. (flutter/engine#13124)
2019-10-15 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from Jv4XM... to 5I4Iw... (flutter/engine#13150)
2019-10-15 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from i5xD1... to qpzUe... (flutter/engine#13149)
2019-10-15 bkonyi@google.com Roll src/third_party/dart fc933312f7..4131d3d7c4 (3 commits)
2019-10-15 chinmaygarde@google.com Document //flutter/runtime/dart_vm (flutter/engine#13144)
2019-10-15 iska.kaushik@gmail.com Revert "Enable/tweak web sdk source maps (flutter#13141)" (flutter/engine#13148)
2019-10-15 jason-simmons@users.noreply.github.com Merge the Fuchsia frontend_server build script into the new flutter_frontend_server target (flutter/engine#13145)
2019-10-15 iska.kaushik@gmail.com Add `flutter_tester` binary to the CIPD package (flutter/engine#13143)
2019-10-15 skia-flutter-autoroll@skia.org Roll src/third_party/skia f22c57ddcc8c..fb6a1abe4567 (2 commits) (flutter/engine#13142)
2019-10-15 vsm@google.com Enable/tweak web sdk source maps (flutter/engine#13141)
2019-10-15 filmil@gmail.com Upgrades the ICU version to 64.2 (flutter/engine#13123)
2019-10-15 rmacnak@google.com [frontend_server] Include bytecode generation in the training run. (flutter/engine#13126)
2019-10-15 wvvwwvw@gmail.com Support empty strings and vectors in standard codec (flutter/engine#12974)
2019-10-15 bkonyi@google.com Roll src/third_party/dart 50f7ae9c5d..fc933312f7 (2 commits)
2019-10-15 skia-flutter-autoroll@skia.org Roll src/third_party/skia 55f9cba6e2e7..f22c57ddcc8c (1 commits) (flutter/engine#13136)
2019-10-15 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from xRgq0... to Jv4XM... (flutter/engine#13135)
2019-10-15 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from Lk7iT... to i5xD1... (flutter/engine#13134)
2019-10-15 skia-flutter-autoroll@skia.org Roll src/third_party/skia 858cf233ef71..55f9cba6e2e7 (3 commits) (flutter/engine#13133)
2019-10-15 bkonyi@google.com Roll src/third_party/dart 70a7ef3f58..50f7ae9c5d (18 commits)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC franciscojma@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@franciscojma86 franciscojma86 franciscojma86 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rokob @franciscojma86 @googlebot