Bug 31933 – Get rid of JSObject::getPropertyAttributes() and all usage of it

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Kent Hansen]

JSObject::get(Own)PropertyDescriptor() provides the attributes, so getPropertyAttributes() is no longer needed.
JavaScriptCore should be refactored to only use get(Own)PropertyDescriptor().

[Kent Hansen]

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.

[Adam Barth]

style-queue ran check-webkit-style on attachment 43949 [details] without any errors.

[Geoffrey Garen]

Comment on attachment 43949 [details]
Proposed patch

r=me

[WebKit Commit Bot]

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

[Kent Hansen]

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?

[Geoffrey Garen]

I think your instinct is right: it's a bug in getOwnPropertyDescriptor.

[Kent Hansen]

(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.

[Eric Seidel (no email)]

I'm unsure of the status of this patch.  Should we r- it because it breaks a test?

[Geoffrey Garen]

Comment on attachment 43949 [details]
Proposed patch

r- since the patch broke tests. Maybe the blocking bug fixed those tests, though?

[Kent Hansen]

(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.

[Geoffrey Garen]

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.

[Eric Seidel (no email)]

Comment on attachment 43949 [details]
Proposed patch

Queueing for commit per Geoff's above recommendation.

[WebKit Commit Bot]

Comment on attachment 43949 [details]
Proposed patch

Clearing flags on attachment: 43949

Committed r51971: <http://trac.webkit.org/changeset/51971>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.