Closed Bug 1260076 Opened 8 years ago Closed 8 years ago

[Mochitest] implement SpecialPowers.loadPrivilegedScript

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: CuveeHsu, Assigned: CuveeHsu)

References

Details

Attachments

(1 file, 6 obsolete files)

Per bug 1257734 Comment 12, we need a SpecialPower to load privileged script in content process
bug 1257734 Comment 12 suggests the following implementation but it's a little far from me.

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);
}
Attached patch loadPrivilegedScript WIP -v1 (obsolete) — Splinter Review
After a quick modification, I found the process in the sandbox still main process, which is not we want.

Moreover,
1. addEventListener doesn't work
2. comment syntax /* */ doesn't work
Flags: needinfo?(bobbyholley)
Comment on attachment 8738972 [details] [diff] [review]
loadPrivilegedScript WIP -v1

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

(In reply to Junior [:junior] from comment #2)
> Created attachment 8738972 [details] [diff] [review]
> loadPrivilegedScript WIP -v1
> 
> After a quick modification, I found the process in the sandbox still main
> process, which is not we want.

This code will definitely run in the same process as the mochitest. Otherwise there'd be no way to .toSource() the function that we pass.

Are you sure you're running the mochitest with e10s enabled? If not, your entire test is going to be running in the main process.

> Moreover,
> 1. addEventListener doesn't work

In what sense? The privileged code doesn't have access to the content DOM. The idea is that you manipulate the DOM in your normal mochitest code (including event listeners etc), and then pass messages to the privileged script to do any privileged operations.

> 2. comment syntax /* */ doesn't work

I'm not sure what this is about. Can you post a testcase?

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersLoadPrivilegedScript.html
@@ +13,5 @@
> +
> +function loadPrivilegedScriptTest() {
> +  function messageListeners() {
> +    addMessageListener("foo", function (message) {
> +      sendAsyncMessage("bar", message);

This isn't the correct API. We're using MessageChannel (a normal DOM interface that you can use on the Web), rather than the proprietary nsIMessageManager interface.

See http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MessagePort.webidl

::: testing/specialpowers/content/specialpowersAPI.js
@@ +457,5 @@
>    },
>  
> +  loadPrivilegedScript: function (aFunction) {
> +    var str = "(" + aFunction.toString() + ")();";
> +    var systemPrincipal = Components.Constructor('@mozilla.org/systemprincipal;1', 'nsIPrincipal')();

Services.scriptSecurityManager.getSystemPrincipal() is more idiomatic.

@@ +463,5 @@
> +    var window = this.window.get();
> +    var mc = new window.MessageChannel();
> +    sb.port = mc.port1;
> +    sb.eval(str);
> +    return this.wrap(mc.port2);

Since the mc is being created in the context of the content window, the port is a content object, and you shouldn't need the wrap here.
(In reply to Bobby Holley (busy) from comment #3)
> This isn't the correct API. We're using MessageChannel (a normal DOM
> interface that you can use on the Web), rather than the proprietary
> nsIMessageManager interface.

The alternative, if we want a more consistent messaging API between loadChromeScript and loadPrivilegedScript, would be to refactor loadChromeScript so that it can run in either the parent or the child process. Having a unified API might be nicer in the end, but the refactoring seems a lot more difficult, which is why I didn't suggest that you do it. You're welcome to if you prefer, of course.
Flags: needinfo?(bobbyholley)
I'm sorry for not doing many experiments since it's very late here.

> This code will definitely run in the same process as the mochitest.
> Otherwise there'd be no way to .toSource() the function that we pass.
> 
> Are you sure you're running the mochitest with e10s enabled? If not, your
> entire test is going to be running in the main process.
> 

My build is before bug 1243083 so e10s is not default.
But I run |./mach mochitest --e10s| for the attachment and the log shows the script is in main process.
> > Moreover,
> > 1. addEventListener doesn't work
> 
> In what sense? The privileged code doesn't have access to the content DOM.
> The idea is that you manipulate the DOM in your normal mochitest code
> (including event listeners etc), and then pass messages to the privileged
> script to do any privileged operations.
> 
Originally I guess addEventListener should work in the script since I see a MessageChannel.
It seems to have nothing to do with |addEventListener|

> > 2. comment syntax /* */ doesn't work
> 
> I'm not sure what this is about. Can you post a testcase?
Like
function loadPrivilegedScriptTest() {
  /* a comment here*/
  foo();
}
var script = SpecialPowers.loadPrivilegedScript(loadPrivilegedScriptTest.toSource());
But it doesn't happen now, maybe I have a bug previously.
(In reply to Junior [:junior] from comment #6)
> I'm sorry for not doing many experiments since it's very late here.
> 
> > This code will definitely run in the same process as the mochitest.
> > Otherwise there'd be no way to .toSource() the function that we pass.
> > 
> > Are you sure you're running the mochitest with e10s enabled? If not, your
> > entire test is going to be running in the main process.
> > 
> 
> My build is before bug 1243083 so e10s is not default.
> But I run |./mach mochitest --e10s| for the attachment and the log shows the
> script is in main process.


Hm. Maybe the Harness_sanity tests run in the main process or something?

One way to debug this is to do Math.sin in your mochitest, and then break on that in a debugger in both the parent and child processes. That will show you what process the mochitest itself is running in, and I would be very surprised if the SpecialPowers code was in a different process.
(In reply to Bobby Holley (busy) from comment #7)
> (In reply to Junior [:junior] from comment #6)
> > I'm sorry for not doing many experiments since it's very late here.
> > 
> > > This code will definitely run in the same process as the mochitest.
> > > Otherwise there'd be no way to .toSource() the function that we pass.
> > > 
> > > Are you sure you're running the mochitest with e10s enabled? If not, your
> > > entire test is going to be running in the main process.
> > > 
> > 
> > My build is before bug 1243083 so e10s is not default.
> > But I run |./mach mochitest --e10s| for the attachment and the log shows the
> > script is in main process.
> 
> 
> Hm. Maybe the Harness_sanity tests run in the main process or something?
> 
> One way to debug this is to do Math.sin in your mochitest, and then break on
> that in a debugger in both the parent and child processes. That will show
> you what process the mochitest itself is running in, and I would be very
> surprised if the SpecialPowers code was in a different process.

My bad.
I copy the logic to determine if it's main process from |SpecialPowers.isMainProcess|
Cc, Ci in the test are undefined and throw, thus leading a wrong message.
Attached patch loadPrivilegedScript WIP -v2 (obsolete) — Splinter Review
Still with some problems:
1. I cannot do lots of thing in the sandbox
   Such as import Services.jsm, get a preference or whatever service.

Would show JavaScript error: , line 0: uncaught exception: unknown (can't convert to string)

  Originally I'd like to mock some factory in content process. I guess it would fail in this situation.

2. Is there any way to communicate between the privileged script and web content? For example in this attachment, could I pass the contentProcessType to the privileged script?
Attachment #8738972 - Attachment is obsolete: true
Flags: needinfo?(bobbyholley)
Attached patch loadPrivilegedScript -v3 (obsolete) — Splinter Review
The communication would be MessageChannel as I guess, but not the way we use as loadChromeScript. This is validated in this patch.

So the last problem is mentioned in comment 9.
It seems that we can not do every thing we want such as import Services.jsm.
Is this expected?
Attachment #8739335 - Attachment is obsolete: true
Flags: needinfo?(bobbyholley)
needinfo? for comment 10.
Flags: needinfo?(bobbyholley)
In order to see exceptions propagated by your content script, you'll probably want to try/catch the eval and wrap the exception so that content can see (and stringify) it:

-    sb.eval(str);
+    try {
+      sb.eval(str);
+    } catch (e) {
+      throw wrapIfUnwrapped(e);
+    }

(The alternative would be to just stringify it directly)

I pulled your patch and did that. Looks like something's going on with the pref you're trying to read.

JavaScript error: chrome://specialpowers/content/specialpowersAPI.js line 467 > eval, line 13: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getIntPref]

I don't understand exactly why you're trying to set and read prefs like this. It's kind of complicated in the e10s case, because pushPrefEnv has to send a message to the parent process to set the pref, and then a change notification needs to be dispatched back to the content process. This _should_ work, but it's also not clear to me why you need to be doing that at all.
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (busy) from comment #12)
> In order to see exceptions propagated by your content script, you'll
> probably want to try/catch the eval and wrap the exception so that content
> can see (and stringify) it:
> 
> -    sb.eval(str);
> +    try {
> +      sb.eval(str);
> +    } catch (e) {
> +      throw wrapIfUnwrapped(e);
> +    }
> 
> (The alternative would be to just stringify it directly)
> 
> I pulled your patch and did that. Looks like something's going on with the
> pref you're trying to read.
> 
> JavaScript error: chrome://specialpowers/content/specialpowersAPI.js line
> 467 > eval, line 13: NS_ERROR_UNEXPECTED: Component returned failure code:
> 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getIntPref]
> 
> I don't understand exactly why you're trying to set and read prefs like
> this. It's kind of complicated in the e10s case, because pushPrefEnv has to
> send a message to the parent process to set the pref, and then a change
> notification needs to be dispatched back to the content process. This
> _should_ work, but it's also not clear to me why you need to be doing that
> at all.

The story of using pref isn't very important. Originally I didn't get a way
to communicate between privileged script and the content. Therefore I'd like 
to use pref first instead of the |MessageChannel|.

So pref doesn't matter so much to me now.

Next thing I'd like to do is to test what I originally want to do, mock the
XPCOM factory in the content process.
Attached patch loadPrivilegedScript -v4 (obsolete) — Splinter Review
WIP with test for Message Channel, maybe we should abstract the |port| to |addEventListener|
Attachment #8739383 - Attachment is obsolete: true
Attached patch mockFactory (obsolete) — Splinter Review
It's a test for mocking factory in XPCOM but it fails to create a mocked instance since Cc[contractID] != mockedClassID after |registerFactory| which is different from |loadChromeScript|.
Blocks: 1264513
(In reply to Junior [:junior] from comment #15)
> Created attachment 8740379 [details] [diff] [review]
> mockFactory
> 
> It's a test for mocking factory in XPCOM but it fails to create a mocked
> instance since Cc[contractID] != mockedClassID after |registerFactory| which
> is different from |loadChromeScript|.


It seems a longer road than we original estimate and I spent two more days on it with no gain.

Is there cpp break points to watch |Cc| tables in cpp?

And I can't find when the Cc table should have been changed by |registerFactory| but it doesn't.
https://dxr.mozilla.org/mozilla-central/source/xpcom/components/nsComponentManager.cpp#1609

Any suggestion?
Flags: needinfo?(bobbyholley)
Comment on attachment 8740379 [details] [diff] [review]
mockFactory

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

(In reply to Junior [:junior] from comment #16)
> It seems a longer road than we original estimate and I spent two more days
> on it with no gain.

The problem you're running into has nothing to do with the fact that you're executing the privileged code in a Sandbox rather than via SpecialPowers wrappers. Using the wrappers would just make it more complicated to figure out what's going on. ;-)

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersLoadPrivilegedScript.html
@@ +37,5 @@
> +
> +    if (!registrar.isCIDRegistered(mockedClassId)) {
> +      try {
> +        originalClassId = registrar.contractIDToCID(contractId);
> +        originalFactory = Cm.getClassObject(Cc[contractId], Ci.nsIFactory);

The problem is here: Cc differs from contractIdToCID because it caches its result on the Cc object. That means that, after doing Cc[foo] for a given global, you will always get the same result for Cc[foo], even if the underlying data structures in the component manager have changed.

This basically means that Cc is not reliable when you're changing factories via the component manager, and you need to use contractIdtoCID to avoid stale results.

It's really a pretty terrible situation that people are copy-pasting this very-tricky component manipulation everywhere. We should really have one set of shared Mock Object code that everybody can subclass.
btw, this is where the caching happens: http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCComponents.cpp#743
Flags: needinfo?(bobbyholley)
> 
> The problem is here: Cc differs from contractIdToCID because it caches its
> result on the Cc object. That means that, after doing Cc[foo] for a given
> global, you will always get the same result for Cc[foo], even if the
> underlying data structures in the component manager have changed.
> 
> This basically means that Cc is not reliable when you're changing factories
> via the component manager, and you need to use contractIdtoCID to avoid
> stale results.
Is there any method to make Cc synchronize to contractIDToCID? We can't affect the tested Gecko code even if we make a template Mock object.

And I don't see a cache behaviour in code. IMO it's nothing different from |contractIDToCID|.
http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCJSID.cpp#591

But |contractIdToCID| works after re-registerFactory.
Is the registrar different from ComponentManager?
Flags: needinfo?(bobbyholley)
(In reply to Junior [:junior] from comment #19)
> > 
> > The problem is here: Cc differs from contractIdToCID because it caches its
> > result on the Cc object. That means that, after doing Cc[foo] for a given
> > global, you will always get the same result for Cc[foo], even if the
> > underlying data structures in the component manager have changed.
> > 
> > This basically means that Cc is not reliable when you're changing factories
> > via the component manager, and you need to use contractIdtoCID to avoid
> > stale results.
> Is there any method to make Cc synchronize to contractIDToCID?

Not really. You could delete the property off Cc, and then it _should_ re-resolve, but that's a bit sketchy.

> We can't
> affect the tested Gecko code even if we make a template Mock object.

Well, you will affect C++ code (which doesn't go through Cc), and any JavaScript code that has not yet looked up that contractid. But this does seem like a pretty fundamental bug in the whole "mock object" setup, I agree.

> And I don't see a cache behaviour in code. IMO it's nothing different from
> |contractIDToCID|.
> http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCJSID.cpp#591

I linked you to the code in comment 18. Cc is a javascript object. When you look up a property that doesn't exist, it delegates to the Resolve hook, which generates the nsJSCID, creates a JSObject for it, and defines that JSObject as a property on Cc. Future lookups will get the cached nsJSCID, which stores the original CID.

> But |contractIdToCID| works after re-registerFactory.
> Is the registrar different from ComponentManager?

See above.
Flags: needinfo?(bobbyholley)
> > Is there any method to make Cc synchronize to contractIDToCID?
> 
> Not really. You could delete the property off Cc, and then it _should_
> re-resolve, but that's a bit sketchy.
> 

I'm not sure if it's a X-ray stuff or not.
|delete Cc[contractId]| doesn't work for Sandbox.
The object |Cc[contractId]| can not be deleted by |delete| operator.
Neither does |Cc[contractId] = null/undefined|

Therefore, we can not trigger a re-resolve by this way.

> > And I don't see a cache behaviour in code. IMO it's nothing different from
> > |contractIDToCID|.
> > http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCJSID.cpp#591
> 
> I linked you to the code in comment 18. Cc is a javascript object. When you
> look up a property that doesn't exist, it delegates to the Resolve hook,
> which generates the nsJSCID, creates a JSObject for it, and defines that
> JSObject as a property on Cc. Future lookups will get the cached nsJSCID,
> which stores the original CID.
> 
Thanks for the helpful explanation.
If I understand correctly, the code in comment 18 is the case for cache miss, right?

I'm not sure if we need to sync Cc after |registerFactory|.
If yes, I guess the code comment 18 isn't the right place to modify.
Flags: needinfo?(bobbyholley)
(In reply to Junior [:junior] from comment #21)
> > > Is there any method to make Cc synchronize to contractIDToCID?
> > 
> > Not really. You could delete the property off Cc, and then it _should_
> > re-resolve, but that's a bit sketchy.
> > 
> 
> I'm not sure if it's a X-ray stuff or not.
> |delete Cc[contractId]| doesn't work for Sandbox.
> The object |Cc[contractId]| can not be deleted by |delete| operator.
> Neither does |Cc[contractId] = null/undefined|
> 
> Therefore, we can not trigger a re-resolve by this way.

Yeah, you're right - I just noticed that the code in comment 18 defines the property with JSPROP_PERMANENT, which means that it's non-configurable and can't be deleted.

> 
> > > And I don't see a cache behaviour in code. IMO it's nothing different from
> > > |contractIDToCID|.
> > > http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCJSID.cpp#591
> > 
> > I linked you to the code in comment 18. Cc is a javascript object. When you
> > look up a property that doesn't exist, it delegates to the Resolve hook,
> > which generates the nsJSCID, creates a JSObject for it, and defines that
> > JSObject as a property on Cc. Future lookups will get the cached nsJSCID,
> > which stores the original CID.
> > 
> Thanks for the helpful explanation.
> If I understand correctly, the code in comment 18 is the case for cache
> miss, right?

Correct. It uses machinery for lazy resolution of properties onto JavaScript objects. See https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JSResolveOp#Description

> I'm not sure if we need to sync Cc after |registerFactory|.

We don't really, because Cc is different for each global. So unless we build a (complex) mechanism to clear Cc for every global in the runtime, we can't really use mock objects for things created in JS via Cc.

What this means is that the current Mock Object pattern only works for objects that are created (by contractID) in C++ only (at least for the callsites we care about). This why the MockColorPicker works:

http://searchfox.org/mozilla-central/search?q=%40mozilla.org%2Fcolorpicker%3B1&redirect=true

For @mozilla.org/network/standard-url;1 (which you're trying to hook) it looks most of the cases are in C++, so this should work:

http://searchfox.org/mozilla-central/search?q=NS_STANDARDURL_CONTRACTID&redirect=true
Flags: needinfo?(bobbyholley)
> > I'm not sure if we need to sync Cc after |registerFactory|.
> 
> We don't really, because Cc is different for each global. So unless we build
> a (complex) mechanism to clear Cc for every global in the runtime, we can't
> really use mock objects for things created in JS via Cc.
> 
> What this means is that the current Mock Object pattern only works for
> objects that are created (by contractID) in C++ only (at least for the
> callsites we care about). This why the MockColorPicker works:
> 
> http://searchfox.org/mozilla-central/search?q=%40mozilla.
> org%2Fcolorpicker%3B1&redirect=true
> 
> For @mozilla.org/network/standard-url;1 (which you're trying to hook) it
> looks most of the cases are in C++, so this should work:
> 
> http://searchfox.org/mozilla-central/
> search?q=NS_STANDARDURL_CONTRACTID&redirect=true

I'm not trying to hook a standand-url in my test case.
I use it since it's simple and has a getter/setter.

But this works for me since my use case is also C++.
Attached patch loadPrivilegedScript -v5 (obsolete) — Splinter Review
Attachment #8740371 - Attachment is obsolete: true
Attachment #8740379 - Attachment is obsolete: true
Attachment #8742718 - Flags: review?(bobbyholley)
Comment on attachment 8742718 [details] [diff] [review]
loadPrivilegedScript -v5

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

Looks good - thanks for pushing this through!

::: testing/specialpowers/content/specialpowersAPI.js
@@ +456,5 @@
>      return MockPermissionPrompt;
>    },
>  
> +  /*
> +   * Load privileged script in its process (especially in e10s mode). We use

I would write:

Load a privileged script that runs same-process. This is different than loadChromeScript, which will run in the parent process in e10s mode.
Attachment #8742718 - Flags: review?(bobbyholley) → review+
Modify by reviewer's comment, carry r+.
Assignee: nobody → juhsu
Attachment #8742718 - Attachment is obsolete: true
Attachment #8743643 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dea40ab40c2a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.