Avoid unnecessarty unwwrap of _lastPendingEventTimestamp by reidbaker · Pull Request #121553 · flutter/flutter

reidbaker · 2023-02-27T22:35:18Z

Takeover of /pull/117551
This fixes /issues/112403 and b/249091367

Avoid unnecessary unwrap of _lastPendingEventTimestamp which sometimes causes "Null check operator used on a null"
Fix formatting again
expose checkStart to tests and verify that null timestamp does not crash

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
All existing and new tests are passing.

…mes causes "Null check operator used on a null" Fixes #112403

packages/flutter/pubspec.yaml

christopherfujino

I'll defer approval to @dkwingsmt; I have no idea how this code works, I just copy pasted from buganizer to a github issue :)

dkwingsmt

LGTM

goderbauer · 2023-02-27T22:45:09Z

packages/flutter/lib/src/gestures/monodrag.dart

+// ignore: public_member_api_docs
+//
+// Exposed to test for null timestamp.


fly-by comment: please don't do this. Anything that is public needs docs.

(ideally, we would find a better way to test this without making it public, if we can't it needs real docs because dartdoc will process it and publish the method to our docs)

goderbauer · 2023-02-27T22:46:56Z

packages/flutter/test/gestures/monodrag_test.dart


-    GestureBinding.instance.handleEvent(up90, HitTestEntry(MockHitTestTarget()));
-    GestureBinding.instance.handleEvent(up91, HitTestEntry(MockHitTestTarget()));
+


nit: remove one of the blank lines.

goderbauer · 2023-02-27T22:55:37Z

packages/flutter/test/gestures/monodrag_test.dart

-    GestureBinding.instance.handleEvent(up91, HitTestEntry(MockHitTestTarget()));
+
+    // Regression test for https://github.com/flutter/flutter/issues/112403.
+    v.checkStart(null, down91.pointer);


Hm, I believe this isn't a full regression test. If someone where to accidentally add the exclamation mark back to line 366 in monodrag.dart (the root cause for the crash), this test would continue to pass?

Just calling acceptGesture twice in a row would trigger this bug.

There seem to be several ways that the last event can get nulled out and not reset depending on which sequence of pointer events get submitted. I don't know which of those would be expected or not and which we should have better test coverage of.

goderbauer · 2023-02-27T22:56:33Z

packages/flutter/lib/src/gestures/monodrag.dart

+@visibleForTesting
+void checkStart(Duration? timestamp, int pointer) {


nit: indentation is off

jmagman · 2023-02-28T00:47:34Z

packages/flutter/test/gestures/monodrag_test.dart

-    GestureBinding.instance.handleEvent(up90, HitTestEntry(MockHitTestTarget()));
-    GestureBinding.instance.handleEvent(up91, HitTestEntry(MockHitTestTarget()));
+
+    // Regression test for https://github.com/flutter/flutter/issues/112403.


I believe we don't need tests like this for null safety, it's considered "code refactor with no semantic change".
#114111 (comment)

https://github.com/flutter/flutter/wiki/Tree-hygiene#tests

refactors with no semantic change (e.g. null safety migrations)

However I believe @dkwingsmt's point in the last PR was: do we understand why is this null #117551 (comment) ? If it's just a mistake from the null migration then this test can be removed. However if it's a legitimate state to be in, can we exercise that state in a unit test?

No I do not understand why this is null. I responded to an internal request for a 1p customer who had their pr closed because of a lack of tests. When I saw the pr was a null check crash and they were seeing this happen in the field I thought that it was worth timeboxing writing a test that triggers a null check.

Someone else is welcome to take this pr over if they would like.

reidbaker · 2023-02-28T15:20:18Z

Closing this pr I allocated a small timebox and I am past the effort I decided to spend.

tarobins and others added 5 commits December 22, 2022 11:44

Avoid unnecessary unwrap of _lastPendingEventTimestamp which someti…

b2743fc

…mes causes "Null check operator used on a null" Fixes #112403

Avoid unnecessary unwrap of _lastPendingEventTimestamp which someti…

7728232

…mes causes "Null check operator used on a null" Fixes #112403

Fix formatting

fb7a8de

Fix formatting again

02a66fc

expose checkStart to tests and verify that null timestamp does not crash

22a30cf

flutter-dashboard bot added f: gestures flutter/packages/flutter/gestures repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Feb 27, 2023

reidbaker commented Feb 27, 2023

View reviewed changes

packages/flutter/pubspec.yaml Outdated Show resolved Hide resolved

reidbaker requested review from christopherfujino, dkwingsmt and tarobins February 27, 2023 22:37

reidbaker marked this pull request as ready for review February 27, 2023 22:38

christopherfujino reviewed Feb 27, 2023

View reviewed changes

Merge branch 'master' into npe

4ee74a5

dkwingsmt approved these changes Feb 27, 2023

View reviewed changes

goderbauer reviewed Feb 27, 2023

View reviewed changes

jmagman reviewed Feb 28, 2023

View reviewed changes

reidbaker closed this Feb 28, 2023

dnfield mentioned this pull request Feb 28, 2023

Fix monodrag gestures for #112403 and b/249091367 #121615

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid unnecessarty unwwrap of _lastPendingEventTimestamp#121553

Avoid unnecessarty unwwrap of _lastPendingEventTimestamp#121553
reidbaker wants to merge 6 commits intoflutter:masterfrom
reidbaker:npe

reidbaker commented Feb 27, 2023 •

edited

Loading

Uh oh!

Uh oh!

christopherfujino left a comment

Uh oh!

dkwingsmt left a comment

Uh oh!

goderbauer Feb 27, 2023

Uh oh!

goderbauer Feb 27, 2023

Uh oh!

goderbauer Feb 27, 2023

Uh oh!

goderbauer Feb 27, 2023

Uh oh!

dnfield Feb 28, 2023

Uh oh!

goderbauer Feb 27, 2023

Uh oh!

jmagman Feb 28, 2023

Uh oh!

reidbaker Feb 28, 2023

Uh oh!

reidbaker Feb 28, 2023

Uh oh!

reidbaker commented Feb 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants


		GestureBinding.instance.handleEvent(up90, HitTestEntry(MockHitTestTarget()));
		GestureBinding.instance.handleEvent(up91, HitTestEntry(MockHitTestTarget()));

		@visibleForTesting
		void checkStart(Duration? timestamp, int pointer) {

Conversation

reidbaker commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reidbaker commented Feb 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

reidbaker commented Feb 27, 2023 •

edited

Loading