JSObject::get(Own)PropertyDescriptor() provides the attributes, so getPropertyAttributes() is no longer needed. JavaScriptCore should be refactored to only use get(Own)PropertyDescriptor().
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.
style-queue ran check-webkit-style on attachment 43949 [details] without any errors.
Comment on attachment 43949 [details] Proposed patch r=me
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
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?
I think your instinct is right: it's a bug in getOwnPropertyDescriptor.
(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.
I'm unsure of the status of this patch. Should we r- it because it breaks a test?
Comment on attachment 43949 [details] Proposed patch r- since the patch broke tests. Maybe the blocking bug fixed those tests, though?
(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 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 on attachment 43949 [details] Proposed patch Queueing for commit per Geoff's above recommendation.
Comment on attachment 43949 [details] Proposed patch Clearing flags on attachment: 43949 Committed r51971: <http://trac.webkit.org/changeset/51971>
All reviewed patches have been landed. Closing bug.