Implement fetching JS sources from browser via WebChannel #5506

canova · 2025-07-03T12:31:21Z

Backend bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1916785
(but patches are from multiple bugs, see https://phabricator.services.mozilla.com/D259273 for the whole stack)

This PR adds support for fetching the JS source code via WebChannel and displaying them in the source view.

You should be able to try it yourself if you can compile the backend patches yourself too, which is not super convenient, but currently there is no upload of JS sources, that's why it's only possible to see the sources when we capture the profile.

Currently:

We have a "JS Sources" feature in the backend, and it has to be enabled. I'll probably either remove it or make it the default once we are confident with everything. But for faster iteration, I decided to make it disabled first.

Next steps as follow-ups:

Upload the sources.
Prettify the JS sources.
JS source maps support.
Add column numbers to the JIT frames.
Enable the JS sources by default in the backend.

codecov · 2025-07-03T12:38:04Z

Codecov Report

❌ Patch coverage is 86.51685% with 48 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.75%. Comparing base (2d76b88) to head (446b90b).

Files with missing lines	Patch %	Lines
src/app-logic/browser-connection.ts	11.11%	8 Missing ⚠️
src/app-logic/url-handling.ts	52.94%	8 Missing ⚠️
src/profile-logic/process-profile.ts	69.56%	7 Missing ⚠️
src/utils/query-api.ts	0.00%	6 Missing ⚠️
src/profile-logic/symbolication.ts	81.81%	4 Missing ⚠️
src/components/app/CodeErrorOverlay.tsx	0.00%	3 Missing ⚠️
src/components/app/SourceCodeFetcher.tsx	70.00%	3 Missing ⚠️
src/profile-logic/merge-compare.ts	91.89%	3 Missing ⚠️
src/app-logic/web-channel.ts	0.00%	2 Missing ⚠️
src/components/shared/CallNodeContextMenu.tsx	87.50%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #5506    +/-   ##
========================================
  Coverage   85.74%   85.75%            
========================================
  Files         310      311     +1     
  Lines       30482    30696   +214     
  Branches     8396     8439    +43     
========================================
+ Hits        26137    26323   +186     
- Misses       3922     3950    +28     
  Partials      423      423

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

canova · 2025-08-06T12:47:31Z

@mstange This is ready for review now! Please let me know if you have any questions.

mstange

Nice work!

I have some concerns about the global ID. I would like to decrease the degree to which we rely on the PID. On Windows, in theory, it is possible to have the profiler running for a long enough time frame that you can have profiles from different content processes with the same (recycled) PID. I know we already have existing assumptions about PIDs being unique, but it would be nice to eliminate that assumption eventually, so we shouldn't bake it into new web channel methods.

Could we instead have a single ID which identifies the right source in the right process?

For example, Firefox could generate a UUIDv4 for every source, and put an index-to-(UUID, URL) mapping into a new table in the Gecko format. Then it could also remove the URL from the location string and save some space. And then we can use that UUID when we query the source via the web channel.

I'm not quite sure how to store this information in the processed format. We could have a new sources table in profile.shared.sources, also with UUID-or-null + URL-or-path, and it would allow us to have just a single index for the source which is global. We could then also put that index into the URL, rather than the source path + pid + index. But then the question is whether this only applies to JS sources, or to existing C++ sources as well. In the upgrader, would we need to look through the funcTable and gather up all source paths to create entries in profile.shared.sources for them? And then we'd need a URL upgrader too...

What are your thoughts? From my side the highest priority is to avoid putting the PID into the webchannel message. What we put in the profile and the URL is something we can always change with an upgrader later, but the webchannel "protocol" is more long-lived.

Some other thoughts I had:

For extension JS URLs, we currently create one resource per extension, not one resource per source. So the mapping won't work correctly there.
If we change the string format we use in the Gecko format's "location" string, does this also affect what we put into the Jitdump when profiling with samply?
When thinking about processed profile format changes, let's keep in mind what we would like it to look like once we put all the tables into profile.shared.
We should also keep in mind that we want to have the ability to embed sources into the profile. The profile.shared.sources idea was with this in mind - we could put the actual source into that same table.

I'm going to mark this PR as "Request Changes" only due to the PID in the webchannel message and due to the getDownloadRecipe issue mentioned in a separate comment.

src/profile-logic/gecko-profile-versioning.js

src/utils/fetch-source.js

This is needed for the following patches to make sure that we don't have a circular dependency. Previously this class was only used for our own profile processing from the gecko profile data. But in the next commits I'm changing the profile importers to use this as well for easy profile shared data collection.

canova · 2025-09-18T14:31:18Z

Thanks for the review @mstange!

I've updated the PR after your review comments and it should be ready for another review. I didn't update my backend patches yet but I will update them soon too.

We've discussed it before sync, but here's an overview of the things I did:

Added a profile.shared.sources table to the processed format. It gets processed from gecko profile format's profile.sources and subprofile.sources tables.
Also changed the location string in gecko profile format to include the sourceIndex that's an "index into source table".
- Added a gecko profile upgrader for this.
Instead of changing the resourceTable fields, I'm changing the funcTable now. I removed the fileName array and added source that holds an "index into source table" instead.
- Added a processed profile upgrader.
- This required changes in a lot of places inside the codebase. Partly because we had to pass the source table down to different components, and also for sanitization, importers, merge-compare logic etc.
Changed the url state to keep sourceIndex to the sources table instead of sourceFile name as a string.
- This required a url upgrader.
Implemented the WebChannel API for JS source code fetching using the source uuid. We keep the uuid inside the SourceTable.
Added tests.

The diff says a lot of addition/removal in github, but they are mostly from snapshot updates. I tried to extract them into different commits so they are easier to review.

mstange

I've only reviewed the first two commits so far and they look fine. But I fear that the other commits will have a lot of changes in them that won't be necessary if we put the sources into the derived thread... what do you think?

src/types/profile.ts

src/profile-logic/merge-compare.ts

mstange · 2025-09-18T20:57:23Z

src/profile-logic/process-profile.ts

+    for (const data of geckoProfile.sources.data) {
+      const uuid = data[schema.uuid];
+      const filename = data[schema.filename];
+      globalDataCollector.indexForSource(uuid, filename);


Looks a bit odd to ignore the return value here, but I suspect this will change in upcoming commits

Yeah, currently I'm preadding all the sources. Then I will get the source indexes in the place where I'm processing the location JS string.

Then why is this upfront processing needed? Won't those later calls make sure the right entries are present?

Hmm, I was pretty sure there was a reason when I was doing it, but I can't find it now. Thinking more about it, I think it's better to remove this _processSourcesTable. Done.

src/components/shared/CallNodeContextMenu.tsx

canova · 2025-09-19T09:56:10Z

Wait, this component already has a thread prop. Shouldn't we put the sources into the derived thread, the same way we do with the stringTable?

Oh, I always thought about the shared data as outside of the thread mentally, never thought about putting it inside the derived thread. But it makes sense, and it removes a lot of annoying code where I pass the sources around. Changed it!
Added a new commit named "Add source table to the derived thread", and amended the other threads to remove the unnecessary codes.

mstange

Looks really good now! My main question is about the response format for the GET_JS_SOURCES message. Also, have you thought about how this will interact with source maps?

mstange · 2025-09-19T17:22:19Z

src/profile-logic/profile-data.ts

  stackTable: StackTable,
-  stringTable: StringTable
+  stringTable: StringTable,
+  sources: SourceTable


hmm, should we call it sourceTable instead? No strong opinion and this is something we can always change later

Hmm, to be honest I think I changed my mind twice while I was writing this code 😄 I had the same question both for the name in the profile json and code when I pass it around in argument. I currently like the shorter sources version because:

It's more concise.

It's easier to type

For the profile format: All of our tables are kinda mixed in naming and there is no single rule we follow, which makes it harder to decide which one to pick. I currently prefer the naming without table 😄

But I feel like it's all very subjective and even I changed my mind twice so far 😄 I'll keep it as is for now, but we can always change it later.

src/components/shared/CallNodeContextMenu.tsx

src/profile-logic/import/chrome.ts

src/components/shared/CallNodeContextMenu.tsx

src/components/tooltip/CallNode.tsx

src/test/unit/line-timings.test.ts

mstange · 2025-09-19T18:49:31Z

src/test/unit/source-code-cache.test.ts

+    };
+  }
+
+  function createMockCache(): Map<IndexIntoSourceTable, SourceCodeStatus> {


I'd rather have no test than a test that uses mocking and depends on implementation details

I agree with you in general. But I don't see it that way for testing the source code cache. This is because:

Source code cache itself is an implementation detail, so it's very difficult to test without mocking.

Even though this cache is an implementation detail itself, I wanted to write a test for it to make sure that it works correctly. Because I broke it while working on it :) So I didn't someone else doing the same mistake I did.

