824217 - Remove some uses of JSRESOLVE_ASSIGNING

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

The last resolve flag that shall be destroyed is JSRESOLVE_ASSIGNING.

With JSRESOLVE_DETECTING out of the way, this may be the hardest one yet.  Particularly as the |bool set| in some of the proxy hooks means it's wormed its way beyond merely what can be seen by the obvious MXR search.  There are some easy removals here, first, then it may get tricky.  More investigation is needed to see just how bad it is, but I am optimistic.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hrm.  For the proxy hooks, one issue is that for WebIDL we need to be able to tell apart [[GetOwnProperty]] and [[DefineOwnProperty]].  How do we tell those apart in a proxy?  Or is the idea that "set" is staying but will just not be related to JSRESOLVE_ASSIGNING?

Comment 2

[Jeff Walden [:Waldo]]

The proxy hooks should reflect that difference natively, all but certainly with separate get/define hooks.  |bool set| is really just a hackaround for that distinction not being present in the API currently, as far as I can tell.

Now, whether that larger change must gate the change here, I dunno.  That remains to be seen.  As I said, this one's probably not going to be easy.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, the WebIDL proxies certainly implement get/define.  Does implementing those traps mean that getOwnPropertyDescriptor won't be called for random engine-internal stuff?  It's not quite clear to me what Object.getOwnPropertyDescriptor is supposed to do for those proxies; does that just end up calling [[GetOwnProperty]]?

Comment 4

[Boris Zbarsky [:bzbarsky]]

And in particular....  The spec is written in terms of [[GetOwnProperty]]/[[DefineOwnProperty]], which seem to return property descriptors.  But our proxy API has get/define/getPropertyDescriptor/getOwnPropertyDescriptor hooks.  How do those hooks map onto those internal methods?

Comment 5

[Jeff Walden [:Waldo]]

Not well.

We map them onto our internal hooks in a rather ad hoc way right now.  We've been stumbling along with them just off-kilter for a long time, with issues aplenty at the edges -- but with just enough looking right in general that people mostly haven't screamed bloody murder.  We're going to fix this -- maybe to fix this bug, maybe after we fix this bug.  I'm not sure how exactly it'll work out.  I need to investigate more here before I can know one way or the other.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Sounds good.  Let me know!

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Remove some easy tests for JSRESOLVE_ASSIGNING, add a couple tests

These seem mostly to be about micro-optimizations, but some of them do change behavior, so I probably should explain them all.

nsDOMClassInfo::NewResolve:
This guards, say, document.constructor.  Because the "constructor" property is enumerable/configurable/writable there's no observable difference between non-assigning and assigning from script, so no test.

nsWindowSH::GlobalScopePolluterNewResolve:
This guards window.name/id/etc. gunk.  All of that's defined as non-enumerable, and as |name =| creates an enumerable property there's a difference -- almost.  The old behavior, resolve-only-if-not-assigning followed by assignment, resolves nothing, then creates an enumerable property on |window|.  The new behavior, resolve-regardless followed by assignment, resolves a non-enumerable property, then overwrites its value, resulting in a non-enumerable property.  But per spec window.name/id/etc. gunk should be properties on the GSP, not on |window|.  So an assignment to |window.name| should shadow, with an enumerable property.  Thus this change makes us more ECMAScript-semantics-consistent, but less DOM-window-semantics-consistent.  I could write a test for the behavior change, but not if I want a test that's spec-compliant and not browser-specific.  So no test for this bit.  (And bring on putting window.name on the GSP, so we act like the spec here!)

nsWindowSH::NewResolve for constructor:
window.frameName currently gets resolved on |window|, non-spec-compliantly.  And it's resolved as not-enumerable, when I think WebIDL says it should be.  So if you have <iframe name=constructor>, this is sort of the same issue as the previous hunk regarding testability.  But I wrote a test for window.constructor with an iframe before grokking all this, so, hey -- free test, might as well pocket it.  When frame names move to the GSP the test can be gussied up with a little more testing, but for now we can't really write a good spec-compliant test that tests us too.

nsWindowSH::NewResolve for top:
This is screwy, because if you don't resolve top in here, you get it on the prototype.  So the presence/absence of JSRESOLVE_ASSIGNING seems to make just about no difference.  Ergo no testing.

nsNavigatorSH::NewResolve:
Every property defined here has the default attributes an assignment would, so there's no observable difference here.

nsHTMLDocumentSH::DocumentAllNewResolve:
This defines non-enumerable properties again.  So before, resolving first through assignment would result in an enumerable property; and now, you get a non-enumerable property after assignment.  I think WebIDL requires these to be enumerable, so I can't really test this observable change either.

ResolveWorkerClasses:
This was flat-out wrong, because it was resolving a non-configurable property, but if JSRESOLVE_ASSIGNING short-circuited it, then the non-configurableness would get lost.  Two tests for this, pretty straightforward, the assignment one fails right now.

sandbox_resolve:
REPL test code, who cares, no tests broke on a try push.

