Bug 119972 – Add attributes field to PropertySlot

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

For all JSC types, this makes getOwnPropertyDescriptor redundant.
There will be a bit more hacking required in WebCore to remove GOPD whilst maintaining current behaviour.
(Current behaviour is in many ways broken, particularly in that GOPD & GOPS are inconsistent, but we should fix incrementally).

[Gavin Barraclough]

Created attachment 209020 [details]
Fix

[WebKit Commit Bot]

Attachment 209020 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObjectFunctions.h', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/runtime/Arguments.cpp', u'Source/JavaScriptCore/runtime/JSActivation.cpp', u'Source/JavaScriptCore/runtime/JSArray.cpp', u'Source/JavaScriptCore/runtime/JSArrayBuffer.cpp', u'Source/JavaScriptCore/runtime/JSArrayBufferView.cpp', u'Source/JavaScriptCore/runtime/JSDataView.cpp', u'Source/JavaScriptCore/runtime/JSFunction.cpp', u'Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h', u'Source/JavaScriptCore/runtime/JSObject.cpp', u'Source/JavaScriptCore/runtime/JSObject.h', u'Source/JavaScriptCore/runtime/JSString.h', u'Source/JavaScriptCore/runtime/JSSymbolTableObject.h', u'Source/JavaScriptCore/runtime/Lookup.cpp', u'Source/JavaScriptCore/runtime/Lookup.h', u'Source/JavaScriptCore/runtime/PropertySlot.h', u'Source/JavaScriptCore/runtime/RegExpObject.cpp', u'Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp', u'Source/WebCore/bindings/js/JSDOMWindowCustom.cpp', u'Source/WebCore/bindings/js/JSHistoryCustom.cpp', u'Source/WebCore/bindings/js/JSLocationCustom.cpp', u'Source/WebCore/bindings/js/JSPluginElementFunctions.cpp', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bridge/runtime_array.cpp', u'Source/WebCore/bridge/runtime_method.cpp', u'Source/WebCore/bridge/runtime_object.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/PropertySlot.h:40:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/PropertySlot.h:41:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/PropertySlot.h:42:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/PropertySlot.h:43:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/PropertySlot.h:44:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 209020 [details]
Fix

Attachment 209020 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1470748

New failing tests:
http/tests/security/cross-frame-access-history-get.html
http/tests/history/cross-origin-replace-history-object-child.html

[Build Bot]

Created attachment 209021 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 209020 [details]
Fix

Attachment 209020 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1478394

New failing tests:
http/tests/history/cross-origin-replace-history-object-child.html

[Build Bot]

Created attachment 209024 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Gavin Barraclough]

Created attachment 209036 [details]
Fix for JSHistory bug - accidental use of null entry!

[WebKit Commit Bot]

Attachment 209036 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObjectFunctions.h', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/runtime/Arguments.cpp', u'Source/JavaScriptCore/runtime/JSActivation.cpp', u'Source/JavaScriptCore/runtime/JSArray.cpp', u'Source/JavaScriptCore/runtime/JSArrayBuffer.cpp', u'Source/JavaScriptCore/runtime/JSArrayBufferView.cpp', u'Source/JavaScriptCore/runtime/JSDataView.cpp', u'Source/JavaScriptCore/runtime/JSFunction.cpp', u'Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h', u'Source/JavaScriptCore/runtime/JSObject.cpp', u'Source/JavaScriptCore/runtime/JSObject.h', u'Source/JavaScriptCore/runtime/JSString.h', u'Source/JavaScriptCore/runtime/JSSymbolTableObject.h', u'Source/JavaScriptCore/runtime/Lookup.cpp', u'Source/JavaScriptCore/runtime/Lookup.h', u'Source/JavaScriptCore/runtime/PropertySlot.h', u'Source/JavaScriptCore/runtime/RegExpObject.cpp', u'Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp', u'Source/WebCore/bindings/js/JSDOMWindowCustom.cpp', u'Source/WebCore/bindings/js/JSHistoryCustom.cpp', u'Source/WebCore/bindings/js/JSLocationCustom.cpp', u'Source/WebCore/bindings/js/JSPluginElementFunctions.cpp', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bridge/runtime_array.cpp', u'Source/WebCore/bridge/runtime_method.cpp', u'Source/WebCore/bridge/runtime_object.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/PropertySlot.h:40:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/PropertySlot.h:41:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/PropertySlot.h:42:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/PropertySlot.h:43:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/PropertySlot.h:44:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 209036 [details]
Fix for JSHistory bug - accidental use of null entry!

View in context: https://bugs.webkit.org/attachment.cgi?id=209036&action=review

Hard to tell by reading whether this is correct, but I approve of this approach, so r=me.

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:168
>                      JSValue value = thisObject->getStaticValue(exec, propertyName);
>                      if (value) {
> -                        slot.setValue(thisObject, value);
> +                        slot.setValue(thisObject, ReadOnly | DontEnum, value);
>                          return true;

I think static values have their own attributes, no?

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:177
>              if (OpaqueJSClassStaticFunctionsTable* staticFunctions = jsClass->staticFunctions(exec)) {
>                  if (staticFunctions->contains(name)) {
> -                    slot.setCustom(thisObject, staticFunctionGetter);
> +                    slot.setCustom(thisObject, ReadOnly | DontEnum, staticFunctionGetter);
>                      return true;
>                  }

Ditto for static functions.

[Gavin Barraclough]

(In reply to comment #9)
> Hard to tell by reading whether this is correct, but I approve of this approach, so r=me.

Oh - should have mentioned - I tested by adding an assert in objectConstructorGetOwnPropertyDescriptor to ensure GOPS/GOPD return the same results.  Results match for all objects other than some WebCore ones (particularly the window object) where the GOPS/GOPD behaviour does not currently match (I'll work on aligning that next).

> > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:168
> >                      JSValue value = thisObject->getStaticValue(exec, propertyName);
> I think static values have their own attributes, no?
> 
> > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:177
> >              if (OpaqueJSClassStaticFunctionsTable* staticFunctions = jsClass->staticFunctions(exec)) {
> Ditto for static functions.

Yes – but I intend to leave these as is for now.  I'd like to separate the refactoring of unifying GOPS/GOPD from any changes in behavior, and whilst you're right that this information does exist for static values, GOPD just ignores it.  I'd like to make GOPS do the same for now, and then once I've removed GOPD I'll follow up with a patch to fix the attributes.

[Gavin Barraclough]

Committed revision 154253.