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.

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

Use runtime.onPerformanceWarning event for breakage reports #2462

Merged
merged 1 commit into from Mar 7, 2024

Conversation

kzar
Copy link
Collaborator

@kzar kzar commented Mar 7, 2024

Use runtime.onPerformanceWarning event for breakage reports

The new runtime.onPerformanceWarning event is fired for extensions
when the browser detects a runtime performance issue. This is so that
extension developers can be notified and proactively investigate
performance issues in their extensions.

Initially, the event will be available with Firefox 124 and will fire
only for serious content-script hangs[1]. (For those hangs, the user is
also shown a warning banner[2].) In the future, the event will
hopefully be available on more browsers, will include stack traces
where possible and will fire under more conditions.

For now, let's just include a basic performanceWarning flag with
breakage reports, that indicates if the event fired for the tab in
question. In the future, we can expand this as necessary to include
details of stack traces and warning type and category.

1 - https://bugzilla.mozilla.org/show_bug.cgi?id=1861445
2 - "EXTENSION NAME" is slowing down Firefox. To speed up your browser, stop that extension. Learn more

Reviewer: @jdorweiler
CC: @GioSensation, @sammacbeth

Steps to test this PR:

For the review:

  1. Check the unit tests pass.

How I tested for real:

  1. Created a content-script that hangs, e.g.:
const finish = performance.now() + 10000;
console.log("Hang starting at", new Date());
while (performance.now() < finish) {
    true;
}
console.log("Hang finished at", new Date());
  1. Inspected network requests for the extension background "page".
  2. Sent a breakage report and ensured performanceWarning=false was sent.
  3. Triggered the hanging content script and kept clicking on the page while the hang was active, until Firefox displayed the warning banner.
  4. Sent a breakage report again, and ensuredperformanceWarning=true was sent.

Automated tests:

  • Unit tests
  • Integration tests
Reviewer Checklist:
  • Ensure the PR solves the problem
  • Review every line of code
  • Ensure the PR does no harm by testing the changes thoroughly
  • Get help if you're uncomfortable with any of the above!
  • Determine if there are any quick wins that improve the implementation
PR Author Checklist:
  • Get advice or leverage existing code
  • Agree on technical approach with reviewer (if the changes are nuanced)
  • Ensure that there is a testing strategy (and documented non-automated tests)
  • Ensure there is a documented monitoring strategy (if necessary)
  • Consider systems implications

The new runtime.onPerformanceWarning event is fired for extensions
when the browser detects a runtime performance issue. This is so that
extension developers can be notified and proactively investigate
performance issues in their extensions.

Initially, the event will be available with Firefox 124 and will fire
only for serious content-script hangs[1]. (For those hangs, the user is
also shown a warning banner[2].) In the future, the event will
hopefully be available on more browsers, will include stack traces
where possible and will fire under more conditions.

For now, let's just include a basic `performanceWarning` flag with
breakage reports, that indicates if the event fired for the tab in
question. In the future, we can expand this as necessary to include
details of stack traces and warning type and category.

1 - https://bugzilla.mozilla.org/show_bug.cgi?id=1861445
2 - "EXTENSION NAME" is slowing down Firefox. To speed up your browser, stop that extension. Learn more <Stop>
@kzar kzar merged commit 02deca4 into duckduckgo:main Mar 7, 2024
25 of 26 checks passed
Copy link
Collaborator

@sammacbeth sammacbeth left a comment

Choose a reason for hiding this comment

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

I missed this, but have a refactor suggestion to make this code easier to test and inline with the architecture we're moving towards.

Comment on lines +278 to +287
if (browser.runtime.onPerformanceWarning) {
browser.runtime.onPerformanceWarning.addListener(({ tabId }) => {
if (tabId && tabId > 0) {
const tab = tabManager.get({ tabId })
if (tab) {
tab.performanceWarning = true
}
}
})
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer if this was in a separate file - I'm trying to move us away from having everything registered in events.js. Instead we could:

  • Move this to a 'component', that runs this code in the constructor.
  • Instantiate this component in background.js only for Firefox builds.

This has the advantage that it makes this code much easier to test. I see you had to add a load of stubs to handle importing events.js in a unit-test. Using the component architecture would remove the need for that, and you could also pass in a mock tabManager instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants