[Android] Check for text to paste before trying to retrieve data from URI

[camsim99]

Adds a check to retrieve explicit text from Clipboard items via getText before attempting to retrieve data if the Clipboard item (ClipData.Item) has an associated URI. Also adds more error handling for edge cases concerning URIs (the URI not having the content scheme or being null).

Fixes flutter/flutter#74320. Some content providers will not send URIs containing the text/* pattern or have the content scheme (content://), but ultimately we should expect Clipboard items to either fall not into one of those categories or have an explicit textual value available via getText. In case this is not true, though, checks for non-content URIs and null URIs are added, as well :)

Tested on a Samsung tablet with pasting from MS Word, MS Excel, and Samsung Notes.

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.

[stuartmorgan-g]

Even with the restructuring, shouldn't we be checking that the URI is non-null and also content:? We know from the docs that it's invalid to call openTypedAssetFileDescriptor and we know from the issue that calling this with at least file: will throw an exception that can potentially bring down the whole app.

Hopefully moving getText up will avoid that in most cases, but we've had a lot of weird plugin bugs over the years that are related to other apps sending us bad data (e.g., intent responses that have unexpected nulls), so it seems like we should have a defense-in-depth approach here to make sure that if someone sends us a file: URI with no text it won't throw.

[camsim99]

I think that explicitly returning null in the case where the URI is null makes sense to make sure no weird errors come up like you're referencing.

In terms of adding a check for content: before calling openTypedAssetFileDescriptor, that makes sense as well; I initially assumed a FileNotFoundException would be thrown (which we catch) which does not seem to be the case (edit: I think this is the case for the text/* check actually).

I think it would also be helpful to add some logs to these new branches to make it clear why null is returned.

[camsim99]

Addressed these comments, would you be willing to give it a second look? @stuartmorgan

[reidbaker]

Nonblocking nit: consider extracting uri logic into a method or methods.

[camsim99]

Moved logic trying to extract text from URI to a new method!

[camsim99]

Undid this due to some recent changes here that actually made the new method more confusing.

[reidbaker]

Can any of this setup logic be extracted into a logical helper method to share between tests?

[camsim99]

Moved overlapping logic to setup methods!

[stuartmorgan-g]

LGTM

[stuartmorgan-g]

When possible, it's better to have stand-alone setup methods that return objects, rather than making fixtures stateful; I really like https://abseil.io/tips/122 on this topic (written for C++, but applicable to any language).

Maybe that's not possible here; it looks like a fair number of these variables are individually referenced in tests rather than only being used internally. When it's not I would actually prefer duplication in tests rather than a lot of fixture state, but I don't feel too strongly about it here so if you and @reidbaker prefer this, that's fine with me.

The one exception would be the testPlatformPlugin and testPlatformPluginWithDelegate. I feel pretty strongly that the object under test itself should not be part of the fixture whenever possible, and the fact that there are different versions here, each of which only apply to some tests, is even more indication that these aren't really part of the overall test state, but part of tests. If you want to abstract out the creation, you could always make a stand-alone getter instead. E.g.,:

private PlatformPlugin createTestPlugin(bool withDelegate) {
  if (withDelegate) {
    return new PlatformPlugin(fakeActivity, mockPlatformChannel, mockPlatformPluginDelegate);
  } else {
    return new PlatformPlugin(fakeActivity, mockPlatformChannel);
  }
}

(Or two different methods, one for delegate, one for no delegate)

[camsim99]

Ah okay I was wondering if I went overboard :) thanks for the tips and explanation! This was good to learn.

I ended up keeping the setup method for clipboard tests (the ones related to this PR) and getting rid of the rest of the changes I made, leaving most of the duplication which is likely safer as you described.