Closed Bug 1861445 Opened 7 months ago Closed 4 months ago

Add new hang warning event API for WebExtensions

Categories

(WebExtensions :: General, enhancement, P5)

enhancement

Tracking

(firefox124 fixed)

RESOLVED FIXED
124 Branch
Tracking Status
firefox124 --- fixed

People

(Reporter: kzar, Assigned: kzar)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [wecg][addons-jira])

Attachments

(1 file)

Background

Sometimes when a WebExtension's content script is very slow, an "EXTENSION NAME" is slowing down Firefox. To speed up your browser, stop that extension. Learn more <Stop> warning is displayed to the user. It would be nice to share details of the issue with the extension's developers as well, so that they can proactively improve the performance of their extension.

It would be nice to add an event which fires at the point that the warning is shown, which includes debugging information such as a stack trace and warning details. In the future, this event could be used to warn the extension developers about other issues too - even if they are not serious enough for the user to be notified.

See also the discussion in the WebExtensions Community Group.

What to change

To be decided, but I suggest we add an event called browser.runtime.onWarning

browser.runtime.onWarning {
    // Category of warning, e.g. "performance".
    string category,  
    // Approximate severity of warning, either "high", "medium", or "low".
    string severity,
    // Associated tabId (if applicable).
    [number] tabId,
    // Associated frameId (if applicable).
    [number] frameId,
    // Warning description.
    string description,
    // Optional stack trace (string representation).
    [string] stackTrace
}
Flags: needinfo?(tomica)
Summary: Add new hang event API for WebExtensions → Add new hang warning event API for WebExtensions

Suggest that initially, the event only fires when the "EXTENSION NAME" is slowing down Firefox... message is shown to the user. For that, I would say the severity should be "high" given that the user was impacted, a stringified stack trace could be included along with the tabId for the impacted tab. The category could be something like "performance" and the description could give a little background, maybe even linking to some relevant documentation.

With those details, we'd be giving extension developers a real boost to debug these performance issues. Then in the future, we could think about where else the event could be fired too.

Severity: -- → N/A
Component: Untriaged → General
Priority: -- → P5
Whiteboard: [wecg]

(I've started working on this over the weekend, but don't have permission to assign myself yet.)

When an extension's content script is very slow and causes a webpage
to hang noticeably, a warning banner is displayed to the user. It
would be useful to also notify the extension developer when that
happens, so that they can address the issue.

Let's add a new event runtime.onWarning that can be dispatched when
the browser needs to warn an extension of runtime issues. For now,
let's just dispatch that event when the slow extension warning banner
is displayed.

See also https://github.com/w3c/webextensions/issues/456

Assignee: nobody → kzar
Status: NEW → ASSIGNED
Flags: needinfo?(tomica)
Whiteboard: [wecg] → [wecg][addons-jira]
Attachment #9365503 - Attachment description: Bug 1861445 - Add the runtime.onWarning WebExtension event r=#extension-reviewers,zombie → Bug 1861445 - Add the runtime.onPerformanceWarning WebExtension event r=#extension-reviewers,zombie
Pushed by tjovanovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/199b3c24b8b4
Add the runtime.onPerformanceWarning WebExtension event r=zombie

Backed out for causing failures on browser_ext_slow_script.js

[task 2024-01-04T05:15:17.711Z] 05:15:17     INFO - TEST-START | browser/components/extensions/test/browser/browser_ext_slow_script.js
[task 2024-01-04T05:15:17.878Z] 05:15:17     INFO - GECKO(1687) | [ERROR firefox_on_glean::private::string] Unable to set string metric in non-main process. Ignoring.
[task 2024-01-04T05:16:03.765Z] 05:16:03     INFO - TEST-INFO | started process screencapture
[task 2024-01-04T05:16:03.897Z] 05:16:03     INFO - TEST-INFO | screencapture: exit 0
[task 2024-01-04T05:16:03.898Z] 05:16:03     INFO - Buffered messages logged at 05:15:17
[task 2024-01-04T05:16:03.898Z] 05:16:03     INFO - Entering test bound test_slow_content_script
[task 2024-01-04T05:16:03.899Z] 05:16:03     INFO - Extension loaded
[task 2024-01-04T05:16:03.899Z] 05:16:03     INFO - Console message: Warning: attempting to write 32027 bytes to preference extensions.webextensions.uuids. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file.
[task 2024-01-04T05:16:03.900Z] 05:16:03     INFO - Buffered messages finished
[task 2024-01-04T05:16:03.900Z] 05:16:03     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_slow_script.js | Test timed out - 
[task 2024-01-04T05:16:03.901Z] 05:16:03     INFO - GECKO(1687) | Completed ShutdownLeaks collections in process 1687
Flags: needinfo?(kzar)

Dang, sorry about that! Thanks for the pointers, I'll take a look as soon as I get a chance.

Flags: needinfo?(kzar)
See Also: → 1874317

(In reply to Cosmin Sabou [:CosminS] from comment #6)

Please also have a look at this failure: https://treeherder.mozilla.org/logviewer?job_id=442105343&repo=autoland

I have analyzed this TSAN report and identified the data race. For details, see bug 1874317.

Dave: The TSAN issue is independent of this patch here, so if you continue to encounter the issue, add skip-if = ["tsan"] # Bug 1874317 to the test in the test manifest (toml file) to enable you to land the patch.

Right OK, thanks Rob!

So am I understanding this right? (Sorry if these are dumb questions!)

  • Two test failures were flagged related to my patch, the first relating to browser_ext_slow_script.js and the second relating to TSAN. It now turns out the TSAN failure is unrelated and being looked at with bug 1874317.
  • The next steps for me here are to rebase my patch, try to reproduce the browser_ext_slow_script.js failure, get it passing again, then update the existing Phabricator review + ask for another review pass?

If so, do you have any advice about how to reproduce the browser_ext_slow_script.js failure? When the patch was first reverted, I tried rebasing and running the test locally and found it passed. My plan was to try rebasing again, and then try repeatedly running the test to see if it fails intermittently.

I'm hoping to get some time to look at this tomorrow and over the weekend, sorry for the delay.

Flags: needinfo?(rob)

I think I understand what's going on now, the browser_ext_slow_script.js timeout failure is reproducible when the new browser_ext_runtime_onPerformanceWarning.js tests run first. The timeout seems to be with BrowserTestUtils.waitForGlobalNotificationBar(), I think the new tests are preventing the hang notification that browser_ext_slow_script.js expects from being displayed - possibly since they don't take care to clear away notifications as they are displayed. Just trying to fix that now, bear with me.

Flags: needinfo?(rob)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/9d33a3994005
Add the runtime.onPerformanceWarning WebExtension event r=zombie,robwu
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 124 Branch

BCD: https://github.com/mdn/browser-compat-data/pull/22196
MDN: https://github.com/mdn/content/pull/32222

The only unit tests are on desktop (browser/) directory. That means that there is zero test coverage for Android. Do you want to add unit tests for Android? Could even be a generic xpcshell or mochitest that tests only one scenario as a smoke test.

Sorry I didn't realise that. I'll dig out the old xpcshell tests I wrote before we converted them to browser tests and open a review. Bear with me, but will try and do that this week.

Thanks for offering! Please file a new bug to attach the patches to.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: