Closed Bug 824217 Opened 7 years ago Closed 6 years ago
Remove some uses of JSRESOLVE
9.09 KB, patch
|Details | Diff | Splinter Review|
9.75 KB, patch
|Details | Diff | Splinter Review|
5.32 KB, patch
|Details | Diff | Splinter Review|
ChromeObjectWrapper::getPropertyDescriptor shouldn't pretend no property exists, when it exists along the prototype chain
1.16 KB, patch
|Details | Diff | Splinter Review|
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.
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?
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.
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]]?
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?
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.
Ah, ok. Sounds good. Let me know!
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.
Attachment #696420 - Flags: review?(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).
Attachment #696420 - Flags: review?(bzbarsky) → review+
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 :-\ ).
Whiteboard: [js:t][leave open]
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/96e5f3a75fcf. test_resolveWorker-assignment.html and test_resolveWorker.html failed on all platforms.
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.
Attachment #697524 - Flags: review?(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?
Attachment #697524 - Flags: review?(bzbarsky) → review+
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
Attachment #698064 - Flags: review?(bobbyholley+bmo)
This is either optimization-land, or it serves no purpose. Removing it is green: https://tbpl.mozilla.org/?tree=Try&rev=171764d0f531
Attachment #698066 - Flags: review?(bobbyholley+bmo)
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
Attachment #698064 - Flags: review?(bobbyholley+bmo) → review+
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?
Attachment #698066 - Flags: review?(bobbyholley+bmo) → review+
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.
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.
Patches landed here, so let's open a new bug for this. Bug 987007.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Summary: JSRESOLVE_ASSIGNING delenda est → Remove some uses of JSRESOLVE_ASSIGNING
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?
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.
Whiteboard: [js:t][leave open] → [js:t]
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.