1126014 - [e10s] nsIScriptSecurityManager domainPolicy doesn't seem to work with e10s

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Jason Barnabe (np)]

My extension YesScript uses nsIScriptSecurityManager to maintain a blacklist of hosts not allowed to run JavaScript. The code is viewable at https://github.com/JasonBarnabe/yesscript/blob/master/components/YesScriptStartup.js#L66. The sitesString argument is a space-separated list of host names, like "https://bugzilla.mozilla.org https://github.com".

In Firefox 35, this code works. In Firefox 38.0a1, it doesn't seem to have an effect. I've verified that the code reaches that.domainPolicy.blacklist.add(uri); with a correct value, yet scripts do not get blocked.

Has something about this interface changed?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Does this work in 36?  37?

Comment 2

[Bobby Holley (:bholley)]

I'm not aware of anything that would have changed. But if this is broken in 36, we need to fix that immediately.

Alice, can you help us verify and find a regression range?

Comment 3

[Alice0775 White]

I need a detailed step by step STR to reproduce the problem.

Comment 4

[Jason Barnabe (np)]

It works in both 36.0b3 and 37.0a2. Maybe an e10s thing?

Steps to reproduce:
1. Install YesScript - https://addons.mozilla.org/en-US/firefox/addon/yesscript/
2. Go to the add-ons manager, open Preferences for YesScript.
3. Add https://bugzilla.mozilla.org to the blacklist
4. (Re)load a page on Bugzilla and try a feature that requires JavaScript, for example the reply button.

Firefox 35, 36.0b3, 37.0a2 - reply button doesn't work, as expected
Firefox 38.0a1 - reply button still works, JavaScript is not blocked

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Maybe an e10s thing?

Could be!  Are you adding this stuff to the script security manager in the parent process or the child?

Comment 6

[Jason Barnabe (np)]

This happens in an XPCOM component. It runs at profile-after-change and in a pref observer.

Comment 7

[Alice0775 White]

I can reproduce the problem with e10s window.

And the following error massage in Browser Console:

TypeError: content.document is undefined browserOverlay.js:69:6
unsafe CPOW usage browserOverlay.js:52:0
unsafe CPOW usage browserOverlay.js:69:0
unsafe CPOW usage browserOverlay.js:76:0

Comment 8

[Jason Barnabe (np)]

The Browser Console messages are related to the toolbar icon - I've already fixed these in my repo. The steps to reproduce I provided do not require the use of the toolbar icon.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> This happens in an XPCOM component. It runs at profile-after-change and in a pref
> observer.

Yeah, I bet all that happens in the parent process.

We should consider shipping the domainPolicy stuff to the child process....

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> > This happens in an XPCOM component. It runs at profile-after-change and in a pref
> > observer.
> 
> Yeah, I bet all that happens in the parent process.
> 
> We should consider shipping the domainPolicy stuff to the child process....

Yep, this should probably block shipping e10s, otherwise NoScript-like addons might break in surprising ways, and cause privacy/security problems for users. Simply having the child process domain policy be a dumb mirror of the parent should do the trick.

Chris, how do we get this on the radar?

Comment 11

[Chris Peterson [:cpeterson]]

Flagging for e10s triage.

Comment 12

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #10)
> for users. Simply having the child process domain policy be a dumb mirror of
> the parent should do the trick.

How so? I don't think I get this "dumb mirror" idea.

So one thing I would start with is throwing for ActivateDomainPolicy calls from child processes. Otherwise we should broadcast messages to the chrome process and all the other child processes all the time... In general, simulating a singleton in a multiprocess env. can get tricky.

And then all the domainPolicy getters from the child processes will be only called from here: http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#1553

So we could alter those getters for child processes, or do an update for all the list in one bulk. We should do this sync, but at least this will block only child processes not the chrome process.

This might be slow, depending on how hot this code is. An alternative approach is to broadcast a message every time someone adds a new entry to the domainPolicy lists in the chrome process, and then each child process updates their own copies, but I'm a bit afraid of timing issues in this version (I guess the broadcasted message would have to be async).

Sorry if I just don't see something trivial and overcomplicate things...

Comment 13

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> (In reply to Bobby Holley (:bholley) from comment #10)
> > for users. Simply having the child process domain policy be a dumb mirror of
> > the parent should do the trick.
> 
> How so? I don't think I get this "dumb mirror" idea.

You seem to get it. :-) I think your thoughts below are basically what I had in mind.

> So one thing I would start with is throwing for ActivateDomainPolicy calls
> from child processes.

Yes.

> And then all the domainPolicy getters from the child processes will be only
> called from here:
> http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#1553
> 
> So we could alter those getters for child processes, or do an update for all
> the list in one bulk. We should do this sync, but at least this will block
> only child processes not the chrome process.
> 
> This might be slow, depending on how hot this code is.

This isn't the right approach. In 99% of the cases the domain policy is blank. The model should be push, not pull.

> An alternative
> approach is to broadcast a message every time someone adds a new entry to
> the domainPolicy lists in the chrome process, and then each child process
> updates their own copies, but I'm a bit afraid of timing issues in this
> version (I guess the broadcasted message would have to be async).

Yes, the right thing to do is to send sync messages to the child whenever the nsIDomainPolicy is activated or any DomainSet inside it changes so that the child can basically hold a copy of the state in the parent. This is what I meant by "dumb mirror".

Comment 14

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #13)
> Yes, the right thing to do is to send sync messages to the child whenever
> the nsIDomainPolicy is activated or any DomainSet inside it changes so that
> the child can basically hold a copy of the state in the parent. This is what
> I meant by "dumb mirror".

This is a bit problematic, since we can have multiple child processes in the system,
so we have to broadcast the message at global message manager level and there is no
sync version for that. (Assuming that the DomainPolicy is a singleton for the whole
browser and not just per tab...)

Also I would like to avoid sending sync messages from the parent
process if that's possible in general (typically a consumer will call 'add' in a for
loop, sending sync messages from the chrome side in a loop does not sound very tempting).

On the other end, at the child side, I would like to listen to that message from
C++ code. Since that message listener will have to update the DomainSet, and I 
don't want to expose any method for that (JS code in child process should
not have access to these sets). But as I looked into it, nsIMessageListener is not
really encouraged to be implemented in C++: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIMessageManager.idl#175 (although I see no reasons why I couldn't
do it anyway...)

I'm CC-ing Bill, I'd like to keep him in the loop.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> (In reply to Bobby Holley (:bholley) from comment #13)
> > Yes, the right thing to do is to send sync messages to the child whenever
> > the nsIDomainPolicy is activated or any DomainSet inside it changes so that
> > the child can basically hold a copy of the state in the parent. This is what
> > I meant by "dumb mirror".
> 
> This is a bit problematic, since we can have multiple child processes in the
> system,
> so we have to broadcast the message at global message manager level and
> there is no
> sync version for that. (Assuming that the DomainPolicy is a singleton for
> the whole
> browser and not just per tab...)
> 
> Also I would like to avoid sending sync messages from the parent
> process if that's possible in general (typically a consumer will call 'add'
> in a for
> loop, sending sync messages from the chrome side in a loop does not sound
> very tempting).

Seems reasonable I guess. I was thinking that it would give us stronger guarantees (knowing that, once the add() call returned, the policy was updated everywhere). But I guess if the child is already remote and the _order_ of operations is preserved (so that no subsequent operations that assume the content is blocked can occur before it actually is blocked), it should be ok.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes, using an async message should be fine. Order of messages is preserved.

Gabor, here's some documentation on doing IPC in C++:
https://developer.mozilla.org/en-US/docs/IPDL/Tutorial

If you're looking for a simple example, here's how we broadcast changes to chrome manifests. The broadcasting happens here:
http://mxr.mozilla.org/mozilla-central/source/chrome/nsChromeRegistryChrome.cpp#761

A ContentParent is a reference to an "actor" in the child process. There is one of these actors for each child process. Each child process has a corresponding ContentChild instance. When the message is received, this function is called:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1771

The RegisterChromeItem message is defined here:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#451
From this definition, we automatically generate the C++ code for SendRegisterChromeItem on the ContentParent.

I imagine you'll want to add a similar message on PContent for updating the domain policy. Let me know if you have any other questions.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bill McCloskey (:billm) from comment #16)
> A ContentParent is a reference to an "actor" in the child process.

I meant an actor in the *parent* process. How confusing! Sorry.

Comment 18

[Gabor Krizsanits (INACTIVE)]

Attachment: DomainPolicy support for e10s. v1

