Bug 31933

Summary: Get rid of JSObject::getPropertyAttributes() and all usage of it
Product: WebKit Reporter: Kent Hansen <kent.hansen>
Component: JavaScriptCoreAssignee: Kent Hansen <kent.hansen>
Severity: Normal CC: abarth, commit-queue, eric, ggaren, hausmann, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 32119    
Bug Blocks:    
Description Flags
Proposed patch none

Description Kent Hansen 2009-11-27 06:14:26 PST
JSObject::get(Own)PropertyDescriptor() provides the attributes, so getPropertyAttributes() is no longer needed.
JavaScriptCore should be refactored to only use get(Own)PropertyDescriptor().
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

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.