Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type canonicalization not consistently preserved / reset with ddc hot restart #37259

Open
jonahwilliams opened this issue Jun 13, 2019 · 16 comments
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. customer-flutter P3 A lower priority bug or feature request web-dev-compiler

Comments

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Jun 13, 2019

Note: reproduction of this issue requires a checkout of flutter/flutter#34252 and dart-lang/webdev#440

Dart VM version: 2.3.3-dev.0.0.flutter-3166bbf24b (Tue Jun 11 16:24:18 2019 +0200) on "macos_x64"

Compiling the flutter gallery via the dev compiler/kernel initially succeeds. Applying a hot restart fails in different ways, depending on which library changed.

Case 1: Trigger Hot restart without changing anything

The hot reload fails due to a type error after re-invoking main:

The following assertion was thrown building IconTheme(IconThemeData#4acba(color:
dart_sdk.js:19255 Color(0xff007aff))):
dart_sdk.js:19255 Expected a value of type '(AnimationStatus) => void', but got one of type '(AnimationStatus) => void'

This corresponds to https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/animation/listener_helpers.dart#L170

Case 2: Change something upstream of listener_helpers.dart, like animation.dart.

The hot reload fails due to a slightly different type error

Uncaught (in promise) Error: Expected a value of type '(RawKeyEvent) => void', but got one of type '(RawKeyEvent) => void'

This corresponds to raw_keyboard.dart https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/services/raw_keyboard.dart#L477

Case 3: Change something in foundation.dart

Mostly success!

@kevmoo
Copy link
Member

kevmoo commented Jun 13, 2019

This seems to be the same as flutter/flutter#34289 – no sync to specific PRs required.

@jonahwilliams
Copy link
Contributor Author

Potential workaround: always reload the root module on start.

@jonahwilliams
Copy link
Contributor Author

@jakemac53 @vsmenon has there been any more investigation on fixes vs workarounds here?

@vsmenon
Copy link
Member

vsmenon commented Jul 10, 2019

I have not been able to isolate the root cause yet.

Are you all still seeing the behavior?

@jonahwilliams
Copy link
Contributor Author

It looks like @grouma is getting close to landing the webdev changes the tool needs. I'll wait until that lands and then start from there

@vsmenon
Copy link
Member

vsmenon commented Jul 12, 2019

Okay, I've isolated the root cause. Will work on a fix.

DDC hoists complex type expressions as an optimization - e.g., FrogColor<Color>. It does not clear / recompute these on a hot restart.

After a restart, FrogColor<Color> canonicalizes differently and does not match the hoisted one.

@vsmenon vsmenon self-assigned this Jul 12, 2019
@vsmenon vsmenon added this to the D25 Release milestone Jul 12, 2019
@vsmenon vsmenon added customer-flutter P1 A high priority bug; for example, a single project is unusable or has many test failures labels Jul 12, 2019
@vsmenon vsmenon changed the title Potential canonicalization issue with ddc hot restart Type canonicalization not consistently preserved / reset with ddc hot restart Jul 15, 2019
@vsmenon
Copy link
Member

vsmenon commented Jul 15, 2019

Here's a unit test repro:

https://dart-review.googlesource.com/c/sdk/+/109100

@vsmenon vsmenon added the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Jul 20, 2019
@vsmenon
Copy link
Member

vsmenon commented Jul 22, 2019

No update on this since last week.

@jonahwilliams - how blocking is this right now?

@jonahwilliams
Copy link
Contributor Author

I can't land the dwds integration until this is fixed. In the meantime, I'm using a full browser restart which is good enough. I don't believe anyone is on the flutter based workflow yet though.

@vsmenon
Copy link
Member

vsmenon commented Jul 22, 2019

Fix / workaround here:

https://dart-review.googlesource.com/c/sdk/+/109100

The relevant bit is a one-liner if you are able to try it out. Fixes @kevmoo 's repro and the unit test.

@jonahwilliams
Copy link
Contributor Author

I will take a look tomorrow and report back, thank you!

