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

RESOLVED WONTFIX

Status

()

enhancement
RESOLVED WONTFIX
a year ago
7 days ago

People

(Reporter: cyu, Assigned: cyu)

Tracking

(Blocks 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
(Assignee)

Updated

a year ago
See Also: → socket-proc
(Assignee)

Comment 1

a year ago
Posted patch Proposed change (obsolete) — Splinter Review
Attachment #8943183 - Flags: feedback?(nfroyd)
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.
Attachment #8943183 - Flags: feedback?(nfroyd) → feedback+
(Assignee)

Comment 3

a year ago
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.
Attachment #8943183 - Attachment is obsolete: true
Attachment #8967435 - Flags: review?(nfroyd)
(Assignee)

Comment 4

a year ago
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.
Attachment #8967435 - Flags: review?(nfroyd)
(Assignee)

Comment 5

a year ago
Attachment #8967435 - Attachment is obsolete: true
Attachment #8967639 - Flags: review?(nfroyd)
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.
Attachment #8967639 - Flags: review?(nfroyd)
Status: NEW → RESOLVED
Last Resolved: 7 days ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.