env_resolve:
Id.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696420 [details] [diff] [review]
Remove some easy tests for JSRESOLVE_ASSIGNING, add a couple tests

r=me (though note that the gsp resolve hook defines on the gsp already, so things are fine: it just get shadowed by the assignment unless you're really assigning on the gsp itself, in which case you deserve whatever you get).

Comment 9

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40c1df07b407 for the easy cases (which actually, in hindsight, weren't necessarily easy, except in comparison to others :-\ ).

Comment 10

[Josh Matthews [:jdm]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/96e5f3a75fcf. test_resolveWorker-assignment.html and test_resolveWorker.html failed on all platforms.

Comment 11

[Jeff Walden [:Waldo]]

Ugh.  I'm not sure how those tests ever passed locally for me.  I made the requisite test tweaks, then discovered that ResolveWorkerClasses is guarded by an assigning-test in nsWindowSH::NewResolve, so I just todo_is'd it for now, checked that directory of tests passed locally, and relanded.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce29ce2ea7c

There's another caller of ResolveWorkerClasses that doesn't assigning-test before calling ResolveWorkerClasses, but I'm pretty sure removing the test in nsWindowSH::NewResolve is easy enough that it's better to just kill that and fix the test than to write a test that invokes BackstagePass::NewResolve appropriately.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ce29ce2ea7c

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Remove JSRESOLVE_ASSIGNING test around resolution of DOM and other constructors

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 697524 [details] [diff] [review]
Remove JSRESOLVE_ASSIGNING test around resolution of DOM and other constructors

r=me.

This will bitrot my work on sanity for frame names.  Please land so I can merge before working on it more?

Comment 17

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4bff11905a9

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c4bff11905a9

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Remove the mode argument from the PIERCE macro

The argument is unused, and some of the values provided for it are responsible for JSRESOLVE_ASSIGNING tests.  And if unused doesn't count, it's green so far in this try-push that incidentally included it:

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

Comment 20

[Jeff Walden [:Waldo]]

Attachment: ChromeObjectWrapper::getPropertyDescriptor shouldn't pretend no property exists, when it exists along the prototype chain

This is either optimization-land, or it serves no purpose.  Removing it is green:

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

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 698064 [details] [diff] [review]
Remove the mode argument from the PIERCE macro

We still need to know get vs set in CHECKED, but it looks like that's unconnected to the usage in PIERCE. r=bholley

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 698066 [details] [diff] [review]
ChromeObjectWrapper::getPropertyDescriptor shouldn't pretend no property exists, when it exists along the prototype chain

This used to be pseudo-relevant for security, though it was full of holes anyway. It's all been fixed now. See bug 805807 comment 1.

r=bholley.

Also, it looks like billm dumped some tabs into that file by accident. Can you clean them up while you're there?

Comment 23

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2df7d0c61d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c549b6038ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/c72d0bbb5571

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b2df7d0c61d1
https://hg.mozilla.org/mozilla-central/rev/0c549b6038ef

Comment 25

[Jeff Walden [:Waldo]]

Current status of all this is here now:

https://wiki.mozilla.org/Javascript:SpiderMonkey:PropertyElementStorage#Remove_resolve_flags

Comment 26

[Tom S [:evilpie]]

The only remaining use of JSRESOLVE_ASSIGNING is in nsWindowSH::NewResolve. If window is converted to the new bindings, the we would be able to remove the flag and thus also remove all of the rest of this machinery.

Comment 27

[Jeff Walden [:Waldo]]

I see hits in dom/base/nsDOMClassInfo.cpp and dom/bindings/Codegen.py for this, so not just window stuff.  The latter needs to be resolved by adding "set" traps to the DOM proxy stuff, if I remember correctly.  I'd love to get to this, but I'm stuck on full-Intl-enablement and ES6 testing stuff at the moment.

Comment 28

[Jason Orendorff [:jorendorff]]

Patches landed here, so let's open a new bug for this. Bug 987007.

Comment 29

[Florian Bender]

Actually, looks like these are neither resolved nor covered by Bug 987007:

(In reply to Tom Schuster [:evilpie] from comment #26)
> The only remaining use of JSRESOLVE_ASSIGNING is in nsWindowSH::NewResolve.
> If window is converted to the new bindings, the we would be able to remove
> the flag and thus also remove all of the rest of this machinery.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #27)
> I see hits in dom/base/nsDOMClassInfo.cpp and dom/bindings/Codegen.py for
> this, so not just window stuff.  The latter needs to be resolved by adding
> "set" traps to the DOM proxy stuff, if I remember correctly.  I'd love to
> get to this, but I'm stuck on full-Intl-enablement and ES6 testing stuff at
> the moment.

Jason, can you clarify and/or file new bugs, please?

Comment 30

[Jason Orendorff [:jorendorff]]

To clarify: there is one more use of JSRESOLVE_ASSIGNING in nsDOMClassInfo.cpp (comment 27 is mistaken: this is the same one as Tom mentions in comment 26). I think it can just be deleted. That's the last use anywhere.

The plan after that is to remove resolve flags from the engine.