Bob Owen (:bobowen) Assignee
	Description • 10 years ago

Currently, plugin-container.exe is linked against the sandbox_s library twice. Once directly and also via xul.dll, which links against sandboxbroker.dll, which links in sandbox_s.

This currently causes some issues, that have to be worked around, where different copies of the code are hit when called from different places.

We should look into resolving this so we only have the library linked in once.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧
	Comment 1 • 9 years ago

Move process sandboxing bugs to their new, separate component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)

	Bob Owen (:bobowen) Assignee
	Comment 2 • 8 years ago

This has become a bit more urgent as we're thinking of using firefox.exe for the content child process, which as I understand it means we need the chromium sandbox code linked into the exe for the memory patching to work.

After some code changes and moz.build file juggling, I was hitting [1] because the sandbox code uses malloc.

Then ensued a fairly lengthy disagreement with the linker/windows to try and persuade it to run this hooking code before loading msvcr120(d).dll.
Things were looking bad, particularly because dmajor had clearly already had serious words with the linker.

Then fortunately I did an hg pull and someone had broken the build for VS2013, so I discovered that VS2015 no longer needed this hack.

glandium - do you think we can just get rid of these checks (and in fact eventually all of this static linking) and just drop support for WinXP SP2 when built with VS2013?

[1] https://dxr.mozilla.org/mozilla-central/rev/1da1937a9e03154ae7c60089f2dcf5ad9ee20fa3/toolkit/xre/WindowsCrtPatch.h#126

	Mike Hommey [:glandium]
	Comment 3 • 8 years ago

That would be a sensible solution if we're sure we'll stick to VS2015... which is, unfortunately, not a given (see bug 1265615).

	Bob Owen (:bobowen) Assignee
	Comment 5 • 8 years ago

George - here's the current version of my patch.
It's not thoroughly tested and it looks like it currently causes a shutdown crash.

	Bob Owen (:bobowen) Assignee
	Comment 6 • 8 years ago

