Closed Bug 1303096 Opened 3 years ago Closed 3 years ago

Stop sending sync messages soon after content process start-up

Categories

(Core :: DOM: Content Processes, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: mconley, Assigned: blassey)

References

(Depends on 2 open bugs, Blocks 2 open bugs)

Details

Attachments

(2 files, 5 obsolete files)

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.
Depends on: 1303374
Depends on: 1305818
Attached patch async_init_messages.patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #8801525 - Flags: review?(wmccloskey)
Comment on attachment 8801525 [details] [diff] [review]
async_init_messages.patch

Try push is colorful https://treeherder.mozilla.org/#/jobs?repo=try&revision=de483c97cd9e
Attachment #8801525 - Flags: review?(wmccloskey) → feedback?(wmccloskey)
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.
Attachment #8801525 - Flags: feedback?(wmccloskey) → feedback-
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
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.
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.
Flags: needinfo?(wmccloskey)
I probably won't be able to get to this for a few days. Thanks for the reduced patch.
Flags: needinfo?(wmccloskey)
Attached patch async_mgs_rollup.patch (obsolete) — Splinter Review
Attachment #8821286 - Attachment is obsolete: true
Attachment #8822520 - Flags: review?(wmccloskey)
this still is failing the telemetry xpcshell test. If you have any idea as to why I'm all ears
Can you paste the try results?
Flags: needinfo?(blassey.bugs)
Also, can you explain what the patch does? I remember it was different from the last ones but I don't remember how.
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.
Flags: needinfo?(blassey.bugs)
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 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
Attachment #8822520 - Flags: review?(wmccloskey)
(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.
Attached patch async_proc_init.patch (obsolete) — Splinter Review
Attachment #8822520 - Attachment is obsolete: true
Attachment #8824590 - Flags: review?(wmccloskey)
Mike, looks like you were resppnsible for the telemetry probe in RecvGetXPCOMProcessAttributes(). Are you happy with where it is after this patch?
Flags: needinfo?(mconley)
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.
(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?
Flags: needinfo?(mconley)
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.
Attachment #8824590 - Flags: review?(wmccloskey) → review+
(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)
(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.
Attached patch async_init.patch (obsolete) — Splinter Review
Attachment #8826849 - Flags: review?(wmccloskey)
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?
Attachment #8826849 - Flags: review?(wmccloskey) → review+
Depends on: 1332036
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.
Attached patch async_init.patchSplinter Review
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.
Attachment #8824590 - Attachment is obsolete: true
Attachment #8826849 - Attachment is obsolete: true
Attachment #8832282 - Flags: review?(wmccloskey)
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 on attachment 8832282 [details] [diff] [review]
async_init.patch

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

See the previous comment for my review.
Attachment #8832282 - Flags: review?(wmccloskey) → review+
Pushed by blassey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9d8a75a0dcc
Stop sending sync messages soon after content process start-up r=billm
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
Flags: needinfo?(blassey.bugs)
Flags: needinfo?(blassey.bugs)
See Also: → 1337062
Pushed by blassey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a43dbcb95ca
Stop sending sync messages soon after content process start-up r=billm
Depends on: 1337283
https://hg.mozilla.org/mozilla-central/rev/5a43dbcb95ca
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1337914
Depends on: 1341399
Depends on: 1341887
Depends on: 1342067
So ContentPrefs is apparently something which needs a DOM peer review, but nothing in the code tells what kind of thing ContentPrefs actually is.
Depends on: 1342685
Depends on: 1399503
Depends on: 1412348
You need to log in before you can comment on or make changes to this bug.