Closed Bug 1552845 Opened 8 months ago Closed 7 months ago

Provide an API for saving profiles via nsIProfiler

Categories

(WebExtensions :: Experiments, enhancement, P1)

enhancement

Tracking

(firefox69 fixed)

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file, 1 obsolete file)

With bug 1326572 we got an API for the nsIProfiler. It's used in the Raptor webextension and works great, beside the fact that for huge profiles it is barely usable. Here I speak from data of ~100MB and larger.

Having to request the profile via getProfileAsArrayBuffer() first, and then transfer it via a socket connection to a client outside of Firefox, which can actually store the data to disk is a very long task.

As such would it be possible to offer an API for dumpProfileToFileAsync(), which would let the profiler directly write the profile to disk? If allowing a path as argument is too risky, saving it in TEMP might work?

Who would have to accept that proposal? And once done how does the implementation process look like?

Doug, maybe you can give some advises and hints?

Flags: needinfo?(dothayer)
Summary: Provide an API for nsIProfiler → Provide an API for saving profiles via nsIProfiler

I can only comment on the implementation, which I think would be straightforward. However, I have been away from this for a while. aswan might be more help here WRT approval. Andrew, as this is an internal API controlled by the pref extensions.geckoProfiler.acceptedExtensionIds, do you know who would need to approve / give guidance on the security of this API accepting a path to write a profile to?

Flags: needinfo?(dothayer) → needinfo?(aswan)

Since we're the only users of this api, we don't need to go through any formal process. Adding mconca just so he's aware though.

One challenge is that extensions current don't have direct filesystem access, so there's not really a sane way to generate a useful path. Bringing up a file picker or writing to the default downloads directory would be simple enough if either of those works...

Flags: needinfo?(aswan)

For automation a default path which gets selected by the profiler sounds fine. Could someone please add some links to where the existing code of the extension API is located? I only found the schema so far, but no actual implementation.

I happily would try to get this implemented.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #3)

For automation a default path which gets selected by the profiler sounds fine. Could someone please add some links to where the existing code of the extension API is located? I only found the schema so far, but no actual implementation.

I happily would try to get this implemented.

Sure, the API implementation is here:

and here are the xpcshell tests:

Blocks: 1550702

(In reply to Andrew Swan [:aswan] from comment #2)

One challenge is that extensions current don't have direct filesystem access, so there's not really a sane way to generate a useful path. Bringing up a file picker or writing to the default downloads directory would be simple enough if either of those works...

The extension itself won't need file system access. It only triggers the dumpProfileToFileAsync() method, and Firefox itself will write the data to the specified location.

In regards of a sane default location I would propose the following:

  1. Any exported profile should end-up in the Firefox user profile, as best below a profiler sub directory.
  2. The extension method requires to pass-in a file name, which leafName can then be used to construct the final file path.
  3. The extension method uses await internally, and returns a Promise which resolves to the final file path.

With option 2) any consumer of the API could implement there own logic for requesting a file name from the user if necessary. Also we make sure that users will never be able to store anything on an arbitrary disk location.

I hope that the above sounds fine to you. There is already code running perfectly fine for me locally.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED

By using OS.Path.join() for relative paths I was able to fix the failures locally. Here another try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3209fe2343677abc9baacb527efce773d57d274

Attachment #9066716 - Attachment description: Bug 1552845 - Provide an API for saving profiles via nsIProfiler. r=aswan → Bug 1552845 - Provide an API for saving profiles via nsIProfiler. r=kmag
Priority: -- → P5
Attachment #9068387 - Attachment is obsolete: true

Hey Kris, do you still have cycles to review this? Maybe zombie can take over if you need to focus on other things?

Flags: needinfo?(kmaglione+bmo)
Attachment #9066716 - Attachment description: Bug 1552845 - Provide an API for saving profiles via nsIProfiler. r=kmag → Bug 1552845 - Provide an API for saving profiles via nsIProfiler. r=aswan
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1da1824d64a6
Provide an API for saving profiles via nsIProfiler. r=aswan
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Flags: in-testsuite+
OS: Unspecified → All
Priority: P5 → P1
Hardware: Unspecified → All
Version: 49 Branch → unspecified

Can you please provide some steps so we can manually verify this issue? If no manual testing is needed can you please mark it as "qe-verify- "

This has good automated test coverage, no manual verification needed.

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