Closed Bug 738244 Opened 12 years ago Closed 12 years ago

Remove content-proxy code

Categories

(Core :: XPConnect, defect, P1)

Product:
Core
Component:
XPConnect
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: gkrizsanits, Unassigned)

References

Details

Attachments

(3 files, 6 obsolete files)

Fix proxy behavior when assigning to inherited properties. [landed]
1.48 KB, patch
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
Details | Diff | Splinter Review
Supporting DOM specific collection properties through xray wrappers
24.89 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review 
This bug first requires some platform bugs to be fixed first, but after that the content-proxy code should go.
bug 601295 contains valuable information about my tests done while introducing proxies.
And bug 726949 looks like a blocker for removing proxes.
Depends on: 726949
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?
OS: Windows 7 → All
Hardware: x86_64 → All
Depends on: 658909
(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?
(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 ?
(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...
(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.
(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.
(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.
(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...
(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...
(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.
(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).
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
(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.
(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...
(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? :)
(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. ;-)
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?
Attached patch just an idea... (obsolete) — — Splinter Review 
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.
Attachment #629734 - Flags: feedback?(mrbkap)
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...
Attached patch 2nd try (obsolete) — — Splinter Review 
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.
Attachment #629734 - Attachment is obsolete: true
Attachment #629734 - Flags: feedback?(mrbkap)
Attachment #630147 - Flags: feedback?(mrbkap)
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.
Attachment #630542 - Flags: review?(jimb)
This looks good, but it needs a test; this path doesn't seem to be covered by the JS tests at all. (Yuck!)
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.
Attachment #630542 - Attachment is obsolete: true
Attachment #630542 - Flags: review?(jimb)
Attachment #630679 - Flags: review?(jorendorff)
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.
Attachment #630147 - Flags: feedback?(mrbkap) → feedback+
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.)
Attachment #630679 - Flags: review?(jorendorff) → review+
(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.
Filed bug 762751 as a follow-up, for the attributes other than value.
Landed in inbound; more patches left to land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a02ae7ddd797
Whiteboard: [Leave open after merge]
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.
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."
My hero!
Jim, why did you push this when you knew it would turn the tree orange?
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.
Attachment #630147 - Attachment is obsolete: true
Attachment #631384 - Flags: feedback?(mrbkap)
Attachment #631384 - Attachment is patch: true
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...
Attachment #631384 - Flags: feedback?(mrbkap) → feedback+
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.
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.
Attachment #631384 - Attachment is obsolete: true
Attachment #637075 - Flags: review?(mrbkap)
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.
Attachment #637075 - Flags: review?(mrbkap)
A million apologies for the lag here, btw.
(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.
(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?
Attachment #630679 - Attachment description: Fix proxy behavior when assigning to inherited properties. → Fix proxy behavior when assigning to inherited properties. [landed]

        Attachment #637075 -
        Attachment is obsolete: true

        Attachment #652092 -
        Flags: review?(mrbkap)

    
    

    
  
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

    
    

    
  
(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 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.

        Attachment #652092 -
        Flags: review?(mrbkap)

    
    

    
  


  
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.

        Attachment #652092 -
        Attachment is obsolete: true

        Attachment #654359 -
        Flags: review?(mrbkap)

    
    

    
  
Comment on attachment 654359 [details] [diff] [review]
Supporting DOM specific collection properties through xray wrappers

I like it.

        Attachment #654359 -
        Flags: review?(mrbkap) → review+

    
    

    
  
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

    
  
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

    
  
Component: General → XPConnect
Product: Add-on SDK → Core
Target Milestone: --- → mozilla17

    
    

    
  
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)

    
  
Whiteboard: [Leave open after merge]

    
    

    
  
(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.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: