#6624: Exclude discarded tabs in broadcasts and other "all tabs" actions #6626

fregante · 2023-10-10T13:45:59Z

What does this PR do?

Part of Deal with discarded tabs #6624

It's ready for review. ~~I'll want to test this before merging though~~ Done

Checklist

Add tests
New files added to src/tsconfig.strictNullChecks.json (if possible)
Designate a primary reviewer: @grahamlangford

codecov · 2023-10-10T18:17:19Z

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (d6128c7) 70.15% compared to head (8d8498a) 70.19%.
Report is 25 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6626      +/-   ##
==========================================
+ Coverage   70.15%   70.19%   +0.03%     
==========================================
  Files        1179     1178       -1     
  Lines       36654    36652       -2     
  Branches     6899     6899              
==========================================
+ Hits        25715    25727      +12     
+ Misses      10939    10925      -14

Files	Coverage Δ
src/background/deploymentUpdater.ts	`88.40% <100.00%> (-0.06%)`	⬇️
src/background/modUpdater.ts	`92.07% <100.00%> (ø)`
src/background/navigation.ts	`0.00% <ø> (ø)`
src/background/removeExtensionForEveryTab.ts	`80.00% <100.00%> (+20.00%)`	⬆️
src/background/starterMods.ts	`86.48% <100.00%> (ø)`
src/bricks/renderers/propertyTable.tsx	`26.47% <ø> (ø)`
src/contrib/google/sheets/core/useCurrentOrigin.ts	`100.00% <100.00%> (ø)`
src/utils/promiseUtils.ts	`69.86% <100.00%> (-0.45%)`	⬇️
src/background/telemetry.ts	`65.64% <50.00%> (+0.26%)`	⬆️
src/background/executor.ts	`34.78% <57.14%> (+0.80%)`	⬆️
... and 1 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fregante · 2023-10-10T15:05:58Z

src/background/telemetry.ts