This is a unit test. Even though I agree about mocking for the component tests, unit tests usually depend on mocks.

Let me know how you feel. I guess I can remove the whole test if you prefer. I don't feel strongly about it. Just wanted to make sure it works correctly.

Actually, I just removed the mock cache and switched it to use the actions that we have (finishLoadingSourceCode etc.). That makes the test work without the explicit mock in the state.

src/app-logic/web-channel.ts

mstange · 2025-09-19T18:55:47Z

src/app-logic/web-channel.ts

 type QuerySymbolicationApiResponse = string;
 type GetPageFaviconsResponse = Array<FaviconData | null>;
 type OpenScriptInTabDebuggerResponse = void;
+type GetJSSourcesResponse = Array<string | null>;


Hmm, should we make this an object, so that it can give us a per-source error message? Making it an object would also let us add more fields in the future, e.g. sourceMapUrl and maybe sourceMapUuid.

Hmm I was thinking about having a different webchannel request for the sourcemap but it also makes sense to combine them. Changed it.

locales/en-US/app.ftl

mstange · 2025-09-19T19:21:02Z

I'm looking into https://typescript-eslint.io/getting-started/typed-linting/ now which I think may allow us to catch unnecessary null checks.

mstange · 2025-09-19T23:33:29Z

I'm looking into https://typescript-eslint.io/getting-started/typed-linting/ now which I think may allow us to catch unnecessary null checks.

