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

fix: notify focus change right away rather not on next tick #14453

Merged
merged 12 commits into from Sep 7, 2018

Conversation

Projects
None yet
7 participants
@nitsakh
Contributor

nitsakh commented Sep 4, 2018

Description of Change

This issue fixes #13913. There was a problem with two windows fighting for focus because the focus notification was being sent on next tick.
This PR changes that, so that the focus notification is sent right away.
Also fixes #13549.

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines
Release Notes

Notes: Fixed windows multiple windows focus bug.

@nitsakh nitsakh requested a review from electron/reviewers as a code owner Sep 4, 2018

@nitsakh nitsakh requested review from zcbenz and jkleinsc Sep 4, 2018

@ckerr

This is almost verbatim the reverse of #5131 and so I think it would reintroduce that bug?

@nitsakh

This comment has been minimized.

Show comment
Hide comment
@nitsakh

nitsakh Sep 4, 2018

Contributor

:-( Will check that. Thanks for pointing me to that. 🙇

Contributor

nitsakh commented Sep 4, 2018

:-( Will check that. Thanks for pointing me to that. 🙇

@jkleinsc

This comment has been minimized.

Show comment
Hide comment
@jkleinsc

jkleinsc Sep 4, 2018

Contributor

I just verified that this also fixes #13549.

Contributor

jkleinsc commented Sep 4, 2018

I just verified that this also fixes #13549.

@jkleinsc

This comment has been minimized.

Show comment
Hide comment
@jkleinsc

jkleinsc Sep 4, 2018

Contributor

I just tested this branch against the repro listed here #3893 (fixed by #5131). Unfortunately this fix reintroduces the original issue.

Contributor

jkleinsc commented Sep 4, 2018

I just tested this branch against the repro listed here #3893 (fixed by #5131). Unfortunately this fix reintroduces the original issue.

@zcbenz

The focus event is meant to be emitted after the window is focused. So behaviors on all platforms are matched, and issues like #3893 can be avoided.

The OnWidgetActivationChanged method is called when the window is in the middle of changing focus, so emitting focus here breaks the behavior, and can crash Electron if user decides to close the window now.

My solution was to delay the event one tick later, which fixed part of the problem but it was not the right solution.

We probably have to find out when the views library has just finished the focus changing, and emitting the event there.

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Sep 5, 2018

Member

We probably have to find out when the views library has just finished the focus changing, and emitting the event there.

I'm going to work on that today, then we can land this fix in combination with that change

Member

MarshallOfSound commented Sep 5, 2018

We probably have to find out when the views library has just finished the focus changing, and emitting the event there.

I'm going to work on that today, then we can land this fix in combination with that change

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Sep 5, 2018

Member

OK so did a bunch of work on this today, there are other events we can use to get focus but they introduce other race conditions with the window creation and the first observer being added. The easiest solution to get this fixed in 3.0 is to reintroduce the next tick delay but later in the call stack so it only affects the JS emit calls

Member

MarshallOfSound commented Sep 5, 2018

OK so did a bunch of work on this today, there are other events we can use to get focus but they introduce other race conditions with the window creation and the first observer being added. The easiest solution to get this fixed in 3.0 is to reintroduce the next tick delay but later in the call stack so it only affects the JS emit calls

@nitsakh nitsakh requested review from electron/docs as code owners Sep 5, 2018

@MarshallOfSound MarshallOfSound changed the base branch from 3-0-x to master Sep 5, 2018

nitsakh and others added some commits Sep 4, 2018

@jkleinsc

This comment has been minimized.

Show comment
Hide comment
@jkleinsc

jkleinsc Sep 5, 2018

Contributor

FYI... it looks like the Windows CI failures are a DCHECK failure:

# Fatal error in .\../../v8/src/v8threads.cc, line 30
# Debug check failed: (isolate) != nullptr.
Contributor

jkleinsc commented Sep 5, 2018

FYI... it looks like the Windows CI failures are a DCHECK failure:

# Fatal error in .\../../v8/src/v8threads.cc, line 30
# Debug check failed: (isolate) != nullptr.
@jkleinsc

Windows CI is failing with a DCHECK failure:

#
# Fatal error in .\../../v8/src/v8threads.cc, line 30
# Debug check failed: (isolate) != nullptr.
#
@jkleinsc

This comment has been minimized.

Show comment
Hide comment
@jkleinsc

jkleinsc Sep 5, 2018

Contributor

https://gist.github.com/7eb20129c41f6fc95999404d507fe438 reproduces the DCHECK failure above

Contributor

jkleinsc commented Sep 5, 2018

https://gist.github.com/7eb20129c41f6fc95999404d507fe438 reproduces the DCHECK failure above

@jkleinsc

This comment has been minimized.

Show comment
Hide comment
Contributor

jkleinsc commented Sep 5, 2018

@ckerr

ckerr approved these changes Sep 5, 2018

Assuming CI is happy, I'm 👍 on this approach.

I'd still love to hear if the std::string& being bound was somehow staying in scope in 92de895 due to magic in base::BindOnce...?

@codebytere

This comment has been minimized.

Show comment
Hide comment
@codebytere

codebytere Sep 5, 2018

Member

For windows:

Tests failed with exit code -1073741819

🤔

Member

codebytere commented Sep 5, 2018

For windows:

Tests failed with exit code -1073741819

🤔

@jkleinsc

This comment has been minimized.

Show comment
Hide comment
@jkleinsc

jkleinsc Sep 5, 2018

Contributor

@codebytere that is the dcheck failure I mentioned above.

Contributor

jkleinsc commented Sep 5, 2018

@codebytere that is the dcheck failure I mentioned above.

@codebytere

This comment has been minimized.

Show comment
Hide comment
@codebytere

codebytere Sep 5, 2018

Member

interesting, this failure didn't happen with my changes running the test suite locally (but on MacOS)

Member

codebytere commented Sep 5, 2018

interesting, this failure didn't happen with my changes running the test suite locally (but on MacOS)

ckerr added some commits Sep 6, 2018

fix: bind deferred Emit() calls to a WeakPtr
This is so that the deferred Emit() calls will be canceled
if the TopLevelWindow is destroyed.
@ckerr

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Sep 6, 2018

Member

The new method TopLevelWindow::EmitEvent() exists only because I couldn't make this work instead:

base::BindOnce(&TopLevelWindow::Emit, weak_factory_.GetWeakPtr(), eventName));

If some C++ guru could explain how to make that work, that would make my day. 🤞

Member

ckerr commented Sep 6, 2018

The new method TopLevelWindow::EmitEvent() exists only because I couldn't make this work instead:

base::BindOnce(&TopLevelWindow::Emit, weak_factory_.GetWeakPtr(), eventName));

