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

[Dev Tools] Fix a11y listener memory leak #60639

Merged
merged 2 commits into from
Mar 20, 2020
Merged

[Dev Tools] Fix a11y listener memory leak #60639

merged 2 commits into from
Mar 20, 2020

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Mar 19, 2020 •
edited
Loading

Summary

Fix a memory leak by properly removing a "keydown" listener created on the document.

The bug, on master, can be reproduced by

  1. Navigating to the Console plugin in Kibana
  2. In Chrome's Dev Tools in the "Sources" tab turn on the "keydown" event listener breakpoint
  3. Type a char into Console
  4. The listener named documentKeyDownListener should appear once (among others)
  5. Switch to SearchProfiler, documentKeyDownListener appears twice (among others)
  6. Switch back to Console, documentKeyDownListener appears thrice (among others), and so on.

Code changes

The culprit was inside of use_ui_ace_keyboard_mode.ts which has been moved to a shared code space inside of es_ui_shared, had the fix applied and is now being used inside of both Console and SearchProfiler.

How to test

Same steps as bug reproduction, but this time, documentKeyDownListener should appear only once, even after switching back and forth.

@jloleysens
Fix ace a11y listener
e631d0d 
Also move the hook use_ui_ace_keyboard_mode.tsx into es_ui_shared

This was defined (and used) in both Console and SearchProfiler.
@jloleysens jloleysens added bug Fixes for quality problems that affect the customer experience Feature:Console Dev Tools Console Feature Feature:Dev Tools v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes Feature:Search Profiler v7.7.0 v7.6.2 labels Mar 19, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@jloleysens jloleysens added this to In progress in Elasticsearch UI Mar 19, 2020
@cjcenizal cjcenizal requested review from alisonelizabeth and removed request for cjcenizal March 19, 2020 15:22
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

alisonelizabeth
alisonelizabeth approved these changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

LGTM. Reproduced bug on master, and verified fix on your branch. Thanks for fixing this @jloleysens!

