Closed Bug 584842 Opened 14 years ago Closed 14 years ago

nsIContentPrefService remoting

Categories

(Core :: IPC, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: dougt, Assigned: azakai)

References

()

Details

Attachments

(1 file, 7 obsolete files)

nsIContentPrefService is needed to support preferences for specific websites (Site specific preferences).

Currently nsIContentPrefService directly talks to the sql database.  This fails in the content process, and therefore doesn't register with XPCOM as a service.
blocking2.0: --- → ?
It looks like the only methods in-use are:

HasPref
GetPref
RemovePrefsByName
SetPref
tracking-fennec: --- → 2.0b1+
So, it looks like we can just use the message manager to remote the above APIs without too much trouble.

It would be really nice to be able to send synchronous messages from a js component in the content process.  This would allow us to block and wait for the response from the parent process directly where we need it.  This would reduce the amount of refactoring we will have to make without it.

Olli, do you agree that allowing access to nsIContentFrameMessageManager from a js component in a content child process is safe and something we want?
blocking2.0: ? → ---
Whoever calls the JS component is in a window context, I assume, so you can
access the nsIContentFrameMessageManager object from the window.

If you want global message manager in content process, I don't know how that would
work. Which message manager in chrome process would get those messages?
Hmm, the only option is actually the global message manager in chrome process.
Anyway, that is something which doesn't work atm, since global message manager
is currently for chrome process only.
Depends on: 585173
Assignee: nobody → doug.turner
Assignee: doug.turner → azakai
OS: Mac OS X → All
We can find a technical solution for remoting this, but I don't understand what should be remoted exactly.

See the patch in bug 568270 and it's parent bug 506269. By intent only reading prefs should work, I presume because content processes are untrusted, so we don't want to let them modify or delete preferences.

Regarding reading prefs, that should be ok, and the just-mentioned bugs should already handle that (content prefs uses the prefs service internally I believe).

