1128798 - [e10s] Make a version of nsIContentPolicy that doesn't pass the node as a parameter

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

Attachment: simple-content-policy

nsIContentPolicy is a problem for e10s because it sees the actual DOM element that triggered the load. For now we have a shim that runs the content policy in the parent and passes a CPOW in for the element. However, we want to move away from this approach.

I've looked at a number of add-ons that use nsIContentPolicy. Aside from NoScript and Adblock (the sophisticated ones), the ideal arrangement for most of them would be to run the code in the parent and pass in the <browser> element where the load originated. This is what's needed for Greasemonkey, Lightbeam, and (as far as I can tell) Privacy Badger.

I know that there have been some ideas for a while about creating an async content policy. I don't think that should hold up other improvements to content policy though. Passing in a browser element rather than a node would make it much less likely that the policy would be broken by moving to an async model since the policy no longer has direct access to the page's DOM.

Jonas, asking you for feedback since I've seen you discuss this code before. If it looks okay, I'll write some tests. The name is of course up for discussion.

Comment 1

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

Comment on attachment 8558266 [details] [diff] [review]
simple-content-policy

Sid, do you think you'll have time to look at this? It's kind of important that it makes it into this release since I'd like it on the next ESR.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

I probably won't have time until next week.  Tanvi: any chance you can take a look?  I know you've done nsContentPolicy stuff recently.

Comment 3

[Tanvi Vyas[:tanvi]]

Hi Bill,

Can I have more context on what problem you are trying to solve.  If vidyo is easier, we can also do that.  

I talked to Jonas a bit about this, and if we understand correctly, the problem is that there are too many synchronous calls from parent to child to get information about the node that is needed to enforce an addon-created content policy.  This patch attempts to pass the xul browser element to content policies instead of the node.  But won't we still have the same problem?  The content policy will still need to get some information about the document and hence will still require synchronous calls to the child.

Also, is this new API backwards compatible with addons, or will addons need to update to use it?  Will gecko created content policies continue to use the old API?

Thanks!

Comment 4

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

