From b780c8a0969c82bbe427bb09744c1dc5d5d66205 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 16 Mar 2022 11:15:27 -0700 Subject: [PATCH 1/6] ensure _futurize does not leak uncaught errors into the zone --- lib/ui/painting.dart | 11 ++++++++++- lib/web_ui/lib/initialization.dart | 13 +++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 9f6763f206a99..b0fee832f91ea 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -5601,13 +5601,22 @@ typedef _Callbacker = String? Function(_Callback callback); /// ``` Future _futurize(_Callbacker callbacker) { final Completer completer = Completer.sync(); + // If the callback synchronously throws an error, then synchronously + // rethrow that error instead of adding it to the completer. This + // prevents the Zone from receiving an uncaught exception. + bool sync = true; final String? error = callbacker((T? t) { if (t == null) { - completer.completeError(Exception('operation failed')); + if (sync) { + throw Exception('operation failed'); + } else { + completer.completeError(Exception('operation failed')); + } } else { completer.complete(t); } }); + sync = false; if (error != null) throw Exception(error); return completer.future; diff --git a/lib/web_ui/lib/initialization.dart b/lib/web_ui/lib/initialization.dart index be7572cedcccc..cf6f24595fa66 100644 --- a/lib/web_ui/lib/initialization.dart +++ b/lib/web_ui/lib/initialization.dart @@ -141,13 +141,22 @@ typedef _Callback = void Function(T result); typedef _Callbacker = String? Function(_Callback callback); Future _futurize(_Callbacker callbacker) { final Completer completer = Completer.sync(); - final String? error = callbacker((T t) { + // If the callback synchronously throws an error, then synchronously + // rethrow that error instead of adding it to the completer. This + // prevents the Zone from receiving an uncaught exception. + bool sync = true; + final String? error = callbacker((T? t) { if (t == null) { - completer.completeError(Exception('operation failed')); + if (sync) { + throw Exception('operation failed'); + } else { + completer.completeError(Exception('operation failed')); + } } else { completer.complete(t); } }); + sync = false; if (error != null) { throw Exception(error); } From 592e9e72c7df5d6e374919a1ca89a714bf3d148c Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 16 Mar 2022 11:44:39 -0700 Subject: [PATCH 2/6] add unit test --- lib/ui/fixtures/ui_test.dart | 40 ++++++++++++++++++++++++++++++ lib/ui/painting.dart | 3 +++ lib/web_ui/lib/initialization.dart | 4 +++ 3 files changed, 47 insertions(+) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index ad560db96ba30..65902508fe7b4 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -747,9 +747,49 @@ void hooksTests() { expectEquals(frameNumber, 2); }); + test('_futureize does not leak sync uncaught exceptions into the zone', () async { + void callbacker(String? Function(Object? arg) cb) { + cb(null); // indicates failure + } + Object? error; + try { + await _futurize(callbacker); + } catch (err) { + error = err; + } + expectNotEquals(error, null) + }); + _finish(); } +typedef _Callbacker = String? Function(_Callback callback); + +// This is an exact copy of the function defined in painting.dart. If you change either +// then you must change both. +Future _futurize(_Callbacker callbacker) { + final Completer completer = Completer.sync(); + // If the callback synchronously throws an error, then synchronously + // rethrow that error instead of adding it to the completer. This + // prevents the Zone from receiving an uncaught exception. + bool sync = true; + final String? error = callbacker((T? t) { + if (t == null) { + if (sync) { + throw Exception('operation failed'); + } else { + completer.completeError(Exception('operation failed')); + } + } else { + completer.complete(t); + } + }); + sync = false; + if (error != null) + throw Exception(error); + return completer.future; +} + void _callHook( String name, [ int argCount = 0, diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index b0fee832f91ea..018dfd7cfcc97 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -5599,6 +5599,9 @@ typedef _Callbacker = String? Function(_Callback callback); /// return _futurize(_doSomethingAndCallback); /// } /// ``` +// Note: this function is not directly tested so that it remains private, instead an exact +// copy of it has been inlined into the test at lib/ui/fixtures/ui_test.dart. if you change +// this function, then you must update the test. Future _futurize(_Callbacker callbacker) { final Completer completer = Completer.sync(); // If the callback synchronously throws an error, then synchronously diff --git a/lib/web_ui/lib/initialization.dart b/lib/web_ui/lib/initialization.dart index cf6f24595fa66..4c2c81f0183cd 100644 --- a/lib/web_ui/lib/initialization.dart +++ b/lib/web_ui/lib/initialization.dart @@ -139,6 +139,10 @@ final PlatformViewRegistry platformViewRegistry = PlatformViewRegistry(); // NNBD migration. typedef _Callback = void Function(T result); typedef _Callbacker = String? Function(_Callback callback); + +// Note: this function is not directly tested so that it remains private, instead an exact +// copy of it has been inlined into the test at lib/ui/fixtures/ui_test.dart. if you change +// this function, then you must update the test. Future _futurize(_Callbacker callbacker) { final Completer completer = Completer.sync(); // If the callback synchronously throws an error, then synchronously From 667bd198d0d9d3934fbb09c5ff3c6572b84b9055 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 16 Mar 2022 12:06:25 -0700 Subject: [PATCH 3/6] Update ui_test.dart --- lib/ui/fixtures/ui_test.dart | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index 65902508fe7b4..c451c1b41ff76 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -748,7 +748,7 @@ void hooksTests() { }); test('_futureize does not leak sync uncaught exceptions into the zone', () async { - void callbacker(String? Function(Object? arg) cb) { + String? callbacker(void Function(Object? arg) cb) { cb(null); // indicates failure } Object? error; @@ -757,12 +757,13 @@ void hooksTests() { } catch (err) { error = err; } - expectNotEquals(error, null) + expectNotEquals(error, null); }); _finish(); } +typedef _Callback = void Function(T result); typedef _Callbacker = String? Function(_Callback callback); // This is an exact copy of the function defined in painting.dart. If you change either From 1d31aa2d2f7782eabcf7003bebb2d3b293e7c44c Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 17 Mar 2022 09:40:17 -0700 Subject: [PATCH 4/6] add more tests --- lib/ui/fixtures/ui_test.dart | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index c451c1b41ff76..6a1209ab3352a 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -760,6 +760,41 @@ void hooksTests() { expectNotEquals(error, null); }); + test('_futureize does not leak async uncaught exceptions into the zone', () async { + String? callbacker(void Function(Object? arg) cb) { + Timer.run(() { + cb(null); // indicates failure + }); + } + Object? error; + try { + await _futurize(callbacker); + } catch (err) { + error = err; + } + expectNotEquals(error, null); + }); + + test('_futureize successfully returns a value sync', () async { + String? callbacker(void Function(Object? arg) cb) { + cb(true); + } + final Object? result = await _futurize(callbacker); + + expectEquals(result, true); + }); + + test('_futureize successfully returns a value async', () async { + String? callbacker(void Function(Object? arg) cb) { + Timer.run(() { + cb(true); + }); + } + final Object? result = await _futurize(callbacker); + + expectEquals(error, true); + }); + _finish(); } From b8a9eea9abf87479538ed2e099eca249913d3437 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 17 Mar 2022 09:42:30 -0700 Subject: [PATCH 5/6] And one more --- lib/ui/fixtures/ui_test.dart | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index 6a1209ab3352a..407610a169ee8 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -747,6 +747,19 @@ void hooksTests() { expectEquals(frameNumber, 2); }); + test('_futureize handles callbacker sync error', () async { + String? callbacker(void Function(Object? arg) cb) { + return 'failure' + } + Object? error; + try { + await _futurize(callbacker); + } catch (err) { + error = err; + } + expectNotEquals(error, null); + }); + test('_futureize does not leak sync uncaught exceptions into the zone', () async { String? callbacker(void Function(Object? arg) cb) { cb(null); // indicates failure From b6393cc91e214267e2bde37dee284a59350bf346 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 17 Mar 2022 09:57:49 -0700 Subject: [PATCH 6/6] Update ui_test.dart --- lib/ui/fixtures/ui_test.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index 407610a169ee8..3fd410d49a450 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -749,7 +749,7 @@ void hooksTests() { test('_futureize handles callbacker sync error', () async { String? callbacker(void Function(Object? arg) cb) { - return 'failure' + return 'failure'; } Object? error; try { @@ -805,7 +805,7 @@ void hooksTests() { } final Object? result = await _futurize(callbacker); - expectEquals(error, true); + expectEquals(result, true); }); _finish();