Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
auto-submit merged 12 commits into from
May 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6057b1d
Fixs JSON.stringify() cannot serialize cyclic structures
LinXunFeng Mar 6, 2024
a37f7bf
Update version and CHANGELOG
LinXunFeng Mar 6, 2024
182afac
Remove cyclic object
LinXunFeng Mar 12, 2024
ee66a4a
Remove whitespace
LinXunFeng Mar 12, 2024
a709175
Remove whitespace
LinXunFeng Mar 12, 2024
b7a3661
Improve logic
LinXunFeng Mar 15, 2024
f7ac9bd
Merge branch 'main' into fix_json_stringify_cyclic_structures_error_w…
LinXunFeng Mar 15, 2024
ae1a4a2
Adjust to use _flutter_webview_plugin_overrides namespace
LinXunFeng Apr 30, 2024
979d8ee
Update comment
LinXunFeng Apr 30, 2024
fd9bbcf
Update comment
LinXunFeng Apr 30, 2024
c32dedd
Update AUTHORS
LinXunFeng Apr 30, 2024
3700600
Rename
LinXunFeng May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Improve logic
  • Loading branch information
@LinXunFeng
LinXunFeng committed Mar 15, 2024
commit b7a366182b0b443875b4b1cda9a0afc40e18e6f7
19 changes: 14 additions & 5 deletions ...view_flutter/webview_flutter_wkwebview/example/integration_test/webview_flutter_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1457,12 +1457,18 @@ Future<void> main() async {
<title>WebResourceError test</title>
<script type="text/javascript">
function onLoad() {
const obj1 = {
name: "obj1",
};
const obj2 = {
name: "obj2",
obj1: obj1,
};
const obj = {
a: "Flutter",
b: {},
obj1: obj1,
obj2: obj2,
};
obj.self = obj;
obj.b.self = obj;
console.log(obj);
}
</script>
Expand Down Expand Up @@ -1493,8 +1499,11 @@ Future<void> main() async {
},
));

await expectLater(debugMessageReceived.future,
completion('log:{"a":"Flutter","b":{}}'));
await expectLater(
debugMessageReceived.future,
completion(
'log:{"obj1":{"name":"obj1"},"obj2":{"name":"obj2","obj1":{"name":"obj1"}}}'),
);
});
});
}
Expand Down
13 changes: 10 additions & 3 deletions packages/webview_flutter/webview_flutter_wkwebview/lib/src/webkit_webview_controller.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,11 +639,18 @@ class WebKitWebViewController extends PlatformWebViewController {
const WKUserScript overrideScript = WKUserScript(
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@LinXunFeng LinXunFeng Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

'''
function removeCyclicObject() {
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@ditman ditman Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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))

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ditman ditman Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@LinXunFeng LinXunFeng Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

const objects = new WeakSet();
const levelObjects = [];
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@LinXunFeng LinXunFeng Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

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.

image

return function (k, v) {
if (typeof v !== "object" || v === null) { return v; }
if (objects.has(v)) { return; }
objects.add(v);
const currentParentObj = this;
while (
levelObjects.length > 0 &&
levelObjects[levelObjects.length - 1] !== currentParentObj
) {
levelObjects.pop();
}
if (levelObjects.includes(v)) { return; }
levelObjects.push(v);
return v;
};
}
Expand Down
13 changes: 10 additions & 3 deletions packages/webview_flutter/webview_flutter_wkwebview/test/webkit_webview_controller_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1379,11 +1379,18 @@ void main() {
WKUserScriptInjectionTime.atDocumentStart);
expect(overrideConsoleScript.source, '''
function removeCyclicObject() {
const objects = new WeakSet();
const levelObjects = [];
return function (k, v) {
if (typeof v !== "object" || v === null) { return v; }
if (objects.has(v)) { return; }
objects.add(v);
const currentParentObj = this;
while (
levelObjects.length > 0 &&
levelObjects[levelObjects.length - 1] !== currentParentObj
) {
levelObjects.pop();
}
if (levelObjects.includes(v)) { return; }
levelObjects.push(v);
return v;
};
}
Expand Down