Bug 145709 - Subclasses of JSNonFinalObject with gc'able children need to implement visitChildren()
Summary: Subclasses of JSNonFinalObject with gc'able children need to implement visitC...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-05 12:51 PDT by Mark Lam
Modified: 2015-06-05 17:34 PDT (History)
0 users

See Also:


Attachments
the patch. (4.57 KB, patch)
2015-06-05 12:59 PDT, Mark Lam
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff
patch for landing with test added (6.05 KB, patch)
2015-06-05 17:25 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>.