Thanks Bill, that was all the help I needed. I think we might want to do some changes on this patch before landing (CloneDomainPolicyToChild might be too much message traffic in some cases, if we care...), and I still want to add a test. But other than that this patch should work.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8573771 [details] [diff] [review]
DomainPolicy support for e10s. v1

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

Looks good! mrbkap should probably be the final reviewer for this since I don't actually know anything about these domain policies.

::: dom/ipc/ContentParent.cpp
@@ +3139,5 @@
> +    // XXX: The idea is that each time we spawn a child process, we copy the
> +    // domain policy from the parent to the child. Not sure if this is the
> +    // right place to do that. Also not sure if I should add a new type of
> +    // message that sends all the arrays in one message, instead of sending
> +    // each entry in the arrays in a separate message.

I think it might be better to have a single type, defined in DOMTypes.ipdlh, that encapsulates an entire domain policy. (That type would probably be a struct with some arrays.) Then you could add an extra parameter to RecvGetXPCOMProcessAttributes for the domain policy (and it would have that type). This way you don't need to send a bunch of messages to the new process. Instead, the entire policy is returned as part of GetXPCOMProcessAttributes. It also has the advantage that the domain policy takes effect in the new process right away, rather than asynchronously as it does in this patch.

Comment 20

[Gabor Krizsanits (INACTIVE)]

Attachment: DomainPolicy support for e10s. v2

Updated patch with test.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8575319 [details] [diff] [review]
DomainPolicy support for e10s. v2

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

I have a few nits and a couple of questions that I'd like to see the answers to before I stamp this.

::: caps/DomainPolicy.cpp
@@ +171,5 @@
>  {
>      nsCOMPtr<nsIURI> clone = GetCanonicalClone(aDomain);
>      NS_ENSURE_TRUE(clone, NS_ERROR_FAILURE);
>      mHashTable.PutEntry(clone);
> +    // XXX: Should we turn this on only if e10s is on?

No need for this comment. The idea of e10s being "on" or "off" is pretty fuzzy with "open new (non-)e10s window" and we're on the track to dropping non-e10s Firefox.

::: caps/nsIDomainPolicy.idl
@@ +9,5 @@
>  interface nsIDomainSet;
>  
> +%{ C++
> +    namespace mozilla {
> +        namespace dom {

Nit: In general, we don't indent any of these things:

%{C++

namespace mozilla {
namespace dom {
...
}
}

::: caps/nsIScriptSecurityManager.idl
@@ +245,5 @@
>       * control of the system by invoking activateDomainPolicy().
>       */
>      nsIDomainPolicy activateDomainPolicy();
>      readonly attribute boolean domainPolicyActive;
> +    /**

Nit: here and below: add a blank line after the previous attribute/method.

@@ +247,5 @@
>      nsIDomainPolicy activateDomainPolicy();
>      readonly attribute boolean domainPolicyActive;
> +    /**
> +     * Only the parent process can access the domain policy, the ones
> +     * in the child process are just mirrors to the one in the parent.

I'd modify this slightly to say: "Only the parent process can directly access domain policies, child processes only have a read-only mirror..."

@@ +256,5 @@
> +     */
> +    [noscript] nsIDomainPolicy activateDomainPolicyInternal();
> +    /**
> +     * This function is for internal use only. Every time a child process is spawn
> +     * if the domain policy in the parent process active we must clone it to the

"Every time a child process is spawned, we must clone any active domain policies in the parent to the new child."

::: caps/nsScriptSecurityManager.cpp
@@ +1298,5 @@
>          mDomainPolicy->Deactivate();
> +    }
> +    // XXX: Deactivate call should also null out mDomainPolicy, however if
> +    // it's called too late gScriptSecMan might be already null, in which
> +    // case it cannot. So I had to remove the assertion.

Did you have to remove the assertion in both the parent and the child? Can we keep the assertion in the parent (guarded by an XRE_GetProcessType check) and file a followup on enabling it for the child?

::: dom/ipc/tests/browser_domainPolicy.js
@@ +21,5 @@
> +      function checkScriptEnabled(win, expectEnabled) {
> +        win.wrappedJSObject.gFiredOnclick = false;
> +        win.document.body.dispatchEvent(new win.Event('click'));
> +        deferred.resolve({ passed: win.wrappedJSObject.gFiredOnclick == expectEnabled,
> +                           msg: 'Checking script-enabled for ' + win.name + ' (' + win.location + ')' });

You can use the new backtick (`) operator here:

`Checking script-enabled for ${win.name} (${win.location})`

@@ +43,5 @@
> +
> +    tab.linkedBrowser.loadURI(uri);
> +    let result = yield eventPromise;
> +    ok(result.passed, result.msg);
> +    policy.deactivate();

Won't this throw for the test_domainPolicy(false) case?

@@ +50,5 @@
> +}
> +
> +Services.prefs.setBoolPref("dom.ipc.browser_frames.oop_by_default", true);
> +Services.prefs.setBoolPref("dom.mozBrowserFramesEnabled", true);
> +Services.prefs.setBoolPref("browser.pagethumbnails.capturing_disabled", true);

Don't we have to reset these prefs after this test finishes?

Comment 22

[Gabor Krizsanits (INACTIVE)]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #21)
> No need for this comment. The idea of e10s being "on" or "off" is pretty
> fuzzy with "open new (non-)e10s window" and we're on the track to dropping
> non-e10s Firefox.

Ok, I was not sure in this. Btw I use this XXX thing to add comments for the reviewer,
after the review my intention is to remove them.

> > +    // XXX: Deactivate call should also null out mDomainPolicy, however if
> > +    // it's called too late gScriptSecMan might be already null, in which
> > +    // case it cannot. So I had to remove the assertion.
> 
> Did you have to remove the assertion in both the parent and the child? Can
> we keep the assertion in the parent (guarded by an XRE_GetProcessType check)
> and file a followup on enabling it for the child?

In the parent the assertion should just work fine. With the child however we
shut down xpconnect before destroying the ContentChild that holds a reference to
the policy object. We need this reference to be able to call deactivate on it.

Since there are various cases for finishing a ContentChild, I didn't want to play
a whack-a-mole game with it. I like the XRE_GetProcessType check idea I will do that
and comment why we don't do the assertion for child processes. Not sure about the
follow up... I can imagine some edge cases there... I'm sure I can do something
that will turn tests on try to green, but cannot promise that someone will not
run into an annoying edge case because of it. I can file it though if you find
it important.

> @@ +43,5 @@
> > +
> > +    tab.linkedBrowser.loadURI(uri);
> > +    let result = yield eventPromise;
> > +    ok(result.passed, result.msg);
> > +    policy.deactivate();
> 
> Won't this throw for the test_domainPolicy(false) case?

Why would it? The only difference between the two cases that in one case we activate the policy before the process creation, testing the clone policy stuff. And in the other we activate it after the process creation testing the message sending stuff that keeps the two policy in sync. There should be no detectable difference between the two.

> 
> @@ +50,5 @@
> > +}
> > +
> > +Services.prefs.setBoolPref("dom.ipc.browser_frames.oop_by_default", true);
> > +Services.prefs.setBoolPref("dom.mozBrowserFramesEnabled", true);
> > +Services.prefs.setBoolPref("browser.pagethumbnails.capturing_disabled", true);
> 
> Don't we have to reset these prefs after this test finishes?

I have no idea, some tests are resetting them some are not. I don't think we start each test with a clean profile so I will reset them just in case.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #22)
> I can file it though if you find it important.

Yeah, it's not that big a deal.

> Why would it?

Oops, I didn't see the two calls to activateDomainPolicy.

Comment 24

[Bobby Holley (:bholley)]

Can we modify the existing exhaustive tests for this functionality (caps/tests/mochitest/test_disableScript.xul) to run properly with e10s? It seems like we should do that if at all possible.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

That looks like a mochitest-chrome test. Those don't run in e10s and maybe never will. However, we could probably transition it to a browser-chrome test without too much trouble.

Comment 26

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #25)
> That looks like a mochitest-chrome test. Those don't run in e10s and maybe
> never will. However, we could probably transition it to a browser-chrome
> test without too much trouble.

Ideally we would test both the behavior in the parent process and in the child. One way would be to include the interesting bits of the test in a .js file and load it in both a mochitest-chrome and mochitest-browser test, or set up a mochitest-browser test that checks both.

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We run browser-chrome tests in e10s and non-e10s. Is that not enough?

Comment 28

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #27)
> We run browser-chrome tests in e10s and non-e10s. Is that not enough?

Do we plan to do that long-term? I thought we didn't, which is why I wanted to make sure that we have a lasting way of making sure that domain policy works right in the parent process.

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We're going to run non-e10s tests as long as there's a chance that web content will run in the parent process.

Is there any reason to have domain policies working in the parent if we only do chrome stuff there? If so, I guess I see your point.

Comment 30

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #29)
> We're going to run non-e10s tests as long as there's a chance that web
> content will run in the parent process.
> 
> Is there any reason to have domain policies working in the parent if we only
> do chrome stuff there? If so, I guess I see your point.

Script policy does not affect scripts with system and expanded principal. If we're really going to fully prevent any other scripts from running in the parent process (even via addons and the like), then I guess it's fine. I just didn't think that was on the horizon.

Comment 31

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #26)
> (In reply to Bill McCloskey (:billm) from comment #25)
> > That looks like a mochitest-chrome test. Those don't run in e10s and maybe
> > never will. However, we could probably transition it to a browser-chrome
> > test without too much trouble.
> 
> Ideally we would test both the behavior in the parent process and in the
> child. One way would be to include the interesting bits of the test in a .js
> file and load it in both a mochitest-chrome and mochitest-browser test, or
> set up a mochitest-browser test that checks both.

I can try to do that, but I would prefer doing that in a separate bug. Since it
will take quite some time for me to get it right probably, and that test is testing more
than just domain policy. Or I could just leave the old test as it is and doing something
similar in the new one.

(In reply to Bobby Holley (:bholley) from comment #30)
> Script policy does not affect scripts with system and expanded principal. If
> we're really going to fully prevent any other scripts from running in the
> parent process

I don't think we will. There are too many tests to convert for doing that.
Essentially every chrome mochi test with a content iframe to start with,
but I'm sure there are more cases. and I don't see any good reason for doing it.
Other than testing, panels come to my mind. Those are used by addons to display
some content via iframes if I'm not mistaken. And I would guess panels are still
part of the parent process...

On the other hand as much as I don't like code duplication in general, I could
totally live with some test duplication. I would not mind too much leaving the
old test as it is and extend the new one to do a more extensive testing.
That would duplicate tests sure, but I could totally live with that. Later if we ever
ban the parent process from running content code, we can just remove the old test.

Comment 32

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #31)
> (In reply to Bobby Holley (:bholley) from comment #26)
> > (In reply to Bill McCloskey (:billm) from comment #25)
> > > That looks like a mochitest-chrome test. Those don't run in e10s and maybe
> > > never will. However, we could probably transition it to a browser-chrome
> > > test without too much trouble.
> > 
> > Ideally we would test both the behavior in the parent process and in the
> > child. One way would be to include the interesting bits of the test in a .js
> > file and load it in both a mochitest-chrome and mochitest-browser test, or
> > set up a mochitest-browser test that checks both.
> 
> I can try to do that, but I would prefer doing that in a separate bug.

Sure, just as long as it gets done.

> I don't think we will.

Then I think we need to fully test both configurations.

> On the other hand as much as I don't like code duplication in general, I
> could
> totally live with some test duplication. I would not mind too much leaving
> the
> old test as it is and extend the new one to do a more extensive testing.
> That would duplicate tests sure, but I could totally live with that. Later
> if we ever
> ban the parent process from running content code, we can just remove the old
> test.

That's fine with me. I don't expect that we'll be modifying the test heavily. I just spent a lot of time making it cover all the cases (which are extremely important to NoScript, which in turn is extremely important to a vocal segment of our users), and I want to make sure that it's still testing the right thing.

Comment 33

[Gabor Krizsanits (INACTIVE)]

Attachment: DomainPolicy support for e10s. v3

I've addressed the nits and added a more thorough test. I basically converted the tests from test_disableScript.xul. Only the domain policy related ones, and only the ones that made sense in this setup. Flagging Bobby to, as he might want to take a look at the converted tests.

Comment 34

[Gabor Krizsanits (INACTIVE)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #33)
> converted the tests from test_disableScript.xul.

Plus I made sure I run each test both for the case where we create the child process first and activate the domain policy later and the other way around.

Comment 35

[Bobby Holley (:bholley)]

Comment on attachment 8578584 [details] [diff] [review]
DomainPolicy support for e10s. v3

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

Thank you for making these tests! I don't know enough about Task.jsm to review them easily - so either blake should (if he knows about it) or you should get someone from the browser team.

::: dom/ipc/tests/browser_domainPolicy.js
@@ +226,5 @@
> +  deactivateDomainPolicy();
> +  Services.prefs.setBoolPref("javascript.enabled", true);
> +  Services.prefs.setBoolPref("dom.ipc.browser_frames.oop_by_default", false);
> +  Services.prefs.setBoolPref("dom.mozBrowserFramesEnabled", false);
> +  Services.prefs.setBoolPref("browser.pagethumbnails.capturing_disabled", false);

This is a huge footgun - if the default pref values in all.js change, we'll end up running all subsequent browser-chrome tests with non-default values. Please use SpecialPowers.pushPrefEnv instead.

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8578584 [details] [diff] [review]
DomainPolicy support for e10s. v3

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

I *think* I followed how the test works all the way through, but I must admit to getting lost a bit in all of the promises and evals. If it works, I accept it.

::: caps/DomainPolicy.h
@@ +13,5 @@
>  
>  namespace mozilla {
>  
> +namespace dom {
> +    class nsIContentParent;

Nit: here and below: |namespace foo {| doesn't increase the indentation level.

::: caps/nsIScriptSecurityManager.idl
@@ +245,5 @@
>       * control of the system by invoking activateDomainPolicy().
>       */
>      nsIDomainPolicy activateDomainPolicy();
>      readonly attribute boolean domainPolicyActive;
> +    /**

Nit: Please add a blank line before the start of this comment.

@@ +254,5 @@
> +     * ActivateDomainPolicyInternal directly. New consumer to this
> +     * function should not be addded.
> +     */
> +    [noscript] nsIDomainPolicy activateDomainPolicyInternal();
> +    /**

Here too.

::: dom/ipc/ContentChild.cpp
@@ +2597,5 @@
> +        MOZ_ASSERT(ssm);
> +        ssm->ActivateDomainPolicyInternal(getter_AddRefs(mPolicy));
> +        return !!mPolicy;
> +    } else if (!mPolicy) {
> +        // XXX: should we crash here?

Channeling bent, let's try crashing here (return false). Please also add a MOZ_ASSERT_UNREACHABLE so we can figure out what's going on in debug builds, though.

::: dom/ipc/tests/browser_domainPolicy.js
@@ +41,5 @@
> +    tab.linkedBrowser.loadURI("http://mochi.test:8888/browser/dom/ipc/tests/file_domainPolicy_base.html");
> +    return initPromise;
> +  }
> +
> +  // For a ContentTask is created. We serialise an input object via JSON |ipcArgs| and some shared

I don't understand what the first sentence means. Also, "serialize".

@@ +49,5 @@
> +    obj.checkScriptEnabled = function(win, expectEnabled) {
> +      win.wrappedJSObject.gFiredOnclick = false;
> +      win.document.body.dispatchEvent(new win.Event('click'));
> +      return { passed: win.wrappedJSObject.gFiredOnclick == expectEnabled,
> +               msg: 'Checking script-enabled for ${win.name} (${win.location})'};

I think this string should be using backticks (`) and not single quotes (').

Comment 37

[Gabor Krizsanits (INACTIVE)]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #36)
> I *think* I followed how the test works all the way through, but I must
> admit to getting lost a bit in all of the promises and evals. If it works, I
> accept it.

Sorry about that :( In retrospective maybe the new remote page navigation API
might have helped me make it cleaner... it was quite challenging to write these tests...

> > +  // For a ContentTask is created. We serialise an input object via JSON |ipcArgs| and some shared
> 
> I don't understand what the first sentence means. Also, "serialize".
> 

I think I accidentally removed a part of the sentence... anyway I tried to come up with something better: 

  // We use ContentTask for the tests, but we also want to pass some data and some helper functions too.
  // To do that, we serialize an input object via JSON |ipcArgs| and some shared helper functions |initUtils|
  // and eval them in the content process.

I hope this is more clear.

> I think this string should be using backticks (`) and not single quotes (').

Hmm... it's weird that it worked with single quotes too, but just in case I added the backticks.

Anyway, thanks for the quick review!!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92ade8a39a41

Comment 38

[Gabor Krizsanits (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90d9af9b861a

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/90d9af9b861a

Comment 40

[Jason Barnabe (np)]

Verified that the latest from the YesScript works in 40.0a1. Thanks everyone!