1090537 - Many small cleanups to property setting ([[Set]])

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

A bunch of small patches forthcoming. Each one is either a refactoring that does not change behavior; or a change to improve ES6 compliance of property set operations, mostly involving 'receiver', proxies, and the chaining behavior of 9.1.9 [[Set]] when it walks the prototype chain.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - Remove an old hack where Proxy::hasOwn permitted SET access

The comments being deleted here are incorrect; Proxy::set no longer calls handler->hasOwn().

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Pass receiver argument through from JSObject::setGeneric and setElement to Proxy::set

ES6 specifies [[Set]] as an operation taking both a "this" object and a "receiver" parameter. Both JSObject::setGeneric and Proxy::set support the receiver parameter, but ObjectOps::setGeneric does not; in this patch, we add a little workaround for that. The test shows how this is observable using only standard builtins.

The changes in JSObject::setElement are untestable since currently all call sites pass the same value for obj as for receiver.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: part 3 - In BaseProxyHandler::set(), do not assume that proxy == receiver or that both have the same handler

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: part 4 - Take NativeSet private. No change in behavior

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: part 5 - Remove legacy special case in baseops::SetPropertyHelper that cloned the getter and setter of JSPROP_SHADOWABLE properties when shadowed on another native object

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: part 6 - Factor out a big chunk of SetPropertyHelper into a separate function template, SetPropertyByDefining

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Part 6 with diff -b (makes it a lot easier to see what's going on)

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: part 7 - Eliminate useless attrs argument to SetPropertyByDefining. No change in behavior

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: part 8 - In SetPropertyHelper, if defining a new data property, define it on receiver, not obj

Most of the diff is just renaming an argument. The only new code is necessary because unlike obj, receiver can be non-native.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: part 9 - Just move SetPropertyByDefining. No change in behavior

(The end state here is to carve out more than just one helper function from SetPropertyHelper. Putting those functions below SetPropertyHelper would be unusual style for us and would require more forward declarations.)

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: part 10 - Factor out SetNonexistentProperty. No change in behavior

This involves copying a few lines of code, but we eliminate some of those lines in a later patch, so the duplication ends up being minimal.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: part 11 - Delete a special case for arrays in SetNonexistentProperty that is (a) unreachable; (b) redundant with a check in DefinePropertyOrElement

The code being deleted would be ineffective even if we did have Reflect.set, but Reflect.set does enable a very near miss (where receiver, not obj, is an array), so we leave a sleeper test for that case.

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: part 12 - Change SetNonexistentProperty to use only receiver, not obj. No change in behavior

(The behavior doesn't change because Unqualified is only passed from two call sites, and in both places, the same object is passed for both receiver and obj.)

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: part 13 - Change [[Set]] implementations to fire watchpoints only when the watched object is the receiver

This reconciles watchpoints somewhat better to the ES6 MOP, wherein [[Set]] is called successively on each object along the prototype chain until a property is found.

Comment 15

[Jason Orendorff [:jorendorff]]

I think 13 is a nice spooky number to stop on. Happy Halloween all.

To add to the mood, I shall add a security comment in a sec.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 8513099 [details] [diff] [review]
part 13 - Change [[Set]] implementations to fire watchpoints only when the watched object is the receiver

Review of attachment 8513099 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/NativeObject.cpp
@@ +2038,5 @@
>                             QualifiedBool qualified, MutableHandleValue vp, bool strict)
>  {
>      MOZ_ASSERT(cxArg->isThreadLocal(obj));
>  
> +    if (MOZ_UNLIKELY(obj->watched()) && obj == receiver) {

I am not certain whether these semantics make sense ("sense, lol") for accessor properties.  But whatevs.  Makes sense for data properties, and watching accessors didn't do anything sane before, so la la la...

Comment 18

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=d2c85fa619a3

Comment 19

[Eric Faust [:efaust]]

Comment on attachment 8513018 [details] [diff] [review]
part 2 - Pass receiver argument through from JSObject::setGeneric and setElement to Proxy::set

Review of attachment 8513018 [details] [diff] [review]:
-----------------------------------------------------------------

Sigh. This is the third time I have seen receiver-based calls to Proxy::set inserted to unbreak something that needed it. Thanks for doing it on a wider scale.

If we think that the ObjectOps things are gonna be around for the forseeable future, and they can be massaged into taking a receiver instead it's maybe worth considering. Maybe it's just never worth it.

Certainly this is a good first step. r=me.

Comment 20

[Eric Faust [:efaust]]

Comment on attachment 8513036 [details] [diff] [review]
part 3 - In BaseProxyHandler::set(), do not assume that proxy == receiver or that both have the same handler

Review of attachment 8513036 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, these changes look good. The Proxy:: and BPH:: code has always been questionable on these lines. Good to move closer to spec.

::: js/src/jit-test/tests/proxy/testIndirectProxySet.js
@@ +1,1 @@
> +// 

hyper nit: first line (empty comment) ends in whitespace.

@@ +19,5 @@
> +var hits = 0;
> +
> +// This assignment eventually reaches ScriptedIndirectProxyHandler::set
> +// with arguments (proxy=p, receiver=q). Test that it finishes by defining
> +// a property on receiver, not on proxy.

This is a nice test :)

Comment 21

[Eric Faust [:efaust]]

Comment on attachment 8513040 [details] [diff] [review]
part 4 - Take NativeSet private. No change in behavior

Review of attachment 8513040 [details] [diff] [review]:
-----------------------------------------------------------------

APPROVED.

Comment 22

[Eric Faust [:efaust]]

Comment on attachment 8513061 [details] [diff] [review]
part 6 - Factor out a big chunk of SetPropertyHelper into a separate function template, SetPropertyByDefining

Review of attachment 8513061 [details] [diff] [review]:
-----------------------------------------------------------------

APPROVED.

Comment 23

[Eric Faust [:efaust]]

Comment on attachment 8513064 [details] [diff] [review]
part 7 - Eliminate useless attrs argument to SetPropertyByDefining. No change in behavior

Review of attachment 8513064 [details] [diff] [review]:
-----------------------------------------------------------------

APPROVED.

Comment 24

[Eric Faust [:efaust]]

Comment on attachment 8513036 [details] [diff] [review]
part 3 - In BaseProxyHandler::set(), do not assume that proxy == receiver or that both have the same handler

So, the more I think about this, the more I want Bobby to at least see it, so adding f?. I don't think there's any reason that asking the receiver would have bad implications, but better to make sure

Comment 25

[Eric Faust [:efaust]]

Comment on attachment 8513069 [details] [diff] [review]
part 8 - In SetPropertyHelper, if defining a new data property, define it on receiver, not obj

Review of attachment 8513069 [details] [diff] [review]:
-----------------------------------------------------------------

Nice

::: js/src/vm/NativeObject.cpp
@@ +2151,4 @@
>  {
> +    // If receiver is inextensible, stop. (According to the specification, this
> +    // is supposed to be enforced by [[DefineOwnProperty]], but we haven't
> +    // implemented that yet.)

is the a bugno to put here?

Comment 26

[Eric Faust [:efaust]]

Comment on attachment 8513075 [details] [diff] [review]
part 9 - Just move SetPropertyByDefining. No change in behavior

Review of attachment 8513075 [details] [diff] [review]:
-----------------------------------------------------------------

APPROVED.

Comment 27

[Eric Faust [:efaust]]

Comment on attachment 8513078 [details] [diff] [review]
part 10 - Factor out SetNonexistentProperty. No change in behavior

Review of attachment 8513078 [details] [diff] [review]:
-----------------------------------------------------------------

APPROVED.

Comment 28

[Eric Faust [:efaust]]

Comment on attachment 8513092 [details] [diff] [review]
part 11 - Delete a special case for arrays in SetNonexistentProperty that is (a) unreachable; (b) redundant with a check in DefinePropertyOrElement

Review of attachment 8513092 [details] [diff] [review]:
-----------------------------------------------------------------

Writing tests for future implementers is the nicest thing yet :)

Comment 29

[Eric Faust [:efaust]]

Comment on attachment 8513096 [details] [diff] [review]
part 12 - Change SetNonexistentProperty to use only receiver, not obj. No change in behavior

Review of attachment 8513096 [details] [diff] [review]:
-----------------------------------------------------------------

I agree that this is necessary, but it's not obvious to me that it constitutes "no change in behavior". At any rate, r=me

Comment 31

[Jason Orendorff [:jorendorff]]

(In reply to Eric Faust [:efaust] from comment #29)
> I agree that this is necessary, but it's not obvious to me that it
> constitutes "no change in behavior". At any rate, r=me

Oh, it's totally unobvious. It'll be in the commit message though. The behavior doesn't change because Unqualified is only passed from two call sites, and in both places, as it happens, the same object is passed for both receiver and obj.

Comment 32

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=2166565b8755

Comment 33

[Jason Orendorff [:jorendorff]]

Several test failures I'll look into later today (after reviews):

1.  Can't quite tell what's going on here yet, but two tests have to do with unqualified assignment.
    Both these tests are weird but for different reasons.

    TEST-UNEXPECTED-TIMEOUT
    build/tests/xpcshell/tests/toolkit/devtools/server/tests/unit/test_breakpoint-01.js
    resource://gre/modules/commonjs/toolkit/loader.js
     -> resource://gre/modules/devtools/DevToolsUtils.js:59:
    error: notify event 'closed' threw an exception:
    ReferenceError: assignment to undeclared variable reportError

    FAIL chrome://mochitests/content/browser/browser/components/customizableui/test/browser_901207_searchbar_in_panel.js
    Exception thrown (at line 10 of that file)
    ReferenceError: assignment to undeclared variable openUILinkIn

2.  Obviously broke a policy assertion, even being as careful as possible. Love these assertions!
    though not the fragility and intricacy that makes them necessary...

    FAIL chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_sandbox_bindings.xul
    Assertion failure: cx->runtime()->enteredPolicy->enteredProxy->get() == proxy,
    at js/src/proxy/Proxy.cpp:81
    [@ js::assertEnteredPolicy(JSContext*, JSObject*, jsid, unsigned int)]

3.  Looks like I busted a watchpoint test with part 13. I guess "==" is too strong; I need to also
    consider matching inner and outer objects "equal enough".

    FAIL jsreftest js1_8_1/extensions/regress-520572.js
    watch should innerize the object being watched
    Expected value '2', Actual value '1'

as well as one trivial compiler error, already fixed.

Comment 34

[Bobby Holley (:bholley)]

Comment on attachment 8513036 [details] [diff] [review]
part 3 - In BaseProxyHandler::set(), do not assume that proxy == receiver or that both have the same handler

Review of attachment 8513036 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, as long as we don't unwrap the receiver or anything this should be totally safe.

Comment 35

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=8d64854a8261

Comment 36

[Jason Orendorff [:jorendorff]]

Attachment: part 10 - Factor out SetNonexistentProperty. No change in behavior

Comment 37

[Jason Orendorff [:jorendorff]]

Here are the fixes for these three issues:

1.  Part 10, the patch that factors out SetNonexistentProperty, inadvertently
    changed the behavior in this one weird case where a property *is* found
    on a non-native prototype.

    The original part 10 calls SetNonexistentProperty in that case.
    The fix is to call SetPropertyByDefining instead.
    With that fix there really is no change in behavior.

2.  Part 2 makes some weird stuff possible -- the fix is to add a patch
    which I'll call "part 2b". It's very similar to attachment 8513036 [details] [diff] [review]
    which you already reviewed.

    (In my tree I'm also swapping the order of part 2 and part 3,
    because it makes more sense to apply part 3's bug fixes first,
    before part 2's changes which expose the bugs!)

3.  The fix here is to drop part 13 (the changes to watchpoints).

    Watchpoints don't really make sense in the ES6 MOP
    when combined with proxies (and in particular the WindowProxy).
    I don't want to mess with it, so let's leave them alone for now.

Comment 38

[Jason Orendorff [:jorendorff]]

Attachment: part 2b - In Proxy::set, do not assume that proxy == receiver or that both have the same handler

Comment 39

[Eric Faust [:efaust]]

Comment on attachment 8519235 [details] [diff] [review]
part 10 - Factor out SetNonexistentProperty. No change in behavior

Review of attachment 8519235 [details] [diff] [review]:
-----------------------------------------------------------------

Blech, I feel like I should have caught this maybe, but it's also hard to read these diffs.

Comment 40

[Eric Faust [:efaust]]

Comment on attachment 8519247 [details] [diff] [review]
part 2b - In Proxy::set, do not assume that proxy == receiver or that both have the same handler

Review of attachment 8519247 [details] [diff] [review]:
-----------------------------------------------------------------

You are welcome to ask bobby as well, but this is a longstanding piece of sanity erosion for me, so I am more than happy to see it go.

Comment 41

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b66928f1aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d1001d4403
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8da5fb6460d
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb6ee106d15
https://hg.mozilla.org/integration/mozilla-inbound/rev/be9e874aea12
https://hg.mozilla.org/integration/mozilla-inbound/rev/9317cf955dc7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1791020afc6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/724e3e6efb7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd58d09bcf1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4678a751e9c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/542bb68249ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d518eed470e
https://hg.mozilla.org/integration/mozilla-inbound/rev/aca93ae7eeed

Comment 42

[Jason Orendorff [:jorendorff]]

Thanks very much for the quick follow-up reviews.

Comment 43

[Gary [:streetwolf52]]

Through regression checking I've found that this patch causes the Octane 2.0 benchmark to crash Fx when I click on it's link.  I don't even get into the web site.

GOOD
---- 
20141107193030
https://hg.mozilla.org/integration/mozilla-inbound/rev/818ef4dc2697

BAD
--- 
20141107223006
https://hg.mozilla.org/integration/mozilla-inbound/rev/aca93ae7eeed

Here's the crash report but since I'm on inbound (no symbols?) I don't know if it is helpful:

https://crash-stats.mozilla.com/report/index/c19cd197-9a8b-4fd0-a68a-098f82141109

---------------------
Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0

Comment 44

[Gary [:streetwolf52]]

Link to Octane 2.0:

http://octane-benchmark.googlecode.com/svn/latest/index.html

Comment 45

[Gary [:streetwolf52]]

Looks like https://bugzilla.mozilla.org/show_bug.cgi?id=1095973 might be involved.

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5b66928f1aa
https://hg.mozilla.org/mozilla-central/rev/d5d1001d4403
https://hg.mozilla.org/mozilla-central/rev/c8da5fb6460d
https://hg.mozilla.org/mozilla-central/rev/eeb6ee106d15
https://hg.mozilla.org/mozilla-central/rev/be9e874aea12
https://hg.mozilla.org/mozilla-central/rev/9317cf955dc7
https://hg.mozilla.org/mozilla-central/rev/1791020afc6f
https://hg.mozilla.org/mozilla-central/rev/724e3e6efb7b
https://hg.mozilla.org/mozilla-central/rev/fd58d09bcf1c
https://hg.mozilla.org/mozilla-central/rev/4678a751e9c8
https://hg.mozilla.org/mozilla-central/rev/542bb68249ce
https://hg.mozilla.org/mozilla-central/rev/1d518eed470e
https://hg.mozilla.org/mozilla-central/rev/aca93ae7eeed