Reintroduce Windows lifecycle with guard for posthumous `OnWindowStateEvent` #44344

yaakovschectman · 2023-08-03T20:52:12Z

Previously, destruction of Window called DestroyWindow, which may send WM_KILLFOCUS to the to-be-destroyed window. Because Window's destructor is called after FlutterWindow's, the FlutterWindow vtable was already destroyed at this point, and the subsequent call to the virtual method OnWindowStateEvent would cause a crash. This PR reintroduces the reverted changes for Windows lifecycle with a check before calling the virtual method that the FlutterWindow object has not yet been destructed.

flutter/flutter#131872

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.

yaakovschectman · 2023-08-03T20:53:47Z

Should I incorporate the changes in #44238 into this PR? Doing so will resolve the startup warnings issue at the same time, but will make this a rather broad request.

loic-sharma · 2023-08-04T20:45:55Z

shell/platform/windows/window.h

@@ -249,6 +253,9 @@ class Window : public KeyboardManager::WindowDelegate {
  // Accessibility node that represents an alert.
  std::unique_ptr<ui::AXPlatformNodeWin> alert_node_;

+  // Guard against posthumous vtable access;
+  bool vtable_is_alive = true;


This is spooky. Some potential alternative options:

Merge FlutterWindow and Window to make destruction simpler

Expose a Destroy method to destroy the window manually. This would need to be called before the Window is destructed to ensure no messages are receive post destruction.

Switch from an inheritance pattern to a delegate pattern so that the Window is destructed before the FlutterWindow

What are your thoughts?

I do not see any reason why they need to be separate classes, though I am a little nervous about trying it. As long as it's safe, and no one will complain about needing both classes for some 3rd party or future change, this seems like a valid option.

We could also try this, though do we know from where we could call it, or also importantly, if we have any way to ensure it's called before destruction?

If FlutterWindow is our only use case for Window, merging the classes seems like a better idea.

I do not see any reason why they need to be separate classes, though I am a little nervous about trying it. As long as it's safe, and no one will complain about needing both classes for some 3rd party or future change, this seems like a valid option.

This type hierarchy is a common win32 C++ pattern: https://learn.microsoft.com/windows/win32/learnwin32/managing-application-state-#an-object-oriented-approach

I'm not in love with this pattern though 😅

... though do we know from where we could call it, or also importantly, if we have any way to ensure it's called before destruction?

One potential place might FlutterDesktopViewControllerDestroy. We could manually close the window before destroying our objects:

engine/shell/platform/windows/flutter_windows.cc

Lines 90 to 93 in 138a1ea

void FlutterDesktopViewControllerDestroy(

FlutterDesktopViewControllerRef controller) {

delete controller;

}

... merging the classes seems like a better idea.

Agreed. Right now I'm leaning towards option 2, it seems like the least intrusive option.

/cc @cbracken For thoughts

What do you think about moving the call to Window::Destroy() from ~Window to ~FlutterWindow? I just do not know how we can enforce within a test that window messages are received between the two destructors.

In the interest of getting this landed, agreed that option 2 (make Destroy() protected, then move the destroy call down to the subclass) is probably the quickest fix. Once that's landed, let's follow up with a patch that implements option 1 (merge Window and FlutterWindow).

I don't think there's a strong case for keeping them separate. Now that the UWP embedder is gone let's use the YAGNI rule here and merge the two. If we ever find a good reason for a separation, we can look at that in the future.

cbracken · 2023-08-08T17:57:37Z

shell/platform/windows/window.h

@@ -249,6 +253,9 @@ class Window : public KeyboardManager::WindowDelegate {
  // Accessibility node that represents an alert.
  std::unique_ptr<ui::AXPlatformNodeWin> alert_node_;

+  // Guard against posthumous vtable access;
+  bool vtable_is_alive = true;


On a separate note, I'm surprised the linter didn't catch the missing underscore suffix on this field. Probably irrelevant given the change in approach proposed for this patch, but I'll file a bug to see why the linters aren't running on windows.

loic-sharma · 2023-08-08T23:12:42Z

shell/platform/windows/client_wrapper/include/flutter/flutter_engine.h

@@ -84,6 +85,16 @@ class FlutterEngine : public PluginRegistry {
  // once on the platform thread.
  void SetNextFrameCallback(std::function<void()> callback);

+  // Called to pass an external window message to the engine for lifecycle
+  // state updates. This does not consume the window message. Non-Flutter


Suggested change

// state updates. This does not consume the window message. Non-Flutter

// state updates. Non-Flutter

loic-sharma · 2023-08-08T23:12:56Z

shell/platform/windows/client_wrapper/testing/stub_flutter_windows_api.h

@@ -89,6 +89,17 @@ class StubFlutterWindowsApi {
  // FlutterDesktopPluginRegistrarUnregisterTopLevelWindowProcDelegate.
  virtual void PluginRegistrarUnregisterTopLevelWindowProcDelegate(
      FlutterDesktopWindowProcCallback delegate) {}
+
+  // Claled for FlutterDesktopEngineProcessExternalWindowMessage.


Suggested change

// Claled for FlutterDesktopEngineProcessExternalWindowMessage.

// Called for FlutterDesktopEngineProcessExternalWindowMessage.

loic-sharma · 2023-08-08T23:15:04Z

shell/platform/windows/public/flutter_windows.h

@@ -212,6 +212,19 @@ FLUTTER_EXPORT HWND FlutterDesktopViewGetHWND(FlutterDesktopViewRef view);
 FLUTTER_EXPORT IDXGIAdapter* FlutterDesktopViewGetGraphicsAdapter(
    FlutterDesktopViewRef view);

+// Called to pass an external window message to the engine for lifecycle
+// state updates. This does not consume the window message. Non-Flutter windows


Suggested change

// state updates. This does not consume the window message. Non-Flutter windows

// state updates. Non-Flutter windows

loic-sharma · 2023-08-08T23:15:15Z

shell/platform/windows/public/flutter_windows.h

+// Called to pass an external window message to the engine for lifecycle
+// state updates. This does not consume the window message. Non-Flutter windows
+// must call this method in their WndProc in order to be included in the logic
+// for application lifecycle state updates. Returns true when the message is


Suggested change

// for application lifecycle state updates. Returns true when the message is

// for application lifecycle state updates. Returns true if the message should be

loic-sharma · 2023-08-08T23:15:50Z

shell/platform/windows/client_wrapper/include/flutter/flutter_engine.h

+  // the logic for application lifecycle state updates. Returns a result when
+  // the message has been consumed.


Suggested change

// the logic for application lifecycle state updates. Returns a result when

// the message has been consumed.

// the logic for application lifecycle state updates. Returns a result if

// the message should be consumed.

loic-sharma · 2023-08-09T16:59:26Z

shell/platform/windows/client_wrapper/include/flutter/flutter_engine.h

+  // state updates. Non-Flutter
+  // windows must call this method in their WndProc in order to be included in


Please reformat this to fill the line

Same comment for FlutterDesktopEngineProcessExternalWindowMessage

loic-sharma · 2023-08-09T17:03:30Z

shell/platform/windows/window.h

@@ -236,6 +240,9 @@ class Window : public KeyboardManager::WindowDelegate {
  // Returns the root view accessibility node, or nullptr if none.
  virtual gfx::NativeViewAccessible GetNativeViewAccessible() = 0;

+  // Release OS resources associated with window.


Suggested change

// Release OS resources associated with window.

// Release OS resources associated with the window.

shell/platform/windows/windows_lifecycle_manager.cc

loic-sharma · 2023-08-09T17:08:09Z

@yaakovschectman Before merging this, please verify that this latest approach also works as expected with the internal app team.

yaakovschectman · 2023-08-10T14:39:54Z

@loic-sharma Someone from said team confirmed they experience no crash when testing with this build.

shell/platform/windows/windows_lifecycle_manager.cc

loic-sharma

LGTM, nice work! 🎉

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

cbracken

Awesome! Thanks for getting this relanded!

…WindowStateEvent` (flutter/engine#44344)

…132332) flutter/engine@a9be77e...16b01b9 2023-08-10 skia-flutter-autoroll@skia.org Roll Dart SDK from 46da53e7abe2 to a2eac00da6b8 (1 revision) (flutter/engine#44601) 2023-08-10 huxiaohu2007@gmail.com Fix unexpected pointer change issue and Add test case (flutter/engine#43949) 2023-08-10 47866232+chunhtai@users.noreply.github.com Reland "Android a11y bridge sets importantness" (flutter/engine#44589) 2023-08-10 john@johnmccutchan.com Support for Android Platform Views under Impeller/Vulkan (flutter/engine#44571) 2023-08-10 109111084+yaakovschectman@users.noreply.github.com Reintroduce Windows lifecycle with guard for posthumous `OnWindowStateEvent` (flutter/engine#44344) 2023-08-10 skia-flutter-autoroll@skia.org Roll Skia from 92e6f52b0fa8 to b6492f5ce8c3 (1 revision) (flutter/engine#44597) 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 chinmaygarde@google.com,rmistry@google.com,zra@google.com 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

…eEvent` (flutter#44344) Previously, destruction of `Window` called `DestroyWindow`, which may send `WM_KILLFOCUS` to the to-be-destroyed window. Because `Window`'s destructor is called after `FlutterWindow`'s, the `FlutterWindow` vtable was already destroyed at this point, and the subsequent call to the virtual method `OnWindowStateEvent` would cause a crash. This PR reintroduces the reverted changes for Windows lifecycle with a check before calling the virtual method that the `FlutterWindow` object has not yet been destructed. flutter/flutter#131872 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] 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. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --------- Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

yaakovschectman and others added 3 commits August 2, 2023 15:07

Reintroduce win lifecycle changes

87b8a88

Check for posthumous

7584237

Format

0631b8a

yaakovschectman added affects: desktop platform-windows labels Aug 3, 2023

yaakovschectman requested review from cbracken, jnschulze, loic-sharma, moffatman, gspencergoog and moko256 August 3, 2023 20:52

yaakovschectman self-assigned this Aug 3, 2023

yaakovschectman marked this pull request as ready for review August 3, 2023 21:28

loic-sharma reviewed Aug 4, 2023

View reviewed changes

cbracken reviewed Aug 8, 2023

View reviewed changes

yaakovschectman and others added 3 commits August 8, 2023 14:31

Move Destroy

d334d04

Format

4d445e2

Merge branch 'main' into rewinlc

4ce9a69

loic-sharma reviewed Aug 8, 2023

View reviewed changes

PR comments

54b7629

loic-sharma reviewed Aug 9, 2023

View reviewed changes

shell/platform/windows/windows_lifecycle_manager.cc Show resolved Hide resolved

More comments

b91b1fa

Merge branch 'main' into rewinlc

c4ff30c

yaakovschectman mentioned this pull request Aug 9, 2023

[Windows] Merge FlutterWindow with Window flutter/flutter#132260

Closed

loic-sharma reviewed Aug 10, 2023

View reviewed changes

shell/platform/windows/windows_lifecycle_manager.cc Outdated Show resolved Hide resolved

loic-sharma approved these changes Aug 10, 2023

View reviewed changes

yaakovschectman requested a review from cbracken August 10, 2023 17:30

yaakovschectman and others added 2 commits August 10, 2023 13:31

Update shell/platform/windows/windows_lifecycle_manager.cc

a29e31b

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

Merge branch 'main' into rewinlc

a4351df

cbracken approved these changes Aug 10, 2023

View reviewed changes

yaakovschectman merged commit f53ad9c into flutter:main Aug 10, 2023

yaakovschectman deleted the rewinlc branch August 10, 2023 20:50

engine-flutter-autoroll mentioned this pull request Aug 10, 2023

Roll Flutter Engine from a9be77e6f475 to 16b01b98af20 (6 revisions) flutter/flutter#132332

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 10, 2023

f53ad9c61 Reintroduce Windows lifecycle with guard for posthumous `On…

7d13756

…WindowStateEvent` (flutter/engine#44344)

HansMuller mentioned this pull request Aug 11, 2023

Flutter Framework Team: Notable Commits flutter/flutter#121415

Closed

	void FlutterDesktopViewControllerDestroy(
	FlutterDesktopViewControllerRef controller) {
	delete controller;
	}

	// state updates. This does not consume the window message. Non-Flutter
	// state updates. Non-Flutter

	// Claled for FlutterDesktopEngineProcessExternalWindowMessage.
	// Called for FlutterDesktopEngineProcessExternalWindowMessage.

	// state updates. This does not consume the window message. Non-Flutter windows
	// state updates. Non-Flutter windows

	// for application lifecycle state updates. Returns true when the message is
	// for application lifecycle state updates. Returns true if the message should be

		// the logic for application lifecycle state updates. Returns a result when
		// the message has been consumed.

		// state updates. Non-Flutter
		// windows must call this method in their WndProc in order to be included in

	// Release OS resources associated with window.
	// Release OS resources associated with the window.

Reintroduce Windows lifecycle with guard for posthumous OnWindowStateEvent #44344

Reintroduce Windows lifecycle with guard for posthumous OnWindowStateEvent #44344

Uh oh!

Conversation

yaakovschectman commented Aug 3, 2023

Pre-launch Checklist

Uh oh!

yaakovschectman commented Aug 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

loic-sharma commented Aug 9, 2023

Uh oh!

yaakovschectman commented Aug 10, 2023

Uh oh!

Uh oh!

loic-sharma left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reintroduce Windows lifecycle with guard for posthumous `OnWindowStateEvent` #44344

Reintroduce Windows lifecycle with guard for posthumous `OnWindowStateEvent` #44344

cbracken left a comment •

edited

Loading