Streamline from-browser initialization

[mstange]

If the initial URL is /from-browser/, profile loading goes a rather different path compared to /from-url/ and /public/. This PR fixes that discrepancy.

More in-depth description:

Symbolication happens in finalizeProfileView.
finalizeProfileView can only do symbolication with the help of the browser if it gets passed a browserConnection.
finalizeProfileView is called in two cases: For initial loads it's called from setupInitialUrlState, and for non-initial loads it's called from loadProfile.

Before this patch, the following was true:

• setupInitialUrlState didn't have a browserConnection, so it couldn't pass a browserConnection to finalizeProfileView. Only loadProfile had a browserConnection that it could pass to finalizeProfileView.
• So in order to get symbols from the browser, finalizeProfileView had to be called from loadProfile, even during initial load.
• So the initialization flow for from-browser had to pass initialLoad = false even for the initial load - otherwise loadProfile wouldn't call finalizeProfile.
• This means that, for from-browser, finalizeProfileView was always called twice during the initial load! First from setupInitialUrlState, and then again from loadProfile.
• However, on the first call, finalizeProfileView did an early exit, because the call happened at a time when the profile in the redux state was still null.
• Why was the profile null during the first call? Because retrieveProfileForRawUrl didn't await retrieveProfileFromBrowser, so setupInitialUrlState was called before the profile had been obtained.
• This was the other reason why we were always passing initialLoad = false for from-browser: we were relying on that second call to finalizeProfileView (because the first call didn't do anything).

After this patch, the following is true:

• For initial loads, we call loadProfile with initialLoad = true for all data sources now, including from-browser. So loadProfile no longer calls finalizeProfile during from-browser initial load. Now there is only one call to finalizeProfile: the one from setupInitialUrlState.
• retrieveProfileForRawUrl now awaits retrieveProfileFromBrowser. This means that the profile in the redux state is non-null once setupInitialUrlState gets called.
• This await does not cause us to block on symbolication, as intended: The call to retrieveProfileFromBrowser no longer includes symbolication, because we now call it with initialLoad = true, so its loadProfile call (which now gets initialLoad = true) doesn't call finalizeProfileView, which is what would kick off symbolication.
• Since retrieveProfileForRawUrl now waits until the profile has been obtained, this also means that we now have a non-null profile at the point where we do URL upgrading even for from-browser. We don't really need that, but at least the code paths for the different data sources are now better aligned.
• setupInitialUrlState now has the browserConnection, so it can pass it along to finalizeProfileView, and symbolication works as expected.

This PR also makes the following test fixes, to keep the tests passing:

• UrlManager.test.js now also simulates the WebChannel. Otherwise, retrieveProfileFromBrowser() would time out waiting for the WebChannel. This function was already timing out during this test in the past, but we didn't notice it because nothing was waiting for that function to complete. The new await now puts retrieveProfileFromBrowser() on the critical path for some of the tests in this file, so this situation needs to be rectified.
• receive-profile.test.js now checks for PROFILE_LOADED first and then calls finalizeProfileView in the from-browser test, just like it's already doing in the /public/fakehash/ test. And since it now awaits finalizeProfileView, it means that it waits for symbolication to complete, and needs to expect that profile.meta.symbolicated is true.

[codecov]

Codecov Report

Merging #3649 (7a7fda0) into main (7a7fda0) will not change coverage.
The diff coverage is n/a.

❗ Current head 7a7fda0 differs from pull request most recent head 106f7a7. Consider uploading reports for the commit 106f7a7 to get more accurate results

@@           Coverage Diff           @@
##             main    #3649   +/-   ##
=======================================
  Coverage   88.80%   88.80%           
=======================================
  Files         260      260           
  Lines       21736    21736           
  Branches     5566     5566           
=======================================
  Hits        19302    19302           
  Misses       2256     2256           
  Partials      178      178           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a7fda0...106f7a7. Read the comment docs.

[julienw]

I'd like to apology for the delay, because I wanted to have a clear mind to look at this complex code.

Thanks! This seems to work fine. But I think it would be a lot simpler if UrlManager was creating the browserConnection, would there be a problem moving this creation and error handling there? I'm marking this PR with "request changes" for this reason.

Also I believe that this comment in UrlManager.js may need updating:

profiler/src/components/app/UrlManager.js

Lines 102 to 107 in 7a7fda0

	// Also note the profile may be null for the `from-browser` dataSource since
	// we do not `await` for retrieveProfileFromBrowser function, but also in
	// case of fatal errors in the process of retrieving and processing a
	// profile. To handle the latter case properly, we won't `pushState` if
	// we're in a FATAL_ERROR state.
	const profile = await retrieveProfileForRawUrl(window.location);

The rest of this comment is fully optional, but I wanted to get your feedback on that.
I got this idea while looking at this patch and how we have to pass the browserConnection object around.

Instead of passing the browserConnection object everywhere (which I understand is because we don't want to reuse it so that the initialization is not run again, etc), we could make that a singleton, that would lazy-initialize at the first get, but then always return the same version of it.

One obvious drawback is that it would need a not very clean resetForTestsOnly function. Or we could have a mock of BrowserConnection for most tests except the one testing that class only.

What do you think?
I'm not saying this is strictly better, and usually I don't really like keeping state if we can avoid it, but I feel like that this could reduce the complexity in this case, because you wouldn't need to return it everywhere just so that it's available at the right place.

Another option could be to set it in the redux state, then we could retrieve it in thunk actions (but this sounds like a hack to me, because we don't need it for rendering).

Thanks for reading!

[julienw]

It doesn't feel very appropriate to return the browser connection from retrieveProfileForRawUrl and retrieveProfileFromBrowser just because we need it in the caller. This looks like more a side effect, a byproduct, of retrieving a profile.

What do you think of creating the browserConnection in UrlManager instead, and passing it to retrieveProfileForRawUrl and then retrieveProfileFromBrowser? Also UrlManager looks like the right location to handle the errors related to the browser connection creation.

(I'm still thinking about removing the profile loading part of UrlManager and using ProfileLoader at load time too, then ProfilerLoader could keep all of this)

[mstange]

I fully agree!

This is actually the next step I have in mind. See the "Call createBrowserConnection earlier." commit in #3656. It comes with a fair number of changes so I wanted to break it out into its own commit.

[mstange]

Thanks for the review!

Thanks! This seems to work fine. But I think it would be a lot simpler if UrlManager was creating the browserConnection

Yes, definitely. I want to do it as a follow-up though.

Also I believe that this comment in UrlManager.js may need updating:

profiler/src/components/app/UrlManager.js

Lines 102 to 107 in 7a7fda0

	// Also note the profile may be null for the `from-browser` dataSource since
	// we do not `await` for retrieveProfileFromBrowser function, but also in
	// case of fatal errors in the process of retrieving and processing a
	// profile. To handle the latter case properly, we won't `pushState` if
	// we're in a FATAL_ERROR state.
	const profile = await retrieveProfileForRawUrl(window.location);

Indeed! I missed that.

For the rest of your comment, I have some notes about my opinions on it in #3656. Should we discuss it over there? Or maybe on Matrix / Zoom?

[julienw]

I see, thanks for pointing to #3656. This looks like quite aligned with my own thoughts! I'll read this through and put my notes there.

It feels a bit strange that you're implementing some things here that you rollback in the next commit of #3556 (especially returning the browserConnection from the retrieving functions), but why not, given there's more change too because of the other datasources.

[julienw]

approving following the previous comment
thanks, it's exciting to see all this work coming through!

[mstange]

It feels a bit strange that you're implementing some things here that you rollback in the next commit of #3556 (especially returning the browserConnection from the retrieving functions), but why not, given there's more change too because of the other datasources.

Yeah exactly; it was a small step to the side in order to be able to delay two big steps forward. I wanted to keep this PR as small and focused as possible.