dart-bot pushed a commit that referenced this issue Jul 23, 2019
The combination of type expression hoisting, hot restart, and generic cache
reset triggers occasional breakage (see #37259).  Not clearing the generic
cache should be safe, but will some leak memory: unloaded types will
pollute the cache.  References to those unloaded types in user code,
however, should be cleared.


Bug: #37259
Change-Id: Ia15115a41556db7a19109f0178baa63ec0cfcb9c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/109100
Reviewed-by: Leaf Petersen <leafp@google.com>
Commit-Queue: Vijay Menon <vsm@google.com>
@jonahwilliams
Copy link
Contributor Author

I'm still getting some odd type errors when hot restarting with this change applied. Interestingly this is coming from the precompiled dart sdk and not from any user code (though that may be stopping me from getting to the user code)

Uncaught (in promise) Error: Type '_Future<void>' should be '_Future<void>' to implement expected type 'Future<void>'.
    at Object.throw_ [as throw] (dart_sdk.js:3994)
    at Object.castError (dart_sdk.js:3954)
    at Object.cast [as as] (dart_sdk.js:4276)
    at Function.check_C [as _check] (dart_sdk.js:3910)
    at Array.[dartx.add] (dart_sdk.js:7828)
    at _engine._FontManager.__.registerAsset (dart_sdk.js:128184)
    at _engine.FontCollection.new.registerFonts (dart_sdk.js:128130)
    at registerFonts.next (<anonymous>)
    at onValue (dart_sdk.js:28863)
    at async._RootZone.new.runUnary (dart_sdk.js:28751)
    at _FutureListener.then.handleValue (dart_sdk.js:24677)
    at handleValueCallback (dart_sdk.js:25144)
    at Function._propagateToListeners (dart_sdk.js:25176)
    at _Future.new.[_completeWithValue] (dart_sdk.js:25037)
    at async._AsyncCallbackEntry.new.callback (dart_sdk.js:25055)
    at Object._microtaskLoop (dart_sdk.js:28962)
    at _startMicrotaskLoop (dart_sdk.js:28968)
    at dart_sdk.js:25378
DartError @ dart_sdk.js:5152
throw_ @ dart_sdk.js:3994
castError @ dart_sdk.js:3954
cast @ dart_sdk.js:4276
check_C @ dart_sdk.js:3910
[dartx.add] @ dart_sdk.js:7828
registerAsset @ dart_sdk.js:128184
registerFonts @ dart_sdk.js:128130
onValue @ dart_sdk.js:28863
runUnary @ dart_sdk.js:28751
handleValue @ dart_sdk.js:24677
handleValueCallback @ dart_sdk.js:25144
_propagateToListeners @ dart_sdk.js:25176
[_completeWithValue] @ dart_sdk.js:25037
(anonymous) @ dart_sdk.js:25055
_microtaskLoop @ dart_sdk.js:28962
_startMicrotaskLoop @ dart_sdk.js:28968
(anonymous) @ dart_sdk.js:25378
Promise.then (async)
_scheduleImmediateWithPromise @ dart_sdk.js:25376
_scheduleImmediate @ dart_sdk.js:25382
_scheduleAsyncCallback @ dart_sdk.js:28982
_rootScheduleMicrotask @ dart_sdk.js:29155
scheduleMicrotask @ dart_sdk.js:28771
[_asyncComplete] @ dart_sdk.js:25054
complete @ dart_sdk.js:24580
(anonymous) @ dart_sdk.js:72343
_checkAndCall @ dart_sdk.js:4198
dcall @ dart_sdk.js:4203
(anonymous) @ dart_sdk.js:93132
load (async)
[_addEventListener] @ dart_sdk.js:50826
[dartx.addEventListener] @ dart_sdk.js:50816
[_tryResume] @ dart_sdk.js:93113
_EventStreamSubscription.new @ dart_sdk.js:93133
listen @ dart_sdk.js:92937
request @ dart_sdk.js:72337
load @ dart_sdk.js:117140
runBody @ dart_sdk.js:28882
_async @ dart_sdk.js:28910
load @ dart_sdk.js:117137
registerFonts @ dart_sdk.js:128089
runBody @ dart_sdk.js:28882
_async @ dart_sdk.js:28910
registerFonts @ dart_sdk.js:128086
webOnlySetAssetManager @ dart_sdk.js:116863
runBody @ dart_sdk.js:28882
_async @ dart_sdk.js:28910
webOnlySetAssetManager @ dart_sdk.js:116854
webOnlyInitializePlatform @ dart_sdk.js:116844
runBody @ dart_sdk.js:28882
_async @ dart_sdk.js:28910
webOnlyInitializePlatform @ dart_sdk.js:116839
main @ main_web_entrypoint.dart:5
runBody @ dart_sdk.js:28882
_async @ dart_sdk.js:28910
main$0 @ main_web_entrypoint.dart:4
Function.call$0 @ client.js:26095
call$0 @ client.js:24884
(anonymous) @ client.js:8189
(anonymous) @ client.js:3096
call$2 @ client.js:11462
call$1 @ client.js:11448
runUnary$2$2 @ client.js:12775
call$0 @ client.js:11835
_Future__propagateToListeners @ client.js:3197
_complete$1 @ client.js:11665
complete$1 @ client.js:11505
complete$1 @ client.js:11419
_asyncReturn @ client.js:3069
(anonymous) @ client.js:8226
(anonymous) @ client.js:3096
call$2 @ client.js:11462
call$1 @ client.js:11448
runUnary$2$2 @ client.js:12775
call$0 @ client.js:11835
_Future__propagateToListeners @ client.js:3197
call$0 @ client.js:11764
_microtaskLoop @ client.js:3253
_startMicrotaskLoop @ client.js:3259
call$1 @ client.js:11368
invokeClosure @ client.js:1206
(anonymous) @ client.js:1225
childList (async)
call$1 @ client.js:11378
_scheduleAsyncCallback @ client.js:3272
_rootScheduleMicrotask @ client.js:3395
_asyncComplete$1 @ client.js:11689
complete$1 @ client.js:11489
call$1 @ client.js:17497
call$1 @ client.js:18524
invokeClosure @ client.js:1206
(anonymous) @ client.js:1225
load (async)
_addEventListener$3 @ client.js:17310
addEventListener$3 @ client.js:17304
addEventListener$3$x @ client.js:2881
_tryResume$0 @ client.js:18504
_EventStreamSubscription$ @ client.js:6711
HttpRequest_request @ client.js:6692
(anonymous) @ client.js:8217
(anonymous) @ client.js:3096
call$2 @ client.js:11462
_asyncStartSync @ client.js:3061
_getDigests @ client.js:8229
(anonymous) @ client.js:8142
(anonymous) @ client.js:3096
call$2 @ client.js:11462
call$1 @ client.js:11448
runUnary$2$2 @ client.js:12775
call$0 @ client.js:11835
_Future__propagateToListeners @ client.js:3197
call$0 @ client.js:11764
_microtaskLoop @ client.js:3253
_startMicrotaskLoop @ client.js:3259
call$1 @ client.js:11368
invokeClosure @ client.js:1206
(anonymous) @ client.js:1225
childList (async)
call$1 @ client.js:11378
_scheduleAsyncCallback @ client.js:3272
_rootScheduleMicrotask @ client.js:3395
_asyncComplete$1 @ client.js:11689
complete$1 @ client.js:11489
(anonymous) @ VM159:3
Primitives_applyFunction @ client.js:897
Function_apply @ client.js:4477
_callDartFunctionFast @ client.js:6664
(anonymous) @ client.js:6655
Promise.then (async)
then$2 @ client.js:8860
then$2$x @ client.js:2977
(anonymous) @ client.js:8134
(anonymous) @ client.js:3096
call$2 @ client.js:11462
_asyncStartSync @ client.js:3061
hotRestart @ client.js:8199
call$0 @ client.js:24748
Primitives_applyFunction @ client.js:894
Function_apply @ client.js:4477
_callDartFunctionFast @ client.js:6664
(anonymous) @ client.js:6655
(anonymous) @ VM157:1
Show 51 more frames

@vsmenon
Copy link
Member

vsmenon commented Jul 23, 2019

Thanks for checking - can you give me repro steps?

@jonahwilliams
Copy link
Contributor Author

  1. checkout Integrate dwds into flutter tool for web support flutter/flutter#34252. You may need to force the flutter tool to be rebuilt by deleting bin/cache/flutter_tools.*

  2. checkout flutter/engine following the steps at https://github.com/flutter/flutter/wiki/Setting-up-the-Engine-development-environment. You should have an engine directory adjacent to the flutter directory to make the following steps simpler.

(From here on assuming that flutter/engine is located at engine/src/flutter)

  1. Apply your diff to the dart-sdk located at engine/src/third_party/dart

  2. Set up the gn configuration with ./flutter/tools/gn --unopt --full-dart-sdk and build ninja -C out/host_debug_unopt.

(From here on assuming we're in flutter/flutter)

  1. in flutter/examples/flutter_gallery run flutter run --local-engine=host_debug_unopt -d chrome

  2. Attempt hot restart after build finishes (R)

dart-bot pushed a commit that referenced this issue Jul 31, 2019
This is a somewhat roundabout way to prevent a hot restart from redefining these types.
This fixes the issues arising: #37259.
Should more cleanly separate fields reset on a hot restart from ones preserved.

Change-Id: Id0c45cbeed67b574c3259e1f87a405137ae93575
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/111315
Reviewed-by: Mark Zhou <markzipan@google.com>
Commit-Queue: Vijay Menon <vsm@google.com>
@vsmenon
Copy link
Member

vsmenon commented Jul 31, 2019

The above CL should unblock Flutter Web when it rolls in. I'll keep this issue open to implement a cleaner solution to delineate state that should and should not (internal type tables) be reset.

@jonahwilliams - I'm tentatively lowering the priority here as I think you're unblocked. Please let me know if you still see the issue.

@vsmenon vsmenon added P3 A lower priority bug or feature request and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels Jul 31, 2019
@vsmenon vsmenon removed this from the D25 Release milestone Jul 31, 2019
@vsmenon vsmenon removed their assignment Jul 30, 2020
@srawlins
Copy link
Member

@nshahan this also looks similar to the bug we were debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. customer-flutter P3 A lower priority bug or feature request web-dev-compiler
Projects
None yet
Development

No branches or pull requests

4 participants