Skip to content

core: warn when clear storage times out #14476

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

Merged
merged 5 commits into from
Jan 3, 2023
Merged

Conversation

adamraine
Copy link
Member

Network.clearBrowserCache is a pretty common PROTOCOL_TIMEOUT

It doesn't need to be a fatal error though, we can surface a top level report error telling the user to use incognito similar to our indexeddb/localstorage warning.

Ref
#6512

@adamraine adamraine requested a review from a team as a code owner October 28, 2022 17:45
@adamraine adamraine requested review from connorjclark and removed request for a team October 28, 2022 17:45
@connorjclark
Copy link
Collaborator

connorjclark commented Oct 30, 2022

IIRC our understanding is that the protocol sometimes locks up irrespective of the command. It just happens to be this one often because we do it early on. However, that could be incorrect. Can you show that this results in fewer protocol timeouts? If only we surfaced "clear cache" to sentry as a flag, we could discriminate on that in our error logging...

We should probably run a bunch of runs locally with hella chromium tracing on, and analyze the traces of whatever runs have protocol timeouts.

@adamraine
Copy link
Member Author

Can you show that this results in fewer protocol timeouts?

Not locally. I've just seen many users reporting PROTOCOL_TIMEOUT on this method.

We should probably run a bunch of runs locally with hella chromium tracing on, and analyze the traces of whatever runs have protocol timeouts.

It seems likely that PROTOCOL_TIMEOUT is more likely to happen early on regardless of method. I'm interested in pursuing this.

Is there a downside to this PR if we add some extra sentry instrumentation. It's possible this PR could improve the experience of a lot of users. We already ignore PROTOCOL_TIMEOUT when clearing local storage.

@vercel
Copy link

vercel bot commented Jan 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
lighthouse 🔄 Building (Inspect) Jan 3, 2023 at 9:59PM (UTC)

@adamraine adamraine merged commit 80cdafb into main Jan 3, 2023
@adamraine adamraine deleted the warn-storage-timeout branch January 3, 2023 22:37
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.

3 participants