Windows HasStrings error when backgrounded #32038

justinmc · 2022-03-15T18:16:38Z

This PR works around an error that happened when HasStrings checked the clipboard (in order to see if the Paste button could be shown) when the app was not in the foreground. That information isn't relevant when the app is in the background, so instead of logging an error and worrying developers, I'm swallowing the error and returning false.

Fixes flutter/flutter#95817

flutter-dashboard · 2022-03-15T18:16:42Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

justinmc · 2022-03-15T20:43:17Z

@stuartmorgan This PR is following up on your comment flutter/flutter#95817 (comment).

I'm not sure how to test this since existing tests (added in #27749) are mocking PlatformHandlerWin32::GetHasStrings, the method that would need to be tested directly here.

stuartmorgan-g · 2022-03-24T03:04:04Z

I'm not sure how to test this since existing tests (added in #27749) are mocking PlatformHandlerWin32::GetHasStrings, the method that would need to be tested directly here.

What I've been doing for plugins where we need to mock out Win32 APIs is wrapping them with a very thin class, and mocking that instead. Conveniently, there's already a wrapper class around most of the APIs used here, there's just the GetLastError issue. So I would:

Move error retrieval into ScopedClipboard by either:
- adding a GetLastClipboardError(), which returns a value that's populated internally by the other implementation methods any time something fails, or
- changing all the signatures to return a composite value that includes error (e.g., std::variant<real return type, error int>)
Make a new abstract base class with the same interface as ScopedClipboard, that's not file-internal.
Make ScopedClipboard implement that interface.
Make the PlatformHandlerWin32 constructor allow taking a clipboard implementation, and internally default to an instance of ScopedClipboard if nothing is provided.

Then you can write tests that inject a fake clipboard that has controlled behavior (like emitting this specific error).

justinmc · 2022-03-29T23:05:32Z

@stuartmorgan Thanks for the idea! I've done it and was able to actually test HasStrings. It might need some cleanup still, let me know what you think.

stuartmorgan-g · 2022-04-07T14:39:01Z

shell/platform/windows/platform_handler_win32.cc

-  // successful.
-  bool Open(HWND window);
+  // Attempts to open the clipboard for the given window, returning the error
+  // code in the case of failure and -1 otherwise.


Why are you using -1 for success? That's pretty confusing, especially since ERROR_SUCCESS in Win32 is 0.

Yeah that was super confusing, switching to 0.

stuartmorgan-g · 2022-04-07T14:39:22Z

shell/platform/windows/platform_handler_win32.cc

-  //
-  // On failure, get error information with ::GetLastError().
+  // Sets the string content of the clipboard, returning the error code on
+  // failure and -1 otherwise.


stuartmorgan-g · 2022-04-07T14:42:45Z

shell/platform/windows/platform_handler_win32.cc

+  }
+  std::optional<std::wstring> string = static_cast<wchar_t*>(locked_data.get());
+  if (!string) {
+    return ::GetLastError();


What is the purpose of this new code? I'm not sure why there's an optional, or why you're checking for failure since I'm not clear on how the string creation would fail.

I think the way I wrote this was redundant. I've updated it so it will return an error if locked_data.get fails, otherwise it returns the string.

I'm still not sure why there's a std::optional at all. Why not just:

if (!locked_data.get()) { return ::GetLastError(); } return std::string(static_cast<wchar_t*>(locked_data.get()));

shell/platform/windows/platform_handler_win32.h

stuartmorgan-g · 2022-04-07T14:49:14Z

shell/platform/windows/platform_handler_win32.h

 private:
  // A reference to the Flutter view.
  FlutterWindowsView* view_;
+  // A clipboard instance that can be passed in.


'can be passed in for mocking in tests'

stuartmorgan-g · 2022-04-07T14:50:31Z

shell/platform/windows/platform_handler_win32.cc

+PlatformHandlerWin32::PlatformHandlerWin32(
+    BinaryMessenger* messenger,
+    FlutterWindowsView* view,
+    std::optional<ScopedClipboardInterface*> clipboard_reference = nullptr)


Default values should be in the declaration, not the definition.

shell/platform/windows/platform_handler_win32.cc

stuartmorgan-g · 2022-04-07T14:54:44Z

shell/platform/windows/platform_handler_win32.h

+  explicit PlatformHandlerWin32(
+      BinaryMessenger* messenger,
+      FlutterWindowsView* view,
+      std::optional<ScopedClipboardInterface*> clipboard_reference);


@cbracken Do you have a preference as the primary owner of this code on having constructor-based DI, vs. a private setter with a friend class, for things that will only ever be overridden for test mocks?

I'm going to go ahead and merge, but @cbracken if you would prefer it a different way when you see this then I'm happy to open a new PR to fix it.

stuartmorgan-g · 2022-04-07T14:56:00Z

shell/platform/windows/platform_handler_win32_unittests.cc

+  MOCK_METHOD0(NotImplementedInternal, void());
+};
+
+TEST(PlatformHandlerWin32, HasStringsAccessDeniedReturnsFalseWithoutError) {


We should add other test cases now that this code is being made testable. E.g., testing that other errors are still passed through, since that would have been easy to accidentally break in this PR. (Also that normal success cases work.)

skia-gold · 2022-04-08T19:53:25Z

Gold has detected about 18 new digest(s) on patchset 7.
View them at https://flutter-engine-gold.skia.org/cl/github/32038

skia-gold · 2022-04-08T23:13:25Z

Gold has detected about 18 new digest(s) on patchset 11.
View them at https://flutter-engine-gold.skia.org/cl/github/32038

skia-gold · 2022-04-12T00:08:25Z

Gold has detected about 18 new digest(s) on patchset 12.
View them at https://flutter-engine-gold.skia.org/cl/github/32038

skia-gold · 2022-04-12T17:08:25Z

Gold has detected about 18 new digest(s) on patchset 13.
View them at https://flutter-engine-gold.skia.org/cl/github/32038

skia-gold · 2022-04-12T17:53:25Z

Gold has detected about 18 new digest(s) on patchset 13.
View them at https://flutter-engine-gold.skia.org/cl/github/32038

justinmc · 2022-04-12T19:56:18Z

This gold bot isn't indicating any actual failures, and per @Piinks it should be ignored here.

justinmc · 2022-04-12T19:58:32Z

@stuartmorgan Ready for re-review. Thanks for all of the help here. Let me know if I should make a change for #32038 (comment).

stuartmorgan-g

Small nits, otherwise looks good. Adding @cbracken as a reviewer for the question above about test object injection approach.

stuartmorgan-g · 2022-04-18T19:01:53Z

shell/platform/windows/platform_handler_win32.cc

+  }
+  std::optional<std::wstring> string = static_cast<wchar_t*>(locked_data.get());
+  if (!string) {
+    return ::GetLastError();


I'm still not sure why there's a std::optional at all. Why not just:

if (!locked_data.get()) { return ::GetLastError(); } return std::string(static_cast<wchar_t*>(locked_data.get()));

justinmc · 2022-04-18T20:57:53Z

@stuartmorgan Thanks for the suggestion, done and ready for re-review.

stuartmorgan-g

LGTM with nits.

stuartmorgan-g · 2022-05-02T19:16:28Z

shell/platform/windows/platform_handler_win32_unittests.cc

+// A test version of the private ScopedClipboard.
+class TestScopedClipboard : public ScopedClipboardInterface {
+ public:
+  TestScopedClipboard(int open_error, bool has_strings);


Nit: bool elements are really hard to read at the call site for non-trivial cases (e.g., SetSomeBool(bool)); consider making an enum instead (enum class ClipboardState { kHasString, kNoString }) and using that for the argument type.

stuartmorgan-g · 2022-05-02T19:17:26Z

shell/platform/windows/platform_handler_win32_unittests.cc

+  TestScopedClipboard(TestScopedClipboard const&) = delete;
+  TestScopedClipboard& operator=(TestScopedClipboard const&) = delete;
+
+  int Open(HWND window);


The overrides need override annotations.

stuartmorgan-g · 2022-05-02T19:19:49Z

shell/platform/windows/platform_handler_win32_unittests.cc

+      std::make_unique<::testing::NiceMock<MockWindowBindingHandler>>());
+  // HasStrings will fail.
+  PlatformHandlerWin32 platform_handler(
+      &messenger, &view, std::make_unique<TestScopedClipboard>(1, true));


To make this self-documenting, I would make a local const int arbitraryErrorCode = 1; and pass that in.

…ing logic from this PR

overrides

)

tgucio · 2022-05-06T14:19:53Z

Hi @justinmc,

In the commit I see this:

  if (open_result != kErrorSuccess) {
    rapidjson::Document error_code;
    error_code.SetInt(open_result);
    // Swallow errors of type ERROR_ACCESS_DENIED. These happen when the app is
    // not in the foreground and GetHasStrings is irrelevant.
    // See https://github.com/flutter/flutter/issues/95817.
    if (error_code != kAccessDeniedErrorCode) {
      result->Error(kClipboardError, "Unable to open clipboard", error_code);
      return;
    }
    hasStrings = false;
  } else {
    hasStrings = clipboard_->HasString();
  }

Shouldn't the comparison be if (open_result != kAccessDeniedErrorCode) as error_code is a rapidjson::Document?

stuartmorgan-g · 2022-05-06T15:54:44Z

Shouldn't the comparison be if (open_result != kAccessDeniedErrorCode) as error_code is a rapidjson::Document?

rapidjson::Document has a bunch of implicit conversions and implicit == overrides, so this is probably correct as written.

But it would be clearer (and more efficient in the case of this specific error) to move the creation of error_code completely into the inner if, and switch the check to open_result.

justinmc · 2022-05-06T17:46:22Z

Here's a quick PR to clean that up, thanks for the suggestions: #33174

justinmc self-assigned this Mar 15, 2022

flutter-dashboard bot added the platform-windows label Mar 15, 2022

flutter-dashboard bot added the needs tests label Mar 15, 2022

justinmc requested a review from stuartmorgan-g March 15, 2022 20:40

stuartmorgan-g suggested changes Apr 7, 2022

View reviewed changes

justinmc force-pushed the windows-clipboard-bg-error branch from eb256b1 to 1ce47a1 Compare April 12, 2022 17:32

justinmc requested a review from stuartmorgan-g April 12, 2022 19:56

stuartmorgan-g suggested changes Apr 18, 2022

View reviewed changes

stuartmorgan-g requested a review from cbracken April 18, 2022 19:04

justinmc requested a review from stuartmorgan-g April 18, 2022 20:57

justinmc force-pushed the windows-clipboard-bg-error branch from 9667cd9 to 35ae1b9 Compare April 27, 2022 17:38

stuartmorgan-g approved these changes May 2, 2022

View reviewed changes

justinmc added 6 commits May 2, 2022 15:14

Work around background clipboard error

f4c9d3d

ScopeClipboard returns error codes and internalizes the error swallow…

b92feed

…ing logic from this PR

Can pass in scopedclipboard

fcd81d1

Test by injecting a fake ScopedClipboard

e4f31ba

Formatting

e97172e

License

1b764e6

justinmc added 10 commits May 2, 2022 15:15

Code review: 0 success error code

173da5d

Code review redundant GetStrings

519bc4f

Code review:virtual destructor and moved comments to header and

047bad1

overrides

Code review:overrides and clipboard_reference naming and remove optional

479b568

Code review:clipboard default parameter

437ab10

Code review: unique_ptr

dd97459

Code review: Full tests for hasstrings

33a98e3

Code review: No optional needed

bc6d7d8

Review: override annotations

58f2da5

Review: error code constant

853c6ee

justinmc force-pushed the windows-clipboard-bg-error branch from e46521f to 853c6ee Compare May 3, 2022 00:07

justinmc merged commit 130004b into flutter:main May 3, 2022

justinmc deleted the windows-clipboard-bg-error branch May 3, 2022 18:12

engine-flutter-autoroll mentioned this pull request May 3, 2022

Roll Engine from 37c3d89a78e0 to 6d9e19184d34 (4 revisions) flutter/flutter#102989

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 3, 2022

130004b Windows HasStrings error when backgrounded (flutter/engine#32038

1d52d4c

)

tgucio mentioned this pull request May 6, 2022

[Windows] Flutter app blocks other apps from accessing clipboard flutter/flutter#103205

Closed

justinmc mentioned this pull request May 6, 2022

Clean up Windows background hasstrings error code #33174

Merged

Windows HasStrings error when backgrounded #32038

Windows HasStrings error when backgrounded #32038

Uh oh!

Conversation

justinmc commented Mar 15, 2022

Uh oh!

flutter-dashboard bot commented Mar 15, 2022

Uh oh!

justinmc commented Mar 15, 2022

Uh oh!

stuartmorgan-g commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc commented Mar 29, 2022

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

skia-gold commented Apr 8, 2022

Uh oh!

skia-gold commented Apr 8, 2022

Uh oh!

skia-gold commented Apr 12, 2022

Uh oh!

skia-gold commented Apr 12, 2022

Uh oh!

skia-gold commented Apr 12, 2022

Uh oh!

justinmc commented Apr 12, 2022

Uh oh!

justinmc commented Apr 12, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Apr 18, 2022

Uh oh!

stuartmorgan-g left a comment

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!

tgucio commented May 6, 2022

Uh oh!

stuartmorgan-g commented May 6, 2022

Uh oh!

justinmc commented May 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

stuartmorgan-g commented Mar 24, 2022 •

edited

Loading