[webview_flutter_wkwebview] Fixes crash when sending undefined message via javascript channel

[LinXunFeng]

Fixes flutter/flutter#162352

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[MyBlueford16]

Is there an update on when this PR will be approved and released? Its causing a blocking production issue for us

[bparrishMines]

LGTM

@stuartmorgan / @tarrinneal for secondary review.

[stuartmorgan-g]

How was '' chosen as the value here? Is this what the iOS implementation returned prior to the proxy API migration? Is it what webview_android returns? Other?

The linked issue notably says that it expects a different value (the string undefined), so I'd like to make sure we're picking '' for a clear reason.

[bparrishMines]

From my testing, passing undefined to the JavaScriptChannel leads to a null value in this callback. So in practice, undefined seems indistinguishable from a null value. However, I could see a rational for passing the string 'null' instead. I will check what happens on Android.

[bparrishMines]

It looks like Android returns the string 'undefined' and crashes when passing null.

[LinXunFeng]

Since I can't get the undefined passed through postMessage in the userContentController method, I've now overridden the postMessage method to preserve the undefined and null output.

Since I am not familiar with Android, I did not handle the crash issue when passing null. This may require another PR to handle.

[stuartmorgan-g]

Injecting JS into every single page is problematic; it can lead to a lot of unexpected and hard-to-debug edge cases. It should be a method of absolutely last resort only if there is no other way to accomplish necessary goals. Polyfilling slight differences between iOS and Android behaviors is not sufficiently critical to warrant such an approach.

[LinXunFeng]

I used webview_flutter: 2.8.0 and the value of message.message is "(null)", as shown in the screenshot of the code below.

[LinXunFeng]

The message.body was handled on the OC side in the older version of the iOS plugin, But now the it is passed directly to the Dart side, maybe we can use "${message.body}" to handle it like OC, what do you all think?

[bparrishMines]

I think it makes sense to return the string null when WKScriptMessage.body == null. I think this is better because a user could still differentiate between receiving a null or an empty string the majority of the time.

I think the main problem is that JavaScriptMessage.message is non-null in the platform interface, but changing this is a breaking change.

Your thoughts @stuartmorgan?

[stuartmorgan-g]

I think the main problem is that JavaScriptMessage.message is non-null in the platform interface, but changing this is a breaking change.

Yes, ideally we should have designed this to be more consistent with runJavaScript, where we distinguish unsupported values (in that case with an error, but we'd need something else here).

Given the constraints we have now, I think I would vote for '(null)' for consistency with previous implementations on iOS.

[LinXunFeng]

Okay, now I've adjusted it to return '(null)' when WKScriptMessage.body == null.

[stuartmorgan-g]

Flagging for changes to ensure that the PR doesn't land as currently written.

[stuartmorgan-g]

LGTM with nits.

@bparrishMines Are you okay with '(null)'?

[stuartmorgan-g]

This is misleading example code as written, since .toString can be called on null; the ?? '' part can never execute. You could just remove the ! from the previous code.

[LinXunFeng]

Done

[stuartmorgan-g]

The ! shouldn't be needed here, for the same reason.

[bparrishMines]

LGTM