Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: WebFrameMain.collectJavaScriptCallStack()#44204

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
deepak1556 merged 5 commits intomainfromfeat/crash-call-stack
Dec 3, 2024

Conversation

@samuelmaddock
Copy link
Member

@samuelmaddocksamuelmaddock commentedOct 11, 2024•
edited
Loading

Description of Change

AddsWebFrameMain.collectJavaScriptCallStack() to allow accessing the JavaScript call stack of unresponsive renderers. This is available in Chromium as part of theCrash Reporting API.

With this API, a dialog shown when a renderer becomes unresponsive could have an option to copy the stack trace. This would help application developers figure out why their website might be stuck on long-running JavaScript.

I've marked the API as experimental given that the proposal is still a draft.

Here's an example of the output from afiddle gist:

Renderer unresponsive     at startInfiniteLoop (<anonymous>:5:5)    at HTMLHtmlElement.clickHandler (<anonymous>:11:5)

Checklist

Release Notes

Notes: AddedWebFrameMain.collectJavaScriptCallStack() for accessing the JavaScript call stack of unresponsive renderers.

bpasero, VerteDinde, timfish, gdavidkov, djcas9, and erickzhao reacted with hooray emoji
@electron-cationelectron-cationbot added the new-pr 🌱PR opened recently labelOct 11, 2024
@samuelmaddocksamuelmaddock added the target/33-x-yPR should also be added to the "33-x-y" branch. labelOct 11, 2024
@samuelmaddocksamuelmaddock added the semver/minorbackwards-compatible functionality labelOct 11, 2024
@samuelmaddocksamuelmaddock marked this pull request as ready for reviewOctober 11, 2024 15:31
@github-actionsgithub-actionsbot added the target/34-x-yPR should also be added to the "34-x-y" branch. labelOct 15, 2024
@electron-cationelectron-cationbot removed the new-pr 🌱PR opened recently labelOct 18, 2024
@samuelmaddock
Copy link
MemberAuthor

If we have the APIs to be able to collect a call stack at any time, let’s expose that!

Updated the API to do just that. cc@nornagon

There are still some requirements to be able to collect the call stack:

  • JavaScript must be running. Otherwise theRequestInterrupt never resolves.
  • The webpage owner needs to enable the document-policy header defined by the crash reporting API
  • The origin trial needs to be enabled or theDocumentPolicyIncludeJSCallStacksInCrashReports chromium flag.

@samuelmaddocksamuelmaddock changed the titlefeat: WebFrameMain.unresponsiveDocumentJSCallStackfeat: WebFrameMain.collectJavaScriptCallStack()Nov 12, 2024
####`frame.collectJavaScriptCallStack()`_Experimental_

Returns`Promise<string> | Promise<void>` - A promise that resolves with the currently running JavaScript call
stack. If no JavaScript runs, the promise will never resolve. If the call stack is unable to be collected, it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If no JavaScript runs, the promise will never resolve

Just curious, what is the better way to determine this case ? would checking forcurrent stack position against the threads stack base address help ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Naively, would providing a timeout argument be helpful to provide users with an escape hatch?

Copy link
MemberAuthor

@samuelmaddocksamuelmaddockNov 21, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

would checking forcurrent stack position against the threads stack base address help ?

I think this would be best left as feedback (or a CL) upstream so it can be fixed there. We should mention this inhttps://issues.chromium.org/issues/40268201

javascript_call_stack_collector.cc is specifically where RequestInterrupt is invoked.

Naively, would providing a timeout argument be helpful to provide users with an escape hatch?

If a fix cannot be applied upstream, I think we should consider this. My thinking is we merge this as experimental for now and revisit after seeing results from usage.

Copy link
Member

@deepak1556deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM 👍

// Enable features required by tests.
app.commandLine.appendSwitch('enable-features',[
// spec/api-web-frame-main-spec.ts
'DocumentPolicyIncludeJSCallStacksInCrashReports'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Any reason to not enable this feature by defaultDocumentPolicyIncludeJSCallStacksInCrashReports in the runtime ? Users are still required to opt-in respective documents via the policy header, just wondering if there is necessity to control the feature behind two runtime feature flags. Maybe in our case just the document policy is sufficient, thoughts ?

timfish reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Having this enabled by default would make it easier to automatically use this in the Sentry Electron SDK without the risk of clobbering users existingenable-features switch.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I didn't consider this given the early API availability in Chromium. It's estimated to be in origin trial until Chromium 132 and there hasn't been significant feedback by standards committees based on information provided inthe Chrome Status page.

Assuming it's fully rolled out in Chromium 133, we could expect to not require the flag by Feb 4, 2025.

Maybe in our case just the document policy is sufficient, thoughts?

I'm not against it if folks are okay with this.

timfish reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sounds good, we can revisit this again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There's more discussion about it here:#45356 (comment)

Copy link
Member

@VerteDindeVerteDinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

From a code implementation side, this looks good to me - the diff is relatively small and low-risk 👍 Approved pending API working group approval

Copy link
Member

@erickzhaoerickzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

API LGTM

Copy link
Member

@itsanandersonitsananderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

API LGTM

@deepak1556
Copy link
Member

PR should be good to merge, can you rebase on latestmain

samuelmaddock reacted with thumbs up emoji

@deepak1556deepak1556 merged commit2222920 intomainDec 3, 2024
53 checks passed
@deepak1556deepak1556 deleted the feat/crash-call-stack branchDecember 3, 2024 04:32
@release-clerk
Copy link

Release Notes Persisted

AddedWebFrameMain.collectJavaScriptCallStack() for accessing the JavaScript call stack of unresponsive renderers.

@trop
Copy link
Contributor

tropbot commentedDec 3, 2024

I have automatically backported this PR to "33-x-y", please check out#44937

@troptropbot added in-flight/33-x-y and removed target/33-x-yPR should also be added to the "33-x-y" branch. labelsDec 3, 2024
@trop
Copy link
Contributor

tropbot commentedDec 3, 2024

I have automatically backported this PR to "34-x-y", please check out#44938

@troptropbot added in-flight/34-x-y merged/33-x-yPR was merged to the "33-x-y" branch. merged/34-x-yPR was merged to the "34-x-y" branch. and removed target/34-x-yPR should also be added to the "34-x-y" branch. in-flight/33-x-y in-flight/34-x-y labelsDec 3, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nikwennikwennikwen left review comments

@deepak1556deepak1556deepak1556 approved these changes

@VerteDindeVerteDindeVerteDinde approved these changes

@nornagonnornagonAwaiting requested review from nornagon

+3 more reviewers

@timfishtimfishtimfish left review comments

@itsanandersonitsanandersonitsananderson approved these changes

@erickzhaoerickzhaoerickzhao approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api-review/approved ✅merged/33-x-yPR was merged to the "33-x-y" branch.merged/34-x-yPR was merged to the "34-x-y" branch.semver/minorbackwards-compatible functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@samuelmaddock@deepak1556@nornagon@itsananderson@timfish@nikwen@VerteDinde@erickzhao
[8]ページ先頭
©2009-2025 Movatter.jp