Summary: | Argument object created by "Function dot arguments" should use a clone of argument values | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||
Component: | JavaScriptCore | Assignee: | 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
Mark Lam
2015-01-05 13:07: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. Created attachment 244277 [details]
patch 2: fixed a typo in the ChangeLog.
Comment on attachment 244277 [details]
patch 2: fixed a typo in the ChangeLog.
r=me
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". (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. 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> All reviewed patches have been landed. Closing bug. (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. > 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]; > > })(); > > } Reopened because we've found that the previous patch is insufficient. Will upload a follow up patch in a moment. Created attachment 244676 [details]
patch 2: fixed bugs from the previous patch + add needed tests.
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. Thanks for the review. The patch has been updated based on the feedback, and landed in r178517: <http://trac.webkit.org/r178517>. |