[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