What is the specific situation that should work, but isn't already handled, that we want to fix in this bug?
AFAIK, the main reason that setting prefs from content is disable is due to racing concerns.
(In reply to comment #5)
> AFAIK, the main reason that setting prefs from content is disable is due to
> racing concerns.

Ah, interesting. That is a very good reason.

Security also seems tricky, though - a compromised content process could ask to change dangerous stuff in the prefs, like enabling some feature that is exploitable.
Hmm, on second thought, is there any reason not to remote content prefs using a whitelist? We would whitelist only the prefs that are (1) safe and (2) at no risk of races. That would include the content pref that was the original motivation for this patch, bug 575556.
So why should content process ever *set* any pref? IMO it should just be able to get prefs.
(In reply to comment #8)
> So why should content process ever *set* any pref? IMO it should just be able
> to get prefs.

See bug 575556 for one situation.
Attached patch Part 1: mozilla-central patch (obsolete) — Splinter Review
Remotes content prefs. Gets always work; Sets are whitelisted. Current whitelist includes a fix for bug 575556.

Hmm, who should review this?
Attachment #468867 - Flags: review?
Attached patch Part 2: mobile-browser (obsolete) — Splinter Review
Attachment #468868 - Flags: review?(mark.finkle)
Attachment #468867 - Flags: review? → review?(Olli.Pettay)
Comment on attachment 468868 [details] [diff] [review]
Part 2: mobile-browser

How long term do we want to live with this patch? I have some issues:
* We are loading services unnecessarily, during startup, which isn't good
* We are using an application front-end to init services, without which the strategy doesn't work. I don't think we should rely on Fennec specific source code to make this work.
* Bug 585173 has a cleaner way to make this work, without touching any Fennec specific files - Shouldn't we get it landed first?
(In reply to comment #12)
> Comment on attachment 468868 [details] [diff] [review]
> Part 2: mobile-browser
> 
> How long term do we want to live with this patch? I have some issues:
> * We are loading services unnecessarily, during startup, which isn't good

Good point. That can be fixed, though. If everything else is agreed I can do that very quickly.

> * We are using an application front-end to init services, without which the
> strategy doesn't work. I don't think we should rely on Fennec specific source
> code to make this work.

We can move this to a jsm, and have Firefox also load that jsm when it gets to that point. Or load the jsm from a single shared spot in Firefox and Fennec. It's just moving a small amount of code around.

The main point in this patch is that aside from a few lines of code in browser.js and content.js, we remote content prefs very easily, with little code, and without butchering the content prefs code itself.

> * Bug 585173 has a cleaner way to make this work, without touching any Fennec
> specific files - Shouldn't we get it landed first?

That bug would only prevent the need for the wrappedJSObject messagemanager hack - a few lines of code. Once that bug lands, I can change those few lines.
Comment on attachment 468868 [details] [diff] [review]
Part 2: mobile-browser

>--- a/chrome/content/content.js
>+++ b/chrome/content/content.js
>@@ -954,8 +954,19 @@ var FindHandler = {
>     let scroll = Util.getScrollOffset(content);
>     let rect = range.getBoundingClientRect();
>     rect = new Rect(scroll.x + rect.left, scroll.y + rect.top, rect.width, rect.height);
>     sendAsyncMessage("FindAssist:Show", { rect: rect.isEmpty() ? null: rect , result: findResult });
>   }
> };
> 
> FindHandler.init();
>+
>+// Prepare remoted services. Only needed if we are a remote tab
>+if (Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
>+    .processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
>+  [
>+    Cc["@mozilla.org/content-pref/service;1"].getService(Ci.nsIContentPrefService)
>+  ].forEach(function(service) {
>+    service.wrappedJSObject.messageManager = this; // XXX Hack until bug 585173
This is pretty terrible hack.
Could someone just review bug 585173?
Comment on attachment 468867 [details] [diff] [review]
Part 1: mozilla-central patch

I'm not a toolkit peer, so it would be better if someone else would look at this.
Attachment #468867 - Flags: review?(Olli.Pettay)
Comment on attachment 468867 [details] [diff] [review]
Part 1: mozilla-central patch

Myk, I hope you might be the right person to review this?

The patch basically remotes some of the content prefs API through IPC. I wrote it in a way that doesn't mess with the internals of the content prefs code.
Attachment #468867 - Flags: review?(myk)
Have you actually tested that the patch works after closing the tab which message manager is used?
(In reply to comment #17)
> Have you actually tested that the patch works after closing the tab which
> message manager is used?

I think it would work since a reference is held on the message manager. But the better thing is to wait for bug 585173 I guess. Anyhow it is just a few lines of code here, easy to change.
Comment on attachment 468867 [details] [diff] [review]
Part 1: mozilla-central patch

>+function electrolify(service) {

I believe the technical term is electrolyze. ;-)


>+        switch (aMessage.name) {
>+          case "ContentPref:getPref":
>+            return service.getPref(json.group, json.name);

Note: in theory, there would be a privacy advantage to whitelisting getPref as well, since a content process taken over by malicious code could use it to discover something about a user's browsing history through the prefs that have been set for various sites.  That seems like a relatively minor concern, however.


>+          case "ContentPref:setPref":
>+            // We have a whitelist of allowed settings. This is because
>+            // there are both potential race conditions, as well as
>+            // security issues, if content can send arbitrary setPref
>+            // messages. The whitelist contains only those settings that
>+            // are not at risk for either

Nits:

This explanation seems vague; what are the potential race conditions?

Also, I would replace "content" with "a compromised content process", since "content", the way we usually use the word, means web content generally, which doesn't have access to this API.  The point is to protect against the specific situation in which malicious content manages to compromise the content process.

Finally, the last sentence implies that any settings not on the whitelist are at risk (for either a race condition or a security issue), although we don't actually know that, since we haven't evaluated the rest of them.  It would be better to say that the whitelist contains only those settings for which we have identified a need to set them from the content process and determined that they don't present undue risk.


>+            // We currently whitelist saving the last directory of file
>+            // uploads.
>+            const WHITELIST = ["browser.upload.lastDir"];
>+            if (WHITELIST.indexOf(json.name) != -1) {
>+              service.setPref(json.group, json.name, json.value);
>+              return true; // Return value not really used, but needed
>+                           // because undefined will confuse messagemanager
>+            } else {
>+              return false;
>+            }
>+            break;
>+        }

Nit: the "break" is unneeded, since the statements associated with the case always return.

Finally, nit: it would be handy if the docs in nsIContentPrefService.idl specified which of the nsIContentPrefService interface's methods and properties are available from content processes and that setPref from content processes is restricted to a hardcoded set of whitelisted settings.

Otherwise, these changes look fine.  I can't speak to the message passing implementation, as I'm unfamiliar with the electrolysis APIs, although it seems reasonable.  Nevertheless, it could use some unit tests.  What is the status of unit testing in the electrolyzed world?
Attachment #468867 - Flags: review?(myk) → review+
Attached patch Part 1: mozilla-central patch v2 (obsolete) — Splinter Review
Thanks for the quick review :)

Updated patch:

1. Fixed all the stuff in the comments.
2. Very good point about the privacy issues with getPref. I made it so the whitelist applies to that as well. Better safe than sorry. We don't need more than this anyhow, at this point.
3. Added xpcshell tests (still a little clunky in e10s, but turns out it is possible here).
4. Moved to the new process message manager in bug 585173. This is required for the patch to work now.
Attachment #468867 - Attachment is obsolete: true
Attachment #469620 - Flags: review?(myk)
Attached patch Part 2: mobile-browser patch v2 (obsolete) — Splinter Review
Updated part 2 as well for the new message manager, and mentioned that we can optimize the component loading in the future.

mfinkle, perhaps you also want to take a look at the message manager stuff in part 1?
Attachment #468868 - Attachment is obsolete: true
Attachment #469622 - Flags: review?(mark.finkle)
Attachment #468868 - Flags: review?(mark.finkle)
>+if (Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
>+    .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {

This will break non-libxul builds (ie. seamonkey) as |Cc["@mozilla.org/xre/app-info;1"]| is null.
Attached patch Part 1: mozilla-central patch v3 (obsolete) — Splinter Review
(In reply to comment #22)
> >+if (Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
> >+    .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
> 
> This will break non-libxul builds (ie. seamonkey) as
> |Cc["@mozilla.org/xre/app-info;1"]| is null.

Thanks for the heads up. Updated the patch.
Attachment #469620 - Attachment is obsolete: true
Attachment #469733 - Flags: review?(myk)
Attachment #469733 - Flags: review?(mark.finkle)
Attachment #469620 - Flags: review?(myk)
Comment on attachment 469733 [details] [diff] [review]
Part 1: mozilla-central patch v3

I'll rewrite this patch using bug 591052 for the messaging.
Attachment #469733 - Attachment is obsolete: true
Attachment #469733 - Flags: review?(myk)
Attachment #469733 - Flags: review?(mark.finkle)
Depends on: 591052
Attached patch Part 1: mozilla-central patch v4 (obsolete) — Splinter Review
Patch now uses bug 591052 for messaging; no more hacks, and no more need for the mobile-browser part.

Also improved the ipc test to handle URIs. While I was doing that, I cleaned up the code a little, putting all the parsing code of aGroup into one function, I hope that's ok.
Attachment #469622 - Attachment is obsolete: true
Attachment #470098 - Flags: review?(myk)
Attachment #470098 - Flags: review?(mark.finkle)
Attachment #469622 - Flags: review?(mark.finkle)
Comment on attachment 470098 [details] [diff] [review]
Part 1: mozilla-central patch v4

>+      const NAME_WHITELIST = ["browser.upload.lastDir"];
>+      if (NAME_WHITELIST.indexOf(json.name) == -1)
>+        return null;

null is actually a valid value for a preference in the database, so this method should throw an exception in this case, to distinguish between "has the null value" and "value cannot be retrieved because setting is not on the whitelist."

I'm not that familiar with the message manager interfaces, but it looks like this implementation makes a synchronous call through them, so it's theoretically possible to propagate an exception back to the caller.  Is it possible to do so?

Also, as an aside, this whitelist is going to become painful to maintain over time; we should do some thinking (not in this bug, in a followup bug) about how we might make its maintenance easier, f.e. providing a method accessible only from the chrome process for registering a setting with the whitelist, so whitelist entries can be maintained by the code that uses them.


>+      switch (aMessage.name) {
>+        case "ContentPref:getPref":
>+          return service.getPref(json.group, json.name, json.value);
>+
>+        case "ContentPref:setPref":
>+          service.setPref(json.group, json.name, json.value);
>+          return true; // Return value not really used, but needed
>+                       // because undefined will confuse messagemanager

getPref returns undefined when there is no value in the database for the group/setting combination.  Is that also going to confuse messagemanager, and how might we clarify the situation for our messagemanager friend?


>+  _parseGroupParam: function ContentPrefService__parseGroupParam(aGroup) {
>+    if (aGroup == null)
>+      return null;
>+    if (aGroup.constructor.name == "String")
>+      return aGroup.toString();
>+    if (aGroup instanceof Ci.nsIURI)
>+      return this.grouper.group(aGroup);
>+
>+    throw Components.Exception("aGroup is not a string, nsIURI or null",
>+                               Cr.NS_ERROR_ILLEGAL_VALUE);
>+  },

Great refactoring!
Thanks for the info about the return values. I think the best solution is to return { success: false|true, value: ... }, and the child will throw an exception on !success. That would handle all the undefined/null/etc. cases, and avoid needing to propagate an exception across IPC (which doesn't work yet).

I'll write a new version of this patch with that change once bug 592017 is done, as it will require a few tiny changes to the messaging code here.
(In reply to comment #27)
> Thanks for the info about the return values. I think the best solution is to
> return { success: false|true, value: ... }, and the child will throw an
> exception on !success. That would handle all the undefined/null/etc. cases, and
> avoid needing to propagate an exception across IPC (which doesn't work yet).

Sounds like a plan!
Attachment #470098 - Flags: review?(myk)
Attachment #470098 - Flags: review?(mark.finkle)
Attached patch Patch v5 (obsolete) — Splinter Review
Updated patch with the last stuff discussed.

Turns out we can't do all the nice messagemanager stuff we wanted at this point. But we will fix that later in bug 593407. I added relevant comments in the source.
Attachment #470098 - Attachment is obsolete: true
Attachment #471908 - Flags: review?(myk)
Comment on attachment 471908 [details] [diff] [review]
Patch v5

Looks good, r=myk!
Attachment #471908 - Flags: review?(myk) → review+
http://hg.mozilla.org/mozilla-central/rev/38b763292cb1
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Patch had to be backed out due to OS X ipc test issues.

Fix will be to not run an ipc test on OS X, similarly to http://mxr.mozilla.org/mozilla-central/source/netwerk/test/Makefile.in#103
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch v6Splinter Review
Patch that fixes the OS X issue mentioned before. Changes just to the tests. But sadly to work around the OS X problem it was necessary to add a new directory for the tests and some other boilerplate files, as well as modify the Makefile.
Attachment #471908 - Attachment is obsolete: true
Attachment #472319 - Flags: review?(myk)
Attachment #472319 - Flags: review?(myk) → review+
http://hg.mozilla.org/mozilla-central/rev/cd88c9a85f20
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: