Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DevTools] Handle case when cached network requests have null content in response #22282

Closed
wants to merge 4 commits into from

Conversation

jstejada
Copy link
Contributor

@jstejada jstejada commented Sep 9, 2021

Summary

In #22198 we added support for listening for network requests for JavaScript source files and caching those requests, using the network.onRequestFinished api, in order to speed up downloading source files when attempting to extract hook names for an inspected element.

However, the cached requests might sometimes return null content for the network response (e.g. here), which we weren't handling correctly. When the content was null, we'd end up with a runtime error by trying to access a property on the null object.

This commit makes it so we never use response content that is null by making sure to fall back to fetching the file normally if we are unable to obtain non-null content for a cached request.

Test Plan

  • yarn flow
  • yarn test
  • yarn test-build-devtools
  • verified that parsing hook names (on fb internal) works even when the response content is null
  • verified that when the content isn't null we correctly reuse it without making another request

@lunaruan
Copy link
Contributor

lunaruan commented Sep 9, 2021

Ooh good catch!

@bvaughn
Copy link
Contributor

bvaughn commented Sep 9, 2021

Let's go ahead and merge this so main isn't broken.

I was temporarily able to reproduce the problem but now I can't again, so my attempts at logging or exploring different fixes isn't working.

Long term, I'd like to avoid us having to pre-cache anything. Maybe this means using network.getHAR() instead. I tried that locally though and it returns undefined so...who knows why that is. 🤡

Edit
It looks like the Mozilla docs and the Chrome docs disagree.

Mozilla says getHAR returns a promise:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/devtools/network/getHAR

Chrome says it accepts a callback:
https://developer.chrome.com/docs/extensions/reference/devtools_network/#method-getHAR

Yuck :)

@bvaughn
Copy link
Contributor

bvaughn commented Sep 9, 2021

Actually let's hold off landing this and compare it to an alternate approach using getHAR

@jstejada
Copy link
Contributor Author

Mozilla says getHAR returns a promise:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/devtools/network/getHAR

Chrome says it accepts a callback:
https://developer.chrome.com/docs/extensions/reference/devtools_network/#method-getHAR

Yuck :)

yeah, i noticed it’s the same difference between Chrome and Mozill for getContent() :(

@bvaughn
Copy link
Contributor

bvaughn commented Sep 10, 2021

yeah, i noticed it’s the same difference between Chrome and Mozill for getContent() :(

Oh shoot! That might be why my code wasn't working for Mozilla and I had to add the isChrome wrapper.

Maybe we can fix that 😁

@jstejada
Copy link
Contributor Author

jstejada commented Sep 10, 2021

Update, testing memory impact when attempting to load the content for requests eagerly:

  • Before: Heap size settles at ~157MB
  • After: Heap size settles at ~263MB

@jstejada
Copy link
Contributor Author

jstejada commented Sep 10, 2021

@bvaughn I repurposed this PR to only handle nullability without attempting to get the content eagerly, to avoid any memory impact while we explore the getHAR option in #22285

@jstejada
Copy link
Contributor Author

abandoning in favor of #22285

@jstejada jstejada closed this Sep 10, 2021
@jstejada jstejada deleted the cached-network-events branch September 10, 2021 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants