1021066 - NamedSetter is not called when a webIDL binding is not overridebuiltins and it's used from Xrays

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

This issue has been discussed on IRC. Here the log and the possible solution:

http://logs.glob.uno/?c=mozilla%23jsapi#c413123

15:27	bz	So one plausible approach is as follows:
15:27	bz	1) Always output a setCustom
15:27	bz	2) In the non-overridebuiltins case have setCustom be a no-op if not called over Xrays
15:28	bz	So when called over Xrays or when overridebuiltins, we'd just do the set ignoring the proto chain
15:28	bz	as a first approximation, this is what people want anyway
15:28	bz	And would only be "wrong" if we want to examine the actual proto chain of the Xray _and_ someone redefines stuff on that actual proto chain

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow named setters to be invoked via Xrays even for interfaces that are not OverrideBuiltins

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8435087 [details] [diff] [review]
Allow named setters to be invoked via Xrays even for interfaces that are not OverrideBuiltins

[21:03:58]  <bholley>	 bz: hm. But do we need setCustom in the non-OverrideBuiltins case?
[21:04:09]  <bholley>	 bz: shouldn't it be fine to just do the work in the defineProperty hook?
[21:05:10]  <bholley>	 bz: IIUC the reason that OverrideBuiltins is special is that BaseProxyHandler::set gets confused by properties on the prototype that it shouldn't be looking at
[21:06:10]  <bz>	 So....
[21:06:31]  <bz>	 Just letting BaseProxyHandler::set do its thing is what happens in the non-Xray case
[21:06:39]  <bz>	 The question is what should happen in the Xray case
[21:07:38]  <bholley>	 bz: XrayWrapper::set just bounces to BaseProxyHandler::set, right?
[21:07:52] bz	 is looking
[21:08:02] 	 khuey|away is now known as khuey
[21:08:10]  <bz>	 So XrayWrapper::set calls Traits::set
[21:08:29]  <bholley>	 oh, I guess jorendorff changed that
[21:08:33]  <bz>	 DOMXrayTraits::set does the setCustom thing
[21:08:53]  <bz>	 and then falls through to XrayTraits::set
[21:08:57]  <bholley>	 bz: but we won't have a setCustom
[21:09:04]  <bholley>	 bz: so this should all just fall back to BaseProxyHandler::set
[21:09:25]  <bz>	 right, so we use the one at http://mxr.mozilla.org/mozilla-central/source/dom/bindings/DOMJSProxyHandler.cpp#330
[21:09:52]  <bz>	 So then we call BaseProxyHandler::set, agreed
[21:10:11]  <bholley>	 which, if there's nothing on the prototype, ends up calling defineProperty
[21:10:18]  <bz>	 mmm
[21:11:17]  <bz>	 I'd have to step through this to see what's up
[21:11:26]  <bz>	 Or ask baku where it went astray
[21:11:28]  <bz>	 one sec
[21:12:36] bz	 hacks a build, tests this
[21:16:32]  <bz>	 ok
[21:16:49]  <bz>	 so we land in xpc::XrayWrapper::defineProperty
[21:16:58]  <bz>	 we call xpc::DOMXrayTraits::defineProperty
[21:17:02]  <bz>	 1788        if (!existingDesc.object())
[21:17:02]  <bz>	 (gdb)
[21:17:02]  <bz>	 1789            return true;
[21:17:24]  <bz>	 because we have no such prop already
[21:17:53]  <bz>	 So then we defined as an expando
[21:18:04]  <bholley>	 hm, that !existingDesc check seems fishy
[21:18:07]  <bz>	 instead of defining on the content-side proxy
[21:18:09]  <bz>	 Yes, it does
[21:18:17] bholley	 blames it
[21:18:20] bz	 should have checked this codepath
[21:18:57]  <bz>	 http://hg.mozilla.org/mozilla-central/rev/68e0432939c9
[21:19:22]  <bz>	 So we used to do nothing at all in defineProperty, ever
[21:19:28]  <bz>	 Oh, I see what this is trying to do
[21:19:32]  <bz>	 If chrome sets a prop
[21:19:39]  <bz>	 We basically assume it's an expando
[21:19:39]  <bholley>	 so this dates back to 715156
[21:19:47]  <bholley>	 oh, you found it too
[21:19:47]  <bz>	 unless the object has such a prop already
[21:19:51]  <bz>	 how else would you tell?
[21:20:16]  <bz>	 Something with a named creator it wouldn't be true for
[21:20:19]  <bholley>	 well, for objects with a named setter, we should let XrayDefineProperty decide?
[21:20:22]  <bz>	 but for typical objects...
[21:20:30]  <bz>	 hmm
[21:20:39]  <bholley>	 bz: basically, shouldn't XrayDefineProperty always make this decision?
[21:21:05]  <bz>	 Interesting
[21:21:14]  <bz>	 So XrayDefineProperty only defines on proxies
[21:21:25]  <bholley>	 right
[21:21:31]  <bholley>	 it doesn't make sense otherwise
[21:22:51]  <bz>	 OK
[21:22:55]  <bz>	 So what should XrayDefineProperty do?
[21:23:49]  <bz>	 ideally, only define if we have a relevant setter
[21:23:55]  <bz>	 right?
[21:24:03]  <bholley>	 right
[21:24:18]  <bz>	 Or even more precisely
[21:24:20]  <bholley>	 and it shouldn't worry about OverrideBuiltins
[21:25:16]  <bz>	 It should only define if we have a relevant setter and have a prop already
[21:25:19]  <bz>	 or if we have a creator
[21:25:37]  <bz>	 for named stuff, in practice, we always have a creator
[21:25:48]  <bz>	 indexed stuff, not sure.. but we don't allow setting indexed expandos anyway
[21:26:07]  <bz>	 So maybe as long as we have the relevant setter we can go ahead and do it
[21:26:15]  <bholley>	 sgtm

Comment 3

[Boris Zbarsky [:bzbarsky]]

Andrea, I can try to get to that in a few days, but if you want it before that, take a shot at it yourself?

Comment 4

[Andrea Marchesini [:baku]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Andrea, I can try to get to that in a few days, but if you want it before
> that, take a shot at it yourself?

Ok bz. I don't know if I have time to work on this in the next days.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Make named setters work even for non-overridebuiltins bindings over Xrays

In the end, the actual code change is ... just removing that one check, I think.  I can't shake the feeling that I should be testing something else; please tell me what.

I have r=jorendorff on IRC on the jsproxy fix to report the right exception message.

I did manually test this by making DOMStringMap non-overridebuiltins.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8460564 [details] [diff] [review]
Make named setters work even for non-overridebuiltins bindings over Xrays

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

Looks good. r=bholley

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/588ea38a32fc

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/588ea38a32fc