[webview_flutter_wkwebview] Fixes JSON.stringify() cannot serialize cyclic structures #6274

LinXunFeng · 2024-03-06T04:01:34Z

Using the replacer parameter of JSON.stringify() to remove cyclic object to resolve the following error.

TypeError: JSON.stringify cannot serialize cyclic structures.

~~Related solution: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value~~

Fixes flutter/flutter#144535

Pre-launch Checklist

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

jmagman · 2024-03-06T21:10:55Z

This is in wkwebview but maybe @ditman could review the js code?

ditman · 2024-03-08T00:49:08Z

Can do, thanks for the mention @jmagman, I normally disregard webview stuff because I end up flagged as an owner (but I'm not :P)

Ahem:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value#circular_references

ditman · 2024-03-08T19:52:45Z

The biggest problem I see with this is that this is a "lossy" fix; what you deserialize is not what you were serializing; "[Cyclic]" might be good for the json encoding to not crash, but it doesn't tell you how to recover that cycle when you parse JSON back to an object?

(Also if the cycle is not important: maybe remove it before attempting to serialize to JSON?)

((PS: I don't think I've had to deal with JSON malformed like this before, but maybe with something like GraphQL you could end up with a circular dependency on your data, not sure...))

bparrishMines

LGTM

I didn't originally realize this was preventing an error from a method that was already a part of the script we were already injecting. I think this solution looks good.

cc @ditman or @stuartmorgan for secondary review.

bparrishMines · 2024-03-08T20:06:25Z

The biggest problem I see with this is that this is a "lossy" fix; what you deserialize is not what you were serializing; "[Cyclic]" might be good for the json encoding to not crash, but it doesn't tell you how to recover that cycle when you parse JSON back to an object?

(Also if the cycle is not important: maybe remove it before attempting to serialize to JSON?)

Yea, I was considering this as well. But do think it is reasonable for a user to expect that console.log should be able to handle any object? Does the problem lie with us using JSON.stringify() instead of just using String()? I would consider calling console.log as the equivalent of calling print(...) in a Flutter app and would be surprised if this caused an error on recursion. It might be a breaking change to move away from JSON.stringify() now though, so my reasoning was to avoid the crash as the best solution right now.

bparrishMines · 2024-03-08T20:12:48Z

Looking at the issue and the Android output, it looks like Android basically just calls String() on an object passed to the console. Android just returns [object Object] when passed a recursive object.

ditman · 2024-03-08T20:14:48Z

Ah! I didn't realize this was to implement console.log. In that case, this is probably fine.

The way this works in the browser is by it not attempting to "toString" the whole thing, it just gives you a tree-like structure for the object:

(You get tired of expanding self before the browser runs out of memory, I think :P)

The PR is a C+P from MDN however; @stuartmorgan licensing concerns?

stuartmorgan · 2024-03-11T16:28:20Z

The PR is a C+P from MDN however; @stuartmorgan licensing concerns?

What is done in this PR is not permitted in Flutter.

This code is not complicated enough to warrant us adding a third_party directory to this plugin; we should get a clean implementation of the webkit_webview_controller.dart change (by giving someone who has not seen this code just the unit test and a high-level description of the goal).

stuartmorgan

(Marking as changes needed to ensure this doesn't accidentally land as-is.)

LinXunFeng · 2024-03-12T11:23:29Z

@stuartmorgan Thanks for your reminder.

I have adjusted the implementation to solve this issue by removing the cyclic object, please review the code again.

stuartmorgan · 2024-03-12T15:15:26Z

Unless I'm missing something, the new version is destroying any duplicate objects, which is not the same thing as just removing cycles.

…kwebview # Conflicts: # packages/webview_flutter/webview_flutter_wkwebview/CHANGELOG.md # packages/webview_flutter/webview_flutter_wkwebview/pubspec.yaml

LinXunFeng · 2024-03-15T10:46:38Z

@stuartmorgan I've re-adjusted the removal logic.

jmagman · 2024-04-10T20:56:04Z

cc ~~@hellohuanlin~~ @cbracken to review

stuartmorgan · 2024-04-24T18:36:46Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webkit_webview_controller.dart

    const WKUserScript overrideScript = WKUserScript(
      '''
+function removeCyclicObject() {
+  const levelObjects = [];


I'm not sure what this naming means; what is a "level object"?

levelObjects is used to record objects whose properties are currently being traversed. (Maybe I should rename it to stack or something? )

As shown in the figure below, the properties of the outermost object (blue box) are currently being traversed, and obj1 (green box) is also an object, so the properties of obj1 will be traversed next and pushed in levelObjects.

When traversing all the properties of obj1 (green box) and starting to traverse obj2, currentParentObj will point to the outermost object (blue box) and the last element obj1 (green box) of the current levelObjects is not equal, then obj1 will be popped up, and obj2 will be pushed to levelObjects.

stuartmorgan · 2024-04-24T18:39:05Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webkit_webview_controller.dart

    const WKUserScript overrideScript = WKUserScript(
      '''
+function removeCyclicObject() {


This should have a name that cannot plausibly cause collisions on actual web pages, since it is injected into the page's context. E.g., maybe a Flutter prefix and a dynamically generated UUID? @ditman Any suggestions on best practice for avoiding naming collisions in JS?

@stuartmorgan At the expense of some performance this function could be defined inside of the log function that calls it (line 659), and that way it wouldn't be directly exposed to the host page... But if we're not worried about a function log colliding with the functions of a webpage, I wouldn't worry too much about removeCyclicObject either :)

(In the engine we normally prefix with _ the name of the function so there's a fewer change of a global collision, but there's no guarantee that it won't collide. For a UUID you'd have to strip the - characters for it to be a valid function name, but it could definitely work)

((PS: Generating a UUID in JS: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID, has just become "available" to Flutter web according to our Supported platforms))

At the expense of some performance this function could be defined inside of the log function that calls it (line 659), and that way it wouldn't be directly exposed to the host page

That sounds worth doing. If people are doing performance-affecting levels of print logging (and enabling this feature in release builds, which hopefully they aren't), they have problems already.

But if we're not worried about a function log colliding with the functions of a webpage, I wouldn't worry too much about removeCyclicObject either :)

😬 I am very concerned about that, I just missed it somehow. We should absolutely rename that.

Maybe UUID is overkill; we could also just do something like _flutter_log_override that's pretty unlikely to collide.

((PS: Generating a UUID in JS: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID, has just become "available" to Flutter web according to our Supported platforms))

We wouldn't need to do it in JS; we'd generate it in Dart, and then string interpolate that into the string we inject.

We wouldn't need to do it in JS; we'd generate it in Dart, and then string interpolate that into the string we inject.

Ah you're right, it's "free" then. _flutter_log_override sounds like a good name. You can even have a small _flutter_webview_plugin_overrides "namespace" object that contains all the methods that you're injecting, so it's easy to hide/move them around.

stuartmorgan · 2024-04-24T18:39:56Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webkit_webview_controller.dart

+    // See https://github.com/flutter/flutter/issues/144535.
+    //
+    // Considering this is just looking at the logs printed via console.log,
+    // the cyclic object is not important, so remove it.
    const WKUserScript overrideScript = WKUserScript(


Please add a comment briefly explaining the approach at a high level so readers don't need to reverse-engineer what is being done here.

stuartmorgan

LGTM with one naming suggestion.

stuartmorgan · 2024-05-07T16:54:34Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webkit_webview_controller.dart

-}
+var _flutter_webview_plugin_overrides = _flutter_webview_plugin_overrides || {
+  removeCyclicObject: function() {
+    const levelObjects = [];


Based on the description in the PR comment, traversalStack would probably be a clearer name.

…erialize cyclic structures (flutter/packages#6274)

flutter/packages@fd714bd...87a02e3 2024-05-15 engine-flutter-autoroll@skia.org Roll Flutter from d2da1b2 to 39651e8 (18 revisions) (flutter/packages#6738) 2024-05-15 stuartmorgan@google.com Update the repo for the 3.22 stable release (flutter/packages#6730) 2024-05-15 linxunfeng@yeah.net [webview_flutter_wkwebview] Fixes JSON.stringify() cannot serialize cyclic structures (flutter/packages#6274) 2024-05-14 louisehsu@google.com [in_app_purchase_storekit] migrate main plugin class to swift in preperation to storekit 2 (flutter/packages#6561) 2024-05-14 34871572+gmackall@users.noreply.github.com [image_picker_android] Refactor getting of paths from intent to single helper (flutter/packages#5009) 2024-05-14 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 54e6646 to 5dcb86f (1402 revisions) (flutter/packages#6727) 2024-05-14 10687576+bparrishMines@users.noreply.github.com [webview_flutter_wkwebview] Skip `withWeakReferenceTo` integration test (flutter/packages#6731) 2024-05-14 engine-flutter-autoroll@skia.org Roll Flutter from 1255435 to d2da1b2 (26 revisions) (flutter/packages#6729) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…yclic structures (flutter#6274) Using the `replacer` parameter of `JSON.stringify()` to remove cyclic object to resolve the following error. ``` TypeError: JSON.stringify cannot serialize cyclic structures. ``` ~~Related solution: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value~~ Fixes flutter/flutter#144535

Fixs JSON.stringify() cannot serialize cyclic structures

6057b1d

LinXunFeng requested a review from hellohuanlin as a code owner March 6, 2024 04:01

github-actions bot added platform-ios p: webview_flutter labels Mar 6, 2024

LinXunFeng changed the title ~~[webview_flutter_wkwebview] Fixs JSON.stringify() cannot serialize cyclic structures~~ [webview_flutter_wkwebview] Fixes JSON.stringify() cannot serialize cyclic structures Mar 6, 2024

Update version and CHANGELOG

a37f7bf

LinXunFeng requested review from bparrishMines and mvanbeusekom March 6, 2024 05:33

bparrishMines approved these changes Mar 8, 2024

View reviewed changes

stuartmorgan requested changes Mar 11, 2024

View reviewed changes

bparrishMines self-requested a review March 11, 2024 18:03

LinXunFeng added 3 commits March 12, 2024 18:48

Remove cyclic object

182afac

Remove whitespace

ee66a4a

Remove whitespace

a709175

bparrishMines removed the request for review from mvanbeusekom March 13, 2024 17:50

LinXunFeng added 2 commits March 15, 2024 18:10

Improve logic

b7a3661

Merge branch 'main' into fix_json_stringify_cyclic_structures_error_w…

f7ac9bd

…kwebview # Conflicts: # packages/webview_flutter/webview_flutter_wkwebview/CHANGELOG.md # packages/webview_flutter/webview_flutter_wkwebview/pubspec.yaml

jmagman requested review from cbracken and removed request for hellohuanlin April 10, 2024 20:56

stuartmorgan requested changes Apr 24, 2024

View reviewed changes

LinXunFeng added 4 commits April 30, 2024 09:07

Adjust to use _flutter_webview_plugin_overrides namespace

ae1a4a2

Update comment

979d8ee

Update comment

fd9bbcf

Update AUTHORS

c32dedd

stuartmorgan approved these changes May 7, 2024

View reviewed changes

Rename

3700600

LinXunFeng added the autosubmit Merge PR when tree becomes green via auto submit App label May 15, 2024

auto-submit bot merged commit 0870dc8 into flutter:main May 15, 2024
78 checks passed

engine-flutter-autoroll mentioned this pull request May 15, 2024

Roll Packages from fd714bd7d516 to 788b66669374 (7 revisions) flutter/flutter#148416

Closed

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

0870dc84d [webview_flutter_wkwebview] Fixes JSON.stringify() cannot s…

0151a8e

…erialize cyclic structures (flutter/packages#6274)

engine-flutter-autoroll mentioned this pull request May 15, 2024

Roll Packages from fd714bd7d516 to 87a02e393be0 (8 revisions) flutter/flutter#148419

Merged

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

0870dc84d [webview_flutter_wkwebview] Fixes JSON.stringify() cannot s…

00d2c23

…erialize cyclic structures (flutter/packages#6274)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[webview_flutter_wkwebview] Fixes JSON.stringify() cannot serialize cyclic structures #6274

[webview_flutter_wkwebview] Fixes JSON.stringify() cannot serialize cyclic structures #6274

LinXunFeng commented Mar 6, 2024 •

edited by stuartmorgan

jmagman commented Mar 6, 2024

ditman commented Mar 8, 2024 •

edited

ditman commented Mar 8, 2024 •

edited

bparrishMines left a comment

bparrishMines commented Mar 8, 2024 •

edited

bparrishMines commented Mar 8, 2024

ditman commented Mar 8, 2024 •

edited

stuartmorgan commented Mar 11, 2024 •

edited

stuartmorgan left a comment

LinXunFeng commented Mar 12, 2024

stuartmorgan commented Mar 12, 2024

LinXunFeng commented Mar 15, 2024

jmagman commented Apr 10, 2024 •

edited

stuartmorgan Apr 24, 2024

LinXunFeng Apr 30, 2024

stuartmorgan Apr 24, 2024

ditman Apr 25, 2024 •

edited

stuartmorgan Apr 26, 2024

ditman Apr 26, 2024

LinXunFeng Apr 30, 2024

stuartmorgan Apr 24, 2024

LinXunFeng Apr 30, 2024

stuartmorgan left a comment

stuartmorgan May 7, 2024

LinXunFeng May 8, 2024

[webview_flutter_wkwebview] Fixes JSON.stringify() cannot serialize cyclic structures #6274

[webview_flutter_wkwebview] Fixes JSON.stringify() cannot serialize cyclic structures #6274

Conversation

LinXunFeng commented Mar 6, 2024 • edited by stuartmorgan

Pre-launch Checklist

jmagman commented Mar 6, 2024

ditman commented Mar 8, 2024 • edited

ditman commented Mar 8, 2024 • edited

bparrishMines left a comment

Choose a reason for hiding this comment

bparrishMines commented Mar 8, 2024 • edited

bparrishMines commented Mar 8, 2024

ditman commented Mar 8, 2024 • edited

stuartmorgan commented Mar 11, 2024 • edited

stuartmorgan left a comment

Choose a reason for hiding this comment

LinXunFeng commented Mar 12, 2024

stuartmorgan commented Mar 12, 2024

LinXunFeng commented Mar 15, 2024

jmagman commented Apr 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ditman Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinXunFeng commented Mar 6, 2024 •

edited by stuartmorgan

ditman commented Mar 8, 2024 •

edited

ditman commented Mar 8, 2024 •

edited

bparrishMines commented Mar 8, 2024 •

edited

ditman commented Mar 8, 2024 •

edited

stuartmorgan commented Mar 11, 2024 •

edited

jmagman commented Apr 10, 2024 •

edited

ditman Apr 25, 2024 •

edited