1431022 - Make it more flexible for processes to load XPCOM modules and components

Status: RESOLVED WONTFIX

(Core :: XPCOM, enhancement)

Description

[Cervantes Yu [:cyu] [:cervantes]]

When the component manager loads modules and the components belonging to the modules, it checks the ProcessSelector of the modules/components. Currently we have 2 sets of ProcessSelectors:

* Process selectors for chrome and content processes. Modules/components are loaded by default unless specified otherwise.
- ANY_PROCESS (0x0): this is the default. If unspecified in module definition, it will have this value. The module/component will be loaded in both chrome and content processes.
- MAIN_PROCESS_ONLY: don't load in the content process.
- CONTENT_PROCESS_ONLY: don't load in the chrome process.

* Process selector for the GPU process. Acting as a process that provides specific "service" to the browser, it only loads a minimal set of XPCOM modules/components by using the ALLOW_IN_GPU_PROCESS process selector. It does so by making modules/components not loaded by default unless specified otherwise.

With process isolation work, we can expect more service processes to come. Some of the new processes will need to load minimal, but different set of XPCOM modules/components like the GPU process does. We'll need more flexible loading options.

I am proposing that we extend nsComponentManager and Module::ProcessSelector so that a process can load a basic, minimal set of XPCOM, plus other modules/components for that specific process. The GPU process will load the MINIMAL_XPCOM and ALLOW_IN_GPU_PROCESS set of XPCOM. Similarly, the socket process can load the set MINIMAL_XPCOM and ALLOW_IN_SOCKET_PROCESS set.

Also a new flag, INHERITABLE_IN_MODULE can be applied to the module selector so that the components belonging the module will inherit the setting of the module by default and we don't need to specify the selector, say ALLOW_IN_SOCKET_PROCESS, to many or all of the components in the module. This encourages well-modularized XPCOM.

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Proposed change

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8943183 [details] [diff] [review]
Proposed change

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

I glanced at the component manager bits, but didn't look too carefully at everything else.  I agree that we need something like this.

::: xpcom/components/Module.h
@@ +58,5 @@
> +     * the MINIMAL_XPCOM set.
> +     */
> +    ALLOW_IN_GPU_PROCESS = 0x8,
> +
> +    INHERITABLE_IN_MODULE = 0x20,

This needs some documentation about where and when this gets set.

@@ +71,5 @@
>      const nsCID* cid;
>      bool service;
>      GetFactoryProcPtr getFactoryProc;
>      ConstructorProcPtr constructorProc;
> +    unsigned int processSelector;

I think it would be better if this was more strongly typed somehow, maybe by using mozilla::EnumSet?  (We'd have to change the definitions of `enum ProcessSelector`, but I think that'd be OK.)

@@ +78,5 @@
>    struct ContractIDEntry
>    {
>      const char* contractid;
>      nsID const* cid;
> +    unsigned int processSelector;

Likewise here.

@@ +132,5 @@
>    /**
>     * Optional flags which control whether the module loads on a process-type
>     * basis.
>     */
> +  unsigned int selector;

Likewise here.

Comment 3

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Make it more flexible to load XPCOM modules/components

Change to the previous version: when calling NS_InitMinimalXPCOM(), the component manager will be in the "minimal" mode, where the component manager only loads modules/components that explicitly speficy MINIMAL_XPCOM or ALLOW_IN_GPU_PROCESS.

Comment 4

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8967435 [details] [diff] [review]
Make it more flexible to load XPCOM modules/components

Cancel r? since I still need to address the feedback comments.

Comment 5

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Make it more flexible to load XPCOM modules/components

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8967639 [details] [diff] [review]
Make it more flexible to load XPCOM modules/components

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

Thanks for picking this back up.  I'm not sure if this is the right way to approach things, some comments below.

::: xpcom/components/Module.h
@@ +49,5 @@
> +
> +    /**
> +     * By default, modules are not loaded in a process using minimal XPCOM
> +     * configuration, even if ANY_PROCESS is specified. This flag enables a
> +     * module in the such process as the GPU process.

"in the such process"?  I think this wants to say "in a process such as the GPU process"?  Or perhaps just "This flag enables a module when minimal XPCOM is being used."

@@ +51,5 @@
> +     * By default, modules are not loaded in a process using minimal XPCOM
> +     * configuration, even if ANY_PROCESS is specified. This flag enables a
> +     * module in the such process as the GPU process.
> +     */
> +    MINIMAL_XPCOM,

I'm not excited about having a `ProcessSelector` type with a member that's completely unrelated to what kind of processes we're using.  Maybe we should move this information out to a separate enum:

enum EssentialXPCOM {
  Required,
  Optional,
};

and give Module another member, `EssentialXPCOM essential` or something like that.  We could then do things like:

  MOZ_ASSERT_IF(module->essential == Required, selector == ANY_PROCESS);

WDYT?

@@ +64,5 @@
> +     * This value is used to load all CIDEntrys belonging to the module when
> +     * loading minimal XPCOM. It can only be specified to modules and is ignored
> +     * on CIDEntrys.
> +     */
> +    INHERITABLE_IN_MODULE,

This name seems a bit peculiar for what it does.  Maybe FORCE_LOAD_WHEN_MINIMAL?

It also looks like the value is not actually used in the current patch set.  So perhaps we should just remove it until it's needed?

::: xpcom/components/nsComponentManager.cpp
@@ +293,5 @@
> +
> +nsresult
> +nsComponentManagerImpl::InitMinimal()
> +{
> +  sMinimalXPCOM = true;

Is there a reason to have global state here?  Or could we do instead:

public:
  nsresult InitMinimal();
  nsresult Init();

private:
  enum class InitKind {
    Minimal,
    Maximal,
  };
  nsresult InitWithKind(InitKind);

I guess we'd have to thread that state through all the functions, which is annoying.  But I think that's slightly better than having global state?  Not sure.