@jloleysens jloleysens merged commit b841526 into elastic:master Mar 20, 2020
Elasticsearch UI automation moved this from In progress to Done Mar 20, 2020
@jloleysens jloleysens deleted the fix/console/ace-a11y-listener branch March 20, 2020 09:14
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 20, 2020
@jloleysens @elasticmachine
Fix ace a11y listener (elastic#60639)
97dba8e 
Also move the hook use_ui_ace_keyboard_mode.tsx into es_ui_shared

This was defined (and used) in both Console and SearchProfiler.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@jloleysens jloleysens mentioned this pull request Mar 20, 2020
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 20, 2020
@jloleysens
Fix ace a11y listener (elastic#60639)
ee1a10a 
Also move the hook use_ui_ace_keyboard_mode.tsx into es_ui_shared

This was defined (and used) in both Console and SearchProfiler.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	src/legacy/core_plugins/console/public/np_ready/application/containers/editor/legacy/use_ui_ace_keyboard_mode.tsx
#	src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx
#	src/plugins/console/public/application/containers/editor/legacy/use_ui_ace_keyboard_mode.tsx
#	src/plugins/es_ui_shared/public/index.ts
#	src/plugins/es_ui_shared/public/use_ui_ace_keyboard_mode.tsx
#	x-pack/legacy/plugins/searchprofiler/public/np_ready/application/editor/use_ui_ace_keyboard_mode.tsx
#	x-pack/plugins/searchprofiler/public/application/editor/editor.tsx
@jloleysens jloleysens mentioned this pull request Mar 20, 2020
Merged
jloleysens added a commit that referenced this pull request Mar 20, 2020
@jloleysens @elasticmachine
Fix ace a11y listener (#60639) (#60722)
53ff94f 
Also move the hook use_ui_ace_keyboard_mode.tsx into es_ui_shared

This was defined (and used) in both Console and SearchProfiler.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 20, 2020
@gmmorris
Merge branch 'master' into alerting/api-constant
377acfc 
* master: (52 commits)
  [SIEM] Fix types in rules tests (elastic#60736)
  [Alerting] prevent flickering when fields are updated in an alert (elastic#60666)
  License checks for actions plugin (elastic#59070)
  Implemented ability to clear and properly validate alert interval (elastic#60571)
  WebElementWrapper: add findByTestSubject/findAllByTestSubject to search with data-test-subj (elastic#60568)
  [Maps] Update layer dependencies to NP (elastic#59585)
  [Discover] Remove StateManagementConfigProvider (elastic#60221)
  [ML] Listing all categorization wizard checks (elastic#60502)
  [Upgrade Assistant] First iteration of batch reindex docs (elastic#59887)
  [SIEM] Export timeline (elastic#58368)
  [SIEM] Add support for actions and throttle in Rules (elastic#59641)
  Fix ace a11y listener (elastic#60639)
  Add addInfo toast to core notifications service (elastic#60574)
  fix test description (elastic#60638)
  [SIEM] Cypress screenshots upload to google cloud (elastic#60556)
  [canvas/shareable_runtime] sync sass loaders with kbn/optimizer (elastic#60653)
  [SIEM] Fixes Modification of ML Rules (elastic#60662)
  [SIEM] [Case] Bulk status update, add comment avatar, id => title in breadcrumbs (elastic#60410)
  [Alerting] add functional tests for index threshold alertType (elastic#60597)
  [Ingest]EMT-248: add post action request handler and resources (elastic#60581)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 20, 2020
@gmmorris
Merge branch 'master' into alerting/empty-list
e8c7be8 
* master: (55 commits)
  Update dependency @elastic/charts to v18.1.0 (elastic#60578)
  Only set timezone when user setting is a valid timezone (elastic#57850)
  [NP] Remove `ui/agg_types` dependencies and move paginated table to kibana_legacy (elastic#60276)
  [SIEM] Fix types in rules tests (elastic#60736)
  [Alerting] prevent flickering when fields are updated in an alert (elastic#60666)
  License checks for actions plugin (elastic#59070)
  Implemented ability to clear and properly validate alert interval (elastic#60571)
  WebElementWrapper: add findByTestSubject/findAllByTestSubject to search with data-test-subj (elastic#60568)
  [Maps] Update layer dependencies to NP (elastic#59585)
  [Discover] Remove StateManagementConfigProvider (elastic#60221)
  [ML] Listing all categorization wizard checks (elastic#60502)
  [Upgrade Assistant] First iteration of batch reindex docs (elastic#59887)
  [SIEM] Export timeline (elastic#58368)
  [SIEM] Add support for actions and throttle in Rules (elastic#59641)
  Fix ace a11y listener (elastic#60639)
  Add addInfo toast to core notifications service (elastic#60574)
  fix test description (elastic#60638)
  [SIEM] Cypress screenshots upload to google cloud (elastic#60556)
  [canvas/shareable_runtime] sync sass loaders with kbn/optimizer (elastic#60653)
  [SIEM] Fixes Modification of ML Rules (elastic#60662)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 20, 2020
@gmmorris
Merge branch 'alerting/tls-warning' of github.com:gmmorris/kibana int…
f04878a 
…o alerting/tls-warning

* 'alerting/tls-warning' of github.com:gmmorris/kibana: (32 commits)
  [ML] Listing all categorization wizard checks (elastic#60502)
  [Upgrade Assistant] First iteration of batch reindex docs (elastic#59887)
  [SIEM] Export timeline (elastic#58368)
  [SIEM] Add support for actions and throttle in Rules (elastic#59641)
  Fix ace a11y listener (elastic#60639)
  Add addInfo toast to core notifications service (elastic#60574)
  fix test description (elastic#60638)
  [SIEM] Cypress screenshots upload to google cloud (elastic#60556)
  [canvas/shareable_runtime] sync sass loaders with kbn/optimizer (elastic#60653)
  [SIEM] Fixes Modification of ML Rules (elastic#60662)
  [SIEM] [Case] Bulk status update, add comment avatar, id => title in breadcrumbs (elastic#60410)
  [Alerting] add functional tests for index threshold alertType (elastic#60597)
  [Ingest]EMT-248: add post action request handler and resources (elastic#60581)
  Return incident's url (elastic#60617)
  [Endpoint] TEST: GET alert details - boundary test for first alert retrieval (elastic#60320)
  [ML] Transforms: Fix pivot preview table mapping. (elastic#60609)
  [Endpoint] Log random seed for sample data CLI to console (elastic#60646)
  Use common event model for determining if event is v0 or v1 (elastic#60667)
  Disables PR Project Assigner workflow
  [Reporting] Allow reports to be deleted in Management > Kibana > Reporting (elastic#60077)
  ...
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 21, 2020
jloleysens added a commit that referenced this pull request Mar 21, 2020
@jloleysens
[7.6] Fix ace a11y listener (#60639) (#60726)
bc82d6d 
* Fix ace a11y listener (#60639)

Also move the hook use_ui_ace_keyboard_mode.tsx into es_ui_shared

This was defined (and used) in both Console and SearchProfiler.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	src/legacy/core_plugins/console/public/np_ready/application/containers/editor/legacy/use_ui_ace_keyboard_mode.tsx
#	src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx
#	src/plugins/console/public/application/containers/editor/legacy/use_ui_ace_keyboard_mode.tsx
#	src/plugins/es_ui_shared/public/index.ts
#	src/plugins/es_ui_shared/public/use_ui_ace_keyboard_mode.tsx
#	x-pack/legacy/plugins/searchprofiler/public/np_ready/application/editor/use_ui_ace_keyboard_mode.tsx
#	x-pack/plugins/searchprofiler/public/application/editor/editor.tsx

* Remove unused translations
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience Feature:Console Dev Tools Console Feature Feature:Dev Tools Feature:Search Profiler release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.2 v7.7.0 v8.0.0
Projects
Elasticsearch UI
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jloleysens @elasticmachine @kibanamachine @alisonelizabeth