(In reply to Tanvi Vyas [:tanvi] from comment #3)
> Can I have more context on what problem you are trying to solve.  If vidyo
> is easier, we can also do that.  

We'd like an API that has these properties:
1. It runs in the parent process. This is useful for add-ons that store a lot of state in the parent and that need to consult that state when deciding whether to block a request.
2. It doesn't use CPOWs or sync parent-to-child messages at all. This is for performance.

> I talked to Jonas a bit about this, and if we understand correctly, the
> problem is that there are too many synchronous calls from parent to child to
> get information about the node that is needed to enforce an addon-created
> content policy.  This patch attempts to pass the xul browser element to
> content policies instead of the node.  But won't we still have the same
> problem?  The content policy will still need to get some information about
> the document and hence will still require synchronous calls to the child.

There's already some information stored on the <browser> element in the parent, and it can be accessed without touching the child process. Namely, the URI of the top-level frame and whether it's in a private window. That seems to be all that's required for some of these simpler add-ons (along with the URI being requested of course). So I don't think there should be a need to touch the document in this case.

> Also, is this new API backwards compatible with addons, or will addons need
> to update to use it?  Will gecko created content policies continue to use
> the old API?

It's not backwards compatible. The goal here is to make an API that add-ons can move to in order to avoid CPOWs (which is what they get if they use the old content policy API in the parent process).

They can always use a frame script to create their content policy in the child process and then forward requests to the parent. However, that's a lot of boilerplate code that these add-on authors will need to write (and probably get wrong). The purpose of this API is to make the simple cases simple.

Gecko content policies run in the child process, so they're not affected by this change.

If you have more questions, please let me know. We can talk over vidyo or IRC if need be.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8558266 [details] [diff] [review]
simple-content-policy

r -> tanvi

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If this is a new API that we want developers to migrate to, I'd really recommend putting more thought into it. At the very least it should be asynchronous so that we don't have to do so much IPC.

What I'd really like the API to look like is something like the "Hooks for security policies" section (line 206) from https://etherpad.mozilla.org/BetterNeckoSecurityHooks

Obviously doing that full API would be a lot of work, but perhaps there's a subset that wouldn't be terribly hard to implement.

Comment 7

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

(In reply to Jonas Sicking (:sicking) from comment #6)
> If this is a new API that we want developers to migrate to, I'd really
> recommend putting more thought into it. At the very least it should be
> asynchronous so that we don't have to do so much IPC.

The nice thing about the this API is that it requires almost no changes in order for add-on developers to adopt it since it's almost identical to the old API. If we think up a new API, they're much less likely to switch to it.

> What I'd really like the API to look like is something like the "Hooks for
> security policies" section (line 206) from
> https://etherpad.mozilla.org/BetterNeckoSecurityHooks

Chrome already has a pretty nice API, chrome.WebRequest, that serves the purpose of http-on-* observers as well as content policy. If we decide to add something new, I think we should just adopt that wholesale. That way, add-on developers don't have to maintain two separate implementations (since many add-ons try to work in both Chrome and Firefox).

Realistically, we're not going to dramatically change how content policy stuff works before e10s ships; there are too many other things to fix. The previous time e10s was being worked on, there was a lot of discussion about how content policy was going to be made async and better. It never happened. I'd like to at least make some improvements that fix the worst problems.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Bill McCloskey (:billm) from comment #7)
> Chrome already has a pretty nice API, chrome.WebRequest, that serves the
> purpose of http-on-* observers as well as content policy. If we decide to
> add something new, I think we should just adopt that wholesale. That way,
> add-on developers don't have to maintain two separate implementations (since
> many add-ons try to work in both Chrome and Firefox).

This sounds good to me.

Comment 9

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

Also, this simple content policy is designed for add-ons that don't need to touch the content DOM. In this case, there's nothing stopping us from running these policies asynchronously. Gecko doesn't support that right now, but the API itself doesn't prevent it.

Comment 10

[Tanvi Vyas[:tanvi]]

Chris and I are reviewing.  Cc'ing Dan so he is aware.

Comment 11

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

Comment on attachment 8558266 [details] [diff] [review]
simple-content-policy

Blake, would you be able to give this an additional review? Tanvi and Chris are going to review it, but they're not technically DOM peers. I'd really like to land it before the merge, so please let me know if you don't have time. Thanks.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8558266 [details] [diff] [review]
simple-content-policy

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

Even though it's fine what you are doing I would prefer if you could use
> hg mv nsIContentPolicy.idl to nsIContentPolicyBase.idl
and create a new file for nsIContentPolicy.idl. I think it would make the diff easier to read and also we could make sure that none of the constants accidentally gets converted into something unintentional.

::: dom/base/nsContentPolicy.cpp
@@ +136,5 @@
>      }
>  
> +    nsCOMPtr<nsIDOMElement> topFrameElement;
> +    bool isTopLevel = true;
> +    if (requestPrincipal) {

If you only do QI on the conditional within the if-clause, you can remove it. Assuming you want to branch on requestingContext instead of requestingPrincipal.

@@ +150,5 @@
> +            }
> +            if (doc) {
> +                window = doc->GetWindow();
> +            }
> +        }

Nit: I think you could slighlty simplify the following part by nesting some if-clauses. Since this code is called all the time, it's probably worth optimizing:

nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(requestingContext));
if (!window) {
  nsCOMPtr<nsIContent> node = do_QueryInterface(requestingContext);
  if (node) {
    nsCOMPtr<nsIDocument> doc = node->OwnerDoc();
    if (!doc) {
      doc = do_QueryInterface(requestingContext);
    }
    if (doc) {
      window = doc->GetWindow();
    }
  }
}

Comment 13

[Tanvi Vyas[:tanvi]]

Comment on attachment 8558266 [details] [diff] [review]
simple-content-policy

Please add some tests and do a try push.

>diff --git a/dom/base/nsContentPolicy.cpp b/dom/base/nsContentPolicy.cpp
>+    nsCOMPtr<nsIDOMElement> topFrameElement;
>+    bool isTopLevel = true;
>+    if (requestPrincipal) {
I think you mean if (requestingContext) here.  What happens if you don't have a requestingContext?  I see topFrameElement and isTopLevel are optional, so perhaps the addon will just decide to fail open or fail closed.

>+        nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(requestingContext));
>+        if (!window) {
>+            nsCOMPtr<nsIDocument> doc;


>diff --git a/dom/base/nsISimpleContentPolicy.idl b/dom/base/nsISimpleContentPolicy.idl

Can you add more comments to this file explaining it's purpose so that it is clear how it differentiates from nsIContentPolicy?

Thanks!

Comment 14

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

Attachment: simple-content-policy v2

Flagging you in case you wanted to see this again, Chris.

I added a test, which found an additional problem. Sometimes requestingContext is the <browser> element that triggered the load. In that case we want to call it the topFrameElement.

Blake, can you review the test and also look at the nsContentPolicy.cpp part again?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8566253 [details] [diff] [review]
simple-content-policy v2

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

Still looks good to me!

::: dom/base/nsContentPolicy.cpp
@@ +138,5 @@
> +    nsCOMPtr<nsIDOMElement> topFrameElement;
> +    bool isTopLevel = true;
> +    nsCOMPtr<nsPIDOMWindow> window;
> +    if (nsCOMPtr<nsINode> node = do_QueryInterface(requestingContext)) {
> +        if (nsCOMPtr<nsIDocument> doc = node->OwnerDoc()) {

Nit: I think you could simplify that part; as far as I know OwnerDoc() never returns null.

::: dom/base/test/test_simplecontentpolicy.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for nsISimpleContentPolicy</title>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>        

nit: trailing whitespace

@@ +8,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1128798">Mozilla Bug 1128798</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +  

nit: whitespace

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8566253 [details] [diff] [review]
simple-content-policy v2

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

::: dom/base/nsContentPolicy.cpp
@@ +152,5 @@
> +        loadContext->GetTopFrameElement(getter_AddRefs(topFrameElement));
> +
> +        if (topFrameElement) {
> +            nsCOMPtr<nsIDOMWindow> topWindow;
> +            window->GetTop(getter_AddRefs(topWindow));

I think it makes sense for this to be GetScriptableTop, though it doesn't matter since b2g doesn't have extensions that would implement this interface.

::: dom/base/test/file_simplecontentpolicy.js
@@ +15,5 @@
> +var policyID = Components.ID("{6aadacff-f1f2-46f4-a6db-6d429f884a30}");
> +var policyName = "@mozilla.org/simpletestpolicy;1";
> +var policy = {
> +  // nsISupports implementation
> +  QueryInterface: function(iid) {

Any reason not to import XPCOMUtils.jsm and generateQI? Also, what's with the blank lines before function bodies?

@@ +31,5 @@
> +    return this.QueryInterface(iid);
> +  },
> +
> +  // nsIContentPolicy implementation
> +  shouldLoad: function(contentType, contentLocation, requestOrigin, frame, isTopLevel, mimeTypeGuess, extra) {

Might as well check isTopLevel is set correctly.

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba87714f5cf

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/9ba87714f5cf

Comment 19

[fdsc]

Hello
Maybe I'm doing something wrong. But in Nightly simple-content-policy doesn't work. When loading documents ShouldLoad just not invoked, although there have been a lot of calls from aTopFrameElement == null.

This should work?


Registration code in 'exports.main' (call in startup extension)

var registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar);
registrar.registerFactory(HTTPUACleaner.observer._classID, HTTPUACleaner.observer._classDescription, HTTPUACleaner.observer._contractID, HTTPUACleaner.observer);

var catMan = Cc["@mozilla.org/categorymanager;1"].getService(Ci.nsICategoryManager);

catMan.addCategoryEntry("simple-content-policy", HTTPUACleaner.observer._contractID, HTTPUACleaner.observer._contractID, false, true);

component

QueryInterface: XPCOMUtils.generateQI([Ci.nsISimpleContentPolicy, Ci.nsIFactory]),
		_classDescription: "HTTP UserAgentCleaner Content Policy",
		_classID: components.ID("84389FE4-AD9F-7F6D-817A-95B7EC1AD019"),
		_contractID: "@fxprivacy.8vs.ru/policy;1",

		shouldLoad:
			function(contentType, contentLocation, requestOrigin, node/*aTopFrameElement*/, aIsTopLevel, mimeTypeGuess, extra, aRequestPrincipal)
			{
console.error(contentLocation.spec);

Comment 20

[fdsc]

It seems that the problem is not nsiSimpleContentPolicy. nsIContentPolicy at me too does not work. Created a separate defect.

https://bugzilla.mozilla.org/show_bug.cgi?id=1153852

Comment 21

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

Both nsIContentPolicy and nsISimpleContentPolicy trap loads in the process where the load occurs.

However, the whole point of nsISimpleContentPolicy is that Firefox will automatically forward shouldLoad calls from the child to the parent. However, that part isn't implemented yet. I filed bug 1156489 to do it.

Comment 22

[Will Bamberg [:wbamberg]]

I'm removing the dev-doc-needed from here, as after conversation with billm it seems like we would prefer people to use WebRequest (bug 1157561).

Comment 23

[fdsc]

Where this was discussed?

WebRequest does not replace ContentPolicy for the expected content type.
In fact, very bad, if this functionality is not implemented.

Comment 24

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

(In reply to fdsc from comment #23)
> Where this was discussed?
> 
> WebRequest does not replace ContentPolicy for the expected content type.
> In fact, very bad, if this functionality is not implemented.

I'm not sure what you mean by expected content type. WebRequest includes a ResourceType:
https://developer.chrome.com/extensions/webRequest#type-ResourceType
It's similar to the contentType parameter that content policy provides. Is it not sufficient for you?

Comment 25

[fdsc]

ResourceType enum of
"main_frame", "sub_frame", "stylesheet", "script", "image", "object", "xmlhttprequest", or "other"

ContentType enum of
Ci.nsIContentPolicy.TYPE_DOCUMENT
Ci.nsIContentPolicy.TYPE_SUBDOCUMENT
Ci.nsIContentPolicy.TYPE_OTHER
Ci.nsIContentPolicy.TYPE_OBJECT
Ci.nsIContentPolicy.TYPE_SCRIPT
Ci.nsIContentPolicy.TYPE_IMAGE
Ci.nsIContentPolicy.TYPE_STYLESHEET
Ci.nsIContentPolicy.TYPE_PING
Ci.nsIContentPolicy.TYPE_XBL
Ci.nsIContentPolicy.TYPE_DTD
Ci.nsIContentPolicy.TYPE_XMLHTTPREQUEST
Ci.nsIContentPolicy.TYPE_OBJECT_SUBREQUEST
Ci.nsIContentPolicy.TYPE_FONT
Ci.nsIContentPolicy.TYPE_MEDIA
Ci.nsIContentPolicy.TYPE_WEBSOCKET
Ci.nsIContentPolicy.TYPE_CSP_REPORT
Ci.nsIContentPolicy.TYPE_XSLT
Ci.nsIContentPolicy.TYPE_BEACON
Ci.nsIContentPolicy.TYPE_FETCH
Ci.nsIContentPolicy.TYPE_IMAGESET



At least, the difference here

Ci.nsIContentPolicy.TYPE_MEDIA
Ci.nsIContentPolicy.TYPE_XMLHTTPREQUEST
Ci.nsIContentPolicy.TYPE_FETCH
Ci.nsIContentPolicy.TYPE_WEBSOCKET

 		
For my extension is a significant difference.

Comment 26

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

> Ci.nsIContentPolicy.TYPE_XMLHTTPREQUEST

We already have "xmlhttprequest".

> Ci.nsIContentPolicy.TYPE_MEDIA
> Ci.nsIContentPolicy.TYPE_FETCH
> Ci.nsIContentPolicy.TYPE_WEBSOCKET

OK. I doubt there's much harm in splitting these out as part of ResourceType. Right now they fall under "other". I'm curious why it matters to you though.

Comment 27

[fdsc]

> I'm curious why it matters to you though.

My extension comes from the fact that malware script can be uploaded to the site from outside (XSS).

Therefore, the my extension allows user to block separately ajax, separately fetch, separately websocket, separately CORS ajax, separately requests for media resources and separately for images.
A malicious script could be blocked because it uses a features, that are not normally allowed for this site.

Perhaps this granulation is excessive, but now it is implemented.

Comment 28

[(no longer active)]

Bill, was this ever used for something?  As far as I can tell, nsISimpleContentPolicy is completely unused in the tree, and there is nothing that registers a simple-content-policy category besides the test added here.  Is this used by add-ons?

Comment 29

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

This was intended to be used by add-ons. I think a few do use it (or at least they did). I dropped the ball and never made it e10s compatible though, which was kind of the whole point of it.

Anyway, we can remove this when legacy add-ons are gone.