Bug 158091 – replaceable own properties seem to ignore replacement after property caching

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

replaceable own properties seem to ignore replacement after property caching

[Geoffrey Garen]

Created attachment 279826 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


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

[Chris Dumez]

Comment on attachment 279826 [details]
patch

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

A JSC person should review this, just a few minor comments from me.

> Source/JavaScriptCore/runtime/Lookup.h:295
> +    auto result = thisObj->putDirect(vm, propertyName, value);

I think bool is more readable and as short as bool :)

> Source/JavaScriptCore/runtime/Lookup.h:298
> +        thisObj->JSObject::setStructure(exec->vm(), Structure::attributeChangeTransition(exec->vm(), thisObj->structure(), propertyName, 0));

should be vm not exec->vm()

[Chris Dumez]

Comment on attachment 279826 [details]
patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

OOPS :)

[Build Bot]

Comment on attachment 279826 [details]
patch

Attachment 279826 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1382404

New failing tests:
js/cached-window-properties.html

[Build Bot]

Created attachment 279832 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 279826 [details]
patch

Attachment 279826 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1382403

New failing tests:
js/cached-window-properties.html

[Build Bot]

Created attachment 279834 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279826 [details]
patch

Attachment 279826 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1382430

New failing tests:
js/cached-window-properties.html

[Build Bot]

Created attachment 279835 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 279826 [details]
patch

Attachment 279826 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1382632

New failing tests:
js/cached-window-properties.html

[Build Bot]

Created attachment 279839 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Geoffrey Garen]

Created attachment 279865 [details]
patch

[Darin Adler]

Comment on attachment 279865 [details]
patch

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

> Source/JavaScriptCore/runtime/Lookup.h:292
> +template <class ThisImp>
> +inline bool replaceStaticPropertySlot(ExecState* exec, ThisImp* thisObj, PropertyName propertyName, JSValue value)

Here are things I would have done differently if I had written this:

- I would have written it like this:
    template<typename ThisObjectClass> inline bool ...
Note that I prefer template on the same line, no space after the word template, typename rather than class, no abbreviation in the class name, and in particular unclear why Imp is needed here.
- I would have considered making the thisObject argument a JSObject* rather than making this a template. I am unclear on what we gain by using a function template. Actually, I would have used JSObject& but then I would have run into the rule where you all don’t allow that for JSObject in the JavaScriptCore side of things.
- The first argument would be VM& vm since the function doesn’t need an ExecState and the caller is passing VM to other functions.
- Or the first argument would be ExecState& state rather than ExecState* exec.
- The second argument name would be thisObject rather than thisObj.

> Source/JavaScriptCore/runtime/Lookup.h:300
> +    bool result = thisObj->putDirect(vm, propertyName, value);
> +
> +    if (result && !thisObj->staticFunctionsReified())
> +        thisObj->JSObject::setStructure(exec->vm(), Structure::attributeChangeTransition(vm, thisObj->structure(), propertyName, 0));
> +
> +    return result;

I would have used early return rather than a boolean:

    if (!thisObject.putDirect(vm, propertyName, value))
        return false;
    ...
    return true;

Also, the call to setStructure is using "exec->vm()" instead of just "vm" in one place.

[Geoffrey Garen]

Committed r201428: <http://trac.webkit.org/changeset/201428>