Closed
Bug 1303096
Opened 8 years ago
Closed 8 years ago
Stop sending sync messages soon after content process start-up
Categories
(Core :: DOM: Content Processes, defect)
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)
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•8 years ago
|
Assignee | ||
Comment 1•8 years ago
|
||
Assignee: nobody → blassey.bugs
Attachment #8801525 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 2•8 years ago
|
||
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-
Reporter | ||
Updated•8 years ago
|
Assignee | ||
Comment 4•8 years ago
|
||
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
Assignee | ||
Comment 5•8 years ago
|
||
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?
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 6•8 years ago
|
||
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.
Reporter | ||
Updated•8 years ago
|
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)
Assignee | ||
Comment 8•8 years ago
|
||
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
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)
Assignee | ||
Comment 10•8 years ago
|
||
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
Attachment #8801525 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8821286 -
Attachment is obsolete: true
Attachment #8822520 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 12•8 years ago
|
||
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.
Assignee | ||
Comment 15•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
(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.
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #8822520 -
Attachment is obsolete: true
Attachment #8824590 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 20•8 years ago
|
||
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)
Assignee | ||
Comment 21•8 years ago
|
||
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.
Reporter | ||
Comment 22•8 years ago
|
||
(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+
Assignee | ||
Comment 24•8 years ago
|
||
(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.
Assignee | ||
Comment 26•8 years ago
|
||
Attachment #8826849 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 27•8 years ago
|
||
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+
Comment 29•8 years ago
|
||
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.
Assignee | ||
Comment 30•8 years ago
|
||
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)
Assignee | ||
Comment 31•8 years ago
|
||
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+
Comment 34•8 years ago
|
||
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
Comment 35•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(blassey.bugs)
Updated•8 years ago
|
Comment 36•8 years ago
|
||
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
Comment 37•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 38•8 years ago
|
||
So ContentPrefs is apparently something which needs a DOM peer review, but nothing in the code tells what kind of thing ContentPrefs actually is.
You need to log in
before you can comment on or make changes to this bug.
Description
•