It would work but we'll need some changes to make it pass, see #5620

This patch adds the sources table but it doesn't use that yet. The following commits will be changing the funcTable and then start using it. Note that this gathers all sources, including the JS and native sources. This way, we have a single source of truth for them. But this will happen in the following patches. This patch: - Adds a `geckoProfile.sources` table to the gecko profile format. - Handles the processing of gecko profile -> processed profile format.

Note that this gathers all sources, including the JS and native sources. This way, we have a single source of truth for them. This patch: - Changes the `thread.funcTable` to have a `source` array instead of a `fileName` array. - Changes the rest of the codebase to make sure that everything uses the new format.

…hanges

…ddition

Add an upgrader for frameTable.location changes that include JS sourceIds in brackets.

…states

canova

Thanks for the thorough review @mstange! I've updated the PR.

Oh welp, thanks for catching all these unnecessary if checks. All of the if (sources) checks come from my previous implementation where I made the sources optional in the processed format first. I then decided that it doesn't make sense and removed that optional case and thought I removed these checks. Apparently a lot of them slipped through the cracks. I think the filenameIndex checks were there because before this PR these filenames were in the funcTable that was nullable.

So I've amended the very easy changes to their commits directly (like the variable renaming/removing if checks/comment fixes etc.). And decided to put the logical changes to their own commits so they are easier to review. Last 4 commits are new (the ones after "Add tests for JavaScript source fetching")

I'm looking into https://typescript-eslint.io/getting-started/typed-linting/ now which I think may allow us to catch unnecessary null checks.

It would work but we'll need some changes to make it pass, see #5620

Ah, yeah, that looks nice. It would be good to use it. It looks like the issue you created was closed in favor of microsoft/TypeScript#15838 which is also closed. Does it mean that it's fixed in the main branch? (it looks very old, so I'm not so sure what's going on)

canova · 2025-09-22T11:00:14Z

src/profile-logic/symbolication.ts

