Bug 145709

Summary: Subclasses of JSNonFinalObject with gc'able children need to implement visitChildren()
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch.
ggaren: review+, ggaren: commit-queue-
patch for landing with test added none

Description Mark Lam 2015-06-05 12:51:55 PDT
The ClonedArguments class is missing a visitChildren.  Ditto for the Element class in jsc.cpp.
Comment 1 Mark Lam 2015-06-05 12:59:41 PDT
Created attachment 254374 [details]
the patch.
Comment 2 Geoffrey Garen 2015-06-05 14:28:02 PDT
Comment on attachment 254374 [details]
the patch.

r=me

ClonedArguments should be testable. Just allocate a lot of function.arguments for transient function objects, then force gc, and then access .callee on each one. You'll crash pretty quickly.
Comment 3 Mark Lam 2015-06-05 14:29:32 PDT
(In reply to comment #2)
> ClonedArguments should be testable. Just allocate a lot of
> function.arguments for transient function objects, then force gc, and then
> access .callee on each one. You'll crash pretty quickly.

k, I’ll work on a test.
Comment 4 Mark Lam 2015-06-05 16:02:48 PDT
(In reply to comment #2)
> ClonedArguments should be testable. Just allocate a lot of
> function.arguments for transient function objects, then force gc, and then
> access .callee on each one. You'll crash pretty quickly.

Actually, this turned out to be harder than we thought.  Here's why:
1. With one exception (the StackVisitor), ClonedArguments are only created by op_create_out_of_band_arguments.

2. The BytecodeGenerator will only emit an op_create_out_of_band_arguments if the function is in strict mode.

3. Since ES5, access to arguments.callee is forbidden for a strict mode function.  See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments/callee).

Hence, I can't get a scenario in JS code where I can naturally see this issue manifest.

I mentioned that there was an exception i.e. the StackVisitor, which returns a ClonedArguments object when StackVisitor::arguments() is called.  The remaining question is whether we can build a case around the use of StackVisitor::arguments().
Comment 5 Mark Lam 2015-06-05 16:55:38 PDT
I've found a way to reproduce this now.  The StackVisitor:;Frame::createArguments() (not StackVisitor::arguments()) lead takes me to Function.arguments which does reproduce the issue without strict mode.
Comment 6 Mark Lam 2015-06-05 17:25:17 PDT
Created attachment 254399 [details]
patch for landing with test added
Comment 7 Mark Lam 2015-06-05 17:34:52 PDT
Thanks for the review.  Landed in r185277: <http://trac.webkit.org/r185277>.