[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
https://bugs.webkit.org/show_bug.cgi?id=173662

Attachment 328151: Take 2

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




--- 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 =
JSON::ArrayOf<JSON::Value>::create();

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();
    array->addItem(array);
    array->toJSONString();

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
	assertValidity();
    #endif
	StringBuilder result;
	result.reserveCapacity(512);
	writeJSON(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