[webkit-reviews] review granted: [Bug 173662] Web Inspector: move Inspector::Protocol::Array<T> to JSON namespace : [Attachment 328151] Take 2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Dec 1 14:19:04 PST 2017

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 173662: Web Inspector: move Inspector::Protocol::Array<T> to JSON namespace

Attachment 328151: Take 2


--- Comment #11 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 328151
  --> https://bugs.webkit.org/attachment.cgi?id=328151
Take 2

View in context: https://bugs.webkit.org/attachment.cgi?id=328151&action=review

r=me. Watch the bots closely!

> Tools/TestWebKitAPI/Tests/WTF/JSONValue.cpp:240
> +    Ref<JSON::ArrayOf<JSON::Value>> array =

You normally auto these =)

This test actually brings to mind that JSON doesn't allow cyclic references but
a programmer could create a cyclic reference.

It is certainly a programmer error to do so. But what happens in this case?

    auto array = JSON::ArrayOf<JSON::Value>::create();

It seems we would infinite loop. Since there is no cycle detection.

I wonder if we should have any kind of mitigations / detection with assertions.
Certainly not part of this patch but something to think about. A
straightforward approach would be:

    String toJSONString()
    #ifndef NDEBUG
	StringBuilder result;
	return result.toString();

But catching issues would be even better if the moment an item is added and a
cycle is detected that we catch it.

More information about the webkit-reviews mailing list