Let PerformanceStats.jsm play nicer with process-per-tab

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Summary: Let PerformanceStats.jsm play nicer with tab-per-process → Let PerformanceStats.jsm play nicer with process-per-tab
Created attachment 8634665 [details]
MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley

Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r?mconley
Attachment #8634665 - Flags: review?(mconley)
Comment on attachment 8634665 [details]
MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley

https://reviewboard.mozilla.org/r/13429/#review12085

One suggestion, one question.

::: toolkit/components/perfmonitoring/tests/browser/browser_compartments.js:37
(Diff revision 1)
> +let pid = getpid();

Because of the mixture of scopes in this file, pid is getting overloaded and it gets a bit confusing.

Maybe: `const PARENT_PID = getpid();`?

::: toolkit/components/perfmonitoring/tests/browser/browser_compartments.js:50
(Diff revision 1)
> +      let sysName = Services.sysinfo.getPropertyAsAString("name");
> +      if (sysName == "Windows_NT") {
> +        let libc = ctypes.open("kernel32.dll");
> +        return libc.declare("GetCurrentProcessId",
> +          ctypes.winapi_abi,
> +          ctypes.uint32_t
> +        );
> +      } else {
> +        let libc = ctypes.open(ctypes.libraryName("c"));
> +        return libc.declare("getpid",
> +          ctypes.default_abi,
> +          ctypes.int  // PID
> +        );
> +      }

Will this work with sandboxing?
Attachment #8634665 - Flags: review?(mconley)
https://reviewboard.mozilla.org/r/13429/#review12085

> Will this work with sandboxing?

Well, if `getpid`/`GetCurrentProcessId` is blocked by sandboxing, the C++ code will also break, so I'm going to assume that this should work.
Comment on attachment 8634665 [details]
MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley

Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r?mconley
Attachment #8634665 - Flags: review?(mconley)
Can we run this through Try now so it can land ASAP once it has review? It's blocking a frequent crash in automation :(
Blocks: 1183236
For consistency, I have moved the fixup patch to bug 1175098.
No longer blocks: 1183236
Comment on attachment 8634665 [details]
MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley

https://reviewboard.mozilla.org/r/13429/#review12411

Ship It!
Attachment #8634665 - Flags: review?(mconley) → review+
Comment on attachment 8634665 [details]
MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley

Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley
Attachment #8634665 - Attachment description: MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r?mconley → MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley
Attachment #8634665 - Flags: review+
Assignee: nobody → dteller
Mmmh...
Apparently, my libc call in the test doesn't work on Linux. I'll see what I can do about that.
Comment on attachment 8634665 [details]
MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley

Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley
Created attachment 8638432 [details]
MozReview Request: Bug 1184486 - Publish pid in nsIAppStartup;r?bsmedberg

Bug 1184486 - Publish pid in nsIAppStartup;r?bsmedberg
Attachment #8638432 - Flags: review?(benjamin)
Comment on attachment 8638432 [details]
MozReview Request: Bug 1184486 - Publish pid in nsIAppStartup;r?bsmedberg

Bug 1184486 - Publish pid in nsIAppStartup;r?bsmedberg
Attachment #8638432 - Flags: review?(benjamin)
Comment on attachment 8634665 [details]
MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley

Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley
Attachment #8634665 - Flags: review?(mconley)
Attachment #8638432 - Attachment is obsolete: true
Comment on attachment 8634665 [details]
MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley

Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley
Attachment #8634665 - Flags: review?(mconley)
Attachment #8634665 - Flags: review?(mconley)
Comment on attachment 8634665 [details]
MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley

Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley
Comment on attachment 8634665 [details]
MozReview Request: Bug 1184486 - Let PerformanceStats.jsm play nicer with process-per-tab;r=mconley

https://reviewboard.mozilla.org/r/13429/#review12961

Ship It!
Attachment #8634665 - Flags: review?(mconley) → review+
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4892f93c5e8e

Thanks for the help with mozreview, mconley.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/14ae070d4a41
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.