Run setAssetBundlePath outside of UI thread.

[aam]

Waiting for setAssetBundlePath running this on UI thread could result in timeout since application could be suspended on breakpoint with UI loop not running. So we just post setAssetBundlePath call on UI event queue and don't wait.

Refactor and remove extra layers of running-on-UI methods.

This fixes flutter/flutter#13396

[jason-simmons]

@chinmaygarde is this still relevant after the engine refactoring?

[rmacnak-google]

This lgtm, but I'd wait for Chinmay's answer.

[chinmaygarde]

Yes. This will still be an issue in the refactor because this service protocol method requires a response object to say if the updated asset bundle of was actually installed on the running shell. https://docs.google.com/document/d/1ObJYEfw3xtL_UW-aBCIODhZzFyzLrEVCYDeio1GVIuE/edit#heading=h.2ejbd9fwvkxi. There is no way to determine this upfront. This implementation only tries to find the particular flutter view and fails if it cant. It is possible that the directory specified is invalid, in such cases, the new asset bundle wont be installed but the service protocol response will be a success.

[chinmaygarde]

The refactor has also removed all the extra per platform overrides to set the asset bundle on the UI thread. So that part is not relevant anymore.

[chinmaygarde]

I don't think this is the right (or complete) fix for the linked issue. There are other methods (namely _flutter.flushUIThreadTasks and _flutter.runInView) that also queue onto the UI thread. If pausing the isolate on the debugger suspends the UI thread, then those methods will also need this hack of not requiring a response. We should make sure to continue the debugger before attempting to run service protocol methods.

[chinmaygarde]

Based on comments above.

[chinmaygarde]

After talking to @rmacnak-google, some suggestions:
Registering the extensions with Dart_RegisterIsolateServiceRequestCallback instead of the Dart_RegisterRootServiceRequestCallback we currently use would make the service protocol callback be executed on the UI thread even if the debugger had paused the UI thread. However, this isn't easy to achieve because of the way the engine registers service protocol extensions and the tooling on the other side refers to running isolates within the engine.

The plan then is to use the same mechanism the VM uses to run the callbacks specified using Dart_RegisterRootServiceRequestCallback but allow the embedder to specific custom callbacks. That way, if there is a service protocol extension that needs to be directly run on the UI thread, the engine will first look for the isolate for that thread and invoke the task handler wrapped in the callback given to the VM. If there is no isolate, the engine knows that it won't block the UI thread on its own and so can run the task directly.

[aam]

@chinmaygarde wrote:

I don't think this is the right (or complete) fix for the linked issue. There are other methods (namely _flutter.flushUIThreadTasks and _flutter.runInView) that also queue onto the UI thread.

Linked issue and the fix is specifically about flutter tools hanging during hot reload. Those other methods(flushUIThreadTasks and runInView) are not executed during hot reload. They are run on restart, but restart is handled differently - isolate is being resumed before those requests are sent.

I don't think that this fix for M3 issue should be held until we figure out more generic solution that would avoid scheduling actions on UI thread.

[chinmaygarde]

Linked issue and the fix is specifically about flutter tools hanging during hot reload.

I don't this is true, the issue mentions hot restart (not a reload) with the R (capitalized). I am now confused.

I don't think that this fix for M3 issue should be held until we figure out more generic solution that would avoid scheduling actions on UI thread.

If we need a quick hack for this, the workaround mentioned in this comment should be the way to go. This is because the shell refactor I want to land in this patch separates service protocol registration and dispatch into a generic API that is used by the components that care to respond via the service protocol. So it will be hard to apply the workaround for just this one method in that patch.

[aam]

I don't this is true, the issue mentions hot restart (not a reload) with the R (capitalized). I am now confused.

We were not able to repro "restart issue", but were able to identify "hot reload issue" - see flutter/flutter#13396 (comment). So this is what this fixes.

[aam]

If we need a quick hack for this, the workaround mentioned in this comment should be the way to go.

Like I mentioned on flutter/flutter#13396 (comment) always switching asset bundle path even if hot reload won't be used doesn't feel right.

[chinmaygarde]

always switching asset bundle path even if hot reload won't be used doesn't feel right.

Neither does this. And it will be hard to reconcile with the changes in the linked PR. But I'll figure something out. We should still file a bug saying that not all service protocol handlers that need the UI thread are resilient to pauses in the debugger. I suspect we will keep running into more such edge cases. Feel free to assign the issue to me.

[Hixie]

What is the status of this PR? Can we close it for now while the fix is refactored?

[aam]

What is the status of this PR? Can we close it for now while the fix is refactored?

Sure thing.