Add new hang warning event API for WebExtensions
Categories
(WebExtensions :: General, enhancement, P5)
Tracking
(firefox124 fixed)
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
}
Assignee | ||
Updated•11 months ago
|
Assignee | ||
Comment 1•11 months ago
|
||
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.
Updated•11 months ago
|
Assignee | ||
Comment 2•11 months ago
|
||
(I've started working on this over the weekend, but don't have permission to assign myself yet.)
Assignee | ||
Comment 3•10 months ago
|
||
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.
Updated•10 months ago
|
Updated•10 months ago
|
Updated•10 months ago
|
Updated•10 months ago
|
Comment 5•9 months ago
|
||
Backed out for causing failures on browser_ext_slow_script.js
- backout: https://hg.mozilla.org/integration/autoland/rev/c285a1e59436529ca8d4acd4acf55d45e4277b1b
- push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Y_ShubzWQ8iIgLKgyKHTeg.0&revision=199b3c24b8b43b8053831d7c287bbe83e43037c4
- failure log: https://treeherder.mozilla.org/logviewer?job_id=442098903&repo=autoland&lineNumber=6432
[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
Comment 6•9 months ago
|
||
Please also have a look at this failure: https://treeherder.mozilla.org/logviewer?job_id=442105343&repo=autoland
Assignee | ||
Comment 7•9 months ago
|
||
Dang, sorry about that! Thanks for the pointers, I'll take a look as soon as I get a chance.
Comment 8•9 months ago
|
||
(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.
Assignee | ||
Comment 9•9 months ago
|
||
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.
Assignee | ||
Comment 10•9 months ago
|
||
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.
Comment 11•8 months ago
|
||
Comment 12•8 months ago
|
||
bugherder |
Updated•8 months ago
|
Comment 13•7 months ago
|
||
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.
Assignee | ||
Comment 14•7 months ago
|
||
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.
Comment 15•7 months ago
|
||
Thanks for offering! Please file a new bug to attach the patches to.
Description
•