Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@0xZOne
Copy link
Member

@0xZOne 0xZOne commented Feb 24, 2024

Consider this scenario: In an add-to-app context, where multiple Flutter activities share the same engine, a situation occurs. When navigating from FlutterActivity1 to FlutterActivity2, the Flutter view associated with FlutterActivity1 is detached from the engine. Then, the Flutter view of FlutterActivity2 is attached. Upon navigating back to FlutterActivity1, its Flutter view is re-attached to the shared engine.

The expected behavior is: When a Flutter view detaches from the shared engine, the associated surface should be released. When the Flutter view re-attaches, a new surface should be created.

After #47358, no new surface is created when the Flutter view is attached again. This results in the Flutter view having no underlying surface, which causes the page to appear blank or freeze without responding.

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 and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@0xZOne 0xZOne force-pushed the task/bug_fix branch 2 times, most recently from 3a106f4 to 9dc2f82 Compare February 25, 2024 01:22
@0xZOne 0xZOne changed the title [Android] Fix the missing surface issue in the shared engine scenario [Android] Fix the issue of blank or frozen pages in shared engine scenarios Feb 25, 2024
@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #50947 at sha c74ff37

Copy link
Contributor

@johnmccutchan johnmccutchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expected behavior is: When a Flutter view detaches from the shared engine, the associated surface should be released. When the Flutter view re-attaches, a new surface should be created.
After #47358, no new surface is created when the Flutter view is attached again. This results in the Flutter view having no underlying surface, which causes the page to appear blank or freeze without responding.

Given your description, I would expect you to fix the bug that no new surface is created when the view is attached again. It looks like you've just removed calls to pause() which isn't addressing the root problem.

@0xZOne
Copy link
Member Author

0xZOne commented Mar 7, 2024

The expected behavior is: When a Flutter view detaches from the shared engine, the associated surface should be released. When the Flutter view re-attaches, a new surface should be created.
After #47358, no new surface is created when the Flutter view is attached again. This results in the Flutter view having no underlying surface, which causes the page to appear blank or freeze without responding.

Given your description, I would expect you to fix the bug that no new surface is created when the view is attached again. It looks like you've just removed calls to pause() which isn't addressing the root problem.

It works fine after removing the call to pause() in the detachFromRenderer function. Calling pause() sets isPaused to true, which prevents startRenderingToSurface from creating a new surface.

@0xZOne 0xZOne requested a review from johnmccutchan March 7, 2024 01:32
@0xZOne 0xZOne requested a review from johnmccutchan March 8, 2024 12:27
@0xZOne 0xZOne force-pushed the task/bug_fix branch 2 times, most recently from 8e998c3 to 508dc92 Compare March 9, 2024 02:53
@johnmccutchan
Copy link
Contributor

The expected behavior is: When a Flutter view detaches from the shared engine, the associated surface should be released. When the Flutter view re-attaches, a new surface should be created.
After #47358, no new surface is created when the Flutter view is attached again. This results in the Flutter view having no underlying surface, which causes the page to appear blank or freeze without responding.

Given your description, I would expect you to fix the bug that no new surface is created when the view is attached again. It looks like you've just removed calls to pause() which isn't addressing the root problem.

It works fine after removing the call to pause() in the detachFromRenderer function. Calling pause() sets isPaused to true, which prevents startRenderingToSurface from creating a new surface.

Why can't you just move the isPaused = false; so it executes before connectSurfaceToRenderer is called?

@0xZOne
Copy link
Member Author

0xZOne commented Mar 15, 2024

Why can't you just move the isPaused = false; so it executes before connectSurfaceToRenderer is called?

On one hand, I believe the call to pause() in detachFromRenderer should be removed.

As stated in the comments of the detachFromRenderer function, if the detachFromRenderer function is called, it indicates that the RenderSurface is no longer needed and its underlying surface will also be detached.

  /**
   * Invoked by the owner of this {@code FlutterSurfaceView} when it no longer wants to render a
   * Flutter UI to this {@code FlutterSurfaceView}.
   *
   * <p>This method will cease any on-going rendering from Flutter to this {@code
   * FlutterSurfaceView}.
   */
  public void detachFromRenderer() {
    ...
  }

Calling pause() in detachFromRenderer() is superfluous and contributes nothing to the problem that #47358 aims to solve. Moreover, it results in the required new surface not being created. Therefore, the call to pause() in detachFromRenderer should be removed.

On the other hand, if isPaused = false; is moved to execute before the call to connectSurfaceToRenderer, it would result in #47358 losing its effect. That is to say, a new surface would be recreated upon returning from HC mode.

  public void resume() {
    if (flutterRenderer == null) {
      Log.w(TAG, "resume() invoked when no FlutterRenderer was attached.");
      return;
    }
    this.flutterRenderer.addIsDisplayingFlutterUiListener(flutterUiDisplayListener);

    // If we're already attached to an Android window then we're now attached to both a renderer
    // and the Android window. We can begin rendering now.
    if (isSurfaceAvailableForRendering()) {
      Log.v(
          TAG,
          "Surface is available for rendering. Connecting FlutterRenderer to Android surface.");
      connectSurfaceToRenderer();
    }
    isPaused = false;
  }

@johnmccutchan
Copy link
Contributor

Thanks for your patience and explanations. I believe you're correct and will land this PR soon

@0xZOne 0xZOne added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 22, 2024
@auto-submit auto-submit bot merged commit f9a34ae into flutter:main Mar 22, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 22, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 22, 2024
…145581)

flutter/engine@5a12de1...f9a34ae

2024-03-22 [email protected] [Android] Fix the issue of blank or frozen pages in shared engine scenarios (flutter/engine#50947)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@0xZOne 0xZOne deleted the task/bug_fix branch March 22, 2024 08:23
@godofredoc godofredoc added the cp: stable cherry pick to the stable release candidate branch label Apr 22, 2024
@flutteractionsbot
Copy link

Failed to create CP due to merge conflicts.
You will need to create the PR manually. See the cherrypick wiki for more info.

mdnht pushed a commit to mdnht/engine that referenced this pull request Apr 25, 2024
…narios (flutter#50947)

Consider this scenario: In an add-to-app context, where multiple Flutter activities share the same engine, a situation occurs. When navigating from FlutterActivity1 to FlutterActivity2, the Flutter view associated with FlutterActivity1 is detached from the engine. Then, the Flutter view of FlutterActivity2 is attached. Upon navigating back to FlutterActivity1, its Flutter view is re-attached to the shared engine.

The expected behavior is: When a Flutter view detaches from the shared engine, the associated surface should be released. When the Flutter view re-attaches, a new surface should be created.

After flutter#47358, no new surface is created when the Flutter view is attached again. This results in the Flutter view having no underlying surface, which causes the page to appear blank or freeze without responding.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App cp: stable cherry pick to the stable release candidate branch platform-android will affect goldens

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants