1470783 - Use sharedData rather than initialProcessData for extension metadata

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P1)

Description

[Kris Maglione [:kmag]]

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 1

[Kris Maglione [:kmag]]

Attachment: Bug 1470783: Migrate extensions framework to use sharedData for cross-process data.

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.

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

Comment 2

[Code Review Bot [:reviewbot]]

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]

Comment 3

[Tomislav Jovanovic :zombie]

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

Comment 4

[Kris Maglione [:kmag]]

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?

Comment 5

[Kris Maglione [:kmag]]

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

Comment 6

[Eliza Balazs [:ebalazs_]]

Backed out 10 changesets (bug 1470783, bug 1463587) for causing multiple leakcheck failures on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8074c985095c9951171311dac840684b915a57f6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=187729601

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187729601&repo=mozilla-inbound

Backout push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6085b77ada2767a77a1fb9fa0bd9032855cfad10&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-classifiedState=unclassified

Comment 7

[Kris Maglione [:kmag]]

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

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/71af1a8b7ead

Comment 9

[Vlad Jiman]

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!