644349 - Somehow deal with stack overflow in JS_DeepFreezeObject

Status: NEW

(Core :: JavaScript Engine, defect, P5)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I ran into a problem with JS_DeepFreezeObject in bug 569422. The problem is that the testDeepFreeze_deep jsapi-test overflows the stack limit. The problem was triggered by my patch because it adds a GC point inside JS_DeepFreezeObject, and the GC asserts that we haven't overflowed the stack limit. As a quick fix, I changed the test to freeze a smaller tree.

I talked this over with Jeff and we're a bit unsure how to address it. I would like to add an assertion to JS_DeepFreezeObject to ensure that you don't pass it anything too big. But I don't know what this function is used for and whether this is really safe.

Igor, do you have any advice?

Comment 1

[Jeff Walden [:Waldo]]

I'm inclined to say we should remove JS_DeepFreezeObject, or make it use a work list, or something.  Freezing object graphs has bad corner cases like over-deep recursion (how deep is too deep? stack limit? &c.), cycles in the graph, and more.  The policies for how these should be handled seem like things the JSAPI client should decide for himself.  So I think the best choice is to remove JS_DeepFreezeObject and instead have the user write the loop-over-properties-and-recur code.

Comment 2

[Igor Bukanov]

As a rule any function that can trigger recursion must check for the stack size using JS_CHECK_RECURSION macro. So we should either add JS_CHECK_RECURSION checks or implement a version of JS_DeepFreezeObject that avoids recursion.

Comment 3

[Brendan Eich [:brendan]]

Easy to fix per comment 2.

/be