Closed Bug 1114647 (e10s-rename) Opened 5 years ago Closed 3 years ago

Rename the content process to something intelligible.

Categories

(Core :: IPC, defect, P1)

37 Branch
All
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
e10s + ---
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: johan.charlez, Assigned: bobowen)

References

Details

Attachments

(5 files, 19 obsolete files)

64.99 KB, image/jpeg
Details
31.52 KB, image/png
Details
1.15 MB, image/png
Details
336.87 KB, image/png
Details
18.41 KB, patch
jld
: review+
Details | Diff | Splinter Review
"plugin-container.exe" is indistinguishable from actual plugin processes, and e10s should not ship like this.

Is it possible for either extensions or plugins to depend on the name of the process? If that's the case it's probably best to change the name as soon as possible. Is something blocking this from landing?
tracking-e10s: --- → ?
Bug 1011225 fixed this on Mac and also, as far as I can tell, introduced a welcomed changes to the name actual plug-in processes (e.g. "Nightly Plugin Content (Shockwave Flash)". 

What puzzled me is the decision to only make this change on OS X.
(In reply to Johan C from comment #1)
> puzzled
puzzles*
Assignee: nobody → gwright
Looks like bug 905073 already covers this?

Since this bug has a tracking flag I'll leave it up to someone else to decide which bug to mark as a dupe.
See Also: → 905073
Duplicate of this bug: 905073
Here's a screen-shot of the "Volume Mixer"-dialog in Windows 7. Firefox's content process is listed as plugin-container.exe and it lacks an icon.

The average user is likely to confuse this entry with an actual plugin, and that's assuming they know that "plugin-container.exe" is something that "belongs" to Firefox.
Some may even be concerned that it could be malicious, especially if you consider the lack of an icon, but this is perhaps better suited for another bug/discussion somewhere else.

Maybe this bug should block e10s' release?
See Also: 9050731041599
There is no way to programatically change a process's name or description under Windows (like there is on OSX); they're read directly from the .exe attributes.

But, we should:

1 - rename plugin-container{.exe} on all platforms to something more useful, e.g. {product}-content.exe or gecko-content.exe.  {product} would be preferred since then it would sort adjacent to the parent in process lists.

2 - embed the product icon into the content process as well as the main process executable.

3 - update the Windows description from "Plugin Container for {product}"
If anyone considers that the component is not the right one, please change it to a more appropriate one.
Component: Untriaged → DOM: Content Processes
Going by comment #6, this would be about the IPC core, rather than the multiprocess parts of DOM API implementation.
Component: DOM: Content Processes → IPC
I think this should almost block shipping e10s. My recommendation is that we just ship a second copy of plugin-content, named firefox-webcontent or similar with the proper icon etc.
Priority: -- → P1
Jeff, would you prefer we have one single name covering both content and plugin processes ("firefox-helper.exe" or similar), or would you rather we have separate ones like "firefox-webcontent.exe" and "firefox-plugin-container.exe"?
Flags: needinfo?(jgriffiths)
If it makes any difference to your opinion, I reckon I'm most of the way to having a functioning patch for the two-name scheme.
(In reply to George Wright (:gw280) (:gwright) from comment #10)
> Jeff, would you prefer we have one single name covering both content and
> plugin processes ("firefox-helper.exe" or similar), or would you rather we
> have separate ones like "firefox-webcontent.exe" and
> "firefox-plugin-container.exe"?

(In reply to George Wright (:gw280) (:gwright) from comment #11)
> If it makes any difference to your opinion, I reckon I'm most of the way to
> having a functioning patch for the two-name scheme.

Given this, two names is preferable. As a relatively technical user debugging plugin-related problems on a system, it would be useful for me to be able to differentiate between plugin processes and web content processes.
Flags: needinfo?(jgriffiths)
So this works on Windows at least; haven't tested it on other platforms yet.

What I've done here is:

- Renamed the default child process name from plugin-container to firefox-webcontent
- Added MOZ_PLUGIN_PROCESS_NAME for win32 only which creates a new executable target called firefox-plugin-container with different metadata embedded in the .exe
- firefox-webcontent.exe and firefox-plugin-container.exe are identical except that they have different manifests and different resource files embedded into them (I believe this is done at compile time, so we can't just cp firefox-webcontent.exe firefox-plugin-container.exe)
- Modified GetPathForBinary to return the correct binary on Windows depending on what XRE_GetProcessType() returns.

I've confirmed on my Win8 box that in task manager the processes show up correctly as "Nightly Web Content Helper" and "Plugin Container for Nightly" based on the resource metadata in ipc/app/{plugin-container,webcontent}/module.ver, and they show up separately in the volume control mixer too.
Attachment #8721001 - Flags: review?(ted)
How does this affect our binary size?
(In reply to George Wright (:gw280) (:gwright) from comment #13)
> and they show up
> separately in the volume control mixer too.

This actually sounds like a mis-feature to me - it should probably be a different bug, but it seems that Firefox should have a single entry on the volume control mixer if at all possible.
(In reply to Bill McCloskey (:billm) from comment #14)
> How does this affect our binary size?

plugin-container and firefox-webcontent are both around 250kB on Windows.

(In reply to Mark Hammond [:markh] from comment #15)
> (In reply to George Wright (:gw280) (:gwright) from comment #13)
> > and they show up
> > separately in the volume control mixer too.
> 
> This actually sounds like a mis-feature to me - it should probably be a
> different bug, but it seems that Firefox should have a single entry on the
> volume control mixer if at all possible.

Agreed, we should file a separate bug about that. I included the datapoint because the original reporter mentioned it :)
(In reply to Mark Hammond [:markh] from comment #15)
> (In reply to George Wright (:gw280) (:gwright) from comment #13)
> > and they show up
> > separately in the volume control mixer too.
> 
> This actually sounds like a mis-feature to me - it should probably be a
> different bug, but it seems that Firefox should have a single entry on the
> volume control mixer if at all possible.
Is existing bug 1041599
Attached patch plugin-container-rename.patch (obsolete) — Splinter Review
Some fixes to make it build on Windows. It doesn't build yet on !Windows but I want to address any concerns you have first.
Attachment #8721001 - Attachment is obsolete: true
Attachment #8721001 - Flags: review?(ted)
Attachment #8726074 - Flags: review?(ted)
Needinfo'ing RyanVM

Ryan: this change may be risky, we have theories that renaming the process name will interact with AV software. I'd like to see some QA on this change with popular AV software after it lands.
Flags: needinfo?(ryanvm)
Rares, what AV software does SV have available to it for testing?
Flags: needinfo?(ryanvm) → needinfo?(rares.bologa)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)
> Rares, what AV software does SV have available to it for testing?

Ryan, we have licenses for AVG & Kaspersky, but we could use a lot more other AV trials for testing on a short period of time.
Flags: needinfo?(rares.bologa)
George, can you please rebase this patch and kick off a Try push for it? I'd like my team to look at builds containing this patch before it lands on Nightly so we don't accidentally hose our test population.
Flags: needinfo?(gwright)
Full try run, attempt 1:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f28b2990f930

I'm currently looking at why Android is burning. Linux is behaving a bit weirdly too (at least, locally)
Flags: needinfo?(gwright)
Ryan, I think I've fixed all the issues now and this try run should be green.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b074578a8d91
Flags: needinfo?(ryanvm)
This *should* be green now on try.
Attachment #8726074 - Attachment is obsolete: true
Attachment #8726074 - Flags: review?(ted)
Attachment #8728761 - Flags: review?(ted)
Looks like you've got windows bustage.
Flags: needinfo?(ryanvm)
(In reply to George Wright (:gw280) (:gwright) from comment #28) 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2168c2c2ebc6

These Try builds are ready for testing per our earlier discussions.
Flags: needinfo?(rares.bologa)
Hi Ryan,

We need to setup the environment first for this tests, but we will look into it as soon as possible and return with a report for current working builds.

Thanks,
Paul
Flags: needinfo?(rares.bologa)
Hi,

I have started to test this with different antivirus clients on Windows 8 x64, Windows XP x32 and Linux x32,x64. Tested  also with e10s enabled/disalbe to see if there are some changes or issue while navigating trough different pages (eg: YouTube).

In Ubuntu 14.0.4 x64 the renamed process appears now as "Web Content" and encountered no conflicts, works properly.
In Ubuntu 12.0.4 x32 the renamed process appears now as "Web Content" and encountered no conflicts, works properly.

In Windows 8 x64 the renamed process appears now as "Nightly Web Content Helper" and this are the result for each AV:
- Kaspersky 10: No conflict, works properly
- Bitdefender Free Edition: No conflict, works properly
- Eset Nod 32 trial: No conflict, works properly
- AVG trial: No conflict, works properly
- Avira Free: No conflict, works properly
- Panda Internet Security 2016 trial: "Suspicious file neutralized: firefox-webcontainer.exe, firefox.exe" (see attachement)
- Avast: No conflict, works properly
- McAfee: No conflict, works properly
- Norton Security: No conflict, works properly

In Windows XP x32 the renamed process appears now as "firefox-webcontent.exe" and this are the result for each AV:
- Kaspersky 10: No conflict, works properly
- Panda Internet Security 2016 trial: "Suspicious file neutralized: firefox-webcontainer.exe, firefox.exe"

It seams that Panda AV has something to share with this change until now. Do you guys have any idea of other known AV clients that we should test ?

I will continue Monday to test this on Windows XP x32, Windows 10 x64, and Mac OS 10.11 with the AV clients available and I will return with the results found.

Thanks,
Cosmin.
Comment on attachment 8728761 [details] [diff] [review]
0001-Bug-1114647-Rename-plugin-container-to-firefox-webco.patch

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

I had written a pair of review comments before you pushed that MozReview request, so here they are.

::: dom/media/gmp/GMPLoader.h
@@ +32,4 @@
>  // Encapsulates generating the device-bound node id, activating the sandbox,
>  // loading the GMP, and passing the node id to the GMP (in that order).
>  //
> +// In Desktop Gecko, the implementation of this lives in firefox-webcontent,

Maybe take the chance to make this more generic? "In the plugin host binary"?

::: ipc/app/app-common.mozbuild
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# Any changes that affect Android need to be made in pie/moz.build as well.

nit: effect
Attachment #8728761 - Flags: review?(ted)
Oh wait, sorry, just confused by my review queue!
Comment on attachment 8728761 [details] [diff] [review]
0001-Bug-1114647-Rename-plugin-container-to-firefox-webco.patch

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

This needs a few things fixed before I can r+.

::: dom/media/gmp/GMPLoader.h
@@ +32,4 @@
>  // Encapsulates generating the device-bound node id, activating the sandbox,
>  // loading the GMP, and passing the node id to the GMP (in that order).
>  //
> +// In Desktop Gecko, the implementation of this lives in firefox-webcontent,

...re-reading this, I think you've made this incorrect now, because the GMP code should be using the plugin-container, not firefox-webcontent.

::: ipc/app/Makefile.in
@@ +25,5 @@
>  ifeq ($(OS_ARCH),WINNT) #{
>  # Note the manifest file exists in the tree, so we use the explicit filename
>  # here.
> +EXTRA_DEPS += firefox-webcontent.exe.manifest
> +EXTRA_DEPS += plugin-container/firefox-plugin-container.exe.manifest

I don't think this is right--you don't want the binary in this directory to depend on the manifest from the other directory, do you?

::: ipc/app/module.ver
@@ +1,4 @@
>  WIN32_MODULE_COMPANYNAME=Mozilla Corporation
>  WIN32_MODULE_PRODUCTVERSION=@MOZ_APP_WINVERSION@
>  WIN32_MODULE_PRODUCTVERSION_STRING=@MOZ_APP_VERSION@
> +WIN32_MODULE_DESCRIPTION=@MOZ_APP_DISPLAYNAME@ Web Content Helper

On other platforms we just call these processes "Firefox Web Content":
https://dxr.mozilla.org/mozilla-central/rev/102886e9ac63b3fa33a6f1b394aea054abce2dfd/dom/ipc/ContentChild.cpp#733

::: testing/talos/talos/cmanager_win32.py
@@ +84,4 @@
>  class WinCounterManager(CounterManager):
>  
>      def __init__(self, process_name, process, counters,
> +                 childProcess="firefox-webcontent"):

You should have jmaher take a look at the Talos changes.

::: toolkit/mozapps/installer/make-eme.mk
@@ +6,4 @@
>  
>  ifdef MOZ_SIGN_CMD
>    ifeq ($(OS_ARCH),WINNT)
> +    # The argument to this macro is the directory where firefox-webcontent.exe

I am 99% sure that this still wants to be plugin-container, since this is for the EME GMP plugin.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #34)
> ::: dom/media/gmp/GMPLoader.h
> @@ +32,4 @@
> >  // Encapsulates generating the device-bound node id, activating the sandbox,
> >  // loading the GMP, and passing the node id to the GMP (in that order).
> >  //
> > +// In Desktop Gecko, the implementation of this lives in firefox-webcontent,
> 
> ...re-reading this, I think you've made this incorrect now, because the GMP
> code should be using the plugin-container, not firefox-webcontent.

My understanding is that GMP uses GeckoProcessType_GMPlugin: https://dxr.mozilla.org/mozilla-central/source/xpcom/build/nsXULAppAPI.h?from=GeckoProcessType#370. At the moment my patch only changes GetPathToBinary to return plugin-container when it's GeckoProcessType_Plugin. Should I change it to also launch plugin-container on GeckoProcessType_GMPlugin?
Comment on attachment 8728761 [details] [diff] [review]
0001-Bug-1114647-Rename-plugin-container-to-firefox-webco.patch

Flagging jmaher for review as per Ted's request
Attachment #8728761 - Flags: review?(jmaher)
(In reply to George Wright (:gw280) (:gwright) from comment #35)
> My understanding is that GMP uses GeckoProcessType_GMPlugin:
> https://dxr.mozilla.org/mozilla-central/source/xpcom/build/nsXULAppAPI.
> h?from=GeckoProcessType#370. At the moment my patch only changes
> GetPathToBinary to return plugin-container when it's
> GeckoProcessType_Plugin. Should I change it to also launch plugin-container
> on GeckoProcessType_GMPlugin?

Yes, that sounds sensible.
(In reply to Cosmin Muntean [:CosminMCG] from comment #31)
> In Ubuntu 14.0.4 x64 the renamed process appears now as "Web Content" and
> encountered no conflicts, works properly.
> In Ubuntu 12.0.4 x32 the renamed process appears now as "Web Content" and
> encountered no conflicts, works properly.

FYI, this is the existing expected behavior on Linux/OS X.
Comment on attachment 8728761 [details] [diff] [review]
0001-Bug-1114647-Rename-plugin-container-to-firefox-webco.patch

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

reading over this patch, I see we have:
- plugin-container.exe
+ firefox-plugin-container.exe
+ firefox-webcontent.exe

Does this mean that we have 3 processes now?  For the Talos changes, we are s/plugin-container/firefox-webcontent/, which is accurate.  In non-e10s mode do we use firefox-plugin-container.exe?  should we define all of them?

It also looks as though the mach_commands.py is a 2 process environment, not a 3 process environment.  

r- as it is unclear if we have 2 or 3 processes and some of Ted's previous comments were not addressed.  Also want to understand this in non-e10s mode as well.
Attachment #8728761 - Flags: review?(jmaher) → review-
Hi,

I continued to test this with different antivirus clients on Windows XP x32, Windows 10 x64 and Mac OS X 10.7 these are the results for each AV:

Windows 10 X64 (the renamed process appears now as "Nightly Web Content Helper"):

- Kaspersky 10: No conflict, works properly
- Bitdefender Free Edition: No conflict, works properly
- Eset Nod 32 trial: No conflict, works properly
- AVG trial: No conflict, works properly
- Avira Free: No conflict, works properly
- Panda Internet Security 2016 trial: No conflict, works properly
- Avast: No conflict, works properly
- McAfee: No conflict, works properly
- Norton Security: No conflict, works properly

Windos XP x32:

- Bitdefender Free Edition: No conflict, works properly
- Eset Nod 32 trial: No conflict, works properly
- AVG trial: No conflict, works properly
- Avira Free: No conflict, works properly
- Avast: No conflict, works properly
- McAfee: No conflict, works properly
- Norton Security: No conflict, works properly

Mac OS X 10.7: (The process now appears as "Nightly Web Content")
When trying to install the 10.7 build on 10.9, 10.10, 10.11 I've encountered a warning saying "Nightly is damaged and cannot be opened"
George, is this related to the OS on which the build was created? Could you make a build that would work on Mac OS X 10.11, so we can test this on a newer version aswell?

- McAfee - No conflict, works properly
- Nod32 - No conflict, works properly
- Norton - No conflict, works properly
- Panda - No conflict, works properly
- Bitdefender - No conflict, works properly
- Clamxav - No conflict, works properly
- Avast - No conflict, works properly
- AVG - cannot install on 10.7, needs 10.8 or older
- Kaspersky - cannot install on 10.7, needs 10.9 or older
- Avira - cannot install on mac os x 10.6, 10.7, 10.8
- Sophos - cannot install on mac os 10.7

It seams that Panda AV has some conflict just in Windows 8 x64 and Windows XP x32.

Thanks
Flags: needinfo?(gwright)
Needinfoing Jeff to find a Panda AV contact for us.
Flags: needinfo?(gwright) → needinfo?(jgriffiths)
My laptop is running 10.11 and the build worked fine here. Sounds like you had a corrupted download?

I used this: http://archive.mozilla.org/pub/firefox/try-builds/gwright@mozilla.com-2168c2c2ebc6a11715b0a715bb46b91a71d3aecd/try-macosx64/firefox-48.0a1.en-US.mac.dmg
Flags: needinfo?(ciprian.muresan)
(In reply to George Wright (:gw280) (:gwright) from comment #41)
> Needinfoing Jeff to find a Panda AV contact for us.

This is in process.
(In reply to George Wright (:gw280) (:gwright) from comment #42)
> My laptop is running 10.11 and the build worked fine here. Sounds like you
> had a corrupted download?
> 
> I used this:
> http://archive.mozilla.org/pub/firefox/try-builds/gwright@mozilla.com-
> 2168c2c2ebc6a11715b0a715bb46b91a71d3aecd/try-macosx64/firefox-48.0a1.en-US.
> mac.dmg

Hi George,

I don't think the build it's corrupted at download since we could install the build on 3 different machines, 2 x iMacs and 1 MacBook Pro (early 2015). The problem appears when trying to start the build. A pop up appears displaying ""Nightly" is damaged and can't be opened. You should move it to the Trash."

However, the same build downloaded gave the same error on Mac OS X 10.9, 10.10, 10.11 but not on 10.8 and 10.7 on the same machine with multiboot. Could this be a build signing issue ?
Flags: needinfo?(ciprian.muresan)
Update, we managed to do a workaround and we started the build on 10.11 trough terminal. No other methods tried have worked. We will continue our tests on 10.11 today and report back the results.
Hi,

Mac OS X 10.11: (The process now appears as "Nightly Web Content")

- Avira - No conflict, works properly
- Bitdefender - No conflict, works properly
- Panda - No conflict, works properly
- Kaspersky - No conflict, works properly
- Nod32 - No conflict, works properly
- AVG - No conflict, works properly
- McAfee - No conflict, works properly
- Avast - No conflict, works properly

Hovewer, I have found an issue that implies Avast antivirus. The Nightly build will not load any "https" website if you install it after installing the antivirus. This can be avoided only by restarting the PC after installing the Nightly build. 
This does not occur with the Firefox release build.

Thanks
Hrm. What happens if you disable Gatekeeper? This is in the system preferences, under Security and Privacy -> Allow apps downloaded from anywhere.
It is entirely possible this is a build signing issue, but Try builds are never signed. I don't know offhand if there's a way to test signed builds, maybe a project branch, although that feels sort of overkill here.
We've reached out to Panda and they will be updating their scanner to support the new filename. I'm keeping the needinfo until I hear back from them, I imagine we'll need to get an indication from them that they've updated their AV signatures etc before we can re-test.
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #49)
> We've reached out to Panda and they will be updating their scanner to
> support the new filename. I'm keeping the needinfo until I hear back from
> them, I imagine we'll need to get an indication from them that they've
> updated their AV signatures etc before we can re-test.

This issue is now reported as resolved.
Looks like we should re-test this - George, over to you?
Flags: needinfo?(jgriffiths) → needinfo?(gwright)
Hi,

I can confirm what Christopher said in comment 50. Retested this with Panda Internet Security 2016 trial on Windows 10 x64, 8 x64, XP x32 and now the process is no longer marked as suspicious.

Thanks,
Cosmin.
I think this should have addressed all of your review concerns.

Ted:

Re. affect/effect, I think the current word (affect) is correct in this instance. 

Re. GMP plugin, I believe that the GMP process has ProcessType as GeckoProcessType_GMPlugin, which in this patch would return the path to firefox-webcontent rather than firefox-plugin-container in GetPathToBinary. Do you want me to change it so that GetPathToBinary also returns plugin-container for GeckoProcessType_GMPlugin?

Joel:

We discussed this on IRC, and I think I've made the right changes here.
Attachment #8728761 - Attachment is obsolete: true
Flags: needinfo?(gwright)
Attachment #8734094 - Flags: review?(ted)
Attachment #8734094 - Flags: review?(jmaher)
Comment on attachment 8734094 [details] [diff] [review]
0001-Bug-1114647-Rename-plugin-container-to-firefox-webco.patch

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

r=me for the 3 talos files changed!
Attachment #8734094 - Flags: review?(jmaher) → review+
(In reply to George Wright (:gw280) (:gwright) from comment #53)
> Re. GMP plugin, I believe that the GMP process has ProcessType as
> GeckoProcessType_GMPlugin, which in this patch would return the path to
> firefox-webcontent rather than firefox-plugin-container in GetPathToBinary.
> Do you want me to change it so that GetPathToBinary also returns
> plugin-container for GeckoProcessType_GMPlugin?

Yes, I think GMP plugins should use firefox-plugin-container, since they're a form of plugin.
Comment on attachment 8734094 [details] [diff] [review]
0001-Bug-1114647-Rename-plugin-container-to-firefox-webco.patch

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

r=me with the GMP change per my previous comment.

::: toolkit/mozapps/installer/make-eme.mk
@@ +10,2 @@
>      # exists, and where voucher.bin will be generated.
> +    MAKE_SIGN_EME_VOUCHER = $(PYTHON) $(MOZILLA_DIR)/python/eme/gen-eme-voucher.py -input $(1)/firefox-webcontent.exe -output $(1)/voucher.bin && \

If you switch GMP plugins back to plugin-container, make sure to switch this too.
Attachment #8734094 - Flags: review?(ted) → review+
Depends on: 1261290
Depends on: 1261415
Depends on: 1261416
No longer depends on: 1261415
https://hg.mozilla.org/mozilla-central/rev/8f159c5d671e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Hi everyone! SeaMonkey developer here.

If you do revive this bug could you use (MOZ_APP_NAME) instead of hardcoding "firefox"?

> -  MOZ_CHILD_PROCESS_NAME="plugin-container${BIN_SUFFIX}"
> +  MOZ_CHILD_PROCESS_NAME="firefox-webcontent${BIN_SUFFIX}"
> +  MOZ_PLUGIN_PROCESS_NAME="firefox-plugin-container${BIN_SUFFIX}"

Thank you.
Hi everyone, Thunderbird developer here.
Looks like this broke the Thunderbird build. Also, why would other Mozilla applications build a firefox-plugin-container executable?
This is breaking more than just builds; see bug 1261620
Depends on: 1261620
Why even more different executable is added at all? It will make it impossible to share read-only pages between processes. All executables should be folded into firefox.exe as Chrome and IE8+ did. IIRC :bsmedberg also recommended it.
Depends on: 1261639
Depends on: 1261643
I see what looks like regressions from this change in the memory/process collection:
https://treeherder.mozilla.org/perf.html#/alerts?id=708

notice how we go to a value of 0.  This should be figured out.
Blocks: 1261952
We're going to back this out for now. Jeff, given the list of issues we already know about, and (presumably) there're other issues we are not aware of, do we still want to do this?
Flags: needinfo?(jgriffiths)
Here are the potential options we have available to us:

- Call everything "firefox.exe"
 * Pro: far less breakage, as presumably anyone whitelisting "plugin-container.exe" are also whitelisting "firefox.exe"
 * Pro: sandboxing prefers this (citation: jimm)
 * Con: users can't distinguish easily between chrome and content processes in task manager

- Have a separate plugin-container.exe and firefox-webcontent.exe on all platforms (not just Windows)
 * Pro: distinguishable in task manager
 * Pro: no plugin issues
 * Con: won't fix issues where the content process needs to be on the whitelist too, e.g. bug 1261620

- Do nothing
 * Pro: no breakage
 * Con: plugin-container is a misnomer at this point
Alias: e10s-rename
(In reply to George Wright (:gw280) (:gwright) from comment #69)
> Here are the potential options we have available to us:
> 
> - Call everything "firefox.exe"
>  * Pro: far less breakage, as presumably anyone whitelisting
> "plugin-container.exe" are also whitelisting "firefox.exe"
>  * Pro: sandboxing prefers this (citation: jimm)
>  * Con: users can't distinguish easily between chrome and content processes
> in task manager
> 
> - Have a separate plugin-container.exe and firefox-webcontent.exe on all
> platforms (not just Windows)
>  * Pro: distinguishable in task manager
>  * Pro: no plugin issues
>  * Con: won't fix issues where the content process needs to be on the
> whitelist too, e.g. bug 1261620
> 
> - Do nothing
>  * Pro: no breakage
>  * Con: plugin-container is a misnomer at this point

If you want to look at prior art:

* Chrome *
On Linux at least Chrome uses just the 'chrome' process with flags indicating the type. They *do* have a separate 'nacl_helper', which could be interpreted as a plug-in process.

Example output from |ps|
> /opt/google/chrome-unstable/chrome --user-data-dir=/home/erahm/.config/google-chrome-unstable
> /opt/google/chrome-unstable/chrome --type=zygote
> /opt/google/chrome-unstable/nacl_helper
> /opt/google/chrome-unstable/chrome --type=zygote
> /opt/google/chrome-unstable/chrome --type=gpu-process
> /opt/google/chrome-unstable/chrome --type=gpu-broker
> /opt/google/chrome-unstable/chrome --type=renderer
> /opt/google/chrome-unstable/chrome --type=renderer

* Safari *
It's all over the place, but there's a distinct 'Safari' process as well as 'com.apple.WebKit.WebContent'.

There's plenty of others, a few:
 - SearchHelper
 - SafeBrowser.Service
 - ImageDecoder
 - WebKit.Networking
 - SafariServices
 - Safari.History

* IE *
'iexplore.exe' for the parent, something else for the content (I don't have windows running).

* Edge *
'microsoftedgecp.exe' for content, something else for the parent.

So to sum up:
 - Chrome does not distinguish parent and content processes
 - Safari does
 - IE does
 - Edge does
> So to sum up:
>  - Chrome does not distinguish parent and content processes
>  - Safari does
>  - IE does
>  - Edge does

IE doesn't afaict, every process is iexplore.exe.
(In reply to George Wright (:gw280) (:gwright) from comment #69)
> Here are the potential options we have available to us:
> 
> - Call everything "firefox.exe"
>  * Pro: far less breakage, as presumably anyone whitelisting
> "plugin-container.exe" are also whitelisting "firefox.exe"
>  * Pro: sandboxing prefers this (citation: jimm)

Jim: why?

>  * Con: users can't distinguish easily between chrome and content processes
> in task manager

This is not great but I guess for chrome at least is valid prior art
I like options 1) or 3) - preferably 1) because it does at least somewhat address the issue raised in this bug.
Flags: needinfo?(jgriffiths)
(In reply to Jim Mathies [:jimm] from comment #71)
> > So to sum up:
> >  - Chrome does not distinguish parent and content processes
> >  - Safari does
> >  - IE does
> >  - Edge does
> 
> IE doesn't afaict, every process is iexplore.exe.

Sorry you're right, there's a main 'iexplore.exe' and the child processes tack on '/prefetch:2' to the command line.

It seems like naming the content process 'firefox.exe' would make sense, but could we keep flash and friends in 'plugin-container.exe'?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla48 → ---
Ted, I'm by no means an expert in this area of code. Do you know how involved it would be to merge the functionality in plugin-container.exe with firefox.exe, should we decide to go down the "firefox.exe for everything" route? My gut tells me it'll be fairly involved, but I'm not familiar enough with the code to make that call.
Flags: needinfo?(ted)
Ted reckons Bill might be a better person to answer this?
Flags: needinfo?(ted) → needinfo?(wmccloskey)
bsmedberg is probably a better person to answer this. Naively it seems like we could make the first command line argument for the content process be something special (-webcontent or something) and then have the main function test that immediately. However, there's probably all sorts of complexity there. I'm not sure how it would affect Android or B2G for example. We would probably need to keep plugin-container around for those platforms since I think FF desktop uses its own main function.

Looking at the code, we seem to build plugin-container as both a separate executable as well as a library. I don't understand why that is.
Flags: needinfo?(wmccloskey) → needinfo?(benjamin)
I believe that using firefox.exe for everything should be fairly simple technically, but I don't share your confidence that it won't break some existing things including Primetime. I do think we should go ahead and try it for content processes, whether or not we decide to do that for plugin processes.

Because of nuwa, I think FirefoxOS already uses the main binary for everything. But adding flag checks to the beginning of do_main in nsBrowserApp.cpp that then shell directly to content_process_main which is already linked to libxul for nuwa shouldn't be too hard.
Flags: needinfo?(benjamin)
I think we need a decision here. My opinion is that the risk of breaking stuff (especially given Benjamin's lack of confidence that using firefox.exe for everything won't break things) is too high for the (imho) marginal benefit of making our processes in task manager look nicer.

Re-nomming for triage based on this uncertainty.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #79)
> Because of nuwa, I think FirefoxOS already uses the main binary for everything.

For reference that's bug 977026; before that Nuwa (bug 771765) was a plugin-container that forked to create content processes.  And B2G used to still fork/exec plugin-container for content processes under some circumstances, but bug 1155547 might have fixed that?  Also, disabling MOZ_B2G_LOADER might still be supported.
Barbara, we need a decision here. So the gist of it is that at the moment, we have "firefox.exe" for the parent process and "plugin-container.exe" for the content and plugin processes. 

We'd like to make these names more informative, and originally tried to land a change where we used "firefox.exe", "firefox-webcontent.exe" and "firefox-plugin-container.exe". Turns out that broke flash (as it makes sure its host process is called "plugin-container.exe", Silverlight on Mac (change got backed out before I fully figured out what was going on here) and certain AMD GPU drivers (due to the firefox-webcontent.exe name change... issues were large memory leaks and horrible artifacting when scrolling).

At the moment we think our best option is to use the name "firefox.exe" for both parent and content processes, and stick with "plugin-container.exe" for plugin processes. However, we don't know what else will break if we do this. I consider it relatively safe, but after the experiences last week I wouldn't be surprised if there's a tail of issues we don't know about.

I don't know logistically what our options are. I'd prefer us not to block release on this, as I don't feel we'll have enough time with it on a testing population to get a good idea of the breakage it might or might not cause. As part of this I'd like to start the process of outreaching to third parties now to give them a heads up that we're changing the process names and to make sure their stuff isn't broken. That being said, I don't know if it's realistic for us to try and make this change after the initial e10s rollout.
Flags: needinfo?(bbermes)
Thanks for the recap of this.

Bare with me if I ask many questions at the beginning.

Here my two cents & understanding.

- If we don't change it, leave it as is, we are certain we don't break anything. Disadvantage is "just" an aesthetic concern, which comparing this to the risk of breaking and getting in touch with third parties seems not really worth the hassle at the moment. 

- Did the icon make it in (https://bugzilla.mozilla.org/attachment.cgi?id=8668971)

- George, can you share more concerns if we end up adjusting this after the initial e10s rollout?

- Also how is our naming for non-e10s? (parent/content and plugin)

- When/how often do people see this name?
Flags: needinfo?(bbermes) → needinfo?(gwright)
- Yes, the impact if we don't change it is just that it appears different in task manager and possibly volume control. The volume control issue is not fixed (it still shows up with the wrong icon and as "Plugin Container for Nightly"), but the name that it appears as in Volume Control is not set by the executable name but instead the metadata on the executable, which we can change without changing the exe name. However, simply changing the name will be slightly difficult as we use it for both content and plugins, so we'd need to choose something that makes sense for both types of processes (Or just simply "Firefox").

- Benjamin brought up that we might not be able to change this as easily after the initial e10s rollout in our e10s meeting today (although I may have misunderstood him?). My understanding is that once we release, third parties may ship their stuff based on the assumption that our process naming model won't ever change and so "lock" us in to the names we choose for initial rollout. Benjamin, can you elaborate on that?

- Naming in non-e10s is that we use firefox.exe for the main browser process and plugin-container.exe for the plugin process. We don't really have a content process except for thumbnail generation, which would use plugin-container.exe.

- People would see this name in Task Manager and some other system configuration places like AMD's GPU driver advanced configuration (e.g. https://bug1261620.bmoattachments.org/attachment.cgi?id=8737529). I can't think offhand where else the process name itself would show up as I think user-facing places like Volume Control would use the name given in the metadata rather than the executable name.
Flags: needinfo?(gwright) → needinfo?(benjamin)
(In reply to George Wright (:gw280) (:gwright) from comment #84)
> - Yes, the impact if we don't change it is just that it appears different in
> task manager and possibly volume control. The volume control issue is not
> fixed (it still shows up with the wrong icon and as "Plugin Container for
> Nightly"), but the name that it appears as in Volume Control is not set by
> the executable name but instead the metadata on the executable, which we can
> change without changing the exe name. However, simply changing the name will
> be slightly difficult as we use it for both content and plugins, so we'd
> need to choose something that makes sense for both types of processes (Or
> just simply "Firefox").

This should be fixed. (bug 1041599)
Thanks George for the details.

(In reply to George Wright (:gw280) (:gwright) from comment #84)

> - Benjamin brought up that we might not be able to change this as easily
> after the initial e10s rollout in our e10s meeting today (although I may
> have misunderstood him?). My understanding is that once we release, third
> parties may ship their stuff based on the assumption that our process naming
> model won't ever change and so "lock" us in to the names we choose for
> initial rollout. Benjamin, can you elaborate on that?
Ok, so let's not try to change it after, makes sense and rather come up with a solution now.

> 
> - Naming in non-e10s is that we use firefox.exe for the main browser process
> and plugin-container.exe for the plugin process. We don't really have a
> content process except for thumbnail generation, which would use
> plugin-container.exe.

So if we go by your suggestion in comment 82 (below), we should not only have parity/status quo what we currently have in non-e10s but also more descriptive language. The idea of keeping it similar to non-e10s makes me think we probably won't run into many compatibility issues with third parties. So I vote for this.

(In reply to George Wright (:gw280) (:gwright) from comment #82)
> At the moment we think our best option is to use the name "firefox.exe" for
> both parent and content processes, and stick with "plugin-container.exe" for
> plugin processes. However, we don't know what else will break if we do this.
> I consider it relatively safe, but after the experiences last week I
> wouldn't be surprised if there's a tail of issues we don't know about.
Flags: needinfo?(jmathies)
Flags: needinfo?(gwright)
OK, so it sounds like we're going to try and get the solution of using firefox.exe for everything except plugins before e10s rollout.
Flags: needinfo?(gwright)
I agree, switching to firefox.exe for content is preferred over the continued use of plugin-container for child processes. It sucks for debugging since you can't tell which is which, but we already have that problem with multiple plugin-containers, so no loss.

We can work on moving away from plugin-container for plugins and gmp at our leisure.
Flags: needinfo?(jmathies)
As I mentioned in the e10s meeting, if we are going to use firefox.exe for the child content process, we'll need to change firefox.exe to statically link to the Windows chromium sandbox code.

I've been looking into this and I thought we were going to have real problems, but it turns out that now we're using VS2015 they probably disappear. See bug 1035125 comment 2 for details.

gw280 - are you working on using firefox.exe instead of the rename now?
In my WIP patches I'm initialising the broker services towards the end of nsBrowserApp.cpp do_main just before we call XRE_main.
So, given bsmedberg's plan from comment 79, the sandbox initialisation should just work ... content_process_main initialises the chromium sandbox target services needed in the child.
Depends on: 1035125
Flags: needinfo?(gwright)
I believe that it's likely that people will continue doing weird things based on the process name, so doing this now is the best and perhaps only way to do this.
Flags: needinfo?(benjamin)
Since this is about making the processes intelligible why stop at a static filename. Why not work in additional information into the process name. Domain name, plugin name or such.
(In reply to avada from comment #91)
> Since this is about making the processes intelligible why stop at a static
> filename. Why not work in additional information into the process name.
> Domain name, plugin name or such.

Because it's not possible to do this on Windows. Only the filename is displayed in the task manager.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #92)
> Because it's not possible to do this on Windows. Only the filename is
> displayed in the task manager.
Every process is displayed with a description (Description column). firefox.exe says Firefox.
(In reply to Jorg K (GMT+2) from comment #93)
> Every process is displayed with a description (Description column).
> firefox.exe says Firefox.

Yes, that is taken from the version information embedded in the binary. We generate ours with a script at build time, but it's embedded into the binary and can't be changed at runtime:
https://dxr.mozilla.org/mozilla-central/source/config/version_win.pl?from=version_win.pl#357
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #92)
> Because it's not possible to do this on Windows. Only the filename is
> displayed in the task manager.

An exe file can be created on the fly though.
(In reply to avada from comment #95)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #92)
> > Because it's not possible to do this on Windows. Only the filename is
> > displayed in the task manager.
> 
> An exe file can be created on the fly though.

How to sign the binary on the fly? Having a private key?
Also, I bet it will break even more software expecting static filenames/descriptions/whatever.
I don't think this is a productive avenue for discussion, honestly.
Here's my WIP patch. It doesn't currently build unless you have sandboxing disabled. It works on Linux but crashes on Windows (some issues with DLL loading, looking into it atm).

Bob, mostly posting this so you can keep it on your radar.
Flags: needinfo?(gwright) → needinfo?(bobowen.code)
Comment on attachment 8744482 [details] [diff] [review]
0001-Use-firefox-for-child-processes-instead-of-plugin-co.patch

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

With a couple of changes I got this working with all processes running with firefox.exe and sandboxed (with my patch from bug 1035125).
The GMP process was only openh264, so EME ones might cause more issues.

I'll upload the patch for reference.

::: browser/app/nsBrowserApp.cpp
@@ +337,5 @@
>  
> +  // We are launching as a content process, delegate to the appropriate
> +  // main
> +  if (argc > 1 && IsArg(argv[1], "contentproc")) {
> +    return content_process_main(argc, argv);

With this here, on Windows we initialise the DLLBlocklist twice causing a loop, when we load the first DLL afterwards.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +186,4 @@
>  
>    exePath = exePath.AppendASCII(processName);
>  #else
> +  exePath = exePath.AppendASCII(MOZ_APP_NAME);

This only gives a binary name of firefox, on Windows it needs to be firefox.exe.

Also, are we now thinking of using the Firefox binary for all children?
Just hard-coded the exe name.
Called content_process_main at the top of main, after initialising the XRE functions.
Added in xul to the libs.
Added the GMPLoader to the sources.
Changed check to mProcessType != GeckoProcessType_Default - (if we were sticking with this -childproc would make more sense)
Flags: needinfo?(bobowen.code)
Latest iteration of my patch.

I'm hitting some unpleasantness with libxul. Basically, libxul is currently linked into the content process binary at compile time, but it isn't linked into the firefox binary. There is a lot of machinery in nsXPCOMGlue.cpp to dlopen the XUL library and dlsym various functions, which leads me to believe that there is a good reason that libXUL isn't linked into firefox at compile time.

This breaks sandboxing, because e.g. on Linux, plugin-container.cpp (which houses the content process's main()) #includes SandboxInfo.h which references a static that is in libxul and so when building firefox that symbol isn't resolved at compile time:

 0:18.65 /home/george/dev/gecko/obj-linux-clang-rel/dist/include/mozilla/SandboxInfo.h:46: error: undefined reference to 'mozilla::SandboxInfo::sSingleton'

There are similar issues on Windows.

Linking libxul directly into Firefox works on Windows but not Linux:

./obj-linux-clang-rel/dist/bin/firefox: error while loading shared libraries: libxul.so: cannot open shared object file: No such file or directory

And I haven't investigated further because it doesn't seem like the correct approach here.

Benjamin, are you aware of the reasoning behind why our libxul is set up the way it is?
Attachment #8744482 - Attachment is obsolete: true
Attachment #8744884 - Attachment is obsolete: true
Flags: needinfo?(benjamin)
Attachment #8745103 - Flags: feedback?(bobowen.code)
About Linux sandboxing, quoting from my build config rationale infodump in bug 1229136:
> The SandboxInfo module needs to be accessible to Gecko code in both parent and child, and to the sandbox code; also, it has state, and it would be good to avoid duplicating it and running its initializer twice.  So it's in the leafmost object that needs it: libmozsandbox on B2G and libxul on desktop.

It could also be in the executable and accessed by libxul via weak symbols, I think — that's already being done to deal with having the main sandboxing code in plugin-container (on desktop) but started via a call from libxul.
Comment on attachment 8745103 [details] [diff] [review]
0001-Use-firefox-for-child-processes-instead-of-plugin-co.patch

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

Got this working with a couple of changes and the patch from bug 1035125.

Should have the patches for that bug up for review soon.

::: browser/app/nsBrowserApp.cpp
@@ +311,5 @@
> +    rv = XPCOMGlueLoadXULFunctions(kXULFuncs);
> +    if (NS_FAILED(rv)) {
> +      Output("Couldn't load XRE functions.\n");
> +      return 255;
> +    }

I was just trying to get something up and running, but it seems like this initialisation code could be de-duped.

@@ +353,4 @@
>      return 255;
>    }
>  
> +

nit: extra line

::: ipc/contentproc/plugin-container.cpp
@@ +15,1 @@
>  #ifdef XP_WIN

nit: These could be switched with an #else.

@@ +225,4 @@
>      }
>  #endif
>      nsAutoPtr<mozilla::gmp::GMPLoader> loader;
> +#if !defined(MOZ_WIDGET_ANDROID) && !defined(MOZ_WIDGET_GONK) && defined(MOZ_CHILD_PROCESS)

I can't find a MOZ_CHILD_PROCESS, maybe change this to MOZ_PLUGIN_CONTAINER and add the define into ipc/app/moz.build.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +195,5 @@
> +    exePath = exePath.AppendASCII(MOZ_CHILD_PROCESS_NAME);
> +  } else {
> +    exePath = exePath.AppendASCII(MOZ_APP_NAME);
> +#ifdef OS_WIN
> +    exePath.AppendASCII(".exe");

It would be nice if we had something that included the .exe when needed like MOZ_CHILD_PROCESS_NAME.

Also, this gives firefox\.exe (and it would need to be |exePath = | because of the weird way FilePath works).

This works:
    exePath = exePath.ReplaceExtension(L"exe");
Attachment #8745103 - Flags: feedback?(bobowen.code) → feedback+
(In reply to George Wright (:gw280) (:gwright) from comment #101)
> Benjamin, are you aware of the reasoning behind why our libxul is set up the
> way it is?

It looks like we're not giving firefox an rpath directive (which could be executable-path-relative, it looks like) to let it find the libraries if it dynamically links against them.  That would explain why it has to dlopen() libxul — but what I don't understand yet is why we're not using the rpath mechanism.
I don't remember why we weren't using rpath initially, but nowadays, we have good reasons to dlopen() libxul: we can preload it this way.
I spent the day wrestling with the Linux sandbox and this is as far as I can tell the best way to deal with the link dependency insanity that's detailed in bug 1229136.

Bob, I've incorporated your feedback, but I expect this won't build on Windows without some fixes. Shouldn't be too much to make it work, but it's 1am here and I'm done with this for today.

My next step is to work out why libicu is failing to initialise on OS X.
Attachment #8745103 - Attachment is obsolete: true
Attachment #8745868 - Flags: feedback?(jld)
Attachment #8745868 - Flags: feedback?(bobowen.code)
Comment on attachment 8745868 [details] [diff] [review]
0001-Use-firefox-for-child-processes-instead-of-plugin-co.patch

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

Drive-by feedback, from a quick glance.

::: browser/app/nsBrowserApp.cpp
@@ -36,5 @@
>  // support Windows XP SP2 in ASAN builds.
>  #define XRE_DONT_SUPPORT_XPSP2
>  #endif
>  #define XRE_WANT_ENVIRON
> -#include "nsWindowsWMain.cpp"

This is not going to fly. nsWindowsWMain.cpp does important setup steps.

::: old-configure.in
@@ +6897,4 @@
>    MOZ_APP_MAXVERSION=$MOZ_APP_VERSION
>  fi
>  
> +MOZ_APP_PROCESS_NAME="${MOZ_APP_NAME}${BIN_SUFFIX}"

There are plenty of people who rename the executable (especially in enterprise setups). You shouldn't rely on a hardcoded name. We have ways to get the executable name, although they rely on having access to argv[0] (XRE_GetBinaryPath), but that shouldn't be too much of a problem. If you can't pass it down somehow, worst case, you can do something like https://dxr.mozilla.org/mozilla-central/source/ipc/glue/ScopedXREEmbed.cpp#48-54

::: security/sandbox/chromium/base/time/time.cc
@@ +225,4 @@
>    if (time_string[0] == '\0')
>      return false;
>  
> +#if 0

It would be better to remove the method entirely, instead of relying on the fact that nothing is invoking it in practice, because the day some imported code tries to use it, you'd end up with a build failure instead of time string parsing not working for some reason, the former being better.

::: security/sandbox/linux/common/SandboxInfo.h
@@ +67,4 @@
>  private:
>    enum Flags mFlags;
>    // This should be const, but has to allow for ThreadingCheck.
> +  static MOZ_EXPORT __attribute__((weak)) SandboxInfo sSingleton;

You probably should use MFBT_DATA instead of MOZ_EXPORT __attribute__((weak)) here.
(In reply to Mike Hommey [:glandium] from comment #107)
> Drive-by feedback, from a quick glance.
> 
> ::: browser/app/nsBrowserApp.cpp
> @@ -36,5 @@
> >  // support Windows XP SP2 in ASAN builds.
> >  #define XRE_DONT_SUPPORT_XPSP2
> >  #endif
> >  #define XRE_WANT_ENVIRON
> > -#include "nsWindowsWMain.cpp"
> 
> This is not going to fly. nsWindowsWMain.cpp does important setup steps.

We get it from #include plugin-container.cpp a few lines down. I removed it here because we were double including it.

I'll sort out the rest.
I think historically we didn't use rpath because...it didn't exist? That's why run-mozilla.sh existed, anyway. We stopped linking firefox directly to libxul.so at some point because we wanted to preload the file, as glandium said, instead of letting the dynamic linker do it inefficiently.
For more complication, it looks like GCC has a bug where __attribute__((weak)) is ignored on C++ class statics, so that won't work as-is.

If we could revert the effect of bug 1101170 and move the sandboxing code back into libmozsandbox.so on desktop, and have firefox dynamically link against that (but not against libxul.so), then the weak symbol abuse goes away and all the symbol dependencies should nicely follow library dependencies (and we also lose a bunch of B2G ifdefs in the process, and we don't ship multiple copies of the chromium sandbox code, but those are “nice-to-have”s) and this should become a much easier problem.
Work in progress on simplifying the Linux sandbox build, because it seems that we can probably move it back into a shared library.  This appears to not break desktop or B2G, but I've only tested locally so far, not on Try.  (This should probably be its own bug if we decide to actually do this, but leaving it here for now.)

Not done and not sure if it's worthwhile: move the LinuxSandboxStarter out of plugin-container; failing that, at least leave a comment to warn that the actual code is now dynamically linked.

The next step is to see if firefox can be linked against libmozsandbox.so (with appropriate origin-relative rpath) to make the patches here work.
When I tried a couple of days ago to get firefox to link against mozsandbox.so, I got the an issue with the linker:

0:27.00 ../../build/unix/gold/ld: error: /home/george/dev/gecko/obj-linux-clang/toolkit/library/../xre/nsAppRunner.o: requires dynamic R_X86_64_PC32 reloc against '_ZN7mozilla11SandboxInfo14ThreadingCheckEv' which may overflow at runtime; recompile with -fPIC
That's typically a symbol visibility problem.
Comment on attachment 8745868 [details] [diff] [review]
0001-Use-firefox-for-child-processes-instead-of-plugin-co.patch

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

To summarize (and deal with the f?), it looks like we won't be needing the Linux changes in this version of patch.  I can get Linux Firefox to run content processes with the "firefox" executable by combining my patch from comment #111 with the previous attempt (attachment 8745103 [details] [diff] [review]) and this addition to browser/app/moz.build:

if CONFIG['MOZ_SANDBOX'] and CONFIG['OS_TARGET'] == 'Linux':
    USE_LIBS += [
        'mozsandbox',
    ]
    LDFLAGS += ['-Wl,-rpath,$ORIGIN']
Attachment #8745868 - Flags: feedback?(jld)
Comment on attachment 8746350 [details] [diff] [review]
bug1114647-disentangle-sandbox-wip0.diff

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

…except that this will effectively revert bug 1176099, as currently written, so don't do that.
Attachment #8746350 - Flags: review-
Here's an updated patch that is intended to be applied on top of Jed's patch (which I see he has r-d himself) and Bob's patches in bug 1035125.

I've verified this works on Windows, OS X and Linux, but on OS X it doesn't work if the sandbox is enabled (builds but no pages load).
Attachment #8745868 - Attachment is obsolete: true
Attachment #8745868 - Flags: feedback?(bobowen.code)
Attachment #8746928 - Flags: review?(mh+mozilla)
Comment on attachment 8746928 [details] [diff] [review]
0001-Use-firefox-for-child-processes-instead-of-plugin-co.patch

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

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +151,5 @@
> +      processType != GeckoProcessType_GMPlugin) {
> +#if defined(OS_WIN)
> +    exePath = FilePath(WideToUTF8(CommandLine::ForCurrentProcess()->program()));
> +#elif defined(OS_POSIX)
> +    exePath = FilePath(CommandLine::ForCurrentProcess()->argv()[0]);

As I mentioned on IRC, how will this work for xpcshell?
(In reply to George Wright (:gw280) (:gwright) from comment #116)
> I've verified this works on Windows, OS X and Linux, but on OS X it doesn't
> work if the sandbox is enabled (builds but no pages load).

I tried it on Mac (using ./mach run), and I didn't have sandboxing problems, but I do see the content process show up as a separate Dock icon from the parent.  This didn't happen before because plugin-container.app had LSUIElement set in its Info.plist.  I don't know offhand if OS X has a way to do something similar for individual processes, or if we'd need to continue using a separate app for content processes on Mac.
(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #118)
> (In reply to George Wright (:gw280) (:gwright) from comment #116)
> > I've verified this works on Windows, OS X and Linux, but on OS X it doesn't
> > work if the sandbox is enabled (builds but no pages load).
> 
> I tried it on Mac (using ./mach run), and I didn't have sandboxing problems,
> but I do see the content process show up as a separate Dock icon from the
> parent.  This didn't happen before because plugin-container.app had
> LSUIElement set in its Info.plist.  I don't know offhand if OS X has a way
> to do something similar for individual processes, or if we'd need to
> continue using a separate app for content processes on Mac.

It'd be worth checking out what Chrome does in this regard.
(In reply to George Wright (:gw280) (:gwright) from comment #119)
> It'd be worth checking out what Chrome does in this regard.

They run a helper process instead of the main chrome app:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/helper-Info.plist&q=LSUIElemen&sq=package:chromium&dr=C&l=25
Given that, and the fact that we can already rename processes on OS X (content processes display as "Nightly Web Content" on my Mac), maybe we should just leave OS X using the existing plugin-container binary for content processes?
I agree. Let's do that.
Updated patch that keeps the current behaviour for OS X.
Attachment #8746928 - Attachment is obsolete: true
Attachment #8746928 - Flags: review?(mh+mozilla)
Attachment #8748196 - Flags: review?(mh+mozilla)
(It's been agreed that I'll take the bug because I have more time for it and I know most of what's going on here.)  (Also, that needinfo looks to have been answered already.)
Assignee: gwright → jld
Flags: needinfo?(benjamin)
Attachment #8734094 - Attachment is obsolete: true
Attachment #8746350 - Attachment is obsolete: true
(In reply to Bob Owen (:bobowen) from comment #117)
> ::: ipc/glue/GeckoChildProcessHost.cpp
> @@ +151,5 @@
> > +      processType != GeckoProcessType_GMPlugin) {
> > +#if defined(OS_WIN)
> > +    exePath = FilePath(WideToUTF8(CommandLine::ForCurrentProcess()->program()));
> > +#elif defined(OS_POSIX)
> > +    exePath = FilePath(CommandLine::ForCurrentProcess()->argv()[0]);
> 
> As I mentioned on IRC, how will this work for xpcshell?

Maybe nsBrowserApp should “opt in” to being run with -contentproc?
(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #125)

> > > +    exePath = FilePath(WideToUTF8(CommandLine::ForCurrentProcess()->program()));
> > > +#elif defined(OS_POSIX)
> > > +    exePath = FilePath(CommandLine::ForCurrentProcess()->argv()[0]);
> > 
> > As I mentioned on IRC, how will this work for xpcshell?
> 
> Maybe nsBrowserApp should “opt in” to being run with -contentproc?

Possibly, or maybe the executable could pass through a content process executable name.
For firefox it would be the current executable, for xpcshell it would be firefox(.exe).
Can we switch to case sensitive naming of the files on Windows?
The most other dev do it now, too.

Also something like "Firefox-", or "FF-", or "Fx-" before "plugin-container.exe" would help the users to know where the progress belongs too and helps (new users) to find the process if they search for...
Also I think it would be nice to change "the logic" of naming the processes on Windows...

I have ATM just one of the new process from Avira open, but I will attach a screenshot of it...

Avira names e.g. "Avira.ServiceHost.exe"...

So naming "Firefox.Content.Plugin.exe" and "Firefox.Content.Web.exe" would be nice...
... or maybe "Firefox-Content-Plugin.exe" and "Firefox-Content-Web.exe".
...and a re-do of the description of the progresses under Windows would be also helpful...
(In reply to Johan C from comment #1)
> Bug 1011225 fixed this on Mac and also, as far as I can tell, introduced a
> welcomed changes to the name actual plug-in processes (e.g. "Nightly Plugin
> Content (Shockwave Flash)". 
> 
> What puzzled me is the decision to only make this change on OS X.

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #6)
> There is no way to programatically change a process's name or description
> under Windows (like there is on OSX); they're read directly from the .exe
> attributes.

Sorry, but seems there is a way!
When firefox.exe restarts it self after installing a pending update of itself (nightly), the process's name is after that empty.
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #20)
> Needinfo'ing RyanVM
> 
> Ryan: this change may be risky, we have theories that renaming the process
> name will interact with AV software. I'd like to see some QA on this change
> with popular AV software after it lands.

I can help!
Checking it with http://VirusTotal.com and send the files (if needed) to the AV Vendors...
(In reply to RaresB from comment #22)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #21)
> > Rares, what AV software does SV have available to it for testing?
> 
> Ryan, we have licenses for AVG & Kaspersky, but we could use a lot more
> other AV trials for testing on a short period of time.

There's a API for VirusTotal that can be used... So no local AV is needed!

https://en.wikipedia.org/wiki/VirusTotal

https://www.virustotal.com/en/documentation/
(In reply to George Wright (:gw280) (:gwright) from comment #41)
> Needinfoing Jeff to find a Panda AV contact for us.

Normally there is no contact at AV Vendors needed!
Just upload the files to the AV Labs of the Vendors to analyze...


(In reply to George Wright (:gw280) (:gwright) from comment #82)
> We'd like to make these names more informative, and originally tried to land
> a change where we used "firefox.exe", "firefox-webcontent.exe" and
> "firefox-plugin-container.exe". Turns out that broke flash (as it makes sure
> its host process is called "plugin-container.exe", Silverlight on Mac
> (change got backed out before I fully figured out what was going on here)
> and certain AMD GPU drivers (due to the firefox-webcontent.exe name
> change... issues were large memory leaks and horrible artifacting when
> scrolling).

> I don't know logistically what our options are. I'd prefer us not to block
> release on this, as I don't feel we'll have enough time with it on a testing
> population to get a good idea of the breakage it might or might not cause.
> As part of this I'd like to start the process of outreaching to third
> parties now to give them a heads up that we're changing the process names
> and to make sure their stuff isn't broken. That being said, I don't know if
> it's realistic for us to try and make this change after the initial e10s
> rollout.

(In reply to Barbara Bermes [:barbara] from comment #83)
> - If we don't change it, leave it as is, we are certain we don't break
> anything. Disadvantage is "just" an aesthetic concern, which comparing this
> to the risk of breaking and getting in touch with third parties seems not
> really worth the hassle at the moment. 

I think that third parties develop there software for FF...
...and so FF don't have to work like third party software want - third party software have to handle the names FF use!


> - Also how is our naming for non-e10s? (parent/content and plugin)

Think it should be the same!


> - When/how often do people see this name?

Geeks more times - noops less...


(In reply to Ted Mielczarek [:ted.mielczarek] (Vacation May 4th-11th) from comment #92)
> (In reply to avada from comment #91)
> > Since this is about making the processes intelligible why stop at a static
> > filename. Why not work in additional information into the process name.
> > Domain name, plugin name or such.
> Because it's not possible to do this on Windows. Only the filename is
> displayed in the task manager.

As in comment 135 mentioned, it seems to work some times... (even if not intended to do!)
I guess this came through the use of command line parameters for updates for the start of profiles?
So maybe the plugins can start the container with a command line parameter that delivers the process name that should be use...
(In reply to Tobias B. Besemer [:BesTo] (QA) from comment #138)
> (In reply to Barbara Bermes [:barbara] from comment #83)
> > - If we don't change it, leave it as is, we are certain we don't break
> > anything. Disadvantage is "just" an aesthetic concern, which comparing this
> > to the risk of breaking and getting in touch with third parties seems not
> > really worth the hassle at the moment. 
> 
> I think that third parties develop there software for FF...
> ...and so FF don't have to work like third party software want - third party
> software have to handle the names FF use!

This is great in theory, but in practice if we break third-party software we break the Firefox experience for our users, which is bad. We found specific issues that caused noticeable problems (recorded on this bug) when trying to change the filename, so it's not worth causing problems for our users for the minor cosmetic change this brings in the process manager.
(In reply to Ted Mielczarek [:ted.mielczarek] (Vacation May 5th-11th) from comment #139)
> We found specific
> issues that caused noticeable problems (recorded on this bug) when trying to
> change the filename, so it's not worth causing problems for our users for
> the minor cosmetic change this brings in the process manager.

I'd echo the comment 90 made: if we're not doing this now then the above means we're going to be stuck with it even more.

The issue isn't purely cosmetic: users who look into task manager are actively being mislead as to a) why there is a "plugin" running even though they should be disabled b) why there's only a single Firefox process. This will complicate our diagnosis/troubleshooting if e10s ships to beta/release.
The plan of record (AIUI) is to use firefox[.exe] for the chrome and content processes everywhere except OS X, where we'll continue using plugin-container (because we need something that doesn't produce a dock icon), and plugin-container for NPAPI plugins (while they continue to be supported) and GMP plugins. On Linux and OS X this is already mostly a non-issue since we are able to rename the process to something useful.
> (In reply to Barbara Bermes [:barbara] from comment #83)
> > - When/how often do people see this name?
> 
> Geeks more times - noops less...

(In reply to Gian-Carlo Pascutto [:gcp] from comment #140)
> (In reply to Ted Mielczarek [:ted.mielczarek] (Vacation May 5th-11th) from
> comment #139)
> > We found specific
> > issues that caused noticeable problems (recorded on this bug) when trying to
> > change the filename, so it's not worth causing problems for our users for
> > the minor cosmetic change this brings in the process manager.
> 
> I'd echo the comment 90 made: if we're not doing this now then the above
> means we're going to be stuck with it even more.
> 
> The issue isn't purely cosmetic: users who look into task manager are
> actively being mislead as to a) why there is a "plugin" running even though
> they should be disabled b) why there's only a single Firefox process. This
> will complicate our diagnosis/troubleshooting if e10s ships to beta/release.

Even if I echo myself all the time:
Mozilla spend so much time to do everything "perfect", rather to "not yet" release a feature then to ship it "not perfect"...
...but on everything that "only" affects Windows the statement is "not so tragical to let it as it is"!

(Yeah, Windows su*kZ! But this change nothing on the market share Windows have!)

This is related to e.g. the paths of TB, the missing feature to link file extensions to FF, double or not necessary paths in the user directory, case-sensitive naming of files, logical naming of tasks & apps, breakpad, ...

What should the users think about it ???

And: These are "bugs" that are high visible to geeks! So what should they think about it and how should they talk to new/normal users that ask what they think about FF ???
(In reply to Tobias B. Besemer [:BesTo] (QA) from comment #142)
> Even if I echo myself all the time:
> Mozilla spend so much time to do everything "perfect", rather to "not yet"
> release a feature then to ship it "not perfect"...
> ...but on everything that "only" affects Windows the statement is "not so
> tragical to let it as it is"!

This is far from a strive for perfection. Renaming the processes to firefox-webcontent and firefox-plugin-container absolutely broke the browsing experience in multiple ways. In particular, Adobe Flash was broken, Silverlight was broken, EME was broken, some AMD systems showed graphical issues which made the browser unusable.

We simply cannot ship a browser in that state.
(In reply to George Wright (:gw280) (:gwright) from comment #143)
> (In reply to Tobias B. Besemer [:BesTo] (QA) from comment #142)
> > Even if I echo myself all the time:
> > Mozilla spend so much time to do everything "perfect", rather to "not yet"
> > release a feature then to ship it "not perfect"...
> > ...but on everything that "only" affects Windows the statement is "not so
> > tragical to let it as it is"!
> 
> This is far from a strive for perfection. Renaming the processes to
> firefox-webcontent and firefox-plugin-container absolutely broke the
> browsing experience in multiple ways. In particular, Adobe Flash was broken,
> Silverlight was broken, EME was broken, some AMD systems showed graphical
> issues which made the browser unusable.
> 
> We simply cannot ship a browser in that state.

When there was the first releases of 64-bit, this things were all broken, too!
No need to don't do it!
Isn't it done yet, it will be maybe never done!

AFAIK, e10s is now optional, right?
How long will the third parties developers have time from a default activation of e10s in the nightlies till they need to have it fixed, because it get shipped to the public?

Guess every normal extension dev have much less time to get his extensions always updated that they work with the new releases...
If I look at extensions like NoScript, then the programmer should be much more stressed to keep his extension compatible then some big companies like MS, Adobe, AMD, etc. to fix there software to work with other filenames...
Btw.: I never need Silverlight, Flash is losing market share (God, thanks!) and is e.g. no more needed for YT & FB and I have no AMD GFX... So design the browser for some _third_party_ companies and ship it to >500M user because of some companies and just a part of the users (No all!) that can/will have problems ???
And something more:
MS is breaking with every release of Windows _a_lot_ of software!
No need for them to have any worries about it... ;-)
This discussion is off-topic. Please keep any posting in this bug focused on technical issues related to the bug.
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Attachment #8748565 - Attachment is obsolete: true
Attachment #8748566 - Attachment is obsolete: true
Attachment #8748571 - Attachment is obsolete: true
Attachment #8748572 - Attachment is obsolete: true
Attachment #8748573 - Attachment is obsolete: true
Attachment #8748565 - Attachment is obsolete: false
Attachment #8748565 - Attachment is obsolete: true
Attachment #8748566 - Attachment is obsolete: false
Attachment #8748571 - Attachment is obsolete: false
Attachment #8748572 - Attachment is obsolete: false
Attachment #8748573 - Attachment is obsolete: false
Attachment #8748566 - Attachment is obsolete: true
Attachment #8748571 - Attachment is obsolete: true
Attachment #8748572 - Attachment is obsolete: true
Attachment #8748573 - Attachment is obsolete: true
Comment on attachment 8748196 [details] [diff] [review]
0001-Use-firefox-for-child-processes-instead-of-plugin-co.patch

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

::: browser/app/moz.build
@@ +77,5 @@
>  for icon in ('firefox', 'document', 'newwindow', 'newtab', 'pbmode'):
>      DEFINES[icon.upper() + '_ICO'] = '"%s/dist/branding/%s.ico"' % (
>          TOPOBJDIR, icon)
> +
> +if CONFIG['MOZ_SANDBOX'] and CONFIG['OS_TARGET'] == 'Linux':

OS_ARCH instead of OS_TARGET.

::: browser/app/nsBrowserApp.cpp
@@ +302,5 @@
> +  // We are launching as a content process, delegate to the appropriate
> +  // main
> +  if (argc > 1 && IsArg(argv[1], "contentproc")) {
> +    nsIFile *xreDirectory;
> +    nsresult rv = InitXPCOMGlue(argv[0], &xreDirectory);

So we really want to skip everything that happens before InitXPCOMGlue in the normal path (most importantly, the DLL Blocklist initialization; not sure about all the telemetry stuff).

Also, since a content process is never going to start without a Firefox process being around, we should disable DLL preloading.

Note: Not sure whether this -contentproc option shouldn't be within a #ifdef.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +148,2 @@
>  {
> +#if !defined(MOZ_WIDGET_ANDROID) && !defined(MOZ_WIDGET_COCOA)

a (e.g.) MOZ_CONTENT_PROC_USES_APP_BINARY define would make things here and further below clearer, and more straightforward to change if we ever decide to make it happen on OSX too.
Attachment #8748196 - Flags: review?(mh+mozilla)
The part of InitXPCOMGlue that converts paths between UTF-8 and UTF-16 on Windows (for the nsIFile that's going to be ignored/leaked) is also a problem, because it causes refcount logging to happen (via ns*String) before the process type is set, so it tries to open the same log file as the parent, and fails, and crashes.  That's easy enough to fix.

The other problem I'm seeing in https://treeherder.mozilla.org/#/jobs?repo=try&revision=a866b1a16396 are all the clipboard-related failures.  They seem non-intermittent, and that makes sense as something where getting the content process initialization not quite right could break things... but I can't reproduce them locally.
(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #150)
> The other problem I'm seeing in
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a866b1a16396 are all
> the clipboard-related failures.  They seem non-intermittent, and that makes
> sense as something where getting the content process initialization not
> quite right could break things... but I can't reproduce them locally.

Some sort of missed initialization (say a missed call to OleInitialize) might cause this, but I don't see how that can happen with your changes. Building this up to see if I can repo locally.

This hasn't changed the parent side's normal startup flow at all right?
The clipboard failures are due to RelEng testing Win7 AWS VMs on Try at the moment (note that they all happen on machine names with "spot" in them). They're completely safe to ignore.
(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #150)
> The part of InitXPCOMGlue that converts paths between UTF-8 and UTF-16 on
> Windows (for the nsIFile that's going to be ignored/leaked) is also a
> problem, because it causes refcount logging to happen (via ns*String) before
> the process type is set, so it tries to open the same log file as the
> parent, and fails, and crashes.  That's easy enough to fix.

Getting the leak logging working was the main thing I had to fix (on top of an existing rename patch) in this patch:
https://hg.mozilla.org/try/rev/87a1cb6001bee4ffc989d701e5c7c5c1a5a3c203

This was while I was trying to make sure my changes in bug 1035125, actually did mean we could use firefox.exe for the child process on Windows and sandbox it.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #152)
> The clipboard failures are due to RelEng testing Win7 AWS VMs on Try at the
> moment (note that they all happen on machine names with "spot" in them).
> They're completely safe to ignore.

Woot! Thanks Ryan!

FWIW I did test a local build and wasn't able to reproduce. My push to try last night to confirm failed as well.
To be applied after attachment 8748196 [details] [diff] [review].  Fixes xpcshell tests (they'll continue to run plugin-container for out-of-process content), fixes refcount logging issues, and disables firefox-as-content on Mac and (unless --disable-sandbox) on Linux.  (This also means that --disable-sandbox is a convenient way to “preview” this feature on Linux.)

Try is closed at the moment, so I've only tested locally (including Linux with --disable-sandbox, which doesn't have any obvious problems besides that).
Attached patch Full patch (obsolete) — Splinter Review
(See previous comment.  Waiting until Try reopens....)
Comment on attachment 8754598 [details] [diff] [review]
Full patch

Am I going to need to do things to Talos to handle child processes sometimes being the `firefox` executable instead of `plugin-container`?  I notice the earlier patch needed that to deal with `firefox-webcontent`.
Attachment #8754598 - Flags: feedback?(jmaher)
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ce0e316c48f

Lots of "spot" clipboard errors, but it looks like each of them has an accompanying non-"spot" job that passed, this time.
Jed, do not worry about the win7 errors on t-w732-spot-* machines, those are expected to be failing right now.

As for talos, yes- we depend on process names a bit- the previous patch handled all the cases, so use that as an example.
(In reply to Joel Maher (:jmaher) from comment #159)
> As for talos, yes- we depend on process names a bit- the previous patch
> handled all the cases, so use that as an example.

There are two changes there.  The one to testing/talos/talos/config.py: I'm not seeing where that `child_process` key is read.  The one in testing/talos/talos/cmanager_win32.py looks more load-bearing, but it seems not quite usable as-is, because now child processes can be either firefox or plugin-container, and obviously not all firefox.exe processes are children.

But first, I wanted to find out what breaks so I can know if/when I've fixed it.  On Linux with --disable-sandbox with my patches (which runs content processes as `firefox`), ./mach talos-test seems to pass without making any changes to Talos.  But on Windows, it can't even create a child process when run as talos-test, even though it works fine when run normally (./mach run); I'm still trying to investigate that.

I could make Talos runs continue using plugin-container, but that's kind of sketchy — it's not impossible that differences in process startup could affect performance somehow, and for that matter it's not unheard of for drivers to special-case certain popular executable names.
(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #160)
> On Windows, it can't even create a child process when
> run as talos-test, even though it works fine when run normally (./mach run);
> I'm still trying to investigate that.

Somehow it winds up thinking its executable name is "firefox", not "firefox.exe", and that's not good enough for (at least) the sandbox's process-launcher.  I can reproduce this by running the executable directly, as "firefox", from cmd.exe (running it as "firefox" from the MinGW shell apparently fixes up the extension before we see it).
…and I can fix that by using GetModuleFileNameW instead of the CommandLine stuff.
Comment on attachment 8754598 [details] [diff] [review]
Full patch

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

not sure if I am useful for giving feedback on these changes- if you have changes to files in testing/* I am happy to give feedback.
Attachment #8754598 - Flags: feedback?(jmaher)
Comment on attachment 8754598 [details] [diff] [review]
Full patch

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

This generally looks OK, but I think we could make it nicer with some additional work in a follow-up (you've done enough work here, let's get this landed).

::: browser/app/nsBrowserApp.cpp
@@ +51,5 @@
>  #include "mozilla/Telemetry.h"
>  #include "mozilla/WindowsDllBlocklist.h"
>  
> +#if !defined(MOZ_WIDGET_COCOA) && !defined(MOZ_WIDGET_ANDROID) \
> +  && !(defined(XP_LINUX) && defined(MOZ_SANDBOX))

I think this would read more nicely as a whitelist of platforms where we *do* allow this.

::: ipc/glue/GeckoChildProcessHost.h
@@ +133,5 @@
>    // This gets called when actors get destroyed and will schedule the object
>    // for deletion when all actors have cleared their associations.
>    void DissociateActor();
>  
> +  static void EnableSameExecutableForContentProc() { sRunSelfAsContentProc = true; }

Is this a runtime toggle just to keep xpcshell working? Can you file a followup so we could fix this in a nicer way? Considering that all xpcshell does these days is a little initialization and calling into XRE_XPCShellMain:
https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/js/xpconnect/shell/xpcshell.cpp#62

we could probably just make the xpcshell test harness run `firefox -xpcshell` or something like that and hardcode this at build-time instead.
Attachment #8754598 - Flags: review?(ted) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c0be5910bf7 passed the e10s Talos tests without making any changes in testing/, so if that's sufficient then it's done.
Adjusting to fix the self-executable-name issue; carrying over r+.  I'm going to land this and see if anything breaks, and if so then we can fix whatever caused it to pass Try.
Attachment #8748196 - Attachment is obsolete: true
Attachment #8754597 - Attachment is obsolete: true
Attachment #8754598 - Attachment is obsolete: true
Attachment #8759807 - Flags: review+
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e03f2bc5a99
Use firefox for child processes instead of plugin-container. r=ted
https://hg.mozilla.org/mozilla-central/rev/4e03f2bc5a99
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Backed out of mozilla-aurora in https://hg.mozilla.org/releases/mozilla-aurora/rev/e70892847544

Many of the Windows test failures are sort of confusing looking, but the ones which manage to fail with a screenshot like http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-aurora/sha512/e6c82799ad15846069be97138c3a159f606b65676d111973e53354aa25eaf61f905866db46f47fdf978b48bc3ca315d3e1ce90d2ab112231e6267c73a99ecd12 say that this is why there's such total carnage on Windows in https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=cea65ca3d0bdd8325dc556f6bfde6277fc83fe6e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summarizing from IRC: this broke the non-sandboxed case by trying to set up sandbox target services unconditionally, instead of only in processes that are in fact sandbox targets.  So that's more or less fine on Nightly where Windows content sandboxing is enabled (and on Try, similarly), but the same code breaks everything when built as Aurora.
why would this fail in aurora vs trunk?
(In reply to Joel Maher (:jmaher) from comment #171)
> why would this fail in aurora vs trunk?

Because the content sandbox is only enabled on Nightly.

Additional patch arriving soon ...
I misled jld, because the link to a try push I gave him didn't include a hackier version of this.
As the sandbox code knew its process wasn't launched through the broker sandbox code, I also realised that we could get rid of the -sandbox command line flag, which is nice.

MozReview-Commit-ID: EpXy9LYXwQL
Attachment #8760652 - Flags: review?(aklotz)
Assignee: jld → bobowen.code
Status: REOPENED → ASSIGNED
Try push with sandbox enabled, just running e10s media tests to test plugin-container as well:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dfda50f684fa8ad2b4b022d9905131d2827333b

Another push with the content process sandbox disabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8c922a36f930980c56227195eb000e7b43d3da9
This is still on mc, so fixed.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: mozilla49 → mozilla50
Attachment #8760652 - Flags: review?(aklotz) → review+
(In reply to Bob Owen (:bobowen) from comment #174)
> Try push with sandbox enabled, just running e10s media tests to test
> plugin-container as well:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3dfda50f684fa8ad2b4b022d9905131d2827333b
> 
> Another push with the content process sandbox disabled:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b8c922a36f930980c56227195eb000e7b43d3da9

WEe still have some test failure issues here to sort out.
Depends on: 1278528
Comment on attachment 8760652 [details] [diff] [review]
Part 2: Don't try to initialize the sandbox TargetServices when we are not sandboxed

I'm going to land this on a bug 1278528.

The try failures with the sandboxed disabled are unrelated and I'll pick that up separately as well.
Attachment #8760652 - Attachment is obsolete: true
With this patch Talos is showing ~50% improvements in memory and processor time, on Win7 and WinXP:

https://treeherder.mozilla.org/perf.html#/alerts?id=1445

However we believe this is a regression - please verify that in talos we are measuring values for the process names that have changed, thanks!
Flags: needinfo?(jld)
(In reply to Robert Wood [:rwood] from comment #178)
> With this patch Talos is showing ~50% improvements in memory and processor
> time, on Win7 and WinXP:
> 
> https://treeherder.mozilla.org/perf.html#/alerts?id=1445
> 
> However we believe this is a regression - please verify that in talos we are
> measuring values for the process names that have changed, thanks!

Pretty sure this is follow up bug 1254087.
Flags: needinfo?(jld)
Depends on: 1278812
No longer blocks: 1261952
Depends on: 1261952
Blocks: 1333511
No longer blocks: 1333511
See Also: → 1529390
You need to log in before you can comment on or make changes to this bug.