|Summary:||Get rid of JSObject::getPropertyAttributes() and all usage of it|
|Product:||WebKit||Reporter:||Kent Hansen <kent.hansen>|
|Severity:||Normal||CC:||abarth, commit-queue, eric, ggaren, hausmann, oliver|
|Version:||528+ (Nightly build)|
|Bug Depends on:||32119|
Description Kent Hansen 2009-11-27 06:14:26 PST
Comment 1 Kent Hansen 2009-11-27 06:21:39 PST
Created attachment 43949 [details] Proposed patch Not sure what to do with the JSVariableObject::getPropertyAttributes() implementation. Is it really dead code after the move to getOwnPropertyDescriptor()? The tests that were added in r31225 still pass, but I'm wondering about the change to set the DontDelete attribute (part of r33979, SquirrelFish merge). Should that be done in JSGlobalObject::getOwnPropertyDescriptor() now? The alternative I see would be to reimplement getOwnPropertyDescriptor() in JSVariableObject, and set the DontDelete attribute on the result (to mimic getPropertyAttributes()). But then, really, JSActivationObject should reimplement getOwnPropertySlot() too, i.e. both those functions (and hasOwnPropertyForWrite()? and put()? and defineGetter()/Setter()?) should be moved from JSGlobalObject to JSVariableObject. JSVariableObject only implementing "half" the API is error-prone.
Comment 2 Adam Barth 2009-11-30 12:46:42 PST
style-queue ran check-webkit-style on attachment 43949 [details] without any errors.
Comment 3 Geoffrey Garen 2009-11-30 16:32:09 PST
Comment on attachment 43949 [details] Proposed patch r=me
Comment 4 WebKit Commit Bot 2009-12-01 02:17:14 PST
Comment on attachment 43949 [details] Proposed patch Rejecting patch 43949 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11672 test cases. http/tests/security/cross-frame-access-object-prototype.html -> failed Exiting early after 1 failures. 8886 tests run. 219.37s total testing time 8885 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 5 test cases (<1%) had stderr output
Comment 5 Kent Hansen 2009-12-01 06:02:11 PST
Sorry, I missed a test failure. In LayoutTests/http/tests/security/cross-frame-access-object-prototype.html, Object.prototype.propertyIsEnumerable.call(targetFrame, 'myProp') is expected to return false, and it worked because the getPropertyAttributes() reimplementation in JSDOMWindow started like this: // Only allow getting property attributes properties by frames in the same origin. if (!allowsAccessFrom(exec)) return false; but it seems JSDOMWindow::getOwnPropertyDescriptor() is not as strict. I.e. if you "transform" the test to Object.getOwnPropertyDescriptor(targetFrame, 'myProp').enumerable it returns true. This is because JSDOMWindow::getOwnPropertyDescriptor() falls all the way through to calling the base implementation. I would expect the security restrictions to be the same for both (the soon-to-be-dead-for-real-hopefully) getPropertyAttributes() and getOwnPropertyDescriptor(). Is it a bug in getOwnPropertyDescriptor() or is the test wrong?
Comment 6 Geoffrey Garen 2009-12-01 12:24:24 PST
I think your instinct is right: it's a bug in getOwnPropertyDescriptor.
Comment 7 Kent Hansen 2009-12-03 08:26:41 PST
(In reply to comment #6) > I think your instinct is right: it's a bug in getOwnPropertyDescriptor. All right, I spun it off as https://bugs.webkit.org/show_bug.cgi?id=32119, and made it block this task. I'm not a security expert but I can try to dig into what other implementations are doing, if necessary. I noticed that JSDOMWindow::getOwnPropertyDescriptor() mentions cross-domain behavior and Firefox, but nothing about cross-frame.
Comment 8 Eric Seidel (no email) 2009-12-09 14:55:09 PST
I'm unsure of the status of this patch. Should we r- it because it breaks a test?
Comment 9 Geoffrey Garen 2009-12-09 14:56:55 PST
Comment on attachment 43949 [details] Proposed patch r- since the patch broke tests. Maybe the blocking bug fixed those tests, though?
Comment 10 Kent Hansen 2009-12-09 15:21:42 PST
(In reply to comment #9) > (From update of attachment 43949 [details]) > r- since the patch broke tests. Maybe the blocking bug fixed those tests, > though? Correct, the blocking bug fixed the http/tests/security/cross-frame-access-object-prototype.html failure, and the patch still applies without modification.
Comment 11 Geoffrey Garen 2009-12-10 11:48:05 PST
Comment on attachment 43949 [details] Proposed patch OK, since bug 32119 is fixed, I'm going to r+ this bug again and see what the commit-queue bot says.
Comment 12 Eric Seidel (no email) 2009-12-10 12:39:41 PST
Comment on attachment 43949 [details] Proposed patch Queueing for commit per Geoff's above recommendation.
Comment 13 WebKit Commit Bot 2009-12-10 16:06:24 PST
Comment on attachment 43949 [details] Proposed patch Clearing flags on attachment: 43949 Committed r51971: <http://trac.webkit.org/changeset/51971>
Comment 14 WebKit Commit Bot 2009-12-10 16:06:30 PST
All reviewed patches have been landed. Closing bug.