Last Comment Bug 738244 - Remove content-proxy code
: Remove content-proxy code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: mozilla17
Assigned To: Nobody; OK to take it and work on it
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 658909 726949
Blocks: 786976
  Show dependency treegraph
 
Reported: 2012-03-22 07:15 PDT by Gabor Krizsanits [:krizsa :gabor]
Modified: 2013-04-22 07:41 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
just an idea... (9.27 KB, patch)
2012-06-04 03:02 PDT, Gabor Krizsanits [:krizsa :gabor]
no flags Details | Diff | Splinter Review
2nd try (9.89 KB, patch)
2012-06-05 06:30 PDT, Gabor Krizsanits [:krizsa :gabor]
mrbkap: feedback+
Details | Diff | Splinter Review
part 1: fixing BaseProxyHandler::set (800 bytes, patch)
2012-06-06 06:24 PDT, Gabor Krizsanits [:krizsa :gabor]
no flags Details | Diff | Splinter Review
Fix proxy behavior when assigning to inherited properties. [landed] (1.48 KB, patch)
2012-06-06 12:58 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Mark test as now passing: dom/imptests/html/tests/submission/Opera/microdata/test_001.html | the namedItem property must be read/write (872 bytes, patch)
2012-06-07 18:47 PDT, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
dom specific getters (3rd try...) (14.17 KB, patch)
2012-06-08 07:12 PDT, Gabor Krizsanits [:krizsa :gabor]
mrbkap: feedback+
Details | Diff | Splinter Review
Supporting DOM specific collection properties through xray wrappers (14.71 KB, patch)
2012-06-27 04:30 PDT, Gabor Krizsanits [:krizsa :gabor]
no flags Details | Diff | Splinter Review
Supporting DOM specific collection properties through xray wrappers (15.33 KB, patch)
2012-08-15 07:14 PDT, Gabor Krizsanits [:krizsa :gabor]
no flags Details | Diff | Splinter Review
Supporting DOM specific collection properties through xray wrappers (24.89 KB, patch)
2012-08-22 13:43 PDT, Gabor Krizsanits [:krizsa :gabor]
mrbkap: review+
Details | Diff | Splinter Review

Description Gabor Krizsanits [:krizsa :gabor] 2012-03-22 07:15:24 PDT
This bug first requires some platform bugs to be fixed first, but after that the content-proxy code should go.
Comment 1 Alexandre Poirot [:ochameau] 2012-03-22 07:18:46 PDT
bug 601295 contains valuable information about my tests done while introducing proxies.
And bug 726949 looks like a blocker for removing proxes.
Comment 2 Alexandre Poirot [:ochameau] 2012-05-21 14:56:55 PDT
Here is some random stuff that need to be fixed/implemented on XrayWrappers:

* Native method can't be overloaded

// Do nothing, getElementById will always be the native method.
// We expect it to become the user method instead
document.getElementById = function () {}; 

* Various common web shortcut are not implemented

** document["element-name"] are undefined

Some html element can be reference through an document's attribute whose name is the element name attribute.
See element tag name list here (CanHaveName method):
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.cpp#1438

<input name="foo" />
document.foo is equal to the element node

** window["frame-name"] is undefined

document child frame can be accessed through window via an attribute whose name is the frame id.
I think it is done here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#7096

<iframe name="test" />
window.test is equal to the element node

** window.frames[i] is undefined

You can access to the n-th child (i)frame throug window.frame[child frame number]

Seems to be implemented here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#5456

** frame["input name"]

Forms child element can be referenced from form element node through an attribute whose name is child element name attribute

<form>
  <input name="foo" />
</form>
form.foo is equal to input element

* mozMatchesSelector.call(node, selectors) is broken

See bug 658909

Gabor: Do you think we can start opening platform bug for each of these points?
Comment 3 Gabor Krizsanits [:krizsa :gabor] 2012-05-22 01:23:06 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #2)
> Here is some random stuff that need to be fixed/implemented on XrayWrappers:
> 
> * Native method can't be overloaded
> 
> // Do nothing, getElementById will always be the native method.
> // We expect it to become the user method instead
> document.getElementById = function () {}; 

So this is the very definition of xray wrapper. If the content overwrites a native method, xray wrapper 'sees through' it and always call the original native method. If you want to avoid this behavior don't use xray wrappers.
This will never be 'fixed'.

> 
> * Various common web shortcut are not implemented
> 
> ** document["element-name"] are undefined

So I read through this list and I'm pretty sure I tested all of these and they all worked with xray wrapper too the last time I checked. If you can write a simple test for them and any of them are failing we need a regression bug for it, but I really doubt that they are failing.

> 
> * mozMatchesSelector.call(node, selectors) is broken
> 
> See bug 658909

Now this one is a tough one. I spent a lot of time on this one, and know exactly what is broken there, and don't really think it will be fixed any time soon. It is a design issue in the xpc call mechanism, and the only reason it works without xray wrapper is that quick stubs using a different call path, and quick stubs don't suffer from this issue. So the problem is that xray wrappers avoid using quick stubs, and I really have no clue how could I make them use it... If this bug is a blocker, that's a huge problem. 
Any workaround we can come up with here? What is this used for? To my understanding this is something we need only because we have content-proxys around, and once we could get rid of content-proxys we would no longer depending on this bug. Am I wrong?
Comment 4 Alexandre Poirot [:ochameau] 2012-05-22 04:21:53 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> (In reply to Alexandre Poirot (:ochameau) from comment #2)
> > Here is some random stuff that need to be fixed/implemented on XrayWrappers:
> > 
> > * Native method can't be overloaded
> > 
> > // Do nothing, getElementById will always be the native method.
> > // We expect it to become the user method instead
> > document.getElementById = function () {}; 
> 
> So this is the very definition of xray wrapper. If the content overwrites a
> native method, xray wrapper 'sees through' it and always call the original
> native method. If you want to avoid this behavior don't use xray wrappers.
> This will never be 'fixed'.

No no, it is not about the content overloading a native method, but the content script itself. So we expect that *content scripts* can overload these native methods without seing overloads made by the web page.

So we expect this assertion to be true:
  var sb = Cu.sandbox(window);
  sb.document = window.document;
  var js = "document.getElementById = 1; document.getElementById";
  var rv = Cu.evalInSandbox(js, sb);
  assertEqual(rv, 1);


> > ** document["element-name"] are undefined
> 
> So I read through this list and I'm pretty sure I tested all of these and
> they all worked with xray wrapper too the last time I checked. If you can
> write a simple test for them and any of them are failing we need a
> regression bug for it, but I really doubt that they are failing.

It is still broken, you can see it by executing this piece of code in scratchpad (in chrome mode. Be sure to open a regular web page in your current tab)
  var document = content.document;
  var form = document.createElement("form");
  form.setAttribute("name", "foo");
  document.body.appendChild(form);
  Components.utils.reportError(document.foo); // Is undefined
  Components.utils.reportError(document.wrappedJSObject.foo); // Is defined

> > * mozMatchesSelector.call(node, selectors) is broken
> > 
> > See bug 658909
> 
> Now this one is a tough one. I spent a lot of time on this one, and know
> exactly what is broken there, and don't really think it will be fixed any
> time soon. It is a design issue in the xpc call mechanism, and the only
> reason it works without xray wrapper is that quick stubs using a different
> call path, and quick stubs don't suffer from this issue. So the problem is
> that xray wrappers avoid using quick stubs, and I really have no clue how
> could I make them use it... If this bug is a blocker, that's a huge problem. 
> Any workaround we can come up with here? What is this used for? To my
> understanding this is something we need only because we have content-proxys
> around, and once we could get rid of content-proxys we would no longer
> depending on this bug. Am I wrong?

The workaround we found was building these JS proxies. So it is not an issue when using JS proxies, the original issue only happens on xraywrappers. JS proxies are the workaround :o
See https://bugzilla.mozilla.org/show_bug.cgi?id=601295#c31 for more info about this.
It clearly is a blocker as it prevent various JS framework to work in content scripts. Sizzle library being used by multiple framework, including jquery.
Having said that there might be other workaround than JS proxies, but I don't have any idea on top of my head right now. Is there any platform workaround ?
Comment 5 Gabor Krizsanits [:krizsa :gabor] 2012-05-22 05:20:49 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #4)
> So we expect this assertion to be true:
>   var sb = Cu.sandbox(window);
>   sb.document = window.document;
>   var js = "document.getElementById = 1; document.getElementById";
>   var rv = Cu.evalInSandbox(js, sb);
>   assertEqual(rv, 1);

Should not be a problem to have this fixed imo. 

> It is still broken, you can see it by executing this piece of code in
> scratchpad (in chrome mode. Be sure to open a regular web page in your
> current tab)
>   var document = content.document;
>   var form = document.createElement("form");
>   form.setAttribute("name", "foo");
>   document.body.appendChild(form);
>   Components.utils.reportError(document.foo); // Is undefined
>   Components.utils.reportError(document.wrappedJSObject.foo); // Is defined
> 

Will check this soon, my tests were passing thought https://bugzilla.mozilla.org/show_bug.cgi?id=729994#c8

Which version are you using? Maybe the fix did not make to the current released version yet.

> The workaround we found was building these JS proxies. So it is not an issue
> when using JS proxies, the original issue only happens on xraywrappers. JS
> proxies are the workaround :o
> See https://bugzilla.mozilla.org/show_bug.cgi?id=601295#c31 for more info
> about this.
> It clearly is a blocker as it prevent various JS framework to work in
> content scripts. Sizzle library being used by multiple framework, including
> jquery.
> Having said that there might be other workaround than JS proxies, but I
> don't have any idea on top of my head right now. Is there any platform
> workaround ?

I'm currently quite flooded with other tasks. I can get back to this soon. There might be a way to use quick stubs through xray wrappers too, but this is not going to be a trivial patch, and I cannot even guarantee that it's possible to fix it this way. There is a very nasty problem behind this one. 

On the other hand a work around might be as simple as overwriting mozMatchesSelector with a pure js implemented version of the function that calls the native one on this. Call and bind should work on that function. 

For this workaround ofc first it has to be possible to overwrite native method on the content-script side on an xray wrapped object from the content side. That I think was the only one of my tests that was failing...
Comment 6 Alexandre Poirot [:ochameau] 2012-05-28 08:41:39 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> Will check this soon, my tests were passing thought
> https://bugzilla.mozilla.org/show_bug.cgi?id=729994#c8
> 
> Which version are you using? Maybe the fix did not make to the current
> released version yet.

Nightly of the day I tested. 

> On the other hand a work around might be as simple as overwriting
> mozMatchesSelector with a pure js implemented version of the function that
> calls the native one on this. Call and bind should work on that function. 
> 
> For this workaround ofc first it has to be possible to overwrite native
> method on the content-script side on an xray wrapped object from the content
> side. That I think was the only one of my tests that was failing...

I'm totally fine with a workaround, but I don't know any safe Javascript one.
Actually, JS proxies was the only way I knew to be able to trap all mozMatchesSelector call safely (by safely I mean 100% percent calls).
The tricky detail is that we need to trap mozMatchesSelector from child frames. Like an iframe with same domain living in document.
The only alternative workaround I can think about is listening to all document creations and override mozMatchesSelect there.

In any case, now, I think you know enough about this subject. So I think I have to wait for you to have some time to dedicate to these points.
Comment 7 Gabor Krizsanits [:krizsa :gabor] 2012-05-30 04:38:08 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #4)
> It is still broken, you can see it by executing this piece of code in
> scratchpad (in chrome mode. Be sure to open a regular web page in your
> current tab)
>   var document = content.document;
>   var form = document.createElement("form");
>   form.setAttribute("name", "foo");
>   document.body.appendChild(form);
>   Components.utils.reportError(document.foo); // Is undefined
>   Components.utils.reportError(document.wrappedJSObject.foo); // Is defined
> 

bholley:
I debugged this and found something surprising. In wrapper factory, in the rewrap function, for the case where the ctx is chrome the object is not and we create xraywrappers, there is a GetXrayType check (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/WrapperFactory.cpp#298) and for an HTMLFormElement I would expect that to return XrayForDOMObject but instead it returns simply XrayForWrappedNative... 
Is this a bug? Or we simply just not support these kind of DOM specific behaviors through xray wrappers? But then what is XrayForDOMObject case good for? Btw the GetXrayType type is based on mozilla::dom::IsDOMClass which is based on a flag that is not set for most DOM nodes/elements which is weird.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-05-30 10:17:14 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> bholley:
> I debugged this and found something surprising. In wrapper factory, in the
> rewrap function, for the case where the ctx is chrome the object is not and
> we create xraywrappers, there is a GetXrayType check
> (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/
> WrapperFactory.cpp#298) and for an HTMLFormElement I would expect that to
> return XrayForDOMObject but instead it returns simply
> XrayForWrappedNative... 
> Is this a bug? Or we simply just not support these kind of DOM specific
> behaviors through xray wrappers? But then what is XrayForDOMObject case good
> for? Btw the GetXrayType type is based on mozilla::dom::IsDOMClass which is
> based on a flag that is not set for most DOM nodes/elements which is weird.

In this case, "DOM" refers to "new-style dom bindings", which we currently only do for XHR (and I think WebGL+Canvas, now?). Definitely confusing in the transitional period, I know. FWIW, I was in favor of naming them something more revealing.
Comment 9 Gabor Krizsanits [:krizsa :gabor] 2012-05-30 13:45:21 PDT
(In reply to Bobby Holley (:bholley) from comment #8)
> In this case, "DOM" refers to "new-style dom bindings", which we currently
> only do for XHR (and I think WebGL+Canvas, now?). Definitely confusing in
> the transitional period, I know. FWIW, I was in favor of naming them
> something more revealing.

Ok this part is clear. We talked about this once and then you said that this is something that should be already supported and if does not work I should file a bug. But if I recall it correctly many months ago I talked about this with Blake in Paris and he told me that supporting this might not be done safely for xray wrapper. I'd like to dig into this problem, but first I want to collect as much info as I can...
Comment 10 Blake Kaplan (:mrbkap) 2012-05-30 14:51:33 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #4)
> No no, it is not about the content overloading a native method, but the
> content script itself. So we expect that *content scripts* can overload
> these native methods without seing overloads made by the web page.

Looking at the code, I'd actually expect this to work. It looks like we check the expando object before the holder in Xray wrappers. This also seems like something that we want.

> > > ** document["element-name"] are undefined

This (the form thing, and the named frame thing) are the same thing: basically, in cases where content can override default behavior we need to return functions instead. Consider the content page:
<form><input name="appendChild"></div>

And the chrome code: alert(contentDocument.forms[0].appendChild)

This should return the appendChild function, not the input element. I guess we could do something like "return the element unless it would shadow a function" but would be inconsistent with the behavior in the DOM...
Comment 11 Gabor Krizsanits [:krizsa :gabor] 2012-05-31 01:38:41 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #10)
> Looking at the code, I'd actually expect this to work. It looks like we
> check the expando object before the holder in Xray wrappers. This also seems
> like something that we want.

Ok, I will check this one out then.

> "return the element unless it would shadow a function"

This is probably good enough for us.
Comment 12 Alexandre Poirot [:ochameau] 2012-05-31 06:09:48 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> (In reply to Blake Kaplan (:mrbkap) from comment #10)
> > "return the element unless it would shadow a function"
> 
> This is probably good enough for us.

+1. I don't think it would be a blocker. Any different behavior from regular webpage behavior is a potential issue that needs to be mitigated. I'll try to take a look at usages of this feature, especially on frameworks basing some core feature on this, in order to see how much it can hurt them.


Otherwise, I just realized that I forgot one important requirement in order to remove content proxy code. We should prevent access to `wrappedJSObject` in content scripts. As we want to limit access to raw DOM nodes to the global `unsafeWindow` (and eventually avoid any raw access in future).
Comment 13 Alexandre Poirot [:ochameau] 2012-05-31 06:36:55 PDT
BTW, here is some food for thought. An old blog post about how content scripts are implemented in chromium. They are describing the same issue about one DOM and two different scopes playing with it:
http://aaronboodman-com-v1.blogspot.fr/2009/04/content-scripts-in-chromium.html
Comment 14 Gabor Krizsanits [:krizsa :gabor] 2012-05-31 10:21:05 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #12)

> Otherwise, I just realized that I forgot one important requirement in order
> to remove content proxy code. We should prevent access to `wrappedJSObject`
> in content scripts. As we want to limit access to raw DOM nodes to the
> global `unsafeWindow` (and eventually avoid any raw access in future).

This shouldn't be too hard, since we already do it for partially transparent wrappers.
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2012-05-31 11:02:11 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)

> This shouldn't be too hard, since we already do it for partially transparent
> wrappers.

FYI, partially transparent wrappers are going away now that we have CPG. But I'm pretty sure you just mean XOWs here...
Comment 16 Gabor Krizsanits [:krizsa :gabor] 2012-05-31 11:15:40 PDT
(In reply to Bobby Holley (:bholley) from comment #15)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> 
> FYI, partially transparent wrappers are going away now that we have CPG. But
> I'm pretty sure you just mean XOWs here...

I mean this http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/XrayWrapper.cpp#1073

Or is that comment about XOWs and ptw's only there to confuse poor guys like me? :)
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2012-05-31 11:23:20 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16)
> Or is that comment about XOWs and ptw's only there to confuse poor guys like
> me? :)

Yep.

"Partially transparent" Is this trick we do for Xrays where they're sometimes transparent and sometimes not, depending on document.domain. But with CPG we can just recompute wrappers system-wide when document.domain is set, so we can get rid of that logic.

Most of the wrappers right now are theoretically nameless (just identified by the various templated combinations), but they have shorthand that we used, defined in FilteringWrapper.cpp. No comments, of course. ;-)
Comment 18 Gabor Krizsanits [:krizsa :gabor] 2012-06-01 10:39:33 PDT
Blake, could you give me some hints?

I couldn't find a simple solution yet. So the problem is that nsIDOMClassInfo.cpp is full of checks like this one: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#9168

Now if I interpret getPropertyDescriptor on xray wrapper the logic is something like:
1. if transparent then we start with JS_GetPropertyDescriptorById on the unwrapped 'inner'

in other cases usually:

2.Traits::resolveOwnProperty - appart from some partially transparent magic, it's checking expandos on the holder and calls NewResolve if needed on the native

3.JS_GetPropertyDescriptorById on the holder - now this guy is a mistery to me right now, will this call holder_get for reach the WantGetProperty? I don't really think so...

4.Traits::resolveNativeProperty - this is fairly straightforward, checks for native stuff

Now the idea is that if all this fails, for the non transparent case, we could do something like step 1, and at this point we would be sure that no native is overwritten by its result. Only problem is that I don't know what side effects that might have, and that it does not seem to work since after this call, BaseProxyHandler::get will do the get with the wrapper as the |this| instead of the inner object, so a check in nsDOMClassInfo will prevent exposing the poroperty in the end... 
Right now I'm a bit lost (too many new stuff) and out of ideas... Thoughts?
Comment 19 Gabor Krizsanits [:krizsa :gabor] 2012-06-04 03:02:55 PDT
Created attachment 629734 [details] [diff] [review]
just an idea...

So I think this would fix the problem I described in my previous comment. I eliminated the check in the nsHTMLDocumentSH::GetProperty but left it in the nsHTMLDocumentSH::NewResolve. This way if the attached element has a name like "appendChild" it will not shadow the native method but if there is no native method with the given name the attached element will be visible. I'm not sure if the same logic can be applied in every other case as well, and I don't know for sure that this is safe. By safe I mean, that is there another call path I'm not aware of for xray wrappers where the nsHTMLDocumentSH::GetProperty will be called and we would need that check I removed.
Comment 20 Gabor Krizsanits [:krizsa :gabor] 2012-06-04 03:14:49 PDT
Ok, so I just made all the expandos of the content visible from chrome side which is bad. So instead of JS_GetPropertyDescriptorById on the unwrapped 'inner object' I should do something like holder_get, calling the GetProperty directly and then create a property descriptor for the result somehow I guess...
Comment 21 Gabor Krizsanits [:krizsa :gabor] 2012-06-05 06:30:37 PDT
Created attachment 630147 [details] [diff] [review]
2nd try

Something like this... it's hacky, but I hope the idea is clear. Seems to do exactly what we discussed. If you think it's the right approach (or at least it makes sense and good enough) I will create a polished version and will try to follow the same logic for the rest of the DOM getters.
Comment 22 Gabor Krizsanits [:krizsa :gabor] 2012-06-06 06:24:36 PDT
Created attachment 630542 [details] [diff] [review]
part 1: fixing BaseProxyHandler::set

So the reason why hiding native properties with an expando on the xray wrapper did not work is a missing line apparently. Without this line the vp is ignored in some cases and the defineProperty call creates an expando on the holder with the original value instead of the new one.
Comment 23 Jim Blandy :jimb 2012-06-06 12:51:40 PDT
This looks good, but it needs a test; this path doesn't seem to be covered by the JS tests at all. (Yuck!)
Comment 24 Jim Blandy :jimb 2012-06-06 12:58:48 PDT
Created attachment 630679 [details] [diff] [review]
Fix proxy behavior when assigning to inherited properties. [landed]

The path changed by Gabor's patch isn't exercised by the JS test suite. This patch includes Gabor's fix, and a JS-only test by me.
Comment 25 Jim Blandy :jimb 2012-06-06 13:42:09 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=368fdc7d5ad9
Comment 26 Gabor Krizsanits [:krizsa :gabor] 2012-06-07 03:27:38 PDT
(In reply to Jim Blandy :jimb from comment #25)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=368fdc7d5ad9

Looks like it fixes a failing test. http://mxr.mozilla.org/mozilla-central/source/dom/imptests/failures/html/tests/submission/Opera/microdata/test_001.html.json?force=1

line 6 should be removed here
Comment 27 Blake Kaplan (:mrbkap) 2012-06-07 15:30:06 PDT
Comment on attachment 630147 [details] [diff] [review]
2nd try

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

This approach looks reasonable to me.

::: dom/base/nsDOMClassInfo.cpp
@@ -9257,5 @@
>                                JSContext *cx, JSObject *obj, jsid id,
>                                jsval *vp, bool *_retval)
>  {
>    // For native wrappers, do not get random names on document
> -  if (!ObjectIsNativeWrapper(cx, obj)) {

I don't understand how this patch prevents shadowing, won't this hit for names that would be shadowed normally? It seems like you need to communicate an extra bit of information here.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +735,5 @@
> +        if (desc->value.isUndefined())
> +            return true;
> +
> +        desc->obj = wrapper;
> +        desc->getter = XPC_WN_Helper_GetProperty;

Why is this needed? It seems like you should be able to get away with simply using NULL (so we go through holder_get) here.
Comment 28 Jason Orendorff [:jorendorff] 2012-06-07 17:56:02 PDT
Comment on attachment 630679 [details] [diff] [review]
Fix proxy behavior when assigning to inherited properties. [landed]

>+// When we assign to a property that a proxy claims is inherited, the
>+// defineProperty handler call to create the new own property should get
>+// the newly assigned value, but otherwise get the same attributes as the
>+// inherited property.

It's a super bizarre bug, actually, that we inherit the other attributes. This sentence should end after "the newly assigned value".

>+    assertEq(descriptor.writable, true);
>+    assertEq(descriptor.enumerable, false);
>+    assertEq(descriptor.configurable, false);

In order to stick to what's germane, and not forbid this bug being fixed, I suggest deleting these assertions.

Also add an assertion that hits is 1 afterwards. (You'll have to initialize it to 0, I think.)
Comment 29 Jim Blandy :jimb 2012-06-07 18:10:34 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #28)
> In order to stick to what's germane, and not forbid this bug being fixed, I
> suggest deleting these assertions.

Sure thing. And file a follow-up.

> Also add an assertion that hits is 1 afterwards. (You'll have to initialize
> it to 0, I think.)

D'oh. Thanks.
Comment 30 Jim Blandy :jimb 2012-06-07 18:25:57 PDT
Filed bug 762751 as a follow-up, for the attributes other than value.
Comment 31 Jim Blandy :jimb 2012-06-07 18:36:12 PDT
Landed in inbound; more patches left to land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a02ae7ddd797
Comment 32 Jim Blandy :jimb 2012-06-07 18:47:14 PDT
Created attachment 631241 [details] [diff] [review]
Mark test as now passing: dom/imptests/html/tests/submission/Opera/microdata/test_001.html | the namedItem property must be read/write

This fixes the unexpected pass Gabor mentioned in comment 26.
Pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/81bb0865aee2
Comment 33 Jim Blandy :jimb 2012-06-07 18:48:55 PDT
Gabor, once you've landed your "2nd try" patch, please remove the "leave open after merge" marker from the whiteboard, so the inbound merging guys will know to close the bug.
Comment 34 Phil Ringnalda (:philor) 2012-06-07 21:20:09 PDT
Followup followup https://hg.mozilla.org/integration/mozilla-inbound/rev/274d17d6394e, since that was actually "line 6 should be removed here, along with the trailing comma on line 5 since otherwise it'll be invalid json and the test will time out four times and take down the entire suite."
Comment 35 Jim Blandy :jimb 2012-06-07 21:56:23 PDT
My hero!
Comment 36 :Ms2ger (⌚ UTC+1/+2) 2012-06-08 02:38:48 PDT
Jim, why did you push this when you knew it would turn the tree orange?
Comment 38 Gabor Krizsanits [:krizsa :gabor] 2012-06-08 07:12:09 PDT
Created attachment 631384 [details] [diff] [review]
dom specific getters (3rd try...)

Ok I think I have it this time. There are no tests yet, and I need to rename a flag, but so far it seems to work.


(In reply to Blake Kaplan (:mrbkap) from comment #27)
> Comment on attachment 630147 [details] [diff] [review]
> 2nd try
> 
> Review of attachment 630147 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This approach looks reasonable to me.
> 
> ::: dom/base/nsDOMClassInfo.cpp
> @@ -9257,5 @@
> >                                JSContext *cx, JSObject *obj, jsid id,
> >                                jsval *vp, bool *_retval)
> >  {
> >    // For native wrappers, do not get random names on document
> > -  if (!ObjectIsNativeWrapper(cx, obj)) {
> 
> I don't understand how this patch prevents shadowing, won't this hit for
> names that would be shadowed normally? It seems like you need to communicate
> an extra bit of information here.

So the idea is that first NewResolve is called and because of a similar ObjectIsNativeWrapper check, shadowing is prevented for the xray case. Then we check for native methods on the wn, if we found one, this getter will not be called. If  we didn't find anything with the given id only then will we get here in which case we don't have to worry about shadowing. 

> 
> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> @@ +735,5 @@
> > +        if (desc->value.isUndefined())
> > +            return true;
> > +
> > +        desc->obj = wrapper;
> > +        desc->getter = XPC_WN_Helper_GetProperty;
> 
> Why is this needed? It seems like you should be able to get away with simply
> using NULL (so we go through holder_get) here.

You are right. I tried to figure it out why I needed this and I couldn't... In the new version this part is a bit different, but I think I just got confused during debugging or something so it should be just NULL-ed out.

The problem I encountered that for the window GetProperty and NewResolve works differently than for document and form. So I invented a new flag (which should be renamed and I'm looking for a good name) and instead of calling for GetProperty like in the previous version I call NewResolve with this special flag. So to prevent shadowing first we call NewResolve normally, and it will not do much because of the ObjectIsNativeWrapper checks, then check for everything else (including native methods on wn) and finally if nothing found we try NewResolve again, but this time with that special flag that passes through ObjectIsNativeWrapper checks when we need it.
Comment 39 Blake Kaplan (:mrbkap) 2012-06-25 05:11:17 PDT
Comment on attachment 631384 [details] [diff] [review]
dom specific getters (3rd try...)

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

This feels very reasonable. My only thought is that instead of a resolve flag (which the JS guys probably won't like), can we instead set a flag on the wrapper itself an expose xpc::XrayWrapperNotShadowing(obj) or something like that?

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +718,5 @@
> +    XPCWrappedNative *wn = GetWrappedNativeFromHolder(holder);
> +
> +    // Run the resolve hook of the wrapped native.
> +    if (!NATIVE_HAS_FLAG(wn, WantNewResolve)) {
> +        return true;

Nit: no braces.

@@ +733,5 @@
> +        return false;
> +    }
> +
> +    if (pobj) {
> +        if (!JS_GetPropertyDescriptorById(cx, holder, id, JSRESOLVE_QUALIFIED, desc))

Nit: if (pobj && !JS_GetProperty....

@@ +1164,5 @@
>          return false;
>  
>      if (!desc->obj) {
> +        if (id != nsXPConnect::GetRuntimeInstance()->GetStringID(XPCJSRuntime::IDX_TO_STRING)) {
> +            // one last try...

Please expand this comment: We know that we're not shadowing so...
Comment 40 Gabor Krizsanits [:krizsa :gabor] 2012-06-25 06:03:10 PDT
I talked about this with bholley the other day, and he suggested me to merge this resolveOwnPropertySpecial into the resolveNativeProperty hook. So we don't have to invent a new hook on xrays. It would make the code simpler, and personaly I don't mind moving this logic into resolveNativeProperty, so unless you are against it I will just do that.
Comment 41 Blake Kaplan (:mrbkap) 2012-06-25 06:15:02 PDT
Sure.
Comment 42 Gabor Krizsanits [:krizsa :gabor] 2012-06-27 04:30:34 PDT
Created attachment 637075 [details] [diff] [review]
Supporting DOM specific collection properties through xray wrappers

There were no setter for wrapper flags so I added one. I think this was the reason that I did not choose this route first. I thought maybe I should invent a public api for setting wrapper flags, but after I checked the public api for receiving wrapper flags I'd rather not.

I've thought a lot about what would be the best name for this function, I chose resolveDOMCollectionProperty. I'm open for better ideas. It is not another hook just a static function I call from the resolveNativeProperty hook of the regular xray traits.
Comment 43 Blake Kaplan (:mrbkap) 2012-07-20 15:14:01 PDT
Comment on attachment 637075 [details] [diff] [review]
Supporting DOM specific collection properties through xray wrappers

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

One suggestion below. If you disagree with it, re-request review and I'll stamp.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +574,5 @@
> +    // Setting the XRAY_NOT_SHADOWING flag, so this time these DOM specific 
> +    // collections will not be skipped in the NewResolve callbacks.
> +    AbstractWrapper *abstractWrapper = AbstractWrapper::wrapperHandler(wrapper);
> +    unsigned wrapperflags = abstractWrapper->flags();
> +    abstractWrapper->setFlags(wrapperflags | WrapperFactory::XRAY_NOT_SHADOWING);

Last question: should we tie the shadowing/not shadowing question to the id being resolved? That way, if one of the NewResolve hooks manages to run code somehow, we'll still do the Right Thing.

I'm also tempted to ask for an auto-helper to reset the flags instead of relying on nobody adding an early return somewhere in here.
Comment 44 Blake Kaplan (:mrbkap) 2012-07-20 15:14:19 PDT
A million apologies for the lag here, btw.
Comment 45 Gabor Krizsanits [:krizsa :gabor] 2012-07-23 04:11:32 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #43)
> 
> Last question: should we tie the shadowing/not shadowing question to the id
> being resolved? That way, if one of the NewResolve hooks manages to run code
> somehow, we'll still do the Right Thing.

Ok, so it's not that I don't agree I just really don't get this. Could you give me some more details? By 'tying to the id' you mean append something to it, or is there a way to store additional info on an id? Also it's not clear how/where can we use that info if NewResolve manages to run code somehow to do the right thing. 
So you really lost me here, sorry...

> I'm also tempted to ask for an auto-helper to reset the flags instead of
> relying on nobody adding an early return somewhere in here.
Sure, I'll do that.
Comment 46 Jim Blandy :jimb 2012-07-23 11:19:46 PDT
(In reply to :Ms2ger from comment #36)
> Jim, why did you push this when you knew it would turn the tree orange?

Because I screwed up?
Comment 47 Gabor Krizsanits [:krizsa :gabor] 2012-08-15 07:14:11 PDT
Created attachment 652092 [details] [diff] [review]
Supporting DOM specific collection properties through xray wrappers
Comment 48 :Ms2ger (⌚ UTC+1/+2) 2012-08-15 07:30:47 PDT
Comment on attachment 652092 [details] [diff] [review]
Supporting DOM specific collection properties through xray wrappers

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

One thing I'd like a lot (for my education) is an explanation of when one should use

* ObjectIsNativeWrapper(cx, obj),
* ObjectIsNativeWrapper(cx, obj) || xpc::WrapperFactory::XrayWrapperNotShadowing(obj), or
* nothing

in nsDOMClassInfo...

::: dom/base/nsDOMClassInfo.cpp
@@ +7221,5 @@
>    // Hmm, we do an awful lot of QIs here; maybe we should add a
>    // method on an interface that would let us just call into the
>    // window code directly...
>  
> +  if (!ObjectIsNativeWrapper(cx, obj) || 

Trailing whitespace

@@ +9229,5 @@
>      // For native wrappers, do not resolve random names on document
>  
>      JSAutoRequest ar(cx);
>  
> +    if (!ObjectIsNativeWrapper(cx, obj) || 

Also here

@@ +9377,5 @@
>                                  bool *_retval)
>  {
>    // For native wrappers, do not resolve random names on form
>    if ((!(JSRESOLVE_ASSIGNING & flags)) && JSID_IS_STRING(id) &&
> +      (!ObjectIsNativeWrapper(cx, obj) || 

and here

::: js/src/jswrapper.h
@@ +74,5 @@
>          return mFlags;
>      }
>  
> +    void setFlags(unsigned flags) {
> +        mFlags = flags; 

and here

::: js/xpconnect/tests/chrome/test_bug738244.xul
@@ +41,5 @@
> +           "Input element shadows native property with its name through xray");
> +           
> +        is(win.frame1.name, "frame1",
> +           "IFrames cannot be found by name on the window through xray");   
> +        

And all over this file

::: js/xpconnect/wrappers/WrapperFactory.h
@@ +24,5 @@
>             // Prevent scripts from shadowing native properties.
>             // NB: Applies only to Xray wrappers.
>             // NB: This will prevent scriptable helpers from defining special
>             //     handlers for properties defined in IDL. Use with caution.
> +           SHADOWING_FORBIDDEN     = SOW_FLAG << 1,    

And here

@@ +100,5 @@
>      static JSObject *WrapForSameCompartmentXray(JSContext *cx, JSObject *obj);
>  };
>  
> +struct AutoSetWrapperNotShadowing {
> +    AutoSetWrapperNotShadowing(JSObject *wrapperObj) 

And here.

This should probably also have the macros from GuardObjects.h.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +569,5 @@
> +    XPCWrappedNative *wn = GetWrappedNativeFromHolder(holder);
> +    if (!NATIVE_HAS_FLAG(wn, WantNewResolve))
> +        return true;
> +
> +    // Setting the XRAY_NOT_SHADOWING flag, so this time these DOM specific 

More whitespace

@@ +607,5 @@
>      XPCCallContext ccx(JS_CALLER, cx, wnObject, nullptr, id);
>  
> +    // There are no native numeric properties, so we can shortcut here. We will
> +    // not find the property. However we want to support non shadowing dom 
> +    // specific collection properties like window.frames, so we still have to 

More
Comment 49 Gabor Krizsanits [:krizsa :gabor] 2012-08-15 08:29:07 PDT
(In reply to :Ms2ger from comment #48)
> 
> One thing I'd like a lot (for my education) is an explanation of when one
> should use
> 
> * ObjectIsNativeWrapper(cx, obj),
> * ObjectIsNativeWrapper(cx, obj) ||
> xpc::WrapperFactory::XrayWrapperNotShadowing(obj), or
> * nothing

Very good idea, I will try to come up with some sane explanation.

> This should probably also have the macros from GuardObjects.h.

Eh... I never liked those macros. And since there is no default constructor defined for this class, AutoSetWrapperNotShadowing(wrapper); would not compile, since that would be interpreted as AutoSetWrapperNotShadowing wrapper; (at least on msvc...).

> More
Sorry about trailing white spaces... I will eliminate them.
Comment 50 Blake Kaplan (:mrbkap) 2012-08-21 11:55:27 PDT
Comment on attachment 652092 [details] [diff] [review]
Supporting DOM specific collection properties through xray wrappers

Gabor and I talked about this offline and a new patch is on its way.
Comment 51 Gabor Krizsanits [:krizsa :gabor] 2012-08-22 13:43:53 PDT
Created attachment 654359 [details] [diff] [review]
Supporting DOM specific collection properties through xray wrappers

So I added the guard macros (easier than arguing against them), and I added the extra security check Blake mentioned. For that I had to move ResolvingId out from the trait classes so they can be reached from WrapperFactory. Instead of swapping a flag on the wrapper, I just mark the current ResolvingId. If there is no ResolvingId to check that can only happens if resolveNative is called from defineProperty in case of shadowing is forbidden, resolveDOMCollectionProperty should not do anything.
Added some more comments and killed the trailing white spaces.
Comment 52 Blake Kaplan (:mrbkap) 2012-08-22 14:24:32 PDT
Comment on attachment 654359 [details] [diff] [review]
Supporting DOM specific collection properties through xray wrappers

I like it.
Comment 53 :Ms2ger (⌚ UTC+1/+2) 2012-08-23 01:18:30 PDT
Comment on attachment 654359 [details] [diff] [review]
Supporting DOM specific collection properties through xray wrappers

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

::: dom/base/nsDOMClassInfo.h
@@ +135,5 @@
> +   *
> +   * Note: So ObjectIsNativeWrapper(cx, obj) check usually means "through xray
> +   * wrapper this part is not visible" while combined with 
> +   * || xpc::WrapperFactory::XrayWrapperNotShadowing(obj) it means "through 
> +   * xray wrapper it is visible only if it does not hide any native property."

Trailing whitespace

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +625,5 @@
>      return wrapperObj;
>  }
>  
> +
> +bool 

.

@@ +626,5 @@
>  }
>  
> +
> +bool 
> +WrapperFactory::XrayWrapperNotShadowing(JSObject *wrapper, jsid id) {

{ on the next line

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +322,5 @@
> +
> +ResolvingId *
> +ResolvingId::getResolvingId(JSObject *holder)
> +{
> +    return (ResolvingId *)js::GetReservedSlot(holder, JSSLOT_RESOLVING).toPrivate();

Can we assert something about the class of the holder here?

@@ +369,2 @@
>  
> +    static bool resolveDOMCollectionProperty(JSContext *cx, JSObject *wrapper, JSObject *holder, 

Whitespace

@@ +577,5 @@
>      }
>      return true;
>  }
>  
> +class AutoSetWrapperNotShadowing 

And here

@@ +584,5 @@
> +    AutoSetWrapperNotShadowing(JSObject *wrapper MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> +    {
> +        MOZ_GUARD_OBJECT_NOTIFIER_INIT;
> +        MOZ_ASSERT(wrapper);
> +        mResolvigId = ResolvingId::getResolvingIdFromWrapper(wrapper);

Resolvig?

@@ +605,5 @@
> +// like document["formName"] because we already know that it is not shadowing
> +// any native property.
> +bool
> +XPCWrappedNativeXrayTraits::resolveDOMCollectionProperty(JSContext *cx, JSObject *wrapper, 
> +                                                         JSObject *holder, jsid id, bool set, 

Whitespace

@@ +658,5 @@
>      XPCCallContext ccx(JS_CALLER, cx, wnObject, nullptr, id);
>  
> +    // There are no native numeric properties, so we can shortcut here. We will
> +    // not find the property. However we want to support non shadowing dom 
> +    // specific collection properties like window.frames, so we still have to 

.

::: js/xpconnect/wrappers/XrayWrapper.h
@@ +108,5 @@
> +class ResolvingId
> +{
> +public:
> +    ResolvingId(JSObject *wrapper, jsid id);
> +    ResolvingId::~ResolvingId();

I don't think you need the qualification here
Comment 54 Gabor Krizsanits [:krizsa :gabor] 2012-08-27 06:08:17 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/febe1ad166da
Comment 55 Ryan VanderMeulen [:RyanVM] 2012-08-27 12:18:47 PDT
https://hg.mozilla.org/mozilla-central/rev/febe1ad166da
Comment 56 Gabor Krizsanits [:krizsa :gabor] 2012-08-30 02:10:05 PDT
Since this bug contains only platform changes I changed the bug to a core xpconnect bug. Can anyone verify if I set up the Target Milestone correctly? (landed in Comment 55)
Comment 57 :Ms2ger (⌚ UTC+1/+2) 2012-08-30 03:25:03 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #56)
> Since this bug contains only platform changes I changed the bug to a core
> xpconnect bug. Can anyone verify if I set up the Target Milestone correctly?
> (landed in Comment 55)

Verified.

Note You need to log in before you can comment on or make changes to this bug.