Open Bug 1184115 Opened 4 years ago Updated 4 years ago

Services.ppmm is defined for child processes

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

People

(Reporter: Yoric, Unassigned)

Details

Attachments

(1 file)

Looking at my logs, I see code loaded with `loadProcessScript()` loaded twice in the content process, in distinct scopes. Weird behaviors ensues.
Can you post a testcase or something?
Flags: needinfo?(dteller)
I'll try to minimize.
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?
Flags: needinfo?(dteller)
Flags: needinfo?(wmccloskey)
Summary: loadProcessScript() loads code twice in the content process → Services.ppmm is defined for child processes
Bug 1184115 - Fixing double-loading of PerformanceStats content script;r?mconley
Attachment #8635673 - Flags: review?(mconley)
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
> 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!
Flags: needinfo?(wmccloskey)
(bug has been repurposed already)
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)?
Attachment #8635673 - Flags: review?(mconley) → review+
Attachment #8635673 - Attachment description: MozReview Request: Bug 1184115 - Fixing double-loading of PerformanceStats content script;r?mconley → MozReview Request: Bug 1184115 - Services.ppmm should not be defined for child services;r=mconley
Attachment #8635673 - Flags: review+ → review?(mconley)
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 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
});
```
Attachment #8635673 - Flags: review?(mconley)
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
Attachment #8635673 - Flags: review?(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 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
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?
Flags: needinfo?(gps)
I don't have any good ideas.
Flags: needinfo?(gps)
You need to log in before you can comment on or make changes to this bug.