@@ -436,7 +437,7 @@ export async function pong(): Promise<{ timestamp: number }> {
 */
 export async function collectPerformanceDiagnostics(): Promise<Diagnostics> {
  const timestamp = Date.now();
-  const allTabs = await browser.tabs.query({});
+  const allTabs = await getTabsWithAccess();


This now excludes discarded tabs and other tabs where PB does not run. I think it wouldn't make sense to track those. Can revert if needed

fregante · 2023-10-10T15:06:38Z

src/utils/extensionUtils.ts

+export async function getTabsWithAccess(): Promise<TabId[]> {
+  const tabs = await browser.tabs.query({
+    url: ["https://*/*", "http://*/*"],
+    discarded: false,


This is the core of the change:

only query the specific protocols we're looking for

exclude discarded tabs

fregante · 2023-10-10T15:07:41Z

src/utils/extensionUtils.ts

+  const tabs = await getTabsWithAccess();
+
+  if (tabs.length > 20) {
+    console.warn("forEachTab called on more than 20");


Second core part:

Log to console if there are a lot of tabs.

Alternatively:

replace this with reportEvent?

fregante · 2023-10-10T15:10:07Z

src/utils/extensionUtils.ts

+  TCallback extends (target: { tabId: number }) => Promisable<unknown>
+>(
+  callback: TCallback,
+  options?: { exclude: number }


forEachTab now lets you optionally exclude a specific tab. I think this is useful in any instance where the action originates in a tab.

fregante · 2023-10-11T06:54:24Z

src/background/activeTab.ts

@@ -1,39 +0,0 @@
-/** @file It's possible that some of these tabs might lose the permission in the meantime, we can't track that exactly */


"activeTab" referred to some logic that was dropped a few months ago. I moved these functions to extensionUtils

fregante · 2023-10-11T06:54:45Z

src/contrib/google/sheets/core/useCurrentOrigin.ts

-    // The origin should be set to the window.location.protocol + '//' +
-    // window.location.host of the top-most page, if your application is
-    // running in an iframe.
-    return url.protocol + "//" + url.host;


I ended up here looking for tabs.query() usage

The comments in this file seem copy-pasted and out of place (it mentions "in this PR", what PR?). I dropped some.

fregante · 2023-10-11T06:55:45Z

src/background/executor.ts

+    async ({ tabId }) =>
+      safelyRunBrick({ tabId, frameId: TOP_LEVEL_FRAME_ID }, subRequest),
+    {
+      exclude: sourceTabId,


Replacing asyncForEach with the more specific forEachTab here allowed for a slight refactoring, thanks to the included Promise.allSettled

grahamlangford · 2023-10-11T13:09:48Z

src/background/executor.ts

+  if (rejected.length > 0) {
+    console.warn(`Broadcast rejected for ${rejected.length} tabs`, {
+      rejected,
+    });


What are the possible reasons for rejected tabs? Stale or closed tabs? Should we log this to Rollbar as well?

What are the possible reasons for rejected tabs?

Non-scriptable tabs. I can actually include a filter for this

Existing tabs are not connectable after an install or extension update #6548

any other content script failure or timeout

user navigates away while the brick runs

Stale or closed tabs?

Discarded tabs are already excluded.

There's a possible race condition if the user closes a tab between line 165 and line 166 😅 but I reckon it's rare.

Should we log this to Rollbar as well?

I think this generally classifies as user/business error. We don't log errors that are due to unconnectable tabs.

https://github.com/pixiebrix/pixiebrix-extension/blob/3b9730a20839903c643928e9735a32fcc71da5ae/src/errors/errorHelpers.ts/#L69-L72

grahamlangford · 2023-10-11T13:10:49Z

src/background/executor.ts

+  if (rejected.length > 0) {
+    console.warn(`Broadcast rejected for ${rejected.length} frame`, {
+      rejected,
+    });


Same question here: https://github.com/pixiebrix/pixiebrix-extension/pull/6626/files#r1354981167

Same as above, except that frames are more likely to come into existence and disappear. Still classifies as a non-reportable error.

twschiller · 2023-10-11T13:12:56Z

Adding @BLoe as reviewer because he will be helping out with performance issues

github-actions · 2023-10-11T19:14:07Z

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

Limit forEachTab

3ce1c01

fregante marked this pull request as draft October 10, 2023 13:46

vercel bot deployed to Preview October 10, 2023 13:47 View deployment

Reuse in broadcast, telemetry

4f9ef4e

vercel bot deployed to Preview October 10, 2023 14:26 View deployment

Lint

1470032

vercel bot deployed to Preview October 10, 2023 15:03 View deployment

Reuse in requestRunInAllFrames

bb478e8

vercel bot deployed to Preview October 10, 2023 15:35 View deployment

Lint

4a103ed

vercel bot deployed to Preview October 10, 2023 15:51 View deployment

fregante changed the title ~~WIP: Limit forEachTab~~ WIP: Exclude discarded tabs Oct 10, 2023

fregante commented Oct 10, 2023

View reviewed changes

fregante changed the title ~~WIP: Exclude discarded tabs~~ Exclude discarded tabs Oct 10, 2023

fregante changed the title ~~Exclude discarded tabs~~ Exclude discarded tabs in broadcasts and other "all tabs" actions Oct 10, 2023

fregante changed the title ~~Exclude discarded tabs in broadcasts and other "all tabs" actions~~ #6624: Exclude discarded tabs in broadcasts and other "all tabs" actions Oct 10, 2023

fregante added the performance label Oct 10, 2023

fregante marked this pull request as ready for review October 10, 2023 18:35

vercel bot deployed to Preview October 10, 2023 18:38 View deployment

fregante mentioned this pull request Oct 11, 2023

Deal with discarded tabs #6624

Closed

2 tasks

fregante force-pushed the F/perf/exclude-discarded-tabs branch from 59d1db9 to 4a103ed Compare October 11, 2023 06:53

fregante commented Oct 11, 2023

View reviewed changes

fregante requested a review from grahamlangford October 11, 2023 08:12

fregante assigned fregante and grahamlangford Oct 11, 2023

grahamlangford reviewed Oct 11, 2023

View reviewed changes

twschiller requested a review from BLoe October 11, 2023 13:12

grahamlangford approved these changes Oct 11, 2023

View reviewed changes

vercel bot deployed to Preview October 12, 2023 05:50 View deployment

vercel bot deployed to Preview October 12, 2023 07:01 View deployment

Exclude non-scriptable tabs via isScriptableUrl

8d8498a

fregante force-pushed the F/perf/exclude-discarded-tabs branch from 5ad0e78 to 8d8498a Compare October 12, 2023 07:05

vercel bot deployed to Preview October 12, 2023 07:08 View deployment

fregante merged commit 2ab6d92 into main Oct 13, 2023
10 checks passed

fregante deleted the F/perf/exclude-discarded-tabs branch October 13, 2023 07:38

fregante mentioned this pull request Oct 13, 2023

#6624: Don't retry sending messages to discarded tabs #6666

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6624: Exclude discarded tabs in broadcasts and other "all tabs" actions #6626

#6624: Exclude discarded tabs in broadcasts and other "all tabs" actions #6626

fregante commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

fregante Oct 10, 2023

fregante Oct 10, 2023

fregante Oct 10, 2023

fregante Oct 10, 2023 •

edited

Loading

fregante Oct 11, 2023

fregante Oct 11, 2023

fregante Oct 11, 2023

grahamlangford Oct 11, 2023

fregante Oct 11, 2023 •

edited

Loading

grahamlangford Oct 11, 2023

fregante Oct 11, 2023

twschiller commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023

		@@ -1,39 +0,0 @@
		/** @file It's possible that some of these tabs might lose the permission in the meantime, we can't track that exactly */

#6624: Exclude discarded tabs in broadcasts and other "all tabs" actions #6626

#6624: Exclude discarded tabs in broadcasts and other "all tabs" actions #6626

Conversation

fregante commented Oct 10, 2023 • edited Loading

What does this PR do?

Checklist

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller commented Oct 11, 2023 • edited Loading

github-actions bot commented Oct 11, 2023

fregante commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

fregante Oct 10, 2023 •

edited

Loading

fregante Oct 11, 2023 •

edited

Loading

twschiller commented Oct 11, 2023 •

edited

Loading