Bug 140093

Summary: Argument object created by "Function dot arguments" should use a clone of argument values
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren, mark.lam, mmirman, msaboff, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 139827    
Attachments:
Description Flags
the patch.
none
patch 2: fixed a typo in the ChangeLog.
none
patch 2: fixed bugs from the previous patch + add needed tests. ggaren: review+, ggaren: commit-queue-

Mark Lam
Reported 2015-01-05 13:07:04 PST
After https://bugs.webkit.org/show_bug.cgi?id=139827, a few test failures will start failing. One of them is dfg-tear-off-arguments-not-activation.js, which can be run this way: $ jsc --useFTLJIT\=false --enableFunctionDotArguments\=true --testTheFTL\=true --useFTLJIT\=true resources/standalone-pre.js dfg-tear-off-arguments-not-activation.js resources/standalone-post.js The relevant code is as follows: function bar() { return foo.arguments; } function foo(p) { var x = 42; if (p) return (function() { return x; }); else return bar(); } In this case, foo() has no knowledge of bar() needing the LexicalEnvironment and has dead code eliminated the SetLocal that stores it into its designated local. In bar(), the factory for the Arguments object (for creating foo.arguments) tries to read foo's LexicalEnvironment from its designated lexicalEnvironment local, but instead, finds it to be uninitialized. This results in a null pointer access which causes a crash. This can be resolved by having bar() instantiate a clone of the Arguments object instead, and populate its elements with values fetched directly from foo's frame. There's no need to reference foo's LexicalEnvironment (whether present or not).
Attachments
the patch. (4.24 KB, patch)
2015-01-05 13:36 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
patch 2: fixed a typo in the ChangeLog. (4.25 KB, patch)
2015-01-08 11:50 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
patch 2: fixed bugs from the previous patch + add needed tests. (21.78 KB, patch)
2015-01-14 20:40 PST, Mark Lam 		ggaren: review+
ggaren: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2015-01-05 13:28:00 PST
<rdar://problem/19377712>
Mark Lam
Comment 2 2015-01-05 13:36:04 PST
Created attachment 243998 [details] the patch. This patch is for keeping track of the work in progress. Will finalize this fix once the remaining issues that fall out of <https://webkit.org/b/139827> become clearer.
Mark Lam
Comment 3 2015-01-08 11:50:54 PST
Created attachment 244277 [details] patch 2: fixed a typo in the ChangeLog.
Geoffrey Garen
Comment 4 2015-01-08 16:07:19 PST
Comment on attachment 244277 [details] patch 2: fixed a typo in the ChangeLog. r=me
Geoffrey Garen
Comment 5 2015-01-08 16:10:11 PST
You should also add a test, if one doesn't exist, that notes the change in behavior in this case: function a(arg0) { var unused = arguments; arg0 = "changed"; return function b() { var unused = arg0; return a.arguments[0]; } } a("original")(); Before your patch, you'll get "changed", and after you'll get "original".
Mark Lam
Comment 6 2015-01-08 16:12:24 PST
(In reply to comment #5) > You should also add a test, if one doesn't exist, that notes the change in > behavior in this case: ... OK, will do.
WebKit Commit Bot
Comment 7 2015-01-08 16:49:37 PST
Comment on attachment 244277 [details] patch 2: fixed a typo in the ChangeLog. Clearing flags on attachment: 244277 Committed r178145: <http://trac.webkit.org/changeset/178145>
WebKit Commit Bot
Comment 8 2015-01-08 16:49:41 PST
All reviewed patches have been landed. Closing bug.
Mark Lam
Comment 9 2015-01-13 18:48:36 PST
(In reply to comment #5) > You should also add a test, if one doesn't exist, that notes the change in > behavior in this case: > > function a(arg0) > { > var unused = arguments; > arg0 = "changed"; > return function b() > { > var unused = arg0; > return a.arguments[0]; > } > } > > a("original")(); > > Before your patch, you'll get "changed", and after you'll get "original". Turns out this use of function dot argument does not do what we think it does. The value of a.arguments in b turns out to be null. This is consistently so in Safari, Chrome, and Firefox. If b() was invoked while a() is still on the stack (invoked either directly or indirectly from inside a()), then a.arguments[0] will be "changed" (also consistently so across all 3 browsers). With WebKit ToT, something appears to be broken. The cases that should have returned "changed" is now returning undefined. Will investigate.
Geoffrey Garen
Comment 10 2015-01-14 11:08:48 PST
> Turns out this use of function dot argument does not do what we think it > does. The value of a.arguments in b turns out to be null. This is > consistently so in Safari, Chrome, and Firefox. It does what we think it does -- I just wrote the test case incorrectly. Here is the correct test: > > function a(arg0) > > { > > var unused = arguments; > > arg0 = "changed"; > > return (function b() > > { > > var unused = arg0; > > return a.arguments[0]; > > })(); > > }
Mark Lam
Comment 11 2015-01-14 20:39:44 PST
Reopened because we've found that the previous patch is insufficient. Will upload a follow up patch in a moment.
Mark Lam
Comment 12 2015-01-14 20:40:27 PST
Created attachment 244676 [details] patch 2: fixed bugs from the previous patch + add needed tests.
Geoffrey Garen
Comment 13 2015-01-15 10:40:41 PST
Comment on attachment 244676 [details] patch 2: fixed bugs from the previous patch + add needed tests. View in context: https://bugs.webkit.org/attachment.cgi?id=244676&action=review r=me with comments > Source/JavaScriptCore/runtime/Arguments.cpp:389 > + if (isTornOff()) > + return; This should be an ASSERT. Only non-cloned arguments is ambiguous about when it will be torn off. > Source/JavaScriptCore/runtime/Arguments.cpp:399 > + ASSERT(!m_slowArgumentData.get()); No need for .get() here. > Source/JavaScriptCore/runtime/Arguments.cpp:408 > + if (isTornOff()) > + return; Ditto. > Source/JavaScriptCore/runtime/Arguments.cpp:415 > + ASSERT(!m_slowArgumentData.get()); Ditto.
Mark Lam
Comment 14 2015-01-15 11:21:03 PST
Thanks for the review. The patch has been updated based on the feedback, and landed in r178517: <http://trac.webkit.org/r178517>.
Note You need to log in before you can comment on or make changes to this bug.