Ported profiler to a WebExtension #44

squarewave · 2017-04-27T17:39:18Z

Unfortunately the diff is a bit of a nightmare. Anyone interested can however look to https://github.com/squarewave/gecko-profiler-webextension for a slightly more understandable commit log.

Some highlights:

This uses a new geckoProfiler WebExtension API which should be landing on Nightly shortly.
This does not yet support profiling remote tabs. Remote profiling was working for a while with the experimental WebExtension API, but to make it official we'll need to extend the devtools API to support custom panels in WebIDE (really in about:debugging once WebIDE is dead.)
I had to remap Ctrl+Shift+{1,2} to Alt+{1,2} due to the WebExtensions commands API seeming to not work with multiple modifiers. I'm going to try to investigate why shortly and potentially file a bug.

mstange · 2017-04-27T17:47:16Z

The keyboard shortcut change is unfortunate. It would be really good to find a way to avoid it.

mstange · 2017-04-27T18:10:15Z

In terms of updates, updating an SDK-based add-on to a WebExtension seems to be no problem at all - we just need to point the update.rdf file at a different URL.

However, it's not possible to update to an add-on with a different ID. So either we need to keep the ID jid0-edalmuivkozlouyij0lpdx548bc@jetpack, or we need to do something more sophisticated:

Land + package + sign the WebExtension with the ID geckoprofiler@mozilla.com, but keep around the old add-on.
Create a bootstrapped add-on (not SDK-based) with the ID jid0-edalmuivkozlouyij0lpdx548bc@jetpack, whose sole function is to download and install the new WebExtension add-on (using getInstallForURL and friends), and then uninstall itself.
Make the current update.rdf point at that bootstrapped add-on, remove the old add-on.
The update.rdf for the WebExtension add-on will need to live at a slightly different URL.

andymckay · 2017-04-27T18:17:12Z

What is the reason for wanting to change the ID?

kmaglione · 2017-04-27T18:19:33Z

@mozilla.com and @mozilla.org IDs have special protections on AMO, and the profiler API is currently protected by a whitelist of APIs that can access it. I really don't want to add a jid0-a-bunch-of-nonsense@jetpack ID to that list.

andymckay · 2017-04-27T18:45:41Z

It's easy enough to add one or two add-ons to the list on AMO. I'm assuming that the hard coding of ids is a temporary solution until we've got a unified solution coming out of https://bugzilla.mozilla.org/show_bug.cgi?id=1280235. If not, wouldn't mind talking about that more so we don't end up with competing systems.

kmaglione · 2017-04-27T18:55:22Z

Yes, it's meant as a temporary solution, but at this point, getting the profiler off of the SDK is too important to block on that.

andymckay · 2017-04-27T18:57:34Z

Awesome, all cool with me then. 👍

- gecko_profiler.xpi just points to the old SDK-based add-on. - gecko_profiler-0.1.xpi points to the new WebExtension. In order to transition, we'll just need to pull the files from the transition folder into the root folder, overwriting the update.rdf file with the one from transition.

squarewave · 2017-04-28T17:22:15Z

I updated the commit to leave the old xpi and update.rdf files intact, but created a new directory called transtion. This has the code as outlined above to install a temporary add-on which will simply try to install the new add-on and, if successful, uninstall itself.

Unfortunately I'm still stuck on the keyboard shortcut issue.

These won't be needed until we are trying to support remote profiling.

mstange · 2017-04-28T19:14:57Z

resources/panel-container.html

@@ -1,25 +0,0 @@
-<!DOCTYPE html>


I would like to keep this file around because it helps when playing with the popup design.

mstange · 2017-04-28T19:14:57Z

data/panel.js

@@ -33,7 +22,6 @@ function renderState(state) {
  document.querySelector('.relevancy-level-fill').style.width = `${information * 100}%`;

  const height = document.body.getBoundingClientRect().height;
-  self.port.emit('ProfilerControlEvent', { type: 'PanelHeightUpdated', height });


How do you handle resizing the popup when the settings are expanded?

The browserAction API handles that for you, which is nice :)

mstange · 2017-04-28T19:14:57Z

transition/install.rdf

+        <em:name>Gecko Profiler</em:name>
+        <em:description>Collect platform profiles from Firefox Desktop.</em:description>
+        <em:creator>Markus Stange &lt;mstange@themasta.com&gt;</em:creator>
+        <em:updateURL>https://raw.githubusercontent.com/devtools-html/Gecko-Profiler-Addon/master/gecko_profiler.update.rdf</em:updateURL>


