Use sharedData rather than initialProcessData for extension metadata

RESOLVED FIXED in Firefox 63

Status

enhancement
P1
normal
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [overhead:10k])

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
initialProcessData has the unfortunate side-effect of sending an entire copy of all of its data to all content processes, and eagerly decoding it. For the extension framework, this means that we wind up loading an entire copy of all of our schema data, and of every extension's manifest and locale data, into every process, even if we'll never need it.

The sharedData helper allows us to store an encoded copy of that data in a shared memory region, and clone it into the current process only when we need it, which can be a significant savings. For screenshots alone, it saves about 15K on locale and manifest data per content process, plus the size we save on not copying schema data.
Comment hidden (mozreview-request)

Comment 2

10 months ago
mozreview-review
Comment on attachment 8987388 [details]
Bug 1470783: Migrate extensions framework to use sharedData for cross-process data.

https://reviewboard.mozilla.org/r/252622/#review259044


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


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


::: toolkit/components/extensions/ExtensionParent.jsm:156
(Diff revision 1)
>          for (let url of schemaURLs) {
>            promises.push(Schemas.load(url));
>          }
> -        return Promise.all(promises);
> +        return Promise.all(promises).then(() => {
> +          Schemas.broadcastSchemas();
> +        })

Error: Missing semicolon. [eslint: semi]

Updated

10 months ago
Priority: -- → P1

Comment 3

10 months ago
mozreview-review
Comment on attachment 8987388 [details]
Bug 1470783: Migrate extensions framework to use sharedData for cross-process data.

https://reviewboard.mozilla.org/r/252622/#review260848

Far simplified schemas handling, nice!

::: toolkit/components/extensions/Extension.jsm:1549
(Diff revision 1)
> -      principal: this.principal,
>        optionalPermissions: this.manifest.optional_permissions,
> +    };
> +  }
> +
> +  serializeExtended() {

What's the reason for separating this?

I understand these are only needed in the extension process, but other than `childModules`, they don't seem particularly big to make it worth the "savings".


Please either rename to something related to the purpose, or add a comment.

::: toolkit/components/extensions/Extension.jsm:1835
(Diff revision 1)
>      }
>  
> -    let data = Services.ppmm.initialProcessData;
> -    data["Extension:Extensions"] = data["Extension:Extensions"].filter(e => e.id !== this.id);
> +    activeExtensionIDs.delete(this.id);
> +    sharedData.set("extensions/activeIDs", activeExtensionIDs);
> +
> +    for (let key of this.sharedDataKeys) {

"sharedDataKeys" sounds wrong, and I'm not sure it's worth keeping track of keys instead of just always deleting all (5) of them.

::: toolkit/components/extensions/Schemas.jsm:3015
(Diff revision 1)
> +
>    // A separate map of schema JSON which should be available in all
>    // content processes.
>    contentSchemaJSON: new Map(),
>  
> +  privilegedSchemaJSON: new Map(),

Add or update the comment above please.

::: toolkit/components/extensions/Schemas.jsm:3083
(Diff revision 1)
>      if (this._rootSchema) {
>        throw new Error("Schema loaded after root schema populated");
>      }
>    },
>  
> +  broadcastSchemas() {

"setSharedSchemas" perhaps?

Or at least add an explicit `flush()` to make the code match the name.

::: toolkit/components/extensions/extension-process-script.js:34
(Diff revision 1)
>    getInnerWindowID,
>  } = ExtensionUtils;
>  
> +const {sharedData} = Services.cpmm;
> +
> +function getData(extension, key = "") {

nit: called more often with just the id

::: toolkit/components/extensions/extension-process-script.js:334
(Diff revision 1)
>          webAccessibleResources = extension.webAccessibleResources.map(host => new MatchGlob(host));
>        }
>  
> +      let {backgroundScripts} = extension;
> +      if (!backgroundScripts && WebExtensionPolicy.isExtensionProcess) {
> +        ({backgroundScripts} = getData(extension, "extendedData") || {});

ugh
Attachment #8987388 - Flags: review?(tomica) → review+
(Assignee)

Comment 4

10 months ago
mozreview-review-reply
Comment on attachment 8987388 [details]
Bug 1470783: Migrate extensions framework to use sharedData for cross-process data.

https://reviewboard.mozilla.org/r/252622/#review260848

> What's the reason for separating this?
> 
> I understand these are only needed in the extension process, but other than `childModules`, they don't seem particularly big to make it worth the "savings".
> 
> 
> Please either rename to something related to the purpose, or add a comment.

It saves about 2K per process for Screenshots. More for other extensions.

> "sharedDataKeys" sounds wrong, and I'm not sure it's worth keeping track of keys instead of just always deleting all (5) of them.

"just deleting all" of them requires knowing in advance what all of them are. It's much easier to just delete whichever keys we set.

> ugh

inorite?
Whiteboard: [overhead:10k]
(Assignee)

Comment 5

9 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8074c985095c9951171311dac840684b915a57f6
Bug 1470783: Migrate extensions framework to use sharedData for cross-process data. r=zombie
(Assignee)

Comment 7

9 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/71af1a8b7eadef8c377be4d8ae5b4402ea1013fc
Bug 1470783: Migrate extensions framework to use sharedData for cross-process data. r=zombie

Comment 8

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71af1a8b7ead
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Assignee)

Updated

7 months ago
Flags: needinfo?(kmaglione+bmo)

Comment 9

7 months ago
Hello, should manual testing be performed on this bug? if yes, please provide some info on how to validate this. Otherwise please set the qe-verify- flag. Thanks!
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

7 months ago
Flags: needinfo?(kmaglione+bmo) → qe-verify-
You need to log in before you can comment on or make changes to this bug.