FR: Allow comparing non-public profiles

[mhansen]

Hi there, I'm using Firefox Profiler internally with some secret stacktraces I can't upload (sorry!), so I have an internal web server hosting some profiles. It's mostly going pretty well! I just had this issue.

I'm trying to use the comparison function on two internally hosted profiles:

This gives me the error:

AppViewRouter--error-message-compare
Error: Only public uploaded profiles are supported by the comparison function.

The relevant code is here: https://github.com/firefox-devtools/profiler/blob/main/src/actions/receive-profile.js#L1521:

      const hasSupportedDatasources = profileStates.every(
        (state) => state.dataSource === 'public'
      );
      if (!hasSupportedDatasources) {
        throw new Error(
          'Only public uploaded profiles are supported by the comparison function.'
        );
      }

My state has dataSource="from-url". Is there a reason we don't support from-url profiles? Maybe this might be an easy fix to update this to accept from-url if not?

Here's the full stack, in case it helps:

Error: Only public uploaded profiles are supported by the comparison function.
    at receive-profile.js:1524
render @ AppViewRouter.js:89
Gi @ react-dom.production.min.js:187
zi @ react-dom.production.min.js:186
Gs @ react-dom.production.min.js:269
El @ react-dom.production.min.js:250
vl @ react-dom.production.min.js:250
yl @ react-dom.production.min.js:250
pl @ react-dom.production.min.js:243
(anonymous) @ react-dom.production.min.js:123
t.unstable_runWithPriority @ scheduler.production.min.js:18
jo @ react-dom.production.min.js:122
Vo @ react-dom.production.min.js:123
Wo @ react-dom.production.min.js:122
hl @ react-dom.production.min.js:244
notify @ Subscription.js:15
notifyNestedSubs @ Subscription.js:85
o @ Subscription.js:90
p @ redux.js:296
(anonymous) @ index.js:11
dispatch @ redux.js:659
(anonymous) @ receive-profile.js:1576
async function (async)
(anonymous) @ receive-profile.js:1507
(anonymous) @ index.js:8
n.<computed> @ bindActionCreators.js:9
componentDidUpdate @ ProfileLoader.js:93
ps @ react-dom.production.min.js:219
Tl @ react-dom.production.min.js:259
t.unstable_runWithPriority @ scheduler.production.min.js:18
jo @ react-dom.production.min.js:122
xl @ react-dom.production.min.js:252
pl @ react-dom.production.min.js:243
(anonymous) @ react-dom.production.min.js:123
t.unstable_runWithPriority @ scheduler.production.min.js:18
jo @ react-dom.production.min.js:122
Vo @ react-dom.production.min.js:123
Wo @ react-dom.production.min.js:122
Me @ react-dom.production.min.js:292
$t @ react-dom.production.min.js:73

[mhansen]

Ah, looks like there's a good reason for this error, if I patch it out:

--- a/src/actions/receive-profile.js
+++ b/src/actions/receive-profile.js
@@ -1518,11 +1518,11 @@ export function retrieveProfilesToCompare(
       );

       const hasSupportedDatasources = profileStates.every(
-        (state) => state.dataSource === 'public'
+        (state) => state.dataSource === 'public' || state.dataSource === 'from-url'
       );

Then the subsequent code tries to make requests to https://storage.googleapis.com/profile-store/ which 403s as it's not a file. I wonder how hard it would be to update this code path to support loading from arbitrary URLs not just publicly uploaded profiles?

[mhansen]

I suppose this line would be the one to add support for other URLs, not just hashes, to:

profiler/src/actions/receive-profile.js

Line 1532 in 47c91ad

const profileUrl = getProfileUrlForHash(hash);

[julienw]

yeah, we should support this use case.

Maybe this would work by replacing

profiler/src/actions/receive-profile.js

Lines 1531 to 1532 in 1812770

	const promises = profileStates.map(async ({ hash }) => {
	const profileUrl = getProfileUrlForHash(hash);

with:

 const promises = profileStates.map(async ({ dataSource, hash, profileUrl }) => { 
  switch(dataSource) {
    case "public":
      // Use a URL from the public store.
      profileUrl = getProfileUrlForHash(hash);
      break;
    case "from-url":
      // Use the profile URL in the decoded state, decoded from the input URL.
      break;
    default:
      throw new Error("Only public uploaded profiles are supported by the comparison function.")
  }

profileUrl in the url state comes from https://github.com/firefox-devtools/profiler/blob/main/src/app-logic/url-handling.js#L570.

[mhansen]

Yeah, that sounds like a good plan. I can have a go at this if you like, thanks for the pointers and affirmations that this is something you'd like to support.

…

On Tue, 5 Oct 2021 at 21:01, Julien Wajsberg ***@***.***> wrote: yeah, we should support this use case. Maybe this would work by replacing https://github.com/firefox-devtools/profiler/blob/1812770346a4662ce44fcca0f1029a8d24c255e5/src/actions/receive-profile.js#L1531-L1532 with: const promises = profileStates.map(async ({ dataSource, hash, profileUrl }) => { switch(dataSource) { case "public": // Use a URL from the public store. profileUrl = getProfileUrlForHash(hash); break; case "from-url": // Use the profile URL in the decoded state, decoded from the input URL. break; default: throw new Error("Only public uploaded profiles are supported by the comparison function.") } profileUrl in the url state comes from https://github.com/firefox-devtools/profiler/blob/main/src/app-logic/url-handling.js#L570 . — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3589 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZYONDWF7CD6PWCKTZW63UFLEHLANCNFSM5FDUR7TA> .