Closed Bug 496223 Opened 15 years ago Closed 13 years ago

invalid cycles found by toString/toSource

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: luke, Unassigned)

Details

Attachments

(1 file)

User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.10) Gecko/2009042523 Ubuntu/9.04 (jaunty) Firefox/3.0.10 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.10) Gecko/2009042523 Ubuntu/9.04 (jaunty) Firefox/3.0.10 There are two examples. The first: var a1 = [1,2,3]; var o1 = { s:"", toSource:function(){this.s = a1.toSource(); return "{obj}"} }; var b1 = [a1,a1,o1]; print("a1.toSource(): " + a1.toSource()); print("b1.toSource(): " + b1.toSource()); print("should equal a1.toSource(): " + o1.s); Outputs: a1.toSource(): [1, 2, 3] b1.toSource(): [#1=[1, 2, 3], #1#, {obj}] should equal a1.toSource(): #1# Notice that the nested toSource() call has been given the sharped placeholder for a1, even though it was quite independent of the b1.toSource() call. In this example: var o2 = { s:"", toString:function(){this.s = a2.toString(); return "{obj}"} }; var a2 = [1,2,o2]; print("a2.toString(): " + a2.toString()); // should be infinite recursion The inner toString() call returns an empty string. This seems like it is "protecting" the user from their mistake, but it also masks a bug that would be caught immediately by JS_CHECK_RECURSION. The cause of both bugs is that sharpObjectMap is associated with the context, not the "root" of the recursive toSource/toString call. Thus, when there are simultaneous nested roots, they collide. A solution would be to change the key of sharpObjectMap->table from the object ptr to the pair (object ptr, root ptr), where root ptr is activation record of the root invocation. This feature of sharp objects was designed to handle the case shown in bug 484512 comment 6, but perhaps the example given there is also the user's fault, deserving of an infinite recursion? If this allowance was made, then Array.join performance (bug 200505) could be improved by only keeping a record of arrays, not all elements. Reproducible: Always Steps to Reproduce: Run attached file.
Assignee: nobody → general
Component: General → JavaScript Engine
OS: Linux → All
Product: Firefox → Core
QA Contact: general → general
Hardware: x86 → All
Version: unspecified → Trunk
This may be a dup. Igor or Jim may recall the bug. We talked about memoizing more aggressively, but uneval/toSource is a nest of potentially nasty effects. /be
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: DUPEME
There may be a dup bug, but I cannot find it. In any case, one way to address this is to make toSource implementation to detect that an object has the original toSource function and apply cycle detection only in those cases. In particular, the idea is to create the sharp map per top-level toSource innocation and then pass it to the nested toSource calls where toSource is the original (Array|Object).prototype.toSource. The assumption is that if somebody overwrites toSource, then presumably the author knows what he is doing. Any cycles that goes via custom toSource or toSource getter will be then detected via normal infinite recursion protection.
On Friday in #jsapi, we discussed an alternate strategy to fix this and bug 496213, and make toString faster (bug 200505). The idea is to remove the two-pass traversal and replace it with a single-pass traversal that passes around a "builder" object which has two member functions, appendChars and appendObject. appendChars is used to append string literals (like the ',' in an Array's toString) and appendObject is used to recursively append objects (like the elements of an Array). The advantages of this solution are: - By passing *objects* to builder.appendObject, not strings, builder can detect and prevent infinite recursion. Unrelated calls to toString are, by definition, the ones that occur outside the builder and will not be affected, which fixes this bug. - By making a single pass, the odd corner cases identified in bug 496213 cannot occur. In particular, the first example given in the bug description I believe would be difficult to fix with the current two-pass strategy. - The builder can use a single, growing internal buffer to hold the result. This should be a lot more efficient for Array.join and the like, since they require the creation of string objects for each element before creating a string object for the aggregate.
No more sharps.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
Whiteboard: DUPEME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: