Bug 26429 – Make JSON.stringify non-recursive so it can handle objects of arbitrary complexity

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Darin Adler]

Oliver asked me to make JSON.stringify non-recursive. I have done this.

[Darin Adler]

Created attachment 31334 [details]
patch

[Oliver Hunt]

Comment on attachment 31334 [details]
patch

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.

[Darin Adler]

(In reply to comment #2)
> 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 do not think this problem is is real. The code only casts to JSArray* in one place, and I check before doing that using the isJSArray patch just as your original code did. The only thing I removed from the original code was the word "Real" in the name of the boolean, and I suspect that's why you were convinced it would not work. Look again!

> m_holdStack is not a safe way to reference the holders.

Good point! Those need to be marked. I wonder why I didn't see any failures due to this.

> (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.

Using MarkedArgumentBuffer sounds pretty unpleasant. Is it really better than calling gcProtect in Holder's constructor and gcUnprotect in Holder's destructor?

[Darin Adler]

Created attachment 31353 [details]
work in progress

[Darin Adler]

Created attachment 31479 [details]
patch with GC marking issue fixed

[Darin Adler]

http://trac.webkit.org/changeset/44813