The transition add-on shouldn't need to be updated, right? So this is just a last resort if we screwed something up. And I think it'll need to point to .../master/transition/gecko_profiler.update.rdf.

mstange · 2017-04-28T19:14:57Z

content.js

+    if (window.connectToGeckoProfiler) {
+      window.connectToGeckoProfiler({
+        getProfile: () => Promise.resolve(gProfile),
+        getSymbolTable: (debugName, breakpadId) => getSymbolTable(debugName, breakpadId),


I think this line can be just getSymbolTable,.

mstange · 2017-04-28T19:14:57Z

content.js

+      const { resolve, reject } = symbolReplyPromiseMap.get([debugName, breakpadId].join(':'));
+
+      if (status === 'success') {
+        console.log('success ' + debugName);


let's remove this line

mstange · 2017-04-28T19:14:57Z

content.js

+      document.addEventListener("DOMContentLoaded", connectToPage);
+    } else if (event.data.type === 'ProfilerGetSymbolTableReply') {
+      const { debugName, breakpadId, status, result, error } = event.data;
+      const { resolve, reject } = symbolReplyPromiseMap.get([debugName, breakpadId].join(':'));


Should we remove the entry from the map here? Probably.

mstange · 2017-04-28T19:14:57Z

.web-extension-id

@@ -0,0 +1,3 @@
+# This file was created by https://github.com/mozilla/web-ext
+# Your auto-generated extension ID for addons.mozilla.org is:
+geckoprofiler@mozilla.com


The comment sounds like a lie :)

It's certainly a lie, but I'm not the one who told it, since I never touched this file. (i.e. web-ext's messages seem to be misleading.)

mstange · 2017-04-28T19:14:57Z

background.js

+    await startProfiler();
+  } else {
+    await stopProfiler();
+  }


I'm not sure what this is trying to do. But we shouldn't change the profiler state when the add-on starts. For example, when doing startup profiling using the MOZ_PROFILER_STARTUP env variable, the profiler will already be running by the time the add-on is initialized, and we don't want to throw away the profiler buffer contents by stopping the profiler here.

Eh, it was an attempt at making sure issue #10 is resolved, but that's a good point - I'll scrap it.

mstange · 2017-04-28T19:16:53Z

transition/bootstrap.js

@@ -0,0 +1,32 @@
+// NOTE: this file's only purpose is to be packaged into gecko_profiler_shim.xpi,


@kmaglione, could you review this file? Thanks!

Looks good to me.

mstange · 2017-05-01T20:13:20Z

Ehsan reminded me of something that I hadn't thought of before: We'd like to keep the ability to profile release versions of Firefox, which do not have the profiler WebExtensions API. So we can't update all installations of the old add-on to the shim add-on.
I'm not sure what to do about that.

mikeconley · 2017-05-01T20:49:41Z

I'm guessing we'll want the gecko_profiler.update.rdf to offer up the SDK based Gecko Profiler Add-on for 54 and under, and the bootstrap / replacement add-on for 55+. Any holes in that plan?

squarewave · 2017-05-01T20:58:14Z

Quick update on this: I've got a patch waiting for review in Bug 1337545 which fixes the shortcuts issue to allow Ctrl+Shift+1 and friends.

- Versions of FF < 55.0 will still use the old add-on, allowing us to profile Release versions. This will however break on older versions of Nightly before the geckoProfiler API landed. - Kept the scratch HTML files used for quickly iterating on panel design. - Misc other cleanup.

squarewave · 2017-05-03T17:47:43Z

The fix for shortcuts is r+ and just waiting to be landed, so I put the fixup commit here. Quick question, do we care about slightly out-of-date Nightly versions that still don't have the geckoProfiler API? Otherwise mikeconley's suggestion should work, which I've implemented in the latest commit.

mstange

Looks good except for the minVersion thing and the xpi filenames.

Let me make sure I understand what xpis we'll have in which places after this change.

/gecko_profiler.xpi is the SDK-based add-on.
/gecko_profiler_shim.xpi is the transition add-on.
/gecko_profiler-0.1.xpi / /gecko_profiler-0.3.xpi is the WebExtension add-on.

I think we should change this so that:

/gecko_profiler.xpi is the WebExtension add-on.
/gecko_profiler_transition.xpi is the transition add-on.
/gecko_profiler_legacy.xpi is the SDK-based add-on.

mstange · 2017-05-03T19:29:31Z

transition/install.rdf

+        <em:description>Collect platform profiles from Firefox Desktop.</em:description>
+        <em:creator>Markus Stange &lt;mstange@themasta.com&gt;</em:creator>
+        <em:updateURL>https://raw.githubusercontent.com/devtools-html/Gecko-Profiler-Addon/master/transition/gecko_profiler.update.rdf</em:updateURL>
+        <em:optionsURL>data:text/xml,&lt;placeholder/&gt;</em:optionsURL>


What does this do? Is there a way to indicate "no options"?

mstange · 2017-05-03T19:29:31Z

transition/install.rdf

+        <em:targetApplication>
+            <Description>
+                <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
+                <em:minVersion>38.0a1</em:minVersion>


I think this should be 55.0a1. We don't want to allow installing the transition add-on on Firefox versions that don't support the web extension.

mstange · 2017-05-03T19:29:31Z

updates.json

+{
+  "addons": {
+    "geckoprofiler@mozilla.com": [
+      { "version": "0.3", "update_link": "https://raw.githubusercontent.com/devtools-html/Gecko-Profiler-Addon/master/gecko_profiler-0.3.xpi" }


What does this do? Is it used as a source of information when generating the update.rdf file? In any case, let's not make the version number part of the filename.

mstange · 2017-05-03T19:29:31Z

transition/bootstrap.js

+
+Cu.import("resource://gre/modules/AddonManager.jsm");
+
+const NEW_ADDON_XPI_URL = "https://raw.githubusercontent.com/devtools-html/Gecko-Profiler-Addon/master/gecko_profiler-0.1.xpi";


I think we should remove the version number from the filename.

mstange · 2017-05-03T19:42:53Z

I'm guessing we'll want the gecko_profiler.update.rdf to offer up the SDK based Gecko Profiler Add-on for 54 and under, and the bootstrap / replacement add-on for 55+. Any holes in that plan?

I didn't know that you could offer different files to different target application versions. That's great, so I think that plan will work.

squarewave · 2017-05-03T20:39:17Z

@mstange, were you planning on signing the transition xpi before merging this? I wasn't intending this commit to shift everything over all in one go, but if that was your plan I can do it so long as I have that xpi. The reason I mention this is if you want the WebExtension xpi to keep the gecko_profiler.xpi name, I don't want to have users download that and get an error because the existing update.rdf file points to it. So if we want this commit to actually switch users over, we need:

The signed transition XPI as mentioned, which will need to be under gecko_profiler_transition.xpi
The update.rdf file under transition will instead need to replace the gecko_profiler.update.rdf file in the root
We'll need to make sure the shortcuts fix has landed for some amount of time before merging this so we don't temporarily eliminate people's profiler shortcuts.

mstange · 2017-05-03T21:00:05Z

Yes, I was planning on signing the XPIs before merging this PR. I can send the signed files to you, or I could just commit them on top of our branch.

mstange

Looks good to me now! Let's merge it on Friday.

mstange · 2017-05-03T21:56:21Z

README.md

- - <kbd>Ctrl</kbd>+<kbd>Shift</kbd>+<kbd>1</kbd>: Stop / Restart profiling
- - <kbd>Ctrl</kbd>+<kbd>Shift</kbd>+<kbd>2</kbd>: Capture the profile. This captures the current contents of the profiler buffer, opens a tab with perf-html at `https://perf-html.io/`, and sends the profile to it.
+ - <kbd>Alt</kbd>+<kbd>1</kbd>: Stop / Restart profiling
+ - <kbd>Alt</kbd>+<kbd>2</kbd>: Capture the profile. This captures the current contents of the profiler buffer, opens a tab with perf-html at `https://perf-html.io/`, and sends the profile to it.


This can be reverted again.

Ported profiler to a WebExtension

4ddcd36

squarewave added 2 commits Apr 28, 2017

Clean up panel.html and friends

38ef921

These won't be needed until we are trying to support remote profiling.

Fix URL in transition add-on to point to new XPI

1f884eb

mstange suggested changes Apr 28, 2017

View changes

mstange reviewed Apr 28, 2017

View changes

squarewave force-pushed the squarewave:master branch from 3203c04 to 0a98018 May 3, 2017

mstange suggested changes May 3, 2017

View changes

squarewave added 2 commits May 3, 2017

Fix up porting to WebExtension - part 2

7d6d449

Added signed gecko_profiler_transition.xpi

a899ff6

mstange approved these changes May 3, 2017

View changes

Reverted keyboard shortcut description in popup

4d157b0

mstange merged commit 002740c into firefox-devtools:master May 6, 2017

clarkbw mentioned this pull request May 8, 2017

Convert to WebExtension #20

Closed

overholt mentioned this pull request May 16, 2017

Gecko profiler change to web extension break the original control mechanism in Hasal Mozilla-TWQA/Hasal#686

Open

firefox-devtools / Gecko-Profiler-Addon Archived

Ported profiler to a WebExtension #44

Ported profiler to a WebExtension #44

squarewave commented Apr 27, 2017

mstange commented Apr 27, 2017

mstange commented Apr 27, 2017 •

edited

andymckay commented Apr 27, 2017

kmaglione commented Apr 27, 2017

andymckay commented Apr 27, 2017

kmaglione commented Apr 27, 2017

andymckay commented Apr 27, 2017

squarewave commented Apr 28, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mstange commented May 1, 2017

mikeconley commented May 1, 2017

squarewave commented May 1, 2017

squarewave commented May 3, 2017

mstange left a comment •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mstange commented May 3, 2017

squarewave commented May 3, 2017

mstange commented May 3, 2017

mstange left a comment

This comment has been minimized.

		@@ -0,0 +1,32 @@
		// NOTE: this file's only purpose is to be packaged into gecko_profiler_shim.xpi,


		Cu.import("resource://gre/modules/AddonManager.jsm");

		const NEW_ADDON_XPI_URL = "https://raw.githubusercontent.com/devtools-html/Gecko-Profiler-Addon/master/gecko_profiler-0.1.xpi";

firefox-devtools / Gecko-Profiler-Addon Archived

Ported profiler to a WebExtension #44

Ported profiler to a WebExtension #44

Conversation

squarewave commented Apr 27, 2017

mstange commented Apr 27, 2017

mstange commented Apr 27, 2017 • edited

andymckay commented Apr 27, 2017

kmaglione commented Apr 27, 2017

andymckay commented Apr 27, 2017

kmaglione commented Apr 27, 2017

andymckay commented Apr 27, 2017

squarewave commented Apr 28, 2017

This comment has been minimized.

mstange Apr 28, 2017 Contributor

This comment has been minimized.

mstange Apr 28, 2017 Contributor

This comment has been minimized.

squarewave May 3, 2017 Author Collaborator

This comment has been minimized.

mstange Apr 28, 2017 Contributor

This comment has been minimized.

mstange Apr 28, 2017 Contributor

This comment has been minimized.

mstange Apr 28, 2017 Contributor

This comment has been minimized.

mstange Apr 28, 2017 Contributor

This comment has been minimized.

mstange Apr 28, 2017 Contributor

This comment has been minimized.

squarewave Apr 28, 2017 • edited Author Collaborator

This comment has been minimized.

mstange Apr 28, 2017 Contributor

This comment has been minimized.

squarewave Apr 28, 2017 Author Collaborator

This comment has been minimized.

mstange Apr 28, 2017 Contributor

This comment has been minimized.

kmaglione May 3, 2017

mstange commented May 1, 2017

mikeconley commented May 1, 2017

squarewave commented May 1, 2017

squarewave commented May 3, 2017

mstange left a comment • edited

This comment has been minimized.

mstange May 3, 2017 Contributor

This comment has been minimized.

mstange May 3, 2017 Contributor

This comment has been minimized.

mstange May 3, 2017 Contributor

This comment has been minimized.

mstange May 3, 2017 Contributor

mstange commented May 3, 2017

squarewave commented May 3, 2017

mstange commented May 3, 2017

mstange left a comment

This comment has been minimized.

mstange May 3, 2017 Contributor

mstange commented Apr 27, 2017 •

edited

mstange Apr 28, 2017
Contributor

mstange Apr 28, 2017
Contributor

squarewave May 3, 2017
Author Collaborator

mstange Apr 28, 2017
Contributor

mstange Apr 28, 2017
Contributor

mstange Apr 28, 2017
Contributor

mstange Apr 28, 2017
Contributor

mstange Apr 28, 2017
Contributor

squarewave Apr 28, 2017 •

edited

Author Collaborator

mstange Apr 28, 2017
Contributor

squarewave Apr 28, 2017
Author Collaborator

mstange Apr 28, 2017
Contributor

mstange left a comment •

edited

mstange May 3, 2017
Contributor

mstange May 3, 2017
Contributor

mstange May 3, 2017
Contributor

mstange May 3, 2017
Contributor

mstange May 3, 2017
Contributor