Bug 119995 – Start removing custom implementations of getOwnPropertyDescriptor

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

[Gavin Barraclough]

This can now typically implemented in terms of getOwnPropertySlot.
Add a macro to PropertyDescriptor to define an implementation of GOPD in terms of GOPS.
Switch over most classes in JSC & the WebCore bindings generator to use this.

[Gavin Barraclough]

Created attachment 209058 [details]
Fix

[WebKit Commit Bot]

Attachment 209058 [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/debugger/DebuggerActivation.cpp', u'Source/JavaScriptCore/runtime/Arguments.cpp', u'Source/JavaScriptCore/runtime/ArrayConstructor.cpp', u'Source/JavaScriptCore/runtime/ArrayPrototype.cpp', u'Source/JavaScriptCore/runtime/BooleanPrototype.cpp', u'Source/JavaScriptCore/runtime/DateConstructor.cpp', u'Source/JavaScriptCore/runtime/DatePrototype.cpp', u'Source/JavaScriptCore/runtime/ErrorPrototype.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/JSCell.cpp', u'Source/JavaScriptCore/runtime/JSDataView.cpp', u'Source/JavaScriptCore/runtime/JSDataViewPrototype.cpp', u'Source/JavaScriptCore/runtime/JSFunction.cpp', u'Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h', u'Source/JavaScriptCore/runtime/JSNotAnObject.cpp', u'Source/JavaScriptCore/runtime/JSONObject.cpp', u'Source/JavaScriptCore/runtime/JSObject.cpp', u'Source/JavaScriptCore/runtime/NamePrototype.cpp', u'Source/JavaScriptCore/runtime/NumberConstructor.cpp', u'Source/JavaScriptCore/runtime/NumberPrototype.cpp', u'Source/JavaScriptCore/runtime/ObjectConstructor.cpp', u'Source/JavaScriptCore/runtime/PropertyDescriptor.h', u'Source/JavaScriptCore/runtime/PropertySlot.h', u'Source/JavaScriptCore/runtime/RegExpConstructor.cpp', u'Source/JavaScriptCore/runtime/RegExpMatchesArray.cpp', u'Source/JavaScriptCore/runtime/RegExpMatchesArray.h', u'Source/JavaScriptCore/runtime/RegExpObject.cpp', u'Source/JavaScriptCore/runtime/RegExpPrototype.cpp', u'Source/JavaScriptCore/runtime/StringConstructor.cpp', u'Source/JavaScriptCore/runtime/StringObject.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/test/JS/JSTestActiveDOMObject.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestCustomNamedGetter.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestEventConstructor.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestException.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestMediaQueryListListener.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestNamedConstructor.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestNode.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestOverloadedConstructors.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestSerializedScriptValueInterface.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/PropertyDescriptor.h:96:  Else clause should never be on same line as else (use 2 lines)  [whitespace/newline] [4]
Source/JavaScriptCore/runtime/RegExpMatchesArray.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpMatchesArray.h:87:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpMatchesArray.h:87:  The parameter name "propertyName" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpMatchesArray.h:87:  The parameter name "descriptor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 50 files


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

[Sam Weinig]

Comment on attachment 209058 [details]
Fix

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

What remains?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1902
> +            push(@implContent, "GET_OWN_PROPERTY_DESCRIPTOR_IMPL(${className})\n");

I think an extra \n at the end would be be nicer.

[Build Bot]

Comment on attachment 209058 [details]
Fix

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

New failing tests:
http/tests/security/xss-DENIED-defineProperty.html

[Build Bot]

Created attachment 209077 [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 209118 [details]
Updated patch with JSLocation fix

Previously we blocked cross frame defineProperty by making GOPD hide the property altogether.  Now allow it to reflect the presence of the property, just force the attributes to make it read-only, non-configurable.

[Gavin Barraclough]

(In reply to comment #3)
> (From update of attachment 209058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209058&action=review
> 
> What remains?

Updated patch with bug fix, and a few more cases covered.  It's pretty much just the window object left.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1902
> > +            push(@implContent, "GET_OWN_PROPERTY_DESCRIPTOR_IMPL(${className})\n");
> 
> I think an extra \n at the end would be be nicer.

Done.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http/tests/security/xss-DENIED-defineProperty-expected.txt', u'Source/JavaScriptCore/API/JSCallbackObjectFunctions.h', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/debugger/DebuggerActivation.cpp', u'Source/JavaScriptCore/runtime/Arguments.cpp', u'Source/JavaScriptCore/runtime/ArrayConstructor.cpp', u'Source/JavaScriptCore/runtime/ArrayPrototype.cpp', u'Source/JavaScriptCore/runtime/BooleanPrototype.cpp', u'Source/JavaScriptCore/runtime/DateConstructor.cpp', u'Source/JavaScriptCore/runtime/DatePrototype.cpp', u'Source/JavaScriptCore/runtime/ErrorPrototype.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/JSCell.cpp', u'Source/JavaScriptCore/runtime/JSDataView.cpp', u'Source/JavaScriptCore/runtime/JSDataViewPrototype.cpp', u'Source/JavaScriptCore/runtime/JSFunction.cpp', u'Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h', u'Source/JavaScriptCore/runtime/JSNotAnObject.cpp', u'Source/JavaScriptCore/runtime/JSONObject.cpp', u'Source/JavaScriptCore/runtime/JSObject.cpp', u'Source/JavaScriptCore/runtime/NamePrototype.cpp', u'Source/JavaScriptCore/runtime/NumberConstructor.cpp', u'Source/JavaScriptCore/runtime/NumberPrototype.cpp', u'Source/JavaScriptCore/runtime/ObjectConstructor.cpp', u'Source/JavaScriptCore/runtime/PropertyDescriptor.h', u'Source/JavaScriptCore/runtime/PropertySlot.h', u'Source/JavaScriptCore/runtime/RegExpConstructor.cpp', u'Source/JavaScriptCore/runtime/RegExpMatchesArray.cpp', u'Source/JavaScriptCore/runtime/RegExpMatchesArray.h', u'Source/JavaScriptCore/runtime/RegExpObject.cpp', u'Source/JavaScriptCore/runtime/RegExpPrototype.cpp', u'Source/JavaScriptCore/runtime/StringConstructor.cpp', u'Source/JavaScriptCore/runtime/StringObject.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp', u'Source/WebCore/bindings/js/JSHTMLAppletElementCustom.cpp', u'Source/WebCore/bindings/js/JSHTMLEmbedElementCustom.cpp', u'Source/WebCore/bindings/js/JSHTMLObjectElementCustom.cpp', u'Source/WebCore/bindings/js/JSHistoryCustom.cpp', u'Source/WebCore/bindings/js/JSLocationCustom.cpp', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeCustom.cpp', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/test/JS/JSTestActiveDOMObject.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestCustomNamedGetter.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestEventConstructor.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestException.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestMediaQueryListListener.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestNamedConstructor.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestNode.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestOverloadedConstructors.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestSerializedScriptValueInterface.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.cpp', u'Source/WebCore/bridge/objc/objc_runtime.mm', u'Source/WebCore/bridge/runtime_array.cpp', u'Source/WebCore/bridge/runtime_method.cpp', u'Source/WebCore/bridge/runtime_object.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/PropertyDescriptor.h:96:  Else clause should never be on same line as else (use 2 lines)  [whitespace/newline] [4]
Source/JavaScriptCore/runtime/RegExpMatchesArray.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpMatchesArray.h:87:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpMatchesArray.h:87:  The parameter name "propertyName" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpMatchesArray.h:87:  The parameter name "descriptor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 57 files


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

[Gavin Barraclough]

Committed revision 154300.

[Michael Saboff]

Committed r154303: <http://trac.webkit.org/changeset/154303>

[Michael Saboff]

(In reply to comment #10)
> Committed r154303: <http://trac.webkit.org/changeset/154303>

Don't know how, but "webkit-patch land" thought that my change to https://bugs.webkit.org/show_bug.cgi?id=120015 was for this bug!

[Csaba Osztrogonác]

It seems something is wrong with the Qt port, because all dumpastext test
started to "fail" after this change. The tests pass, but they generate
rendertree dump instead of text only output incorrectly.

[Ryosuke Niwa]

It seems like this patch caused http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html to fail or crash:
http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fsecurity%2Fcross-frame-access-getOwnPropertyDescriptor.html

[Csaba Osztrogonác]

(In reply to comment #12)
> It seems something is wrong with the Qt port, because all dumpastext test
> started to "fail" after this change. The tests pass, but they generate
> rendertree dump instead of text only output incorrectly.

I think the but is somewhere in Qt implementation of
DumpRenderTree::initJSObjects() .

cc-ing Simon who touched this function last time - r145708 / bug112144

[Allan Sandfeld Jensen]

(In reply to comment #14)
> (In reply to comment #12)
> > It seems something is wrong with the Qt port, because all dumpastext test
> > started to "fail" after this change. The tests pass, but they generate
> > rendertree dump instead of text only output incorrectly.
> 
> I think the but is somewhere in Qt implementation of
> DumpRenderTree::initJSObjects() .
> 
> cc-ing Simon who touched this function last time - r145708 / bug112144

Also it appears the tests are flaky, in that the only the first dumpAsText works. For instance:

Tools/Scripts/new-run-webkit-tests --qt --iterations 10 http/tests/appcache/404-resource.html

[2/10] http/tests/appcache/404-resource.html failed unexpectedly (text diff)
[3/10] http/tests/appcache/404-resource.html failed unexpectedly (text diff)
[4/10] http/tests/appcache/404-resource.html failed unexpectedly (text diff)
[5/10] http/tests/appcache/404-resource.html failed unexpectedly (text diff)
[6/10] http/tests/appcache/404-resource.html failed unexpectedly (text diff)
[7/10] http/tests/appcache/404-resource.html failed unexpectedly (text diff)
[8/10] http/tests/appcache/404-resource.html failed unexpectedly (text diff)
[9/10] http/tests/appcache/404-resource.html failed unexpectedly (text diff)
[10/10] http/tests/appcache/404-resource.html failed unexpectedly (text diff)

[Ryosuke Niwa]

When a patch for bug A caused a regression bug B, B depends on A.  i.e. A blocks B.

[Csaba Osztrogonác]

(In reply to comment #16)
> When a patch for bug A caused a regression bug B, B depends on A.  i.e. A blocks B.

I don't agree. When a patch for bug A caused a regression bug B, it means
that bug A isn't completely and properly fixed until bug B is fixed. So
fixing bug A (original) depends on bug B (regression bug) == bug B blocks bug A.

We always do it in this way. (irc bot does it for rollouts too)
some historical example:
- https://bugs.webkit.org/show_bug.cgi?id=119961
- https://bugs.webkit.org/show_bug.cgi?id=120023

But unfortunately in 95% of the cases there aren't any blocks/depends on
relation added by folks only a comment in the original bug with the URL
of the regression bug. But many times I can't find neither blocks/depends,
nor comments. :(