1401790 - Remove ProcessArchitecture from IPC

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

IPC has some machinery around launching processes involving enum ProcessArchitecture.  As far as I can tell, this was used only on OS X and only for NPAPI plugins, to run plugins of a different architecture than the browser.  But we no longer build universal binaries as of bug 1339182 (release 54), and our lowest supported version of OS X supports x86_64 only, *and* the only plugin we now support is Flash, whose current version a universal binary that includes x86_64.

So this is effectively dead code, and we should be able remove it.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Bug 1401790 - Remove ProcessArchitecture from IPC.

This was used to support cross-architecture NPAPI plugins on OS X, but
we stopped supporting that in 54 (bug 1339182).

Review commit: https://reviewboard.mozilla.org/r/188550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188550/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8917597 [details]
Bug 1401790 - Remove ProcessArchitecture from IPC.

https://reviewboard.mozilla.org/r/188550/#review193810


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: ipc/glue/GeckoChildProcessHost.cpp:551
(Diff revision 1)
> -GeckoChildProcessHost::RunPerformAsyncLaunch(std::vector<std::string> aExtraOpts,
> +GeckoChildProcessHost::RunPerformAsyncLaunch(std::vector<std::string> aExtraOpts)
> -                                             base::ProcessArchitecture aArch)
>  {
>    InitializeChannel();
>  
> -  bool ok = PerformAsyncLaunch(aExtraOpts, aArch);
> +  bool ok = PerformAsyncLaunch(aExtraOpts);

Warning: Parameter 'aextraopts' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Some of the code I'm removing in this patch seems to be for filtering out plugins that don't have a common architecture with plugin-container (and the OS), and I'm not entirely sure that's safe to remove — we won't run 32-bit plugins (or non-Flash plugins, now), but I don't know what happens if there are old plugins lying around and I don't know if there's an easy way to test that.  (Besides landing it on Nightly and seeing who complains.)

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 8917597 [details]
Bug 1401790 - Remove ProcessArchitecture from IPC.

https://reviewboard.mozilla.org/r/188550/#review195436

Comment 5

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

Comment on attachment 8917597 [details]
Bug 1401790 - Remove ProcessArchitecture from IPC.

https://reviewboard.mozilla.org/r/188550/#review195708

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Static Analysis Bot [:clangbot] from comment #2)
> Warning: Parameter 'aextraopts' is passed by value and only copied once;
> consider moving it to avoid unnecessary copies [clang-tidy:
> performance-unnecessary-value-param]

This is bug 638102, which I've reopened.  I'd prefer not to add unrelated changes to this patch.

Comment 7

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c556a5757473 -d eb5138e5bd55: rebasing 428471:c556a5757473 "Bug 1401790 - Remove ProcessArchitecture from IPC. r=billm,jimm" (tip)
merging ipc/chromium/src/base/process_util_mac.mm
warning: conflicts while merging ipc/chromium/src/base/process_util_mac.mm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8917597 [details]
Bug 1401790 - Remove ProcessArchitecture from IPC.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188550/diff/1-2/

Comment 9

[Code Review Bot [:reviewbot]]

Comment on attachment 8917597 [details]
Bug 1401790 - Remove ProcessArchitecture from IPC.

https://reviewboard.mozilla.org/r/188550/#review196950


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: ipc/glue/GeckoChildProcessHost.cpp:551
(Diff revision 2)
> -GeckoChildProcessHost::RunPerformAsyncLaunch(std::vector<std::string> aExtraOpts,
> +GeckoChildProcessHost::RunPerformAsyncLaunch(std::vector<std::string> aExtraOpts)
> -                                             base::ProcessArchitecture aArch)
>  {
>    InitializeChannel();
>  
> -  bool ok = PerformAsyncLaunch(aExtraOpts, aArch);
> +  bool ok = PerformAsyncLaunch(aExtraOpts);

Warning: Parameter 'aextraopts' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f33ef94ef46c
Remove ProcessArchitecture from IPC. r=billm,jimm

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/f33ef94ef46c