(In reply to Bob Owen (:bobowen) from comment #5)
> George - here's the current version of my patch.
> It's not thoroughly tested and it looks like it currently causes a shutdown
> crash.

This shutdown crash is because for the lazily constructed std::locale::facet the ctype<char> _Ctype._Table is getting allocated with _calloc_crt in _Getctype in _tolower.c.
But it is getting freed by jemalloc.

Not sure how to fix this yet.

	Bob Owen (:bobowen) Assignee
	Comment 7 • 8 years ago

Try push for this bug with a couple of patches on top that make the content process use firefox.exe:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b3a9b0245df72c6aba979e1306cd3f9c0bebd1f

... and another with the leak logging fixed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e710870b3ff32abbbadd56993684a442979e78a7

	Bob Owen (:bobowen) Assignee
	Comment 8 • 8 years ago

And another hopefully fixing the leaks that were uncovered by fixing the leak logging \o/:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=feb55551c567

	Bob Owen (:bobowen) Assignee
	Comment 9 • 8 years ago

Before I upload patches, here are the issues I had with mismatches between allocation code.

Firstly I hit a problem on Windows 10 when compiled with VS2015, because of a silently failing version check in the sandbox code causing a std::ostream to be created, which ends up hitting line 2222 of [1]:

		_Ctype = _Lobj._Getctype();

after going through line 117 of [2] this lands at line 134 of [3].

In the _Getctype function _calloc_crt is used to allocate, which uses the CRT's own allocation, but this memory gets freed at line 2215 of [1], which just uses free, so this goes to jemalloc and we crash.

This looks like a bug in VS2015 to me, but fortunately I could take a patch to the chromium sandbox to get rid of this.

[1] c:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xlocale
[2] c:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xlocinfo
[3] C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\crt\src\stl\_tolower.c

	Bob Owen (:bobowen) Assignee
	Comment 10 • 8 years ago

Secondly I hit a problem when trying to compile with VS2013, where during start-up memory is allocated for environment variables at line 165 of [1].
It seems that _malloc_crt is just defined as malloc, so this goes to jemalloc.
When it comes to allocate more memory it hits line 249 of [1], which uses _recalloc_crt, which gets defined as _recalloc and ends up in [2] line 53 (_recalloc_base also gets defined as _recalloc).
I couldn't find a way to change this with the CRT static linking, so I resorted to removing all the static linking, which I think we wanted to do at some point anyway.

[1] C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\crt\src\setenv.c
[2] C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\crt\src\recalloc.c

	Bob Owen (:bobowen) Assignee
	Comment 11 • 8 years ago

The original changeset that is being backed out had comment:
Bug 1023941 - Part 5: Loader hook to redirect the missing import.

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

	Bob Owen (:bobowen) Assignee
	Comment 12 • 8 years ago

The original changeset that is being backed out had comment:
Bug 1023941 - Part 4: Static-link the CRT into crashreporter.exe.

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

	Bob Owen (:bobowen) Assignee
	Comment 13 • 8 years ago

The original changeset that is being backed out had comment:
Bug 1023941 - Part 3: Static-link the CRT into plugin-hang-ui.exe.

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

	Bob Owen (:bobowen) Assignee
	Comment 14 • 8 years ago

The original changeset that is being backed out had comment:
Bug 1023941 - Part 2: Static-link the CRT into plugin-container.exe.

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

	Bob Owen (:bobowen) Assignee
	Comment 15 • 8 years ago

The original changeset that is being backed out had comment:
Bug 1023941 - Part 1: Static-link the CRT into firefox.exe.

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

	Bob Owen (:bobowen) Assignee
	Comment 16 • 8 years ago

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

	Bob Owen (:bobowen) Assignee
	Comment 17 • 8 years ago

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

	Bob Owen (:bobowen) Assignee
	Comment 18 • 8 years ago

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

	Bob Owen (:bobowen) Assignee
	Comment 19 • 8 years ago

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

	Bob Owen (:bobowen) Assignee
	Comment 20 • 8 years ago

Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

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

	Benjamin Smedberg
	Comment 21 • 8 years ago

Comment on attachment 8748662 [details]
MozReview Request: Bug 1035125 Part 1: Back out changeset 1910714b56c6 and associated subsequent changes. r?bsmedberg

https://reviewboard.mozilla.org/r/50451/#review47337

Please make this commit message much clearer: we can do this now because we're building with MSVC2015, and Firefox still runs on XPSP2 even without these changes.

	Mike Hommey [:glandium]
	Comment 22 • 8 years ago

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #21)
> Comment on attachment 8748662 [details]
> MozReview Request: Bug 1035125 Part 1: Back out changeset 1910714b56c6 and
> associated subsequent changes. r?bsmedberg
> 
> https://reviewboard.mozilla.org/r/50451/#review47337
> 
> Please make this commit message much clearer: we can do this now because
> we're building with MSVC2015, and Firefox still runs on XPSP2 even without
> these changes.

cf. comment 3, that may not stay this way.

	(No longer employed by Mozilla) Aaron Klotz
	Comment 23 • 8 years ago

Comment on attachment 8748667 [details]
MozReview Request: Bug 1035125 Part 6: Take Chromium commit 3181ba39ee787e1b40f4aea4be23f4f666ad0945 to add Windows 10 version to enumeration. r?aklotz

https://reviewboard.mozilla.org/r/50461/#review48141

rs=me

	(No longer employed by Mozilla) Aaron Klotz
	Comment 24 • 8 years ago

Comment on attachment 8748668 [details]
MozReview Request: Bug 1035125 Part 7: Remove unused functions in security/sandbox/chromium/base/time/time.h to avoid nspr dependency. r?aklotz

https://reviewboard.mozilla.org/r/50463/#review48143

	(No longer employed by Mozilla) Aaron Klotz
	Comment 25 • 8 years ago

Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50465/#review48149

	(No longer employed by Mozilla) Aaron Klotz
	Comment 26 • 8 years ago

Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50467/#review48151

Sandbox bits look okay...

	Mike Hommey [:glandium]
	Comment 27 • 8 years ago

Comment on attachment 8748663 [details]
MozReview Request: Bug 1035125 Part 2: Back out changeset 3c59642f6445 and associated subsequent changes. r?glandium

https://reviewboard.mozilla.org/r/50453/#review48561

	Mike Hommey [:glandium]
	Comment 28 • 8 years ago

Comment on attachment 8748664 [details]
MozReview Request: Bug 1035125 Part 3: Back out changeset fa15c3e929d0 and associated subsequent changes. r?glandium

https://reviewboard.mozilla.org/r/50455/#review48563

	Mike Hommey [:glandium]
	Comment 29 • 8 years ago

Comment on attachment 8748665 [details]
MozReview Request: Bug 1035125 Part 4: Back out changeset 8ae39d920f5c and associated subsequent changes. r?glandium

https://reviewboard.mozilla.org/r/50457/#review48565

::: dom/media/gmp/rlz/moz.build:23
(Diff revision 1)
>      UNIFIED_SOURCES += [
>          'win/lib/machine_id_win.cc',
>      ]
>  
>  if CONFIG['OS_TARGET'] == 'Darwin':
> +    USE_STATIC_LIBS = True

USE_STATIC_LIBS has no effect on platforms other than Windows (and even on Windows, it's only when building with MSVC). No need to keep it here.

::: ipc/app/moz.build:17
(Diff revision 1)
>          'MozillaRuntimeMainAndroid.cpp',
>      ]
>  
>      DIRS += ['pie']
>  else:
> -    kwargs = {
> +    GeckoProgram(CONFIG['MOZ_CHILD_PROCESS_NAME'], linkage=None)

I think you can change linkage to 'dependent' here and remove the USE_LIBS for nspr and xul, while you're here.

::: ipc/app/moz.build:41
(Diff revision 1)
> +if CONFIG['OS_TARGET'] == 'WINNT':
> +    DISABLE_STL_WRAPPING = True

why?

	Mike Hommey [:glandium]
	Comment 30 • 8 years ago

Comment on attachment 8748666 [details]
MozReview Request: Bug 1035125 Part 5: Back out changeset baa3f852133b and associated subsequent changes. r?glandium

https://reviewboard.mozilla.org/r/50459/#review48583

	Mike Hommey [:glandium]
	Comment 31 • 8 years ago

https://reviewboard.mozilla.org/r/50463/#review48585

You have a typo in the commit message.

	Mike Hommey [:glandium]
	Comment 32 • 8 years ago

Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50465/#review48587

::: ipc/contentproc/plugin-container.cpp:228
(Diff revision 1)
>  #if !defined(MOZ_WIDGET_ANDROID) && !defined(MOZ_WIDGET_GONK)
>      // On desktop, the GMPLoader lives in plugin-container, so that its
>      // code can be covered by an EME/GMP vendor's voucher.
>      nsAutoPtr<mozilla::gmp::SandboxStarter> starter(MakeSandboxStarter());
>      if (XRE_GetProcessType() == GeckoProcessType_GMPlugin) {
> -        loader = mozilla::gmp::CreateGMPLoader(starter);
> +        childData.gmpLoader.reset(mozilla::gmp::CreateGMPLoader(starter));

You should be able to use a "normal" assignment.

::: security/sandbox/win/SandboxInitialization.h:22
(Diff revision 1)
> +
> +static sandbox::TargetServices*
> +GetInitializedTargetServices()
> +{
> +  static sandbox::TargetServices* sInitializedTargetServices =
> +    []() -> sandbox::TargetServices* {

I'm not convinced using a lambda makes it particularly better than just using a separate function.

But why not make all this a static method of TargetServices ?

::: toolkit/xre/nsEmbedFunctions.cpp:317
(Diff revision 1)
>      MakeUnique<base::StatisticsRecorder>();
>  
>  #if !defined(MOZ_WIDGET_ANDROID) && !defined(MOZ_WIDGET_GONK)
>    // On non-Fennec Gecko, the GMPLoader code resides in plugin-container,
>    // and we must forward it through to the GMP code here.
> -  GMPProcessChild::SetGMPLoader(aGMPLoader);
> +  GMPProcessChild::SetGMPLoader(aChildData->gmpLoader.get());

Here and further below, you should test aChildData is not nullptr before dereferencing it.

::: xpcom/build/nsXREChildData.h:29
(Diff revision 1)
> +}
> +
> +/**
> + * Data needed to start a child process.
> + */
> +struct nsXREChildData

We should probably not add new classes with a "ns" prefix.

::: xpcom/build/nsXREChildData.h:40
(Diff revision 1)
> +
> +#if defined(XP_WIN) && defined(MOZ_SANDBOX)
> +  /**
> +   * Chromium sandbox TargetServices.
> +   */
> +  sandbox::TargetServices* sandboxTargetServices = nullptr;

Does this build on MSVC 2013?

	Mike Hommey [:glandium]
	Comment 33 • 8 years ago

Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50467/#review48593

::: browser/app/nsBrowserApp.cpp:187
(Diff revision 2)
>      for (int i = 1; i < argc; i++) {
>        argv[i] = argv[i + 1];
>      }
> -    return XRE_XPCShellMain(--argc, argv, envp);
> +
> +    nsXREShellData shellData;
> +#if defined(XP_WIN) && defined(MOZ_SANDBOX)

This seems like a lot of (hard to find) places to change if we ever end up needing that on other platforms (or any other kind of modification)...  how about having a MOZ_SANDBOX_BROKER define? (the same probably applies to plugin-container code)

::: js/xpconnect/src/XPCShellImpl.cpp:1532
(Diff revision 2)
>          // asynchronized.
>          AutoAudioSession audioSession;
>  
>  #if defined(MOZ_SANDBOX)
>          // Required for sandboxed child processes.
> -        if (!SandboxBroker::Initialize()) {
> +        if (aShellData->sandboxBrokerServices) {

null check on aShellData

::: security/sandbox/win/SandboxInitialization.h:43
(Diff revision 2)
>  
> +static sandbox::BrokerServices*
> +GetInitializedBrokerServices()
> +{
> +  static sandbox::BrokerServices* sInitializedBrokerServices =
> +    []() -> sandbox::BrokerServices* {

Same as the TargetServices comment.

::: xpcom/build/nsXREShellData.h:19
(Diff revision 2)
> +#endif
> +
> +/**
> + * Data needed by XRE_XPCShellMain.
> + */
> +struct nsXREShellData

Same comment as with nsXREAppData.

	Bob Owen (:bobowen) Assignee
	Comment 34 • 8 years ago

Comment on attachment 8748662 [details]
MozReview Request: Bug 1035125 Part 1: Back out changeset 1910714b56c6 and associated subsequent changes. r?bsmedberg

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

	Bob Owen (:bobowen) Assignee
	Comment 35 • 8 years ago

Comment on attachment 8748663 [details]
MozReview Request: Bug 1035125 Part 2: Back out changeset 3c59642f6445 and associated subsequent changes. r?glandium

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

	Bob Owen (:bobowen) Assignee
	Comment 36 • 8 years ago

Comment on attachment 8748664 [details]
MozReview Request: Bug 1035125 Part 3: Back out changeset fa15c3e929d0 and associated subsequent changes. r?glandium

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

	Bob Owen (:bobowen) Assignee
	Comment 37 • 8 years ago

Comment on attachment 8748665 [details]
MozReview Request: Bug 1035125 Part 4: Back out changeset 8ae39d920f5c and associated subsequent changes. r?glandium

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

	Bob Owen (:bobowen) Assignee
	Comment 38 • 8 years ago

Comment on attachment 8748666 [details]
MozReview Request: Bug 1035125 Part 5: Back out changeset baa3f852133b and associated subsequent changes. r?glandium

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

	Bob Owen (:bobowen) Assignee
	Comment 39 • 8 years ago

Comment on attachment 8748667 [details]
MozReview Request: Bug 1035125 Part 6: Take Chromium commit 3181ba39ee787e1b40f4aea4be23f4f666ad0945 to add Windows 10 version to enumeration. r?aklotz

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

	Bob Owen (:bobowen) Assignee
	Comment 40 • 8 years ago

Comment on attachment 8748668 [details]
MozReview Request: Bug 1035125 Part 7: Remove unused functions in security/sandbox/chromium/base/time/time.h to avoid nspr dependency. r?aklotz

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

	Bob Owen (:bobowen) Assignee
	Comment 41 • 8 years ago

Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

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

	Bob Owen (:bobowen) Assignee
	Comment 42 • 8 years ago

Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

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

	Bob Owen (:bobowen) Assignee
	Comment 43 • 8 years ago

https://reviewboard.mozilla.org/r/50451/#review47337

Added an explanation.

	Bob Owen (:bobowen) Assignee
	Comment 44 • 8 years ago

https://reviewboard.mozilla.org/r/50463/#review48585

Fixed thanks.

	Bob Owen (:bobowen) Assignee
	Comment 45 • 8 years ago

https://reviewboard.mozilla.org/r/50457/#review48565

Thanks for all the reviews.

> USE_STATIC_LIBS has no effect on platforms other than Windows (and even on Windows, it's only when building with MSVC). No need to keep it here.

Removed.

> I think you can change linkage to 'dependent' here and remove the USE_LIBS for nspr and xul, while you're here.

Seems to work, thanks.

> why?

I have no idea.
I can only guess that this was when I was trying to resolve allocation mismatch problems with firefox.exe and I put this in the wrong file by mistake, sorry.

	Bob Owen (:bobowen) Assignee
	Comment 46 • 8 years ago

https://reviewboard.mozilla.org/r/50465/#review48587

> You should be able to use a "normal" assignment.

This is now a UniquePtr, so I couldn't just assign it, but I've reworked CreateGMPLoader to return a UniquePtr.

> I'm not convinced using a lambda makes it particularly better than just using a separate function.
> 
> But why not make all this a static method of TargetServices ?

It was just because I was doing this in the header file, so if I added another function someone might call it by mistake.
TargetServices is part of the Chromium sandbox code, so we don't want to change that unless we have to.

I've put the implementation into a cpp file compiled into the sandbox library and used a separate function in there.
This actually makes things a bit better, because it's not affected by the IPC code headers.

> Here and further below, you should test aChildData is not nullptr before dereferencing it.

I addded a MOZ_ASSERT, hopefully this is OK.

> We should probably not add new classes with a "ns" prefix.

Removed.

> Does this build on MSVC 2013?

Yes member initializers were first supported in VS2013.

	Bob Owen (:bobowen) Assignee
	Comment 47 • 8 years ago

https://reviewboard.mozilla.org/r/50467/#review48593

> This seems like a lot of (hard to find) places to change if we ever end up needing that on other platforms (or any other kind of modification)...  how about having a MOZ_SANDBOX_BROKER define? (the same probably applies to plugin-container code)

I'm not quite sure what you're propsing here.
Is it to define a MOZ_SANDBOX_BROKER the same as MOZ_SANDBOX and use it in the broker related parts of  firefox.exe code?
What about the broker related code in xul?

Also to have something like MOZ_SANDBOX_TARGET for the target services stuff in plugin-container and presumably firefox once we start using that as the child.

We'd probably have to keep MOZ_SANDBOX for some common bits, so I'm not sure it would make things better overall.

> null check on aShellData

Added MOZ_ASSERT.

> Same as the TargetServices comment.

Fixed in same way.

> Same comment as with nsXREAppData.

ns dropped.

	Bob Owen (:bobowen) Assignee
	Comment 48 • 8 years ago

Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50467/diff/3-4/

	Mike Hommey [:glandium]
	Comment 49 • 8 years ago

Comment on attachment 8748665 [details]
MozReview Request: Bug 1035125 Part 4: Back out changeset 8ae39d920f5c and associated subsequent changes. r?glandium

https://reviewboard.mozilla.org/r/50457/#review49331

	Mike Hommey [:glandium]
	Comment 50 • 8 years ago

Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50465/#review49333

::: security/sandbox/win/SandboxInitialization.cpp:30
(Diff revision 2)
> +  }
> +
> +  return targetServices;
> +}
> +
> +/* static */

/* static */ usually documents static (class) methods, but here we're not talking about a static method, we're talking about a public namespaced method. So please remove this /* static */ and the one for LowerSandbox.

	Mike Hommey [:glandium]
	Comment 51 • 8 years ago

Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50467/#review49335

	Mike Hommey [:glandium]
	Comment 52 • 8 years ago

(In reply to Bob Owen (:bobowen) from comment #47)
> https://reviewboard.mozilla.org/r/50467/#review48593
> 
> > This seems like a lot of (hard to find) places to change if we ever end up needing that on other platforms (or any other kind of modification)...  how about having a MOZ_SANDBOX_BROKER define? (the same probably applies to plugin-container code)
> 
> I'm not quite sure what you're propsing here.
> Is it to define a MOZ_SANDBOX_BROKER the same as MOZ_SANDBOX and use it in
> the broker related parts of  firefox.exe code?
> What about the broker related code in xul?
> 
> Also to have something like MOZ_SANDBOX_TARGET for the target services stuff
> in plugin-container and presumably firefox once we start using that as the
> child.
> 
> We'd probably have to keep MOZ_SANDBOX for some common bits, so I'm not sure
> it would make things better overall.

The point I was trying to make is that you have a lot of #if defined(XP_WIN) && defined(MOZ_SANDBOX). If you ever need those bits to be enabled on, say, OSX, then you have to go through all of them and add || defined(XP_DARWIN) or something like that. If instead you had a single define that's "automatically" defined for the equivalent conditions, in, say, configure, then you'd only have to change configure to enable the code on another platform.

How you chose to split things between broker and target (or not to) is up to you.

	Bob Owen (:bobowen) Assignee
	Comment 53 • 8 years ago

Comment on attachment 8748662 [details]
MozReview Request: Bug 1035125 Part 1: Back out changeset 1910714b56c6 and associated subsequent changes. r?bsmedberg

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

	Bob Owen (:bobowen) Assignee
	Comment 54 • 8 years ago

Comment on attachment 8748663 [details]
MozReview Request: Bug 1035125 Part 2: Back out changeset 3c59642f6445 and associated subsequent changes. r?glandium

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

	Bob Owen (:bobowen) Assignee
	Comment 55 • 8 years ago

Comment on attachment 8748664 [details]
MozReview Request: Bug 1035125 Part 3: Back out changeset fa15c3e929d0 and associated subsequent changes. r?glandium

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

	Bob Owen (:bobowen) Assignee
	Comment 56 • 8 years ago

Comment on attachment 8748665 [details]
MozReview Request: Bug 1035125 Part 4: Back out changeset 8ae39d920f5c and associated subsequent changes. r?glandium

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

	Bob Owen (:bobowen) Assignee
	Comment 57 • 8 years ago

Comment on attachment 8748666 [details]
MozReview Request: Bug 1035125 Part 5: Back out changeset baa3f852133b and associated subsequent changes. r?glandium

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

	Bob Owen (:bobowen) Assignee
	Comment 58 • 8 years ago

Comment on attachment 8748667 [details]
MozReview Request: Bug 1035125 Part 6: Take Chromium commit 3181ba39ee787e1b40f4aea4be23f4f666ad0945 to add Windows 10 version to enumeration. r?aklotz

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

	Bob Owen (:bobowen) Assignee
	Comment 59 • 8 years ago

Comment on attachment 8748668 [details]
MozReview Request: Bug 1035125 Part 7: Remove unused functions in security/sandbox/chromium/base/time/time.h to avoid nspr dependency. r?aklotz

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

	Bob Owen (:bobowen) Assignee
	Comment 60 • 8 years ago

Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

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

	Bob Owen (:bobowen) Assignee
	Comment 61 • 8 years ago

Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50467/diff/4-5/

	Bob Owen (:bobowen) Assignee
	Comment 62 • 8 years ago

https://reviewboard.mozilla.org/r/50465/#review49333

> /* static */ usually documents static (class) methods, but here we're not talking about a static method, we're talking about a public namespaced method. So please remove this /* static */ and the one for LowerSandbox.

Damn, I copied the signature with static and converted it to a comment, before I'd removed it from the header.
Thanks, removed the one from part 9 as well.

	Bob Owen (:bobowen) Assignee
	Comment 63 • 8 years ago

https://reviewboard.mozilla.org/r/50467/#review48593

> I'm not quite sure what you're propsing here.
> Is it to define a MOZ_SANDBOX_BROKER the same as MOZ_SANDBOX and use it in the broker related parts of  firefox.exe code?
> What about the broker related code in xul?
> 
> Also to have something like MOZ_SANDBOX_TARGET for the target services stuff in plugin-container and presumably firefox once we start using that as the child.
> 
> We'd probably have to keep MOZ_SANDBOX for some common bits, so I'm not sure it would make things better overall.

Agreed on IRC to pick this up as part of future refactorings.

	Bob Owen (:bobowen) Assignee
	Comment 64 • 8 years ago

Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

Just thought that this patch has ended up with a few more GMP changes after review.

I don't think there's anything controversial, but requesting feedback so that you're aware of it.

	Bob Owen (:bobowen) Assignee
	Comment 65 • 8 years ago

Comment on attachment 8748662 [details]
MozReview Request: Bug 1035125 Part 1: Back out changeset 1910714b56c6 and associated subsequent changes. r?bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50451/diff/3-4/

	Bob Owen (:bobowen) Assignee
	Comment 66 • 8 years ago

Comment on attachment 8748663 [details]
MozReview Request: Bug 1035125 Part 2: Back out changeset 3c59642f6445 and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50453/diff/3-4/

	Bob Owen (:bobowen) Assignee
	Comment 67 • 8 years ago

Comment on attachment 8748664 [details]
MozReview Request: Bug 1035125 Part 3: Back out changeset fa15c3e929d0 and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50455/diff/3-4/

	Bob Owen (:bobowen) Assignee
	Comment 68 • 8 years ago

Comment on attachment 8748665 [details]
MozReview Request: Bug 1035125 Part 4: Back out changeset 8ae39d920f5c and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50457/diff/3-4/

	Bob Owen (:bobowen) Assignee
	Comment 69 • 8 years ago

Comment on attachment 8748666 [details]
MozReview Request: Bug 1035125 Part 5: Back out changeset baa3f852133b and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50459/diff/3-4/

	Bob Owen (:bobowen) Assignee
	Comment 70 • 8 years ago

Comment on attachment 8748667 [details]
MozReview Request: Bug 1035125 Part 6: Take Chromium commit 3181ba39ee787e1b40f4aea4be23f4f666ad0945 to add Windows 10 version to enumeration. r?aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50461/diff/3-4/

	Bob Owen (:bobowen) Assignee
	Comment 71 • 8 years ago

Comment on attachment 8748668 [details]
MozReview Request: Bug 1035125 Part 7: Remove unused functions in security/sandbox/chromium/base/time/time.h to avoid nspr dependency. r?aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50463/diff/3-4/

	Bob Owen (:bobowen) Assignee
	Comment 72 • 8 years ago

Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50465/diff/3-4/

	Bob Owen (:bobowen) Assignee
	Comment 73 • 8 years ago

Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50467/diff/5-6/

	Bob Owen (:bobowen) Assignee
	Comment 74 • 8 years ago

Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

Just thought that this patch has ended up with a few more GMP changes after review.

I don't think there's anything controversial, but requesting feedback so that you're aware of it.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 75 • 8 years ago

Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

Looks fine to me.

	Pulsebot
	Comment 76 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d5f6606674
https://hg.mozilla.org/integration/mozilla-inbound/rev/1978e4289d25
https://hg.mozilla.org/integration/mozilla-inbound/rev/74634eed179d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8de111c0df7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9b7b90c5f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/89c0989d9cf9
https://hg.mozilla.org/integration/mozilla-inbound/rev/477b991bf6fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/da96cd1bf70c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a416c55e6648

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 77 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f6d5f6606674
https://hg.mozilla.org/mozilla-central/rev/89c0989d9cf9
https://hg.mozilla.org/mozilla-central/rev/477b991bf6fa
https://hg.mozilla.org/mozilla-central/rev/da96cd1bf70c
https://hg.mozilla.org/mozilla-central/rev/a416c55e6648

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 78 • 8 years ago

The other four commits did get merged to m-c with the rest, for the record. Our merge marking tool just got confused by the "backout" bit in the commit messages:

https://hg.mozilla.org/mozilla-central/rev/1978e4289d25
https://hg.mozilla.org/mozilla-central/rev/74634eed179d
https://hg.mozilla.org/mozilla-central/rev/8de111c0df7c
https://hg.mozilla.org/mozilla-central/rev/3b9b7b90c5f4