[google_sign_in] Fix crash on iOS caused by null hostedDomain

[ryanheise]

Description

Fixes google_sign_in crash on iOS when calling signIn() when hostedDomain is null.

Related Issues

Fixes flutter/flutter#44564

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
• All existing and new tests are passing.
• I updated/added relevant documentation (doc comments with ///).
• The analyzer (flutter analyze) does not report any problems on my PR.
• I read and followed the [Flutter Style Guide].
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
• I updated CHANGELOG.md to add a description of the change.
• I signed the [CLA].
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

• Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
• No, this is not a breaking change.

[Hixie]

Thanks for the contribution!
This will need tests before we can click the merge button.

[ryanheise]

Hi @Hixie ,

My PR address two causes for a crash on iOS, although there may be more potential causes that all seem to be going under the one umbrella.

The first change I made prevented the GoogleSignIn native code from crashing by ensuring hostedDomain was never null. But in the 4 months since I submitted this, it seems the underlying issue in the GoogleSignIn native code was fixed, thereby making my fix redundant. So I've reverted that change, and that also resolved a problem with the tests that I broke.

The second change was to delete this line:

        [e raise];

This will immediately crash the app and serves no useful purpose (any uncaught exception on the Objective C side will crash the whole app). The line immediately prior to this actually sends a FlutterError over the platform channel, which is a good idea, but this is all for naught if we crash the app immediately afterwards.

I don't think any specific new tests are required or possible for this one line change.

As for all the other potential causes of an iOS crash, I don't know if this will help, but it may be a good idea to wrap the entire handleMethodCall in a try/catch, and forward the details of the exception over the platform channel in a FlutterError, since any uncaught exceptions will cause the whole Flutter app to crash.

In any case, I will leave that for someone else to do because I wrote the original PR 4 months ago on a rented MacInCloud instance and it is not convenient for me to do so.

[ryanheise]

Perhaps some can help explain why lint_darwin_plugins failed and what I need to do if anything? I can't imagine that failed because of anything I did if you look at the diffs.

Note: I have heard that the hostedDomain check that I took out actually still is required. So I'll need to add that back into the PR. This time, I'll add it in the Objective C code rather than as a default parameter in Dart so that the tests don't need to be rewritten.

[ryanheise]

@Hixie can you check why the submit-queue CI check failed? It doesn't appear to be anything caused by my change:

The following packages have analyzer errors (see above):
 * e2e

Maybe this was caused by the previous PR before mine:

## 4.5.2

* Update package:e2e reference to use the local version in the flutter/plugins
  repository.

As for my PR, I have added the hostedDomain null check back into the PR, and moved it into the Objective C code. This PR doesn't introduce a material change to the API, and it works as before, only it guards against a couple of reasons for the iOS crash.

[cyanglaz]

Hi @ryanheise. Do you plan to work on adding tests for this PR? :) #2599 has set up some unit tests for the plugin, maybe you can follow that pattern?

[ryanheise]

Hi @cyanglaz thanks for pointing to that. I was trying to learn how to do this from https://flutter.dev/docs/cookbook/testing but what you describe as unit testing sounds like it would only be possible in flutter with integration testing so that it can run on device. Maybe that is what e2e helps with, but is there a guide for this or another PR you can point me to that only adds a single test?

I'm also not sure how to test for the presence or absence of the GoogleService-Info.plist file, or presence and absence of a section in the Runner/Info.plist file. I can at least try to add a test for a null hostedDomain.

[cyanglaz]

For the purpose of this PR, although it is great, it might not be necessary to test if [GIDSignIn sharedInstance].hostedDomain actually works. Because [GIDSignIn sharedInstance].hostedDomain is from the google sign in sdk and we can assume it works.
However, we do want to test that if call.arguments[@"hostedDomain"] will set the correct value in [GIDSignIn sharedInstance].hostedDomain. An unit test with mocks would be sufficient.

[Hixie]

FWIW, if the bug was that the code crashed when doing X, the usual test is one that does X and verifies that the code doesn't crash. :-)

[stuartmorgan-g]

Can you explain why you added this? It doesn't look like the docs say this comes back on a different thread. Did you see this happening in production?

[ryanheise]

That was more a stab in the dark at addressing someone else's crash (which I couldn't reproduce myself): flutter/flutter#44564 (comment) , and I had intended to revert it after learning that it didn't fix his problem.

I've since moved off my macincloud subscription where I created this PR, so when I get a chance I'll need to set this up in my new dev environment.

[stuartmorgan-g]

Adding to the backlog; this is something we should add tests for as time allows so that it can be landed (modulo the question about the redispatch to main).

[stuartmorgan-g]

@jmagman @cyanglaz Could someone from the iOS platform team adopt this? It's a fix for a known crasher that just needs to be cleaned up and tested, so it would be great to get over the line.

[cyanglaz]

@stuartmorgan I'll take it. It could be some holiday chill hack for me :)

[cyanglaz]

The same issue was fixed in #3805. I'm going to close this.