Closed Bug 1401790 Opened 2 years ago Closed 2 years ago

Remove ProcessArchitecture from IPC

Categories

(Core :: IPC, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

Attachments

(1 file)

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.
Priority: -- → P3
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]
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 on attachment 8917597 [details]
Bug 1401790 - Remove ProcessArchitecture from IPC.

https://reviewboard.mozilla.org/r/188550/#review195436
Attachment #8917597 - Flags: review?(jmathies) → review+
Comment on attachment 8917597 [details]
Bug 1401790 - Remove ProcessArchitecture from IPC.

https://reviewboard.mozilla.org/r/188550/#review195708
Attachment #8917597 - Flags: review?(wmccloskey) → review+
(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.
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 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]
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f33ef94ef46c
Remove ProcessArchitecture from IPC. r=billm,jimm
https://hg.mozilla.org/mozilla-central/rev/f33ef94ef46c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.