If some C++ guru could explain how to make that work, that would make my day. 🤞

@MarshallOfSound MarshallOfSound changed the title from fix: Notify focus change right away, not on next tick to fix: notify focus change right away, not on next tick Sep 7, 2018

ckerr added some commits Sep 7, 2018

Revert "Merge branch 'fix-win-focus' of github.com:electron/electron …
…into fix-win-focus"

This reverts commit 9057680, reversing
changes made to 9c13e47.
Restore 704722c, which was removed in error.
We apologise again for the fault in the subtitles. Those responsible for sacking the people who have just been sacked have been sacked.

@ckerr ckerr changed the title from fix: notify focus change right away, not on next tick to fix: notify focus change right away rather not on next tick Sep 7, 2018

@ckerr ckerr merged commit a2ab0d8 into master Sep 7, 2018

31 of 33 checks passed

ci/circleci: electron-gn-mas-testing-fyi Your tests failed on CircleCI
Details
WIP work in progress
Details
Semantic Pull Request ready to be squashed
Details
appveyor: electron-ia32-branch AppVeyor build succeeded
Details
appveyor: electron-ia32-pr AppVeyor build succeeded
Details
appveyor: electron-x64-branch AppVeyor build succeeded
Details
appveyor: electron-x64-pr AppVeyor build succeeded
Details
appveyor: gn-win-ia32-debug AppVeyor build succeeded
Details
appveyor: gn-win-ia32-testing AppVeyor build succeeded
Details
appveyor: gn-win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: gn-win-x64-debug AppVeyor build succeeded
Details
appveyor: gn-win-x64-testing AppVeyor build succeeded
Details
appveyor: gn-win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: electron-gn-linux-arm-debug-fyi Your tests passed on CircleCI!
Details
ci/circleci: electron-gn-linux-arm-testing-fyi Your tests passed on CircleCI!
Details
ci/circleci: electron-gn-linux-arm64-debug-fyi Your tests passed on CircleCI!
Details
ci/circleci: electron-gn-linux-arm64-testing-fyi Your tests passed on CircleCI!
Details
ci/circleci: electron-gn-linux-ia32-debug-fyi Your tests passed on CircleCI!
Details
ci/circleci: electron-gn-linux-ia32-testing-fyi Your tests passed on CircleCI!
Details
ci/circleci: electron-gn-linux-x64-debug-fyi Your tests passed on CircleCI!
Details
ci/circleci: electron-gn-linux-x64-testing-fyi Your tests passed on CircleCI!
Details
ci/circleci: electron-gn-mas-debug-fyi Your tests passed on CircleCI!
Details
ci/circleci: electron-gn-osx-debug-fyi Your tests passed on CircleCI!
Details
ci/circleci: electron-gn-osx-testing-fyi Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Show comment
Hide comment
@release-clerk

release-clerk bot Sep 7, 2018

Release Notes Persisted

Fixed windows multiple windows focus bug.

release-clerk bot commented Sep 7, 2018

Release Notes Persisted

Fixed windows multiple windows focus bug.

@ckerr ckerr deleted the fix-win-focus branch Sep 7, 2018

@trop

This comment has been minimized.

Show comment
Hide comment
@trop

trop bot Sep 7, 2018

An error occurred while attempting to backport this PR to "3-0-x",
you will need to perform this backport manually

trop bot commented Sep 7, 2018

An error occurred while attempting to backport this PR to "3-0-x",
you will need to perform this backport manually

jkleinsc added a commit that referenced this pull request Sep 7, 2018

fix: notify focus change right away rather not on next tick (#14453)
* fix: Notify focus change right away, not on next tick

* fix: emit the JS blur/focus events on next tick to avoid race condition

* address feedback from review

* fix: bind deferred Emit() calls to a WeakPtr

This is so that the deferred Emit() calls will be canceled
if the TopLevelWindow is destroyed.

* chore: remove wip/test code cruft

* fix: make linter happy

* Enable disabled tests

* refactor: cleaner impl of EmitEventSoon()

* Revert "Merge branch 'fix-win-focus' of github.com:electron/electron into fix-win-focus"

This reverts commit 9057680, reversing
changes made to 9c13e47.

* Restore 704722c, which was removed in error.

We apologise again for the fault in the subtitles. Those responsible for sacking the people who have just been sacked have been sacked.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment