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.

Sign up for GitHub

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

Jump to bottom

feat: add app render-process-gone event #23560

Merged
merged 1 commit into from May 17, 2020
Merged

feat: add app render-process-gone event #23560

merged 1 commit into from May 17, 2020

Conversation

miniak
Copy link
Contributor

@miniak miniak commented May 13, 2020 •
edited

Description of Change

Follow up to #23096.

Checklist

Release Notes

Notes: Added new render-process-gone event on app to replace the renderer-process-crashed event.

@miniak miniak self-assigned this May 13, 2020
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 13, 2020
@miniak miniak force-pushed the miniak/render-process-gone branch from a5532f2 to 746bc11 Compare May 13, 2020 13:53
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 14, 2020
@miniak miniak force-pushed the miniak/render-process-gone branch from 746bc11 to 776f024 Compare May 14, 2020 21:13
@miniak miniak requested review from nornagon and zcbenz May 14, 2020 21:20
zcbenz
zcbenz requested changes May 15, 2020
View reviewed changes
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

The test is failing on Linux:

not ok 25 app module BrowserWindow events should emit render-process-gone event when renderer crashes
  AssertionError: expected 'abnormal-exit' to equal 'crashed'
      at Context.<anonymous> (electron/spec-main/api-app-spec.ts:473:56)

I'm not sure of the root cause, but if it is because of some platform differences we might want to fix/document it.

MarshallOfSound
MarshallOfSound requested changes May 15, 2020
View reviewed changes
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

This attempts to land half of #20006 again which I still have issues with. This code fundamentally depends on grouping webContents that crashed by assuming that crashed webContents will have their events emitted in the same tick. Which is an implementation detail and fundamentally will not stay true forever.

For instance we'll want to land #20818 or something similar eventually and that would break this new API

@miniak miniak force-pushed the miniak/render-process-gone branch from 776f024 to 5cad9c1 Compare May 15, 2020 13:43
@miniak
Copy link
Contributor Author

miniak commented May 15, 2020 •
edited

@MarshallOfSound I've removed the grouping. Btw #20818 was done in a way to still allow the grouping.

@miniak miniak force-pushed the miniak/render-process-gone branch from 5cad9c1 to 09570c5 Compare May 15, 2020 23:56
@miniak
Copy link
Contributor Author

miniak commented May 15, 2020

@MarshallOfSound do you know why we are getting 'abnormal-exit' instead of 'crashed' on some platforms?

@MarshallOfSound
Copy link
Member

Nope, not super clear to me. You'd have to trace chromium logic to see where it determines that value

alexeykuzmin
alexeykuzmin reviewed May 16, 2020
View reviewed changes
spec-main/api-app-spec.ts
@@ -432,6 +432,25 @@ describe('app module', () => {
expect(webContents).to.equal(w.webContents);
});

it('should emit render-process-gone event when renderer crashes', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ifit?

@miniak miniak force-pushed the miniak/render-process-gone branch from 09570c5 to 3221e9c Compare May 16, 2020 11:19
@miniak
Copy link
Contributor Author

miniak commented May 16, 2020

@zcbenz the tests are all green now, can you please unblock?

@alexeykuzmin alexeykuzmin merged commit 52b50e6 into master May 17, 2020
@release-clerk
Copy link

release-clerk bot commented May 17, 2020

Release Notes Persisted

Added new render-process-gone event on app to replace the renderer-process-crashed event.

@alexeykuzmin alexeykuzmin deleted the miniak/render-process-gone branch May 17, 2020 15:05
@miniak
Copy link
Contributor Author

miniak commented Jun 26, 2020

/trop run backport-to 8-x-y

@trop
Copy link
Contributor

trop bot commented Jun 26, 2020

The backport process for this PR has been manually initiated -
sending your commits to "8-x-y"!

@trop trop bot mentioned this pull request Jun 26, 2020
Merged
@trop
Copy link
Contributor

trop bot commented Jun 26, 2020

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

@trop trop bot added the in-flight/8-x-y label Jun 26, 2020
@miniak
Copy link
Contributor Author

miniak commented Jun 26, 2020

/trop run backport-to 9-x-y

@trop
Copy link
Contributor

trop bot commented Jun 26, 2020

The backport process for this PR has been manually initiated -
sending your commits to "9-x-y"!

@trop trop bot mentioned this pull request Jun 26, 2020
Merged
@trop
Copy link
Contributor

trop bot commented Jun 26, 2020

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

@trop trop bot added the in-flight/9-x-y label Jun 26, 2020
@miniak miniak mentioned this pull request Jul 3, 2020
Merged
5 tasks
This was referenced Nov 18, 2020
Merged
Closed
@miniak miniak mentioned this pull request Feb 16, 2023
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexeykuzmin alexeykuzmin alexeykuzmin approved these changes

@zcbenz zcbenz zcbenz approved these changes

@MarshallOfSound MarshallOfSound MarshallOfSound approved these changes

@nornagon nornagon Awaiting requested review from nornagon

Assignees

@miniak miniak

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@miniak @MarshallOfSound @alexeykuzmin @zcbenz