1454406 - cull tabs.onUpdated listeners when they are no longer valid

Status: NEW

(WebExtensions :: General, enhancement, P3)

Description

[Shane Caraveo (:mixedpuppy)]

If filtering on tabId or windowId, when those close the listener is essentially useless, we should remove them automatically.

Comment 1

[Caitlin Neiman [:caitmuenster]]

Shane, could you provide some guidance on how to fix this bug?

If this is your first contribution, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for information on how to get started.

Mentor: mixedpuppy

Comment 2

[Ajitesh13]

Hello Shane, Can you please provide the STR ?

Comment 3

[Shane Caraveo (:mixedpuppy)]

There really is not an STR here, rather we keep a list of listeners, some of which may be specific to a tab or window. We do not remove those listeners when that tab or window closes. So this would be a bit of cleanup. It's an edge case, but probably happens with some addons.

Our listeners are tracked here:

https://searchfox.org/mozilla-central/rev/623de665034eee43a54ff02939b61385ffd5990d/browser/components/extensions/parent/ext-tabs.js#380

We'd probably want a window close listener that releases any of the above tracked listeners if the windowId matches:

https://searchfox.org/mozilla-central/rev/623de665034eee43a54ff02939b61385ffd5990d/toolkit/components/extensions/parent/ext-tabs-base.js#1592

And similar for tab closing, which could listen for TabClose via windowTracker.addListener, releasing any filter with a matching tabId when a tab closes.

Comment 4

[Harsh]

Hi Shane,
Can I work on this if no one else is working on it?
Thanks

Comment 5

[Ajitesh13]

Sorry Harsh, I have been working on this bug for quit some time, i am very soon going to submit a patch for this bug.

Comment 6

[Ajitesh13]

Attachment: Bug 1454406 - test revised r=mixedpuppy

Comment 7

[Ajitesh13]

Attachment: Bug 1454406 - test revised r=mixedpuppy

Depends on D63073

Comment 8

[Ajitesh13]

Attachment: Bug 1454406 - cull tab.onUpdated listeners when they are no longer valid r=mixedpuppy

Depends on D63074

Comment 9

[Ajitesh13]

By mistake, two useless revisions are created in phabricator, i have edited the dependencies in the phabricator, How does the attached file can be deleted here ?

Comment 10

[Rob Wu [:robwu]]

Mark the revision as "Abandoned" in Phabricator.

Comment 11

[Rob Wu [:robwu]]

@Ajitesh Removing the bug number from Phabricator hides them from the default view, but they still show up under "attachments". If you want to remove the patch, go to Phabricator, scroll down to the comment box, select "Abandoned" in the dropdown and submit the change.

Comment 12

[Ajitesh13]

ok, thanks for help

Comment 13

[Ajitesh13]

I have abandoned the useless ones, but still its showing under attachments. Are there any way of deleting them from the attachments' section ?

Comment 14

[Rob Wu [:robwu]]

I expect the Phabricator bot to visit this bug and update the attachments. But I wouldn't be surprised if it did not happen due to the removal of the bug numbers from those Phabricator revisions.

To manually hide those attachments, click on Details, click on Edit details at both attachments, check the "obsolete" checkbox and submit the changes.

Comment 15

[Caitlin Neiman [:caitmuenster]]

Hi Ajitesh, thanks for letting other folks work on some bugs. :) I'm going to formally unassign you and open this up for other contributors.

Comment 16

[Ajitesh13]

Attachment: Bug 1454406 - cull tab.onUpdated listeners when they are no longer valid r=mixedpuppy

Comment 17

[Ajitesh13]

I accidentally created a new revision for my changes. Should I merge both of them or mark one of them as obsolete ?

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Ajitesh13 from comment #17)

I accidentally created a new revision for my changes. Should I merge both of them or mark one of them as obsolete ?

If you can merge so prior review comments remain, that is preferable, but otherwise just obsolete.

Comment 19

[Ajitesh13]

Hello Shane, can you please look into https://phabricator.services.mozilla.com/D70796 and verify if the windowTracker part looks good. Similarly i will start working on for tabTracker.

Comment 20

[Ajitesh13]

I have made all the changes, pushed into the Revision https://phabricator.services.mozilla.com/D63075 (which have the prior review comments) and will mark the extra revision as obsolete. I am very sorry for this mistake.

Comment 21

[Caitlin Neiman [:caitmuenster]]

Hey Ajitest! How's it going with this bug?

Comment 22

[kernp25]

Attachment: Bug 1454406 - cull tabs.onUpdated listeners when they are no longer valid r?mixedpuppy

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.