This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Add null check in FLETextInputPlugin#8538
Merged
cbracken merged 1 commit intoflutter:masterfrom Apr 13, 2019
cbracken:nil-check
Merged
Add null check in FLETextInputPlugin#8538cbracken merged 1 commit intoflutter:masterfrom cbracken:nil-check
cbracken merged 1 commit intoflutter:masterfrom
cbracken:nil-check
Conversation
Contributor
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
This patch works off a paranoid local copy to reduce the chance of future threading naughtiness.
Nothing about this class is threadsafe, and it would need a major overhaul if it ever needed to be. I'm not a fan of speculative bits of thread safety, since they can be confusing (and I would argue don't actually help; a method that's partially threadsafe is still just a method that isn't threadsafe).
Alternatively, we could just swap this out for an if (self.activeModel != nil) for consistency with other methods, for the sake of readability. The tradeoff against future paranoia is it looks like this is one of the few/only cases we'll actively blow up if this field is nil. Thoughts?
I think it's better to go with the consistency, only because the clientID and activeModel aspects of this class need to reworked (#30659), and I think having a consistent guard will make that easier to reason about during the update. After that rewrite, I would expect clientID to be an int rather than an NSNumber, which will eliminate the nil problem anyway.
Adds a guard on `_activeClientID` in `insertNewline:`. The conditional around the `insertText:replacementRange:` call already catches this, but we then unconditionally pack `_activeClientID` into an `NSArray`, which disallows nil. This patch works off a paranoid local copy. This isn't strictly required given that all async callbacks that modify `_activeClientID` occur over platform thread callbacks.
Member
Author
SGTM - done!
Even better :) |
stuartmorgan-g
approved these changes
Apr 13, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Apr 15, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Apr 15, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Apr 15, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Apr 15, 2019
aam
added a commit
to aam/flutter
that referenced
this pull request
Apr 15, 2019
git log c3824f0..571ce94 --no-merges --oneline 571ce94 Roll src/third_party/skia 1875353110d1..bf15b6676843 (8 commits) (flutter/engine#8582) b7d484e Roll src/third_party/skia 1fe0b86f17f3..1875353110d1 (5 commits) (flutter/engine#8580) 73f455a Roll src/third_party/skia e1c5ea6779f4..1fe0b86f17f3 (1 commits) (flutter/engine#8579) 42d06b3 Roll src/third_party/skia 3611ee1bb157..e1c5ea6779f4 (3 commits) (flutter/engine#8578) eb575e2 Roll src/third_party/skia c9f55de2ed39..3611ee1bb157 (1 commits) (flutter/engine#8577) 40473d3 Roll src/third_party/skia b5e57e9a3d0f..c9f55de2ed39 (1 commits) (flutter/engine#8576) dc952bc Roll src/third_party/skia 5c6b565bdfb9..b5e57e9a3d0f (1 commits) (flutter/engine#8575) 14a1db2 Roll src/third_party/skia 12cf258193dc..5c6b565bdfb9 (1 commits) (flutter/engine#8573) dcd0209 Roll src/third_party/skia 33233a09fef8..12cf258193dc (1 commits) (flutter/engine#8572) e0b9bc1 Roll src/third_party/skia 69f54f8f0f22..33233a09fef8 (1 commits) (flutter/engine#8571) 9f9d5d6 Roll src/third_party/skia 990bfc785891..69f54f8f0f22 (1 commits) (flutter/engine#8570) 7c7df0d Add null check in FLETextInputPlugin (flutter/engine#8538)
engine-flutter-autoroll
added a commit
to flutter/flutter
that referenced
this pull request
Apr 15, 2019
flutter/engine@c3824f0...324b840 git log c3824f0..324b840 --no-merges --oneline 324b840 Remove the flutter_aot GN argument. (flutter/engine#8581) 571ce94 Roll src/third_party/skia 1875353110d1..bf15b6676843 (8 commits) (flutter/engine#8582) b7d484e Roll src/third_party/skia 1fe0b86f17f3..1875353110d1 (5 commits) (flutter/engine#8580) 73f455a Roll src/third_party/skia e1c5ea6779f4..1fe0b86f17f3 (1 commits) (flutter/engine#8579) 42d06b3 Roll src/third_party/skia 3611ee1bb157..e1c5ea6779f4 (3 commits) (flutter/engine#8578) eb575e2 Roll src/third_party/skia c9f55de2ed39..3611ee1bb157 (1 commits) (flutter/engine#8577) 40473d3 Roll src/third_party/skia b5e57e9a3d0f..c9f55de2ed39 (1 commits) (flutter/engine#8576) dc952bc Roll src/third_party/skia 5c6b565bdfb9..b5e57e9a3d0f (1 commits) (flutter/engine#8575) 14a1db2 Roll src/third_party/skia 12cf258193dc..5c6b565bdfb9 (1 commits) (flutter/engine#8573) dcd0209 Roll src/third_party/skia 33233a09fef8..12cf258193dc (1 commits) (flutter/engine#8572) e0b9bc1 Roll src/third_party/skia 69f54f8f0f22..33233a09fef8 (1 commits) (flutter/engine#8571) 9f9d5d6 Roll src/third_party/skia 990bfc785891..69f54f8f0f22 (1 commits) (flutter/engine#8570) 7c7df0d Add null check in FLETextInputPlugin (flutter/engine#8538) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a guard on
_activeClientIDininsertNewline:. The conditional around theinsertText:replacementRange:call already catches this for that call, but we then unconditionally pack_activeClientIDinto anNSArray, which disallows nil.Review notes:
This patch works off a paranoid local copy to reduce the chance of future threading naughtiness. This isn't strictly required given that all async callbacks that modify
_activeClientIDoccur overplatform thread callbacks.
Alternatively, we could just swap this out for an
if (self.activeModel != nil)for consistency with other methods, for the sake of readability. The tradeoff against future paranoia is it looks like this is one of the few/only cases we'll actively blow up if this field is nil. Thoughts?