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

Reland "[ios_platform_view] only recycle maskView when the view is applying mutators #42115" #42823

Merged
merged 2 commits into from
Jun 13, 2023

Conversation

cyanglaz
Copy link
Contributor

Relands #42115, which was reverted in #42231 due to a crash in the framework test.

The crash is due to a memory management issue that I fixed it in this PR, I also added a scenario test to catch the crash.

fixes: flutter/flutter#125620

See also: orignal PR #41573 for more details.

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 Hixie said 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.

[maskView reset];
}
self.availableIndex++;
FlutterClippingMaskView* maskView = [[[self.pool anyObject] retain] autorelease];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The retain] autorelease] is the fix for the crash. [self.pool removeObject:maskView]; below makes the maskView ownerless, which sometimes caused the maskView to be released unwantedly.

@@ -1430,6 +1609,92 @@ class PlatformViewScrollingUnderWidget extends Scenario
}
}

/// Builds a scenario where many platform views with clips scrolling.
class PlatformViewsWithClipsScrolling extends Scenario
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the test added for the crash. It mimics the behavior of the benchmark test that originally caught the crash.

@cyanglaz cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 13, 2023
@auto-submit auto-submit bot merged commit 727b441 into flutter:main Jun 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 14, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 14, 2023
…128841)

flutter/engine@66a5761...727b441

2023-06-13 [email protected] Reland "[ios_platform_view] only recycle maskView when the view is applying mutators #42115" (flutter/engine#42823)
2023-06-13 [email protected] Roll Dart SDK from 41234fa4b22d to 2465228c0c21 (1 revision) (flutter/engine#42822)
2023-06-13 [email protected] [Impeller] Added cache for command buffers in vulkan (flutter/engine#42793)
2023-06-13 [email protected] setupDefultFontManager correctly clear out cache (flutter/engine#42178)
2023-06-13 [email protected] [Impeller] Reland attempt Vulkan setup and fallback to GLES. (flutter/engine#42820)
2023-06-13 [email protected] Added CI step for building with validation layers (flutter/engine#42724)
2023-06-13 [email protected] [Impeller] Null check for the device holder in the Vulkan context destructor (flutter/engine#42821)
2023-06-13 [email protected] Add missing includes (flutter/engine#42812)
2023-06-13 [email protected] Roll Dart SDK from 4dce1093ad94 to 41234fa4b22d (1 revision) (flutter/engine#42810)
2023-06-13 [email protected] [iOS][Keyboard] Wait vsync on UI thread and update viewport inset to avoid jitter. (flutter/engine#42312)

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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@cyanglaz cyanglaz deleted the reland_platform_view_mask_view branch June 14, 2023 17:30
cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Jun 20, 2023
…plying mutators flutter#42115" (flutter#42823)

Relands flutter#42115, which was reverted in flutter#42231 due to a crash in the framework test.

The crash is due to a memory management issue that I fixed it in this PR, I also added a scenario test to catch the crash.

fixes: flutter/flutter#125620

See also: orignal PR flutter#41573 for more details.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
auto-submit bot pushed a commit that referenced this pull request Jun 20, 2023
cp #42823.

The PR fixes flutter/flutter#125620, which was a regression caused by #38989. We cannot simply revert #38989 because #38989 addressed a critical performance issue 

[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 platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PlatformViews are drawn outside of parent widget (beta channel)
2 participants