iframe-proxy

floitschG · 2018-05-28T12:49:39Z

Enables --sync-async for Flutter.

mravn-google

LGTM

mravn-google · 2018-05-28T12:52:16Z

  void handleBeginFrame(Duration rawTimeStamp) {
+    // Don't run this function when `handleBeginFrame` was invoked
+    // immediately before.
+    // TODO(17963): Remove this line.


The Flutter format for TODOs:

// TODO(username): Some explanation. See http://link/to/a/bug/123

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#comments

mravn-google · 2018-05-28T12:52:45Z

  void handleDrawFrame() {
+    // Don't run this function when `handleBeginFrame` wasn't invoked
+    // immediately before.
+    // TODO(17963): Remove this line.


mravn-google

LGTM

This reverts commit 3ced55a.

Hixie · 2018-05-28T20:48:54Z

+    // immediately before without a call of `handleDrawFrame` in between.
+    // TODO(floitsch): Remove this line when the spurious calls from the
+    //                 framework don't happen anymore. See
+    //                 https://github.com/flutter/flutter/issues/17963


I would rather we didn't check in this workaround and instead figured out what was going on and resolved that. This is very low-level code and not fully understanding what's going on here is likely to mean there are unexpected and extremely hard to debug bugs which our users are going to run into.

For the record, the underlying issue is present and consistently causing assertion errors during test runs also without sync-async semantics. See e.g. this log for a microbenchmarks run upon landing b6ceff5:

[ +4 ms] Connected to Moto G 4 [ ] An Observatory debugger and profiler on Moto G 4 is available at http://127.0.0.1:8100/ [ ] For a more detailed help message, press "h". To quit, press "q". [+3098 ms] I/flutter (14203): (The following exception is now available via WidgetTester.takeException:) [ +3 ms] I/flutter (14203): ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════ [ ] I/flutter (14203): The following assertion was thrown running a test: [ ] I/flutter (14203): Failed assertion: boolean expression must not be null [ ] I/flutter (14203): [ ] I/flutter (14203): Either the assertion indicates an error in the framework itself, or we should provide substantially [ ] I/flutter (14203): more information in this error message to help you determine and fix the underlying cause. [ ] I/flutter (14203): In either case, please report this assertion by filing a bug on GitHub: [ ] I/flutter (14203): https://github.com/flutter/flutter/issues/new [ ] I/flutter (14203): [ ] I/flutter (14203): When the exception was thrown, this was the stack: [ ] I/flutter (14203): #0 LiveTestWidgetsFlutterBinding.handleDrawFrame (package:flutter_test/src/binding.dart:0) [ ] I/flutter (14203): #1 _TestWidgetsFlutterBinding&BindingBase&SchedulerBinding._handleDrawFrame (package:flutter/src/scheduler/binding.dart:842) [ ] I/flutter (14203): #2 main.. (file:///Users/flutter/.cocoon/flutter/dev/benchmarks/microbenchmarks/lib/stocks/layout_bench.dart:51) [ ] I/flutter (14203): #11 _Timer._runTimers (dart:isolate/runtime/libtimer_impl.dart:382) [ ] I/flutter (14203): #12 _Timer._handleMessage (dart:isolate/runtime/libtimer_impl.dart:416) [ ] I/flutter (14203): #13 _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:165) [ ] I/flutter (14203): (elided 8 frames from package dart:async) [ ] I/flutter (14203): [ ] I/flutter (14203): [ ] I/flutter (14203): ════════════════════════════════════════════════════════════════════════════════════════════════════ [ ] I/flutter (14203): (If WidgetTester.takeException is called, the above exception will be ignored. If it is not, then the above exception will be dumped when another exception is caught by the framework or when the test ends, whichever happens first, and then the test will fail due to having not caught or expected the exception.) [ ] I/flutter (14203): ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════ [ ] I/flutter (14203): The following assertion was thrown running a test: [ ] I/flutter (14203): Failed assertion: boolean expression must not be null [ ] I/flutter (14203): [ ] I/flutter (14203): Either the assertion indicates an error in the framework itself, or we should provide substantially [ ] I/flutter (14203): more information in this error message to help you determine and fix the underlying cause. [ ] I/flutter (14203): In either case, please report this assertion by filing a bug on GitHub: [ ] I/flutter (14203): https://github.com/flutter/flutter/issues/new [ ] I/flutter (14203): [ ] I/flutter (14203): When the exception was thrown, this was the stack: [ ] I/flutter (14203): #0 LiveTestWidgetsFlutterBinding.handleDrawFrame (package:flutter_test/src/binding.dart:0) [ ] I/flutter (14203): #1 _TestWidgetsFlutterBinding&BindingBase&SchedulerBinding._handleDrawFrame (package:flutter/src/scheduler/binding.dart:842) [ ] I/flutter (14203): #2 main.. (file:///Users/flutter/.cocoon/flutter/dev/benchmarks/microbenchmarks/lib/stocks/layout_bench.dart:51) [ ] I/flutter (14203): #11 _Timer._runTimers (dart:isolate/runtime/libtimer_impl.dart:382) [ ] I/flutter (14203): #12 _Timer._handleMessage (dart:isolate/runtime/libtimer_impl.dart:416) [ ] I/flutter (14203): #13 _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:165) [ ] I/flutter (14203): (elided 8 frames from package dart:async) [ ] I/flutter (14203): [ ] I/flutter (14203): [ ] I/flutter (14203): ════════════════════════════════════════════════════════════════════════════════════════════════════ [ ] E/flutter (14203): [ERROR:topaz/lib/tonic/logging/dart_error.cc(16)] Unhandled exception: [ ] E/flutter (14203): Test failed. See exception logs above. [ ] I/flutter (14203): ================ RESULTS ================ [ ] I/flutter (14203): :::JSON::: {"stock_layout_iteration":5645.528205128205} [ ] I/flutter (14203): ================ FORMATTED ============== Running lib/stocks/build_bench.dart

Is this the issue @tvolkert fixed recently?

It's not the same stack trace, and the fix was submitted 6 days ago. @mravn-google presumably you saw this error against a more recent commit than 6-days-old?

16 hours ago, just before Florian's commit: https://flutter-dashboard.appspot.com/api/get-log?ownerKey=ahNzfmZsdXR0ZXItZGFzaGJvYXJkclgLEglDaGVja2xpc3QiOGZsdXR0ZXIvZmx1dHRlci9iYjg4NzY2NDQ4MWRhNDE5ZTk3NmIwMjNlYTJmNTM5YTdkMDc0M2IyDAsSBFRhc2sYgICAgICAkAsM

Hixie · 2018-05-28T20:49:11Z

+    //                 framework don't happen anymore. See
+    //                 https://github.com/flutter/flutter/issues/17963
+    if (_doDrawThisFrame == null)
+      return;


Hixie · 2018-05-28T20:50:07Z

+      // When the test had an exception, the test-framework already
+      // ran the teardown functions, removing the _fakeAsync function.
+      if (_fakeAsync == null)
+        return null;


Is this really the cleanest way to solve this? I would hope that a refactor could find a much cleaner solution to this problem.

Hixie · 2018-05-28T20:50:54Z

I would rather this was reverted and better fixes found. This seems like it just adds even more complexity to an already difficult to understand part of the codebase.

tvolkert · 2018-05-29T17:58:42Z

Given @Hixie's comments and the fact that this has been implicated in #17989, I think we should revert this.

mravn-google · 2018-05-29T18:22:20Z

This reverts commit 3ced55a.

floitschG added 4 commits May 24, 2018 11:41

Enable --sync-async in Flutter.

c015605

Return null in function that returns Future<Null>.

252d7d0

Make Flutter analysis happy.

a8cdcab

Add more fixes.

a388db3

floitschG requested a review from mravn-google May 28, 2018 12:49

googlebot added the cla: yes label May 28, 2018

mravn-google approved these changes May 28, 2018

View reviewed changes

floitschG added 2 commits May 28, 2018 15:04

Fix comment (style).

f3999a0

Fix style.

61dfa28

mravn-google approved these changes May 28, 2018

View reviewed changes

floitschG merged commit 3ced55a into flutter:master May 28, 2018

floitschG deleted the sync_async2 branch May 28, 2018 14:21

mravn-google added a commit that referenced this pull request May 28, 2018

Revert "Sync async2 (#17964)"

901e515

This reverts commit 3ced55a.

Hixie reviewed May 28, 2018

View reviewed changes

aam mentioned this pull request May 29, 2018

"Shell subprocess crashed with segmentation fault after tests finished" #17989

Closed

mravn-google added a commit to mravn-google/flutter that referenced this pull request May 29, 2018

Revert "Sync async2 (flutter#17964)"

b8d3fb1

This reverts commit 3ced55a.

github-actions Bot locked as resolved and limited conversation to collaborators Aug 10, 2021

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

floitschG commented May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mravn-google 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!

mravn-google 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!

Hixie commented May 28, 2018

Uh oh!

tvolkert commented May 29, 2018

Uh oh!

mravn-google commented May 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

floitschG commented May 28, 2018 •

edited

Loading