1303096 - Stop sending sync messages soon after content process start-up

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

When ContentChild calls Init, it sends a sync message ("GetProcessAttributes") here:

http://searchfox.org/mozilla-central/rev/515bca4fca5e2e8233ed30e6b6d48fcab8497dbf/dom/ipc/ContentChild.cpp#606

not long after, once XPCOM starts up in the content process, we send another one here:

http://searchfox.org/mozilla-central/rev/515bca4fca5e2e8233ed30e6b6d48fcab8497dbf/dom/ipc/ContentChild.cpp#929

as soon as we need to paint something that requires nsXPLookAndFeel, we send yet another one here:

http://searchfox.org/mozilla-central/rev/515bca4fca5e2e8233ed30e6b6d48fcab8497dbf/widget/nsXPLookAndFeel.cpp#478

and finally, when the Preferences service starts up, we send another one here:

http://searchfox.org/mozilla-central/rev/515bca4fca5e2e8233ed30e6b6d48fcab8497dbf/modules/libpref/Preferences.cpp#563

There might be more, but these are the ones I know about.

Sending sync messages soon after start-up is an anti-pattern - especially if the parent is likely busy doing other things. We should try to initialize the content process with as much information as possible - perhaps over command line arguments or an async message from the parent with everything it needs.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: async_init_messages.patch

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8801525 [details] [diff] [review]
async_init_messages.patch

Try push is colorful https://treeherder.mozilla.org/#/jobs?repo=try&revision=de483c97cd9e

Comment 3

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

Comment on attachment 8801525 [details] [diff] [review]
async_init_messages.patch

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

Maybe I've missed something, but I think that the structure here can be significantly simplified. If you don't pass aCanDequeue=false (i.e., don't introduce this param at all), then WaitForIncomingMessage will actually run the message handler right away. So you won't need to do any of the deserialization yourself. You'll need to move some code into the Recv functions for your new async setters, but that looks doable to me (and in fact should separate things more nicely). You'll need to have a field on ContentChild to signal that you've received everything, but that's fine.

Also, we may still want to do #2 in bug 1303374 comment 4 since you're still blocking here a lot.

Not sure why this is orange.

::: dom/ipc/ContentParent.cpp
@@ +2073,5 @@
>       base::GetProcId(mSubprocess->GetChildProcessHandle()));
> +  SendSetProcessAttributes(mChildID, IsForApp(), IsForBrowser());
> +  SendSetLookAndFeelCache(LookAndFeel::GetIntCache());
> +  nsTArray<PrefSetting> prefArray;
> +  RecvReadPrefsArray(&prefArray);

Please remove the no-longer-used messages like ReadPrefsArray. The name of RecvReadPrefsArray should be changed to something like SendPrefs, and that function should actually send the async message.

Same for some of these other messages.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Just as an update, mac and linux are looking good https://treeherder.mozilla.org/#/jobs?repo=try&revision=122fb0aecf7c&selectedJob=30146917

still need to figure out what's going on on Windows

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=580b7c297cb340a8419f21ecde250888fce17ed4&selectedJob=31192164

This patch is hitting this assertion http://searchfox.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#1548

Bill, any ideas?

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

pushed to try again logging what messages were being run from WaitForIncomingMessage(), and it doesn't appear that those are the messages that are coming out of order.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4428747ef5747479d5da52c8292a6e1750f1d1a5&selectedJob=31290386

One thing I notice is that PContent::Msg_AsyncMessage tends to be the out of order message. Not sure if that's just a common message in general or if there's something special about it that's going wrong.

Comment 7

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

I looked at this for a while today and didn't find anything. It would help if you could remove the code that's not actually running and reduce the diff to be as small as possible. I got a little lost in the commented-out changes.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

sorry, was away for a bit. Here's a reduced patch https://hg.mozilla.org/try/rev/ed528dd18f5505f08eee120b583275f61283a1f6

and a failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=32357280&repo=try#L22725

Comment 9

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

I probably won't be able to get to this for a few days. Thanks for the reduced patch.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: async_rollup.patch

This patch shows an improvement in session restore https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=f179934df0c1&newProject=try&newRevision=b5f5af2fecfd548da5ae66ff4640142f31c18f27&framework=1&showOnlyImportant=0

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: async_mgs_rollup.patch

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

this still is failing the telemetry xpcshell test. If you have any idea as to why I'm all ears

Comment 13

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

Can you paste the try results?

Comment 14

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

Also, can you explain what the patch does? I remember it was different from the last ones but I don't remember how.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Sorry, thought it was in my previous comment https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e468b443be314e70dd14c2a604e8b687cccb5a6&selectedJob=65288613

This sends the child id and sandbox level as command line flags since those are needed immediately and then sends the rest of the initialization data as async messages. Additionally, since preferences don't come to the content process synchronously, I've added release assertions to protect against preferences being accessed before they are initialized with the values. The slow script preferences are excepted from the assertions because they use default arguments which are returned and also add listeners for preference value changes, so will get the right value once the initialization message comes.

Comment 16

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

Can you rebase onto a more recent m-c and push to try again? I remember there was a permafail like this that appeared around Christmastime. You might just be hitting that.

Comment 17

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

Comment on attachment 8822520 [details] [diff] [review]
async_mgs_rollup.patch

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

Lots of small nits, but this looks really nice!

::: dom/ipc/ContentChild.cpp
@@ +961,5 @@
>  }
>  
>  void
> +ContentChild::InitXPCOM(const XPCOMInit& xpcomInit,
> +                        const mozilla::dom::ipc::StructuredCloneData& initialData)

Names should be aFoo.

@@ +966,2 @@
>  {
> +  Preferences::SetPreferences(&xpcomInit.prefs());

New paragraph after this line.

@@ +1003,5 @@
>      ssm->ActivateDomainPolicyInternal(getter_AddRefs(mPolicy));
>      if (!mPolicy) {
>        MOZ_CRASH("Failed to activate domain policy.");
>      }
> +    mPolicy->ApplyClone(const_cast<mozilla::dom::DomainPolicyClone *>(reinterpret_cast<const mozilla::dom::DomainPolicyClone*>(&xpcomInit.domainPolicy())));

What is going on with this cast? The reinterpret_cast doesn't seem necessary. For the const_cast, I think you can instead mark the parameter to ApplyClone as const.

::: dom/ipc/ContentParent.cpp
@@ +1766,5 @@
>    PROFILER_LABEL_FUNC(js::ProfileEntry::Category::OTHER);
>  
>    std::vector<std::string> extraArgs;
> +  extraArgs.push_back("-childID");
> +  char idStr[19];

The longest 64-bit int takes 20 characters to represent in decimal, so this should probably be at least 21 bytes.

@@ +1851,5 @@
>                              bool aSetupOffMainThreadCompositing,
>                              bool aSendRegisteredChrome)
>  {
> +  XPCOMInit xpcomInit;
> +  Preferences::GetPreferences(&xpcomInit.prefs());

Paragraph break after this line.

@@ +1852,5 @@
>                              bool aSendRegisteredChrome)
>  {
> +  XPCOMInit xpcomInit;
> +  Preferences::GetPreferences(&xpcomInit.prefs());
> +  StructuredCloneData initialData;

Please move this declaration closer to where it's used.

@@ +1853,5 @@
>  {
> +  XPCOMInit xpcomInit;
> +  Preferences::GetPreferences(&xpcomInit.prefs());
> +  StructuredCloneData initialData;
> +  nsTArray<LookAndFeelInt> lnfCache;

Same here.

::: dom/ipc/ContentProcess.cpp
@@ +62,5 @@
>  #endif
>  
>  #if (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX)
>  static void
> +SetUpSandboxEnvironment(uint32_t aSamdboxLevel)

aSandboxLevel

@@ +71,5 @@
> +  if (
> +#ifdef XP_WIN
> +      IsVistaOrLater() &&
> +#endif
> +      aSamdboxLevel >= 1) {

Please keep the IsSandboxTempDirRequired function and just pass the sandbox level to it. That way people have a name for what this condition is testing.

@@ +84,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return;
>    }
>  
> +  // Change the gecko defined temp directory to our sandbox-writable one

Mistake?

::: dom/ipc/PContent.ipdl
@@ +342,5 @@
>      nsCString version;
>      GMPAPITags[] capabilities;
>  };
>  
> +struct XPCOMInit

Can you call this XPCOMInitData?

@@ +540,5 @@
>       * Send BlobURLRegistrationData to child process.
>       */
>      async InitBlobURLs(BlobURLRegistrationData[] registrations);
>  
> +    async SetXPCOMProcessAttributes(XPCOMInit xpcomInit, StructuredCloneData initialData, LookAndFeelInt[] lookAndFeelIntCache);

Why wouldn't the last two arguments go in the struct?

::: toolkit/xre/nsEmbedFunctions.cpp
@@ +656,5 @@
>          process = new PluginProcessChild(parentPID);
>          break;
>  
>        case GeckoProcessType_Content: {
>            process = new ContentProcess(parentPID);

I know we discussed this, but I don't remember any reason why we couldn't do the argument parsing inside ContentProcess. It seems a lot cleaner. The code wouldn't have to live in this big case statement, and you wouldn't need to add a bunch of setters.

@@ +661,5 @@
>            // If passed in grab the application path for xpcom init
>            bool foundAppdir = false;
> +          bool foundChildID = false;
> +          bool foundIsForBrowser = false;
> +          //bool foundTempDir = false;

If we don't need this temp dir thing, please remove it.

@@ +681,5 @@
>                static_cast<ContentProcess*>(process.get())->SetAppDir(appDir);
>                foundAppdir = true;
>              }
>  
> +            if (aArgv[idx] && !strcmp(aArgv[idx], "-childID")) {

Not your fault, but this loop is pretty weird. It starts at aArgv[aArgc], which is always null (due to a weird C rule). We should start at aArgc - 1. And there shouldn't be any need to check if aArgv[idx] is null then.

@@ +686,5 @@
> +              MOZ_ASSERT(!foundChildID);
> +              if (foundChildID) {
> +                continue;
> +              }
> +              if (aArgv[idx + 1]) {

It would be clearer to do this as |if (idx + 1 < aArgv)|.

@@ +687,5 @@
> +              if (foundChildID) {
> +                continue;
> +              }
> +              if (aArgv[idx + 1]) {
> +                uint64_t childID = strtoul(aArgv[idx + 1], NULL, 0);

This won't work on 32-bit systems since long is only 32-bits there. strtoull should work. Also, pass nullptr instead of NULL and pass 10 for the base (since it will always be 10).

@@ +702,5 @@
> +              static_cast<ContentProcess*>(process.get())->SetIsForBrowser(strcmp(aArgv[idx], "-notForBrowser"));
> +              foundIsForBrowser = true;
> +            }
> +
> +            if (aArgv[idx] && !strncmp(aArgv[idx], "-sandboxLevel:",14)) {

Space before |14|. However, why not use a separate argument for the level like you did for the childId? It seems easier.

@@ +712,5 @@
> +              static_cast<ContentProcess*>(process.get())->SetSandboxLevel(sbLevel);
> +              foundSandboxLevel = true;
> +            }
> +
> +            /*if (aArgv[idx] && !strcmp(aArgv[idx], "-contentTempDir")) {

Remove if not needed.

@@ +734,5 @@
>                profile.Assign(nsDependentCString(aArgv[idx+1]));
>                static_cast<ContentProcess*>(process.get())->SetProfile(profile);
>                foundProfile = true;
>              }
> +            if (foundProfile &&

Please do something like this:
bool foundAll = foundAppDir && ...;
#ifdef ...
foundAll &= foundProfile;
#endif

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Bill McCloskey (:billm) from comment #17)
> @@ +702,5 @@
> > +              static_cast<ContentProcess*>(process.get())->SetIsForBrowser(strcmp(aArgv[idx], "-notForBrowser"));
> > +              foundIsForBrowser = true;
> > +            }
> > +
> > +            if (aArgv[idx] && !strncmp(aArgv[idx], "-sandboxLevel:",14)) {
> 
> Space before |14|. However, why not use a separate argument for the level
> like you did for the childId? It seems easier.

Funny, I actually thought this was cleaner and was thinking of switching the -childid to use this pattern.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: async_proc_init.patch

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

Mike, looks like you were resppnsible for the telemetry probe in RecvGetXPCOMProcessAttributes(). Are you happy with where it is after this patch?

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

I figured out that there are more prefs I'm not accounting for that are accessed in XRE start up before we send the preferences. Feel free to review, but I think I need to do something about that.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #20)
> Mike, looks like you were resppnsible for the telemetry probe in
> RecvGetXPCOMProcessAttributes(). Are you happy with where it is after this
> patch?

Hm - I think this actually ends up wiping out the usefulness of that probe. The probe is meant to record how long it takes from creating a ContentParent, to first hearing a message from the ContentChild actor. Is there another early message that we'd get from a ContentChild that we could use instead? Perhaps at a lower level - in the IPC communications pipeline setup stuff?

Comment 23

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

Comment on attachment 8824590 [details] [diff] [review]
async_proc_init.patch

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

::: dom/ipc/ContentParent.cpp
@@ +2572,5 @@
>    mHangMonitorActor = CreateHangMonitorParent(this, aTransport, aOtherProcess);
>    return mHangMonitorActor;
>  }
>  
> +/*

What's this for?

::: dom/ipc/ContentProcess.cpp
@@ +62,3 @@
>  {
>    // On OSX, use the sandbox-writable temp when the pref level >= 1.
> +  return (uint32_t aSandboxLevel >= 1);

Copy/paste error.

@@ +113,5 @@
>  {
> +  // If passed in grab the application path for xpcom init
> +  bool foundAppdir = false;
> +  bool foundChildID = false;
> +  bool foundIsForBrowser = false;

All these found vars, including the sandbox and profile ones, can be declared inside the loop.

@@ +143,2 @@
>  
> +    if (!strcmp(aArgv[idx], "-childID")) {

But oh, the cycles!? Can you |else if| for all of these (or at least the ones where it's not awkward to do so)?

@@ +146,5 @@
> +      if (foundChildID) {
> +        continue;
> +      }
> +      if (idx + 1 < aArgc) {
> +        childID = strtoul(aArgv[idx + 1], nullptr, 10);

Again, you absolutely need to use strtoull here or else bad things will happen on 32-bit.

@@ +163,5 @@
> +
> +    bool allFound = foundAppdir && foundChildID && foundIsForBrowser;
> +
> +#if (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX)
> +    if (aArgv[idx] && !strncmp(aArgv[idx], "-sandboxLevel:", 14)) {

Please remove the |aArgv[idx]| part.

@@ +168,5 @@
> +      MOZ_ASSERT(!foundSandboxLevel);
> +      if (foundSandboxLevel) {
> +        continue;
> +      }
> +      sandboxLevel = strtoul(aArgv[idx] + 14, NULL, 0);

nullptr instead of NULL.

@@ +177,5 @@
> +#endif /* (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX) */
> +
> +
> +#if defined(XP_MACOSX) && defined(MOZ_CONTENT_SANDBOX)
> +    if (aArgv[idx] && !strcmp(aArgv[idx], "-profile")) {

Same |aArgv[idx]| remark.

@@ +197,5 @@
> +
> +    allFound &= foundProfile;
> +#endif /* XP_MACOSX && MOZ_CONTENT_SANDBOX */
> +
> +

Needless newline.

::: dom/ipc/PContent.ipdl
@@ +540,5 @@
>       * Send BlobURLRegistrationData to child process.
>       */
>      async InitBlobURLs(BlobURLRegistrationData[] registrations);
>  
> +    async SetXPCOMProcessAttributes(XPCOMInitData xpcomInit, StructuredCloneData initialData, LookAndFeelInt[] lookAndFeelIntCache);

Still wondering why initialData and lookAndFeelIntCache are separated out.

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Bill McCloskey (:billm) from comment #23)
> Comment on attachment 8824590 [details] [diff] [review]
> >  {
> > +  // If passed in grab the application path for xpcom init
> > +  bool foundAppdir = false;
> > +  bool foundChildID = false;
> > +  bool foundIsForBrowser = false;
> 
> All these found vars, including the sandbox and profile ones, can be
> declared inside the loop.
Maybe I'm misunderstanding you, but if they're declared inside the loop only one can be true at a time and allFound will never be true.

> ::: dom/ipc/PContent.ipdl
> @@ +540,5 @@
> >       * Send BlobURLRegistrationData to child process.
> >       */
> >      async InitBlobURLs(BlobURLRegistrationData[] registrations);
> >  
> > +    async SetXPCOMProcessAttributes(XPCOMInitData xpcomInit, StructuredCloneData initialData, LookAndFeelInt[] lookAndFeelIntCache);
> 
> Still wondering why initialData and lookAndFeelIntCache are separated out.

Should have replied to this the first time. Our generated code for structs uses == operators to test for equality and both StructuredCloneData and LookAndFeelInt don't support those (or at least throw an error saying that operator==() doesn't exist for the passed parameters)

Comment 25

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

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #24)
> > All these found vars, including the sandbox and profile ones, can be
> > declared inside the loop.
> Maybe I'm misunderstanding you, but if they're declared inside the loop only
> one can be true at a time and allFound will never be true.

Yes, of course you're right.

> Should have replied to this the first time. Our generated code for structs
> uses == operators to test for equality and both StructuredCloneData and
> LookAndFeelInt don't support those (or at least throw an error saying that
> operator==() doesn't exist for the passed parameters)

OK.

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: async_init.patch

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e79cded6c8aee708f6f5c325772d216a5306fd6&selectedJob=69053756

Comment 28

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

Comment on attachment 8826849 [details] [diff] [review]
async_init.patch

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

::: dom/ipc/ContentParent.cpp
@@ +1771,5 @@
> +  extraArgs.push_back(idStr);
> +  extraArgs.push_back(IsForBrowser() ? "-isForBrowser" : "-notForBrowser");
> +
> +  char sbLevel[32];
> +  SprintfLiteral(sbLevel, "-sandboxLevel:%d" , Preferences::GetInt("security.sandbox.content.level"));

You have an extra space before the last comma.

@@ +1776,5 @@
> +  extraArgs.push_back(sbLevel);
> +
> +  std::string boolPrefs;
> +  std::string intPrefs;
> +  std::string stringPrefs;

This would be cleaner and more efficient if you use stringstreams here. Then you can write to them via:
  boolPrefs << i << ':' << Preferences::GetInt(ContentPrefs::gInitPrefs[i]) << '|';
And finally call .str() to get the final string.

@@ +1801,5 @@
> +      stringPrefs.append(":");
> +      stringPrefs.append(value);
> +      stringPrefs.append("|");
> +      break;
> +    }

Please add a default case that crashes.

::: dom/ipc/ContentPrefs.cpp
@@ +1,1 @@
> +const char* ContentPrefs::gInitPrefs[] = {

This should have a license and it should include ContentPrefs.h.

@@ +229,5 @@
> +  "ui.popup.disable_autohide",
> +  "ui.use_activity_cursor",
> +  "view_source.editor.external"};
> +
> +const size_t ContentPrefs::gInitPrefsLen = sizeof(ContentPrefs::gInitPrefs)/sizeof(const char*);

Need spaces around the /.

::: dom/ipc/ContentPrefs.h
@@ +1,1 @@
> +#ifndef CONTENT_PREFS_H

You need a license at the top. And this should be in mozilla::dom. And the #ifndef should be mozilla_dom_ContentPrefs_h.

@@ +1,5 @@
> +#ifndef CONTENT_PREFS_H
> +#define CONTENT_PREFS_H
> +class ContentPrefs {
> + public:
> +  static const char*  gInitPrefs[];

Can you add static public methods like this?
  const char** GetContentPrefs(size_t* aCount);
  const char* GetContentPref(size_t aIndex);

Then gInitPrefs and gInitPrefsLen can be private. These methods could be inline if you want.

::: dom/ipc/ContentProcess.cpp
@@ +62,3 @@
>  {
>    // On OSX, use the sandbox-writable temp when the pref level >= 1.
> +  return (aSandboxLevel >= 1);

No need for parens here.

@@ +190,5 @@
> +        int32_t length = strtol(str, &str, 10);
> +        str++;
> +        nsCString value(str, length);
> +        Preferences::SetCString(ContentPrefs::gInitPrefs[index], value);
> +        str+= (length + 1);

Need a space after |str|. Also can drop the parens.

::: modules/libpref/Preferences.cpp
@@ +744,5 @@
>    pref_SetPref(aPref);
>  }
>  
>  void
> +Preferences::SetPreferences(const nsTArray<PrefSetting>* aPrefs)

Why do you need this? Couldn't the caller call SetPreference in a loop by themselves?

Comment 29

[(no longer active)]

I added another one of these in bug 1331676, sorry about that!  I filed a follow-up (bug 1332036) to remove it once this lands so please ignore that when rebasing.

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: async_init.patch

besides addressing nits, this adds some assertions to ensure that new start up prefs don't get added and not added to the list.

Also, I found that the prefs set before mXREEmbed.start() is called get cleared, which was the source of the test problems I was having. So this has a solution to pass an array of PrefSettings that will get used as part of the init.

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: async_init.interdiff

Comment 32

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

Comment on attachment 8832283 [details] [diff] [review]
async_init.interdiff

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

::: dom/ipc/ContentPrefs.cpp
@@ +255,5 @@
>    "ui.use_activity_cursor",
>    "view_source.editor.external"};
>  
> +const char** mozilla::dom::ContentPrefs::GetContentPrefs(size_t* aCount) {
> +  *aCount = sizeof(ContentPrefs::gInitPrefs) / sizeof(const char*);

You can use ArrayLength instead.

@@ +260,5 @@
> +  return gInitPrefs;
> +}
> +
> +const char*  mozilla::dom::ContentPrefs::GetContentPref(size_t aIndex) {
> +  return gInitPrefs[aIndex];

Please assert that aIndex is in range.

::: dom/ipc/ContentPrefs.h
@@ +13,2 @@
>  class ContentPrefs {
> +  public:

This should not be indented.

@@ +14,5 @@
> +  public:
> +    static const char** GetContentPrefs(size_t* aCount);
> +    static const char* GetContentPref(size_t aIndex);
> +
> +  private:

Same.

::: modules/libpref/Preferences.cpp
@@ +550,5 @@
> +
> +/*static*/
> +void
> +Preferences::SetInitPreferences(nsTArray<PrefSetting>* aPrefs) {
> +  gInitPrefs = new InfallibleTArray<PrefSetting>(*aPrefs);

Please use Move(*aPrefs). This will avoid copying the whole thing.

::: modules/libpref/prefapi.cpp
@@ +737,5 @@
>      }
>      return flags;
>  }
> +#ifdef DEBUG
> +uint32_t gPhase = 0;

This should be declared static. Also, it should be an enum rather than a number. Otherwise no one will know what 0, 1, 2 mean.

@@ +738,5 @@
>      return flags;
>  }
> +#ifdef DEBUG
> +uint32_t gPhase = 0;
> +void pref_SetInitPhase(uint32_t phase) {

Brace should be on its own line.

@@ +742,5 @@
> +void pref_SetInitPhase(uint32_t phase) {
> +    gPhase = phase;
> +}
> +
> +bool inInitArray(const char* key) {

bool and the brace should be on their own line. The function should be declared static. And the name should be capitablized.

@@ +749,5 @@
> +    unsigned int l = 0, r = prefsLen;
> +    while (r >= l) {
> +        unsigned int m = floor((l + r) / 2);
> +        int comp = strcmp(mozilla::dom::ContentPrefs::GetContentPref(m), key);
> +        if (comp == 0) {

Please use BinarySearchIf from mfbt/BinarySearch.h instead of implementing it yourself.

Comment 33

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

Comment on attachment 8832282 [details] [diff] [review]
async_init.patch

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

See the previous comment for my review.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9d8a75a0dcc
Stop sending sync messages soon after content process start-up r=billm

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing various tests on Android 4.3 debug (e.g. test_saveHeapSnapshot_e10s_01.html):

https://hg.mozilla.org/integration/mozilla-inbound/rev/274fe9a179cf83e1a308de5214556c7073219f17

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e9d8a75a0dcceedeaabf2924bcf8459db2da01f5
Failure log (example): https://treeherder.mozilla.org/logviewer.html#?job_id=74571882&repo=mozilla-inbound

[task 2017-02-05T06:46:11.958537Z] 06:46:11     INFO -  13 INFO TEST-START | devtools/shared/heapsnapshot/tests/mochitest/test_saveHeapSnapshot_e10s_01.html
[task 2017-02-05T06:51:31.579637Z] 06:51:31     INFO -  Buffered messages logged at 06:46:13
[task 2017-02-05T06:51:31.579726Z] 06:51:31     INFO -  14 INFO window.onload fired
[task 2017-02-05T06:51:31.579808Z] 06:51:31     INFO -  Buffered messages logged at 06:46:14
[task 2017-02-05T06:51:31.579849Z] 06:51:31     INFO -  15 INFO Loading iframe
[task 2017-02-05T06:51:31.580960Z] 06:51:31     INFO -  Buffered messages finished
[task 2017-02-05T06:51:31.581035Z] 06:51:31     INFO -  16 INFO TEST-UNEXPECTED-FAIL | devtools/shared/heapsnapshot/tests/mochitest/test_saveHeapSnapshot_e10s_01.html | Test timed out.
[task 2017-02-05T06:51:31.581086Z] 06:51:31     INFO -      reportError@SimpleTest/TestRunner.js:114:7
[task 2017-02-05T06:51:31.581143Z] 06:51:31     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:135:7

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a43dbcb95ca
Stop sending sync messages soon after content process start-up r=billm

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a43dbcb95ca

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

So ContentPrefs is apparently something which needs a DOM peer review, but nothing in the code tells what kind of thing ContentPrefs actually is.