Bug 26429 - Make JSON.stringify non-recursive so it can handle objects of arbitrary complexity
Summary: Make JSON.stringify non-recursive so it can handle objects of arbitrary compl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-15 21:54 PDT by Darin Adler
Modified: 2009-06-18 12:22 PDT (History)
0 users

See Also:


Attachments
patch (37.08 KB, patch)
2009-06-15 22:53 PDT, Darin Adler
oliver: review-
Details | Formatted Diff | Diff
work in progress (41.82 KB, patch)
2009-06-16 10:06 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch with GC marking issue fixed (46.98 KB, patch)
2009-06-17 18:08 PDT, Darin Adler
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2009-06-15 21:54:41 PDT
Oliver asked me to make JSON.stringify non-recursive. I have done this.
Comment 1 Darin Adler 2009-06-15 22:53:47 PDT
Created attachment 31334 [details]
patch
Comment 2 Oliver Hunt 2009-06-15 23:51:25 PDT
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.
Comment 3 Darin Adler 2009-06-16 00:00:50 PDT
(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?
Comment 4 Darin Adler 2009-06-16 10:06:28 PDT
Created attachment 31353 [details]
work in progress
Comment 5 Darin Adler 2009-06-17 18:08:22 PDT
Created attachment 31479 [details]
patch with GC marking issue fixed
Comment 6 Darin Adler 2009-06-18 12:22:11 PDT
http://trac.webkit.org/changeset/44813