If an object in content should be wrapped for chrome code, the getter/setter in the object does not work.

submit the testing code

Hello Joel, May I have your review? Thanks.

Comment on attachment 8732087 [details] [diff] [review] Support set/get for mocked object in content for mochitest, v1 Review of attachment 8732087 [details] [diff] [review]: ----------------------------------------------------------------- I really don't understand the changes made and the test. This is topics which I have no knowledge of. Possibly :ted.m or :bz would be good reviewers. ::: testing/mochitest/tests/Harness_sanity/mochitest.ini @@ +33,5 @@ > [test_SpecialPowersLoadChromeScript.html] > support-files = SpecialPowersLoadChromeScript.js > [test_SpecialPowersLoadChromeScript_function.html] > +[test_bug1257734.html] > +skip-if = toolkit == e10s can you just do |skip-if = e10s| ? ::: testing/mochitest/tests/Harness_sanity/test_bug1257734.html @@ +69,5 @@ > + } > + return this.QueryInterface(aIID); > + }, > + get spec() { > + info("get spec"); are these info statements useful in troubleshooting?

(In reply to Joel Maher (:jmaher) from comment #3) > Comment on attachment 8732087 [details] [diff] [review] > Support set/get for mocked object in content for mochitest, v1 > > Review of attachment 8732087 [details] [diff] [review]: > ----------------------------------------------------------------- > > I really don't understand the changes made and the test. This is topics > which I have no knowledge of. Possibly :ted.m or :bz would be good > reviewers. Thanks for your suggestion :) > > ::: testing/mochitest/tests/Harness_sanity/mochitest.ini > @@ +33,5 @@ > > [test_SpecialPowersLoadChromeScript.html] > > support-files = SpecialPowersLoadChromeScript.js > > [test_SpecialPowersLoadChromeScript_function.html] > > +[test_bug1257734.html] > > +skip-if = toolkit == e10s > > can you just do |skip-if = e10s| ? Yes, my bad > > ::: testing/mochitest/tests/Harness_sanity/test_bug1257734.html > @@ +69,5 @@ > > + } > > + return this.QueryInterface(aIID); > > + }, > > + get spec() { > > + info("get spec"); > > are these info statements useful in troubleshooting? Yes, this issue results from malfunction of setter and getter.

Comment on attachment 8732087 [details] [diff] [review] Support set/get for mocked object in content for mochitest, v1 Hello bz, Could I have your review? Thanks.

Comment on attachment 8732087 [details] [diff] [review] Support set/get for mocked object in content for mochitest, v1 Going to punt this to Bobby, because I'm not sure what the invariants for this stuff are.

Comment on attachment 8732087 [details] [diff] [review] Support set/get for mocked object in content for mochitest, v1 Review of attachment 8732087 [details] [diff] [review]: ----------------------------------------------------------------- The patch seems fine on its own, but the use-case in the test seems like the wrong approach to me. Yes, it's possible to make it work, but doing we shouldn't be writing new code with complicated QI implementations in content JS via a pile of wrappers. The SpecialPowers code was designed for: (a) Very isolated one-off privileged things that we need to do from a mochitest. (b) Converting old things from enablePrivilege that never should have been mochitests in the first place. Tests like the one in this patch should be written with mochitest-chrome, with a content iframe as necessary. Or if it really needs to be mochitest-plain for some reason, you can use SpecialPowers.loadChromeScript to isolate the privileged code into privileged compartment. Can you elaborate on the use-case that is motivating this? Is it similar to to the test in this patch?

(In reply to Bobby Holley (busy) from comment #7) > Comment on attachment 8732087 [details] [diff] [review] > Support set/get for mocked object in content for mochitest, v1 > > Review of attachment 8732087 [details] [diff] [review]: > ----------------------------------------------------------------- > > The patch seems fine on its own, but the use-case in the test seems like the > wrong approach to me. Yes, it's possible to make it work, but doing we > shouldn't be writing new code with complicated QI implementations in content > JS via a pile of wrappers. > > The SpecialPowers code was designed for: > (a) Very isolated one-off privileged things that we need to do from a > mochitest. > (b) Converting old things from enablePrivilege that never should have been > mochitests in the first place. > > Tests like the one in this patch should be written with mochitest-chrome, > with a content iframe as necessary. Or if it really needs to be > mochitest-plain for some reason, you can use SpecialPowers.loadChromeScript > to isolate the privileged code into privileged compartment. loadChromeScript doesn't work if I like to load script in content process for e10s. > > Can you elaborate on the use-case that is motivating this? Is it similar to > to the test in this patch? The intuition of lots of QI in the test is pretty simple, mocking factory and pass objects in the content to chrome. This bug resolves malfunction of getter/setter of the passed objects. We can see many needs here: https://dxr.mozilla.org/mozilla-central/search?q=SpecialPowers.wrap%28SpecialPowers.Components%29.manager&redirect=false&case=false I know there's other use-case for passing objects from content to chrome, but I can't find another general one. Moreover, since lots of needs and dup codes, refactoring to SpecialPowers API might be the next bug to file. That would facilitate less weird QIs in the content. (We already have one for chrome process https://dxr.mozilla.org/mozilla-central/source/testing/modules/MockRegistrar.jsm) How do you think?

modified by comment 3

Sorry I haven't had a chance to look at this yet. I'll try to tomorrow.

Comment on attachment 8732706 [details] [diff] [review] Support set/get for mocked object in content for mochitest, v2 Review of attachment 8732706 [details] [diff] [review]: ----------------------------------------------------------------- > loadChromeScript doesn't work if I like to load script in content process for e10s. Ok. I think the correct thing to do here is to make loadChromeScript accept an option to run the script in the content process (with a privileged global). Or perhaps more simply, add SpecialPowers.loadPrivilegedScript. This would simply execute the script in a new Cu.Sandbox with a privileged global, and return a MessageChannel port to be used for communication. Something like: loadPrivilegedScript: function (urlOrFunction) { var str = /* get the source from the arg, like loadChromeScript does. */; var sb = new Cu.sandbox(this); var mc = new MessageChannel(); sb.port = mc.port1; sb.eval(str); return wrap(mc.port2); } I would much prefer if we did something like that, rather than adding more complicated dependencies on black-wrapper-magic. I am potentially willing to take the existing patch here if you really need it, but I would like to be sure that we really need to use wrappers for this stuff. ::: testing/specialpowers/content/specialpowersAPI.js @@ +358,5 @@ > var wrapper = {}; > for (var i in obj) { > if (typeof obj[i] == 'function') > wrapper[i] = wrapCallback(obj[i]); > + else { We can just use the |else| case to handle both situations, right? Seems like we should remove the conditional.

(In reply to Bobby Holley (busy) from comment #12) > Comment on attachment 8732706 [details] [diff] [review] > Support set/get for mocked object in content for mochitest, v2 > > Review of attachment 8732706 [details] [diff] [review]: > ----------------------------------------------------------------- > > > loadChromeScript doesn't work if I like to load script in content process for e10s. > > Ok. I think the correct thing to do here is to make loadChromeScript accept > an option to run the script in the content process (with a privileged > global). Or perhaps more simply, add SpecialPowers.loadPrivilegedScript. > This would simply execute the script in a new Cu.Sandbox with a privileged > global, and return a MessageChannel port to be used for communication. > > Something like: > > loadPrivilegedScript: function (urlOrFunction) { > var str = /* get the source from the arg, like loadChromeScript does. */; > var sb = new Cu.sandbox(this); > var mc = new MessageChannel(); > sb.port = mc.port1; > sb.eval(str); > return wrap(mc.port2); > } > > I would much prefer if we did something like that, rather than adding more > complicated dependencies on black-wrapper-magic. I am potentially willing to > take the existing patch here if you really need it, but I would like to be > sure that we really need to use wrappers for this stuff. That's would be great if we have an API like |SpecialPowers.loadPrevilegedScript|. If we have this, we could import "MockRegistar.jsm" and do the same thing easily. However, I still think we need the patch (at least the changes in SpecialPowers) since we may wrap a callback with a setter/getter in the future (or already need but we don't know) Hence, let's make this patch good to land first. Then cook |loadPrevilegedScript| and refactor next (I'm willing to take this in the near future, or welcome to contribute if someone likes to) How about this idea? > > ::: testing/specialpowers/content/specialpowersAPI.js > @@ +358,5 @@ > > var wrapper = {}; > > for (var i in obj) { > > if (typeof obj[i] == 'function') > > wrapper[i] = wrapCallback(obj[i]); > > + else { > > We can just use the |else| case to handle both situations, right? Seems like > we should remove the conditional. I'm with you.

needinfo? for comment 13

(In reply to Junior [:junior] from comment #13) > (In reply to Bobby Holley (busy) from comment #12) > > Comment on attachment 8732706 [details] [diff] [review] > > Support set/get for mocked object in content for mochitest, v2 > > > > Review of attachment 8732706 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > > loadChromeScript doesn't work if I like to load script in content process for e10s. > > > > Ok. I think the correct thing to do here is to make loadChromeScript accept > > an option to run the script in the content process (with a privileged > > global). Or perhaps more simply, add SpecialPowers.loadPrivilegedScript. > > This would simply execute the script in a new Cu.Sandbox with a privileged > > global, and return a MessageChannel port to be used for communication. > > > > Something like: > > > > loadPrivilegedScript: function (urlOrFunction) { > > var str = /* get the source from the arg, like loadChromeScript does. */; > > var sb = new Cu.sandbox(this); > > var mc = new MessageChannel(); > > sb.port = mc.port1; > > sb.eval(str); > > return wrap(mc.port2); > > } > > > > I would much prefer if we did something like that, rather than adding more > > complicated dependencies on black-wrapper-magic. I am potentially willing to > > take the existing patch here if you really need it, but I would like to be > > sure that we really need to use wrappers for this stuff. > That's would be great if we have an API like > |SpecialPowers.loadPrevilegedScript|. > If we have this, we could import "MockRegistar.jsm" and do the same thing > easily. > > However, I still think we need the patch (at least the changes in > SpecialPowers) > since we may wrap a callback with a setter/getter in the future (or already > need but we don't know) > > Hence, let's make this patch good to land first. > Then cook |loadPrevilegedScript| and refactor next > (I'm willing to take this in the near future, or welcome to contribute if > someone likes to) > How about this idea? Ok, sounds good. It should be a simple patch - roughly what I wrote in comment 12. I'm ok to land this for now I guess. Please remove the test from this patch though, because it's doing exactly the kind of complicated stuff that I don't want to encourage. ;-)

I remove the test from this patch. I file Bug 1260076 for tracking SpecialPowers.loadPrevilegedScript

Leave the test patch here for those who want to have a check

backed out test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=24842686&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/2eeff30a5502

(In reply to Carsten Book [:Tomcat] from comment #20) > backed out test failures like > https://treeherder.mozilla.org/logviewer.html#?job_id=24842686&repo=mozilla- > inbound I'm working on it

> > ::: testing/specialpowers/content/specialpowersAPI.js > > @@ +358,5 @@ > > > var wrapper = {}; > > > for (var i in obj) { > > > if (typeof obj[i] == 'function') > > > wrapper[i] = wrapCallback(obj[i]); > > > + else { > > > > We can just use the |else| case to handle both situations, right? Seems like > > we should remove the conditional. > > I'm with you. We still need the |if| part since a function attribute is without a property. That's the reason of try-break.

Modify by comment 23, ask for review since the logic changed And I guess |SpecialPowers.wrapCallbackObject| can not handle the object with a complicated attribute. Such as: WrapperCallBack = { _complicatedAttribute: { get one() { return 1; } } } Maybe we need to file another bug?

Comment on attachment 8737111 [details] [diff] [review] Support set/get for mocked object in content for mochitest, v4 Review of attachment 8737111 [details] [diff] [review]: ----------------------------------------------------------------- This is pretty hacky. We first try to invoke the getter and use the return-type (assuming that it's side-effect-free and always returns the same-type) to determine whether we want to wrap the return-value or the getter itself. I'm not willing to add more craziness like this to the wrapper layer. Let's do the thing in comment 12.

I misinterpret the try-break message. The attribute can't get the property in some case (e.g. in the prototype) Could we deal with this like, for (var attribute in obj) { var property = Object.getOwnPropertyDescriptor(obj, attribute); if (property) { // Deal with the property } else { // Wrap as before } } I guess this way is less hacky and more like r+'ed patch in comment 16. And facilitate my implementation. How about this idea?

That means that we'd effectively support |own| accessors but not ones inherited from the prototype, which feels pretty arbitrary. There's also the question of whether we should wrap getters, or only setters. And what we should do with non-enumerable properties. And non-configurable properties. And things on the prototype chain. I really don't want to make this stuff more complicated. Can you please try the solution from comment 12?

(In reply to Bobby Holley (busy) from comment #28) > That means that we'd effectively support |own| accessors but not ones > inherited from the prototype, which feels pretty arbitrary. There's also the > question of whether we should wrap getters, or only setters. And what we > should do with non-enumerable properties. And non-configurable properties. > And things on the prototype chain. > > I really don't want to make this stuff more complicated. Can you please try > the solution from comment 12? I know this patch doesn't do great with prototype and prototype chain. But it's better than original one. At least save lots time for further developers. Or we need to do some document in the code. I can try the solution from comment 12, but it needs much more time and differs lots from |loadChromeScript| in my first impression. Maybe I'll find it's easier than I think after I dive into it.

(In reply to Junior [:junior] from comment #29) > (In reply to Bobby Holley (busy) from comment #28) > > That means that we'd effectively support |own| accessors but not ones > > inherited from the prototype, which feels pretty arbitrary. There's also the > > question of whether we should wrap getters, or only setters. And what we > > should do with non-enumerable properties. And non-configurable properties. > > And things on the prototype chain. > > > > I really don't want to make this stuff more complicated. Can you please try > > the solution from comment 12? > I know this patch doesn't do great with prototype and prototype chain. > But it's better than original one. At least save lots time for further > developers. > > Or we need to do some document in the code. The SpecialPowers wrapper layer, especially the more tricky parts that you're using, is really unpredictable and buggy. I don't want people to use it, because they'll waste a lot of time, just like it wasted a lot of your time to figure out what was wrong. There are a lot of hacks that were implemented in a hurry, and they spread because people copied the patterns. > I can try the solution from comment 12, but it needs much more time and > differs lots from |loadChromeScript| in my first impression. > > Maybe I'll find it's easier than I think after I dive into it. It doesn't need much time at all - I wrote all the code in comment 12. If you want to make it easier, you can even skip the "get the source from the arg" part, and pass a string directly. To use it, you can wrap all your privileged code in a function like this: function privilegedStuff() { function foo() {...} function bar() {...} // do some privileged stuff. } And then do: var port = SpecialPowers.loadChromeScript('(' + privilegedStuff.toSource() + ')()'); Are there any complications you see that I'm missing?

I've done with a very fast test but fail as following, seems needs a principal. 13 INFO TEST-UNEXPECTED-FAIL | testing/mochitest/tests/Harness_sanity/test_SpecialPowersLoadPrivilegedScript.html | uncaught exception - TypeError: Cu.sandbox is not a constructor at SpecialPowersAPI.prototype.loadPrivilegedScript@chrome:/ /specialpowers/content/specialpowersAPI.js:461:14 @http://mochi.test:8888/tests/testing/mochitest/tests/Harness_sanity/test_SpecialPowersLoadPrivilegedScript.html:15:14

I guess we should move to bug 1260076 for this discussion

(In reply to Junior [:junior] from comment #32) > I guess we should move to bug 1260076 for this discussion I commented there - thanks for being willing to give this a try!