[webkit-reviews] review denied: [Bug 26429] Make JSON.stringify non-recursive so it can handle objects of arbitrary complexity : [Attachment 31334] patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 26429: Make JSON.stringify non-recursive so it can handle objects of
arbitrary complexity
https://bugs.webkit.org/show_bug.cgi?id=26429

Attachment 31334: patch
https://bugs.webkit.org/attachment.cgi?id=31334&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
r- due to potential problems with arrays and GC.

The array issue may not be real, but in my first reading of this patch i became
convinced that it would not correctly handle non-strict-JSArray instances like
RegExpResultArray and runtime_array.  I've reread the patch and am no longer
sure that the behaviour is wrong, but i do think the tests need to actually
include serialisation tests for these objects.	I will endeavour to write some
up and get such landed tonight.

m_holdStack is not a safe way to reference the holders.  Due to the potential
for arbitrary js code execution it is possible to create a scenario in which
there would be no "live" references to an object that is actually live
(according to the holder stack).  I recently updated BufferedArgList to give it
isEmpty() and removeLast() functionality (i haven't yet broken it out or made
it more generalised), so you should use that as an additional reference for the
raw JSObjects that are currently active holders -- in the general case i don't
believe it should be significantly slower to control this additional structure
and in general (from my profiling) string concat, etc was the perf bottleneck
anyway by a fairly large margin.

Other than these issues i think the patch is good.