1184115 - Services.ppmm is defined for child processes

Status: NEW

(Core :: DOM: Content Processes, defect, P5)

Description

[David Teller [:Yoric] - still alive but not very active]

Looking at my logs, I see code loaded with `loadProcessScript()` loaded twice in the content process, in distinct scopes. Weird behaviors ensues.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Can you post a testcase or something?

Comment 2

[David Teller [:Yoric] - still alive but not very active]

I'll try to minimize.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Ok, found the issue, false alert.

The script was double-loaded, but for other reasons. I was using `Services.ppmm == null` to find out whether we were in a child process, and apparently, `Services.ppmm` is never `null`.

Is it normal that each child has a Global Parent Process Message Manager?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley

Bug 1184115 - Fixing double-loading of PerformanceStats content script;r?mconley

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8635673 [details]
MozReview Request: Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley

Bug 1184115 - Fixing double-loading of PerformanceStats content script;r?mconley

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> Is it normal that each child has a Global Parent Process Message Manager?

That's a bug. We should fix it. It looks like this bug is for something else, so can you file something? Thanks for finding it!

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(bug has been repurposed already)

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8635673 [details]
MozReview Request: Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley

https://reviewboard.mozilla.org/r/13585/#review12305

I feel like we should also be patching the underlying issue - can you also supply a patch that returns undefined for ppmm if we're in the content process? We might also want to do something similar for the cpmm in the parent process.

::: toolkit/components/perfmonitoring/PerformanceStats.jsm:696
(Diff revision 2)
> -    if (this._initialized) {
> -      return this._loader;
> +    if (isContent) {
> +      return null;
>      }
> -    this._initialized = true;
> +    if (this._loader) {

if (isContent || this.\_loader)?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8635673 [details]
MozReview Request: Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley

Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8635673 [details]
MozReview Request: Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley

https://reviewboard.mozilla.org/r/13585/#review12415

The fix looks fine - but I have some suggestions for the test. See below. Thanks!

::: toolkit/modules/tests/browser/browser_Services.js:54
(Diff revision 3)
> +  yield ContentTask.spawn(newTab.linkedBrowser, null, frameScript);

I think we're missing out on some of the nice readability of ContentTask here. Instead of splitting it out into the frameScript and promiseContentResponse functions, we can probably just do:

```
info("Checking parent process");
Assert.ok(Services.ppmm, "Services.ppmm is defined in the parent process");

if (!gMultiProcessBrowser) {
  return;
}

info("Checking content");

let hasPPMM = yield ContentTask.spawn(browser, null, function*() {
  const Cu = Components.utils;
  let Services = Cu.import("resource://gre/modules/Services.jsm", {}).Services;
  
  return !!Services.ppmm;
});

Assert.ok(!hasPPMM, "Should not have Services.ppmm defined in content processes");

::: toolkit/modules/tests/browser/browser_Services.js:52
(Diff revision 3)
> +  let newTab = gBrowser.addTab();

Use BrowserTestUtils.withNewTab here - it'll take care of closing the tab for you when the test finishes.

Example:

```
yield BrowserTestUtils.withNewTab({
  gBrowser,
  url: "about:blank",
}, function*(browser) {
  // .. now do things with your new browser
});
```

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8635673 [details]
MozReview Request: Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley

Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8635673 [details]
MozReview Request: Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley

https://reviewboard.mozilla.org/r/13585/#review12429

Looks good! Just one minor suggestion below to combine your two ContentTasks.

Also, in the future, when you fix issues in a previous revision, I suggest clicking "Fixed" for each issue in the associated review - that way, I as a reviewer, know that you have addressed my comment.

Cheers!

::: toolkit/modules/tests/browser/browser_Services.js:17
(Diff revision 4)
> +    let hasPPMM = yield ContentTask.spawn(browser, null, function*() {
> +      const Cu = Components.utils;
> +      let Services = Cu.import("resource://gre/modules/Services.jsm", {}).Services;
> +      return !!Services.ppmm;
> +    });
> +
> +    let isContentProcess = yield ContentTask.spawn(browser, null, function*() {
> +      const Cu = Components.utils;
> +      let Services = Cu.import("resource://gre/modules/Services.jsm", {}).Services;
> +      return Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT;
> +    });

We can probably just do a single ContentTask for this, like:

```
let {hasPPMM, isContentProcess} = yield ContentTask.spawn(browser, null, function*() {
  const Cu = Components.utils;
  let Services = Cu.import("resource://gre/modules/Services.jsm", {}).Services;
  
  return {
    hasPPMM: !!Services.ppmm,
    isContentProcess: Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT,
  };
});

// ...
```

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8635673 [details]
MozReview Request: Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley

Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47a58f3fd47d

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/106e6bbffe44

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for test_experiment.js xpcshell failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=3895207&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/4207226366e9

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Apparently, test_experiments.js doesn't like when we call `Services.appinfo` too early. I assume that's because of the `createAppInfo` in the code. I don't have good ideas on how to work around this.

Any idea?

Comment 18

[Gregory Szorc [:gps]]

I don't have any good ideas.

Comment 19

[Jens Stutte [:jstutte]]

(In reply to Pulsebot from comment #15)

https://hg.mozilla.org/integration/fx-team/rev/106e6bbffe44

Someone could take this original patch and resubmit it to phabricator and then we could check if there are still problems with this.