+          // Find or create source entry
+          let sourceIndex = null;
+          for (let i = 0; i < sources.filename.length; i++) {
+            if (sources.filename[i] === fileNameStringIndex) {


Hmm, I think it's very unlikely to have the same filename with a UUID to collide with this. Before this patch, we were treating fileName as a unique identifier (filename was kept in the url). With the JS sources coming, still I can't think of a way that they could clash. But on the other hand, it's a small/harmless check that's not worth thinking too much. I just added still, just so we are more defensive about it.

Adding that check makes it not possible to use indexOf or findIndex btw. It's better to use a normal for loop for that case, so I kept it.

canova · 2025-09-22T11:03:53Z

src/selectors/per-thread/index.ts

    }
  );

  const getLib: Selector<string> = createSelector(


Heh yeah, it's a bit strange 😄

canova · 2025-09-22T11:10:51Z

src/profile-logic/profile-data.ts

  stackTable: StackTable,
-  stringTable: StringTable
+  stringTable: StringTable,
+  sources: SourceTable


Hmm, to be honest I think I changed my mind twice while I was writing this code 😄 I had the same question both for the name in the profile json and code when I pass it around in argument. I currently like the shorter sources version because:

It's more concise.

It's easier to type

For the profile format: All of our tables are kinda mixed in naming and there is no single rule we follow, which makes it harder to decide which one to pick. I currently prefer the naming without table 😄

But I feel like it's all very subjective and even I changed my mind twice so far 😄 I'll keep it as is for now, but we can always change it later.

canova · 2025-09-22T11:17:36Z

src/test/fixtures/utils.ts

+  for (let i = 0; i < sources.filename.length; i++) {
+    if (sources.filename[i] === urlStringIndex && sources.uuid[i] === uuid) {
+      return i;
+    }
+  }


We have two arrays to iterate there not a single array (filename and uuid). We usually go with a for loop in cases like that since it's simpler.

canova · 2025-09-22T11:25:12Z

src/profile-logic/process-profile.ts

  extensions: ExtensionTable = getEmptyExtensions(),
-  globalDataCollector: GlobalDataCollector
+  globalDataCollector: GlobalDataCollector,
+  processSourceTable: GeckoSourceTable | undefined


Yes, currently in the backend this feature is behind the "jssources" flag.

canova · 2025-09-22T11:56:50Z

src/app-logic/web-channel.ts

 type QuerySymbolicationApiResponse = string;
 type GetPageFaviconsResponse = Array<FaviconData | null>;
 type OpenScriptInTabDebuggerResponse = void;
+type GetJSSourcesResponse = Array<string | null>;


Hmm I was thinking about having a different webchannel request for the sourcemap but it also makes sense to combine them. Changed it.

canova · 2025-09-22T19:18:29Z

src/profile-logic/profile-compacting.ts

+    // Note: source indexes don't need translation as they point to sources table, not strings
+    // Source table will be handled separately in _createSourcesTableWithTranslatedStringIndexes


Hmm, I didn't do this initially thinking that probably it's not going to be very big. I was thinking about doing it in a follow-up (even though the comment here doesn't state that). But let's do it here. It shouldn't be too difficult. Updated the code.

canova · 2025-09-22T19:45:55Z

src/profile-logic/process-profile.ts

+    for (const data of geckoProfile.sources.data) {
+      const uuid = data[schema.uuid];
+      const filename = data[schema.filename];
+      globalDataCollector.indexForSource(uuid, filename);


Hmm, I was pretty sure there was a reason when I was doing it, but I can't find it now. Thinking more about it, I think it's better to remove this _processSourcesTable. Done.

canova · 2025-09-22T19:55:34Z

src/test/unit/source-code-cache.test.ts

+    };
+  }
+
+  function createMockCache(): Map<IndexIntoSourceTable, SourceCodeStatus> {


I agree with you in general. But I don't see it that way for testing the source code cache. This is because:

Source code cache itself is an implementation detail, so it's very difficult to test without mocking.

Even though this cache is an implementation detail itself, I wanted to write a test for it to make sure that it works correctly. Because I broke it while working on it :) So I didn't someone else doing the same mistake I did.

This is a unit test. Even though I agree about mocking for the component tests, unit tests usually depend on mocks.

Let me know how you feel. I guess I can remove the whole test if you prefer. I don't feel strongly about it. Just wanted to make sure it works correctly.

canova · 2025-09-22T19:59:55Z

src/test/unit/source-code-cache.test.ts

+    };
+  }
+
+  function createMockCache(): Map<IndexIntoSourceTable, SourceCodeStatus> {


Actually, I just removed the mock cache and switched it to use the actions that we have (finishLoadingSourceCode etc.). That makes the test work without the explicit mock in the state.

mstange

Looks good!

mstange · 2025-09-23T13:38:03Z

src/test/fixtures/utils.ts

+  for (let i = 0; i < sources.filename.length; i++) {
+    if (sources.filename[i] === urlStringIndex && sources.uuid[i] === uuid) {
+      return i;
+    }
+  }


oops, of course

mstange · 2025-09-23T13:58:53Z

src/app-logic/url-handling.ts

+    const { sources, stringArray } = profile.shared;
+
+    // Find the filename string index
+    const filenameStringIndex = stringArray.indexOf(filename);


I think it's worth using StringTable here - we'll likely already have a cached lookup map for this stringArray, or if not, we'll need it pretty soon.

mstange · 2025-09-23T14:19:44Z

It would work but we'll need some changes to make it pass, see #5620

Ah, yeah, that looks nice. It would be good to use it. It looks like the issue you created was closed in favor of microsoft/TypeScript#15838 which is also closed. Does it mean that it's fixed in the main branch? (it looks very old, so I'm not so sure what's going on)

Oh I had actually missed that it was closed. No it's not fixed - I guess they closed it as a duplicate of the more general issue which is still open, microsoft/TypeScript#9998 .

…filename Now that we changed the place where we send information to this place, we can simplify it.

Implement browser communication to fetch JS source code using GlobalJSSourceId. This enables the profiler to retrieve original source content directly from the browser's JavaScript engine. Backend bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1916785

…user properly This commit changes the response type to a object, so it will be easier to expand later for the sourcemap support.

…essing the JS functions

canova · 2025-09-24T14:10:09Z

Thanks for the reviews!

canova force-pushed the js-source-view branch from 9e2a125 to 19c5826 Compare August 6, 2025 12:37

canova marked this pull request as ready for review August 6, 2025 12:46

canova requested a review from a team as a code owner August 6, 2025 12:46

canova requested a review from mstange August 6, 2025 12:46

flodolo approved these changes Aug 7, 2025

View reviewed changes

mstange requested changes Aug 7, 2025

View reviewed changes

src/profile-logic/gecko-profile-versioning.js Outdated Show resolved Hide resolved

src/utils/fetch-source.js Outdated Show resolved Hide resolved

canova force-pushed the js-source-view branch 2 times, most recently from e75754e to 4776a15 Compare September 18, 2025 13:09

canova requested a review from mstange September 18, 2025 14:31

mstange requested changes Sep 18, 2025

View reviewed changes

canova force-pushed the js-source-view branch 2 times, most recently from c670ec3 to 380193c Compare September 19, 2025 09:52

canova requested a review from mstange September 19, 2025 09:57

mstange requested changes Sep 19, 2025

View reviewed changes

canova added 9 commits September 22, 2025 14:50

Add source table to the derived thread

0a6b001

Update the test snapshots after the new sources table and funcTable c…

cd6b096

…hanges

Add a processed profile upgrader after the profile.shared.sources a…

aac2c13

…ddition

Bump gecko profile format version for the new frameTable location format

7afe9d6

Add an upgrader for frameTable.location changes that include JS sourceIds in brackets.

Add tests for the source table processing

d200dbc

Add tests for GlobalDataCollector for sources

ec7ba3a

Add tests for merge-compare code with the source table

0ec9366

Store sourceIndex instead of sourceFile in url state and source view …

dd00c8a

…states

canova force-pushed the js-source-view branch 2 times, most recently from 566a813 to 446b90b Compare September 22, 2025 20:20

canova commented Sep 22, 2025

View reviewed changes

canova requested a review from mstange September 22, 2025 20:32

mstange approved these changes Sep 23, 2025

View reviewed changes

canova added 11 commits September 24, 2025 16:05

Add a url upgrader for the changed sourceView query parameter

0d758c1

Simplify the getStackLineInfo code by passing sourceIndex instead of …

f455e83

…filename Now that we changed the place where we send information to this place, we can simplify it.

Add tests for the source code cache

0f897f0

Pass sourceUuid through the source fetching pipeline

3721104

Fetch the JS source from the browser

a705106

Add tests for JavaScript source fetching

059ccc2

Change the response type of GET_JS_SOURCE and print the error to the …

06d187d

…user properly This commit changes the response type to a object, so it will be easier to expand later for the sourcemap support.

Implement compacting for the shared source table

722ff87

Remove _processSourceTable and only add the sources while we are proc…

a4b90ed

…essing the JS functions

Make the source code cache tests use the proper actions

9f0aefb

canova force-pushed the js-source-view branch from 446b90b to 9f0aefb Compare September 24, 2025 14:06

canova merged commit b57b603 into firefox-devtools:main Sep 24, 2025
12 of 13 checks passed

canova mentioned this pull request Oct 21, 2025

Deploy October 21, 2025 #5638

Merged

		// Note: source indexes don't need translation as they point to sources table, not strings
		// Source table will be handled separately in _createSourcesTableWithTranslatedStringIndexes

Implement fetching JS sources from browser via WebChannel #5506

Implement fetching JS sources from browser via WebChannel #5506

Uh oh!

Conversation

canova commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

canova commented Aug 6, 2025

Uh oh!

mstange left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

canova commented Sep 18, 2025

Uh oh!

mstange left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

canova commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstange left a comment

Choose a reason for hiding this comment

Uh oh!

mstange Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mstange commented Sep 19, 2025

Uh oh!

mstange commented Sep 19, 2025

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canova commented Jul 3, 2025 •

edited

Loading

codecov bot commented Jul 3, 2025 •

edited

Loading

canova commented Sep 19, 2025 •

edited

Loading

mstange Sep 19, 2025 •

edited

Loading