1027898 - [e10s] ContentPrefService2.jsm broken in content process

Status: RESOLVED FIXED

(Toolkit :: General, defect, P3)

Description

[Allison Naaktgeboren :ally]

Has caused issues with the filepicker and spellchecker. 
calls such as 
  nsCOMPtr<nsIContentPrefService2> contentPrefService =
    do_GetService(NS_CONTENT_PREF_SERVICE_CONTRACTID);

do not work, return null ptrs.

There were many components for Preferences. Please feel free to move this bug if this is not the correct place for it.

Comment 1

[Allison Naaktgeboren :ally]

was discovered in https://bugzilla.mozilla.org/show_bug.cgi?id=910384

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Most of a patch

This implements nsIContentPrefService2 in the content process. It has a bunch of tests, though they are incomplete. Here are things that still need to happen before checkin:

* Test private browsing content prefs
* Test the rest of the functions (right now coverage is very light).

I'm not entirely happy with the amount of code that I was unable to share between ContentPrefService2 and ContentPrefServiceContent (I'm also not thrilled with my names). I did what I could, but some things -- especially the private browsing and cache manipulations would have required serious restructuring as far as I could tell. I'm open to suggestions on fixing that. That being said, the relationship between all of the JS objects involved here is quite incestuous. Let me know if that's a problem.

Other than that, this was actually pretty straightforward.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

One question I have is how private browsing content prefs should work. The current patch has a per-process private browsing pref store, but it could just as easily be global. Drew, what do you think?

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8465053 [details] [diff] [review]
Most of a patch

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

Does the content process really need the getCached* functions?  In m-c, we only use getCachedByDomainAndName, and then only in the one spot -- chrome -- that actually prompted the creation of the getCached* functions.  I don't think there's any need to implement them there, which would make this patch simpler.

I think a global private browsing store would be fine and would also definitely simplify this patch, since the child could rely on the store in the parent.  If content processes mapped one-to-one to windows, then it might make more sense to keep a separate store per process, but as it is, I don't think it would.  And a global store is what we use now.

With those two changes, the child jsm should basically only have to forward calls to the parent.

(In reply to Blake Kaplan (:mrbkap) from comment #3)
> (I'm also not thrilled with my names).

The only name that seems kind of awkward to me is ContentPrefServiceContent.  ContentPrefServiceChild?  Matches your ContentPrefServiceParent.

::: toolkit/components/contentprefs/ContentPrefServiceContent.jsm
@@ +131,5 @@
> +    }
> +  },
> +
> +  _sendMessage: function(message, args, callback) {
> +    let requestId = this._getRandomId();

I think you could just use a counter instead of generating a UUID each time and all the overhead that probably has.

::: toolkit/components/contentprefs/ContentPrefServiceParent.jsm
@@ +22,5 @@
> +
> +    globalMM.addMessageListener("ContentPrefs:getByName", this);
> +    globalMM.addMessageListener("ContentPrefs:getByDomainAndName", this);
> +    globalMM.addMessageListener("ContentPrefs:getBySubdomainAndName", this);
> +    globalMM.addMessageListener("ContentPrefs:getGlobal", this);

Instead of defining all these separate message types, it'd be much simpler to have a single ContentPrefs type and then include the subtype along with the message data.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Drew Willcoxon :adw from comment #6)
> Does the content process really need the getCached* functions?  In m-c, we

Let's see how far we get without them! The cache was pretty easy to implement (especially compared to the private browsing code) so we could always add it back if needed.

Actually, looking through mxr results, the only caller not in test code is <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fullZoom.js?rev=4884b0efae7f#272>, which already deals with a non-cached value, I wonder if we could just remove these methods altogether.

> With those two changes, the child jsm should basically only have to forward
> calls to the parent.

There's still a bit of code to deal with content pref observers, but that's pretty simple.

> (In reply to Blake Kaplan (:mrbkap) from comment #3)
> I think you could just use a counter instead of generating a UUID each time
> and all the overhead that probably has.

There's actually a fair amount of code that uses IIDs for request IDs. It avoids having to deal with overflow and any overhead from that code will be swamped by the IPC overhead.

Comment 8

[Drew Willcoxon :adw]

(In reply to Blake Kaplan (:mrbkap) from comment #7)
> Actually, looking through mxr results, the only caller not in test code is
> <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> fullZoom.js?rev=4884b0efae7f#272>, which already deals with a non-cached
> value, I wonder if we could just remove these methods altogether.

Yeah, that's the one caller I mentioned.  It syncly accesses the cache whenever possible to provide a better UX and only falls back to the async function as a last resort.  When it does fall back to the async function, there's a visible, jarring jump in the zoom level.

The other getCached* methods aren't used in m-c, but I added them for completeness, which maybe is debatable.

> There's actually a fair amount of code that uses IIDs for request IDs. It
> avoids having to deal with overflow and any overhead from that code will be
> swamped by the IPC overhead.

Interesting, thanks for explaining.  Curious though, could you not just keep the callbacks in _sendMessage in a queue, and then dequeue them in receiveMessage to avoid IDs altogether?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

The tests are still a little lacking, but I think this is ready for review.

Anything I didn't mention in comment 7 should be addressed.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Drew Willcoxon :adw from comment #8)
> Interesting, thanks for explaining.  Curious though, could you not just keep
> the callbacks in _sendMessage in a queue, and then dequeue them in
> receiveMessage to avoid IDs altogether?

I guess this would be possible – IPC messages are ordered – but to do so would also require that we can assume that all of the database stuff happens in order as well and generally seems more fragile for relatively little gain (IMO).

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Another reason not to use sequential IDs for messages is that once we have multiple content processes, we'll have to not confuse messages from the various content processes (which can be done by keying on msg.target+requestId) but this avoids that problem altogether.

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8466399 [details] [diff] [review]
patch v1

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

Sorry for the delay, Blake.

::: browser/components/nsBrowserGlue.js
@@ -506,5 @@
>      SessionStore.init();
>      BrowserUITelemetry.init();
>      ContentSearch.init();
>  
> -    if (Services.appinfo.browserTabsRemote) {

Why this change?  It looks unrelated.

::: toolkit/components/contentprefs/ContentPrefServiceChild.jsm
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = [ "ContentPrefService" ];

Please call this ContentPrefServiceChild so it matches the file name and more importantly so it's easier to talk and think about in the context of all these ContentPref objects.

@@ +25,5 @@
> +function NYI() {
> +  return null;
> +}
> +
> +function callbackCaller(callback) {

Please capitalize this since it's a constructor.

@@ +49,5 @@
> +
> +let ContentPrefService = {
> +  QueryInterface: XPCOMUtils.generateQI([ Ci.nsIContentPrefService2 ]),
> +
> +  // Map from name -> array of observers

"Map from pref name..." to be clearer, and it's a Set of observers.

@@ +68,5 @@
> +    return Cc["@mozilla.org/uuid-generator;1"]
> +             .getService(Ci.nsIUUIDGenerator).generateUUID().toString();
> +  },
> +
> +  _requests: new Map(),

A comment here would be helpful, like:

// Map from random ID string -> CallbackCaller, per request

@@ +103,5 @@
> +          break;
> +
> +        for (let observer of observerList) {
> +          safeCallback(observer, data.callback, data.args);
> +        }

Please break here even though it's not necessary right now.

@@ +117,5 @@
> +
> +    this._requests.set(requestId, new callbackCaller(callback));
> +  },
> +
> +  getByName: function(name, context, callback) {

Nit: I won't ask you to rewrite all these, but it would be nicer to automatically generate these functions that simply forward to the parent instead of implementing each by hand.

@@ +143,5 @@
> +  },
> +
> +  getCachedByDomainAndName: NYI,
> +  getCachedBySubdomainAndName: NYI,
> +  getCachedGlobal: NYI,

These should throw an Error instead of returning null so we can more easily catch new code that expects them to work.

@@ +203,5 @@
> +    if (!set) {
> +      set = new Set();
> +      if (this._observers.size === 0) {
> +        // This is the first observer of any kind. Start listening for changes.
> +        this._mm.addMessageListener("ContentPrefs:NotifyObservers", this);

Nit: This smarts with the NotifyObservers listener is fine, but it would be simpler to always listen.  Is there some non-negligible overhead or downside?

@@ +208,5 @@
> +      }
> +
> +      // This is the first observer for this name. Start listening for changes
> +      // to it.
> +      this._mm.sendAsyncMessage("ContentPrefs:AddObserverFor", { name: name });

Would be clearer to call this AddObserverForName so it matches the function name.

@@ +223,5 @@
> +
> +    set.delete(observer);
> +    if (set.size === 0) {
> +      // This was the last observer for this name. Stop listening for changes.
> +      this._mm.sendAsyncMessage("ContentPrefs:RemoveObserverFor", { name: name });

Same here.

@@ +234,5 @@
> +      }
> +    }
> +  },
> +
> +  extractDomain: function(str) {

Hmm, we only use this one place in the tree, and I doubt the content process needs to use this, so I'd kind of like to ask you to throw a not-implemented Error again here.  That would simplify this patch more because you could get rid of _grouper and keep parseGroup where it is.  But I already asked you to do that with the getCached* methods and you've already done the work for this, so I guess it's OK.

Now that I think about it, content probably doesn't need to add observers either...

::: toolkit/components/contentprefs/ContentPrefServiceParent.jsm
@@ +72,5 @@
> +
> +      observer._names.add(prefName);
> +
> +      this._cps2.addObserverForName(prefName, observer);
> +    } else {

// RemoveObserverFor

::: toolkit/components/contentprefs/tests/mochitest/test_remoteContentPrefs.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for registering/unregistering chrome OOP</title>

I think the title needs to be updated?

Comment 13

[Drew Willcoxon :adw]

Hope it's OK if I move this to the right component.

Comment 14

[Drew Willcoxon :adw]

(In reply to Blake Kaplan (:mrbkap) from comment #11)
> Another reason not to use sequential IDs for messages is that once we have
> multiple content processes, we'll have to not confuse messages from the
> various content processes (which can be done by keying on
> msg.target+requestId) but this avoids that problem altogether.

It's a moot point now, but where would that be a problem?  The only time the parent uses request IDs is to ping-pong them back to the children that sent them.  Are you thinking of the possibility they might be used in the future?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Drew Willcoxon :adw from comment #12)
> Why this change?  It looks unrelated.

It's incorrect to only initialize these components if the autostart pref is true because of the "New e10s window". Since I was adding a new entry there, I just removed the if statement entirely.

> Nit: This smarts with the NotifyObservers listener is fine, but it would be
> simpler to always listen.  Is there some non-negligible overhead or downside?

We refuse to add a given listener for a given message more than once, so we have to only remove once at the end. Given that we have to track that anyway, we might as well only add the listener once.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch for checkin

This patch addresses all of adw's comments.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Hey Blake, do you have a link to a recent Try run handy? :)

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=f77fe238c814

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f62111caff

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G desktop mochitest orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f7bc67a623b

https://tbpl.mozilla.org/php/getParsedLog.php?id=45585371&tree=Mozilla-Inbound

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Android too.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84d14acbb09b

The test doesn't work on Android or b2g, so I disabled it there.

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/84d14acbb09b