1329507 - tabs.onUpdated should have a filter template like webRequest.onBeforeRequest

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[21Naown]

If you are not restricted by Chrome's specs, what about add a filter similarly to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeRequest#Parameters ?
To raise only event(s) we need.

Comment 1

[Caitlin Neiman [:caitmuenster]]

Hi @21Naown, this has been added to the agenda for the May 9 WebExtensions triage meeting. Would you be able to join us? 

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage

Agenda: https://docs.google.com/document/d/1q7UD3DxsT5z0lO3EbOyF9Iln32Wg_e9LYMdHZ80BEb4/edit#heading=h.34n4lirhljve

Comment 2

[21Naown]

Hello, I will not be able to join the meeting you mentioned. Thank you for proposing to me!

Comment 3

[Andy McKay]

In the tabs.onUpdated event you can examine the tabs and decide which tab to process and which tab to ignore. It might make it a few lines of code shorter, perhaps, but then there's a whole filter syntax we have to maintain long term when a few lines of JS would work.

It might save a little bit of time because the WebExtension code no longer needs to call the extension. But that seems like a very minimal win for something that doesn't get called as much as, say the webRequest API.

Since this can already be done and doesn't offer any performance benefit but does increase code maintainence this doesn't seem worth it.

Comment 4

[Kris Maglione [:kmag]]

This will actually help us a lot with performance, since we wind up sending huge numbers of these events even when most of them aren't needed, so I'd like us to implement it.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1329507 add filtering to tabs.onUpdated,

Review commit: https://reviewboard.mozilla.org/r/224554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224554/

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8955375 [details]
Bug 1329507 add filtering to tabs.onUpdated,

I haven't written tests for the new filtering, but the existing onUpdated tests pass.  Just getting feedback.

Comment 7

[Shane Caraveo (:mixedpuppy)]

tabs.onUpdated has come up a few times recently, so I decided to give it a quick spin.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8955375 [details]
Bug 1329507 add filtering to tabs.onUpdated,

https://reviewboard.mozilla.org/r/224554/#review230508

::: browser/components/extensions/ext-tabs.js:98
(Diff revision 1)
> +        if (!perms.overlapsAll(filter.urls)) {
> +          Cu.reportError("The proxy.addListener filter doesn't overlap with host permissions.");
> +        }

`proxy.addListener`?

In any case, the overlapping permissions check is not necessary. All that matters here is that they have the "tabs" permission.

::: browser/components/extensions/ext-tabs.js:103
(Diff revision 1)
> +        if (!perms.overlapsAll(filter.urls)) {
> +          Cu.reportError("The proxy.addListener filter doesn't overlap with host permissions.");
> +        }
> +      }
> +      let needsModified = true;
> +      if (!filter.events) {

These aren't events. They're properties.

::: browser/components/extensions/ext-tabs.js:105
(Diff revision 1)
> +        }
> +      }
> +      let needsModified = true;
> +      if (!filter.events) {
> +        // Default is to listen for all events.
> +        filter.events = [

This should be a `Set`.

::: browser/components/extensions/ext-tabs.js:118
(Diff revision 1)
> +          "sharingState",
> +          "status",
> +          "title",
> +        ];
> +      } else {
> +        let allAttrs = ["audible", "favIconUrl", "mutedInfo", "sharingState", "title"];

This should be a globally-declared `Set`.

::: browser/components/extensions/ext-tabs.js:177
(Diff revision 1)
> +        if (filter.tabId != null && tab.id != filter.tabId) {
> +          return;
> +        }
> +        if (filter.windowId != null && tab.windowId != filter.windowId) {
> +          return;
> +        }

There should be an `if (filter)` around this. The overhead will add up in the cases where we don't have a filter.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8955375 [details]
Bug 1329507 add filtering to tabs.onUpdated,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224554/diff/1-2/

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8955375 [details]
Bug 1329507 add filtering to tabs.onUpdated,

https://reviewboard.mozilla.org/r/224554/#review231416

::: browser/components/extensions/ext-tabs.js:102
(Diff revision 2)
>      return deferred.promise;
>    },
>  };
>  
> +const allAttrs = new Set(["audible", "favIconUrl", "mutedInfo", "sharingState", "title"]);
> +const allProperties = [

`allProperties = new Set(...)`

::: browser/components/extensions/ext-tabs.js:121
(Diff revision 2)
> +  constructor(context, eventName) {
> +    let {extension} = context;
> +    let {tabManager} = extension;
> +
> +    let register = (fire, filterProps) => {
> +      const restricted = ["url", "favIconUrl", "title"];

Let's make this a global Set

::: browser/components/extensions/ext-tabs.js:128
(Diff revision 2)
> +      let filter = {...filterProps};
> +      if (filter.urls) {
> +        filter.urls = new MatchPatternSet(filter.urls);
> +      }
> +      let needsModified = true;
> +      if (!filter.propertyNames) {

:

    if (filter.propertyNames) {
      needsModified = filter.propertyNames.some(p => allAttrs.has(p));
      filter.propertyNames = new Set(filter.propertyNames);
    } else {
      filter.propertyNames = allProperties;
    }

::: browser/components/extensions/ext-tabs.js:145
(Diff revision 2)
> +          if (hasTabs || !restricted.includes(prop)) {
> +            nonempty = true;
> +            result[prop] = changeInfo[prop];
> +          }
> +        }
> +        return [nonempty, result];

Just return null if empty, I think.

::: browser/components/extensions/ext-tabs.js:149
(Diff revision 2)
> +        }
> +        return [nonempty, result];
> +      }
> +
> +      function getWindowID(windowId) {
> +        if (windowId == WINDOW_ID_CURRENT) {

Nit: `===`

::: browser/components/extensions/ext-tabs.js:167
(Diff revision 2)
> +        if (filter.windowId != null && tab.windowId != getWindowID(filter.windowId)) {
> +          return false;
> +        }
> +        if (filter.urls) {
> +          let nativeTab = tabTracker.getTab(tab.id);
> +          return filter.urls.matches(nativeTab.linkedBrowser.currentURI);

We need to check whether the extension has permission to read this tab's URI, otherwise the filters can be used to figure out when specific sites are visited. Please use `tab.uri` here. That automatically checks permissions.

::: browser/components/extensions/schemas/tabs.json:321
(Diff revision 2)
> +            "description": "A list of URLs or URL patterns. Events that cannot match any of the URLs will be filtered out.",
> +            "optional": true,
> +            "items": { "type": "string" },
> +            "minItems": 1
> +          },
> +          "propertyNames": {

Can we just call this "properties"?

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:24
(Diff revision 2)
> +      browser.tabs.onUpdated.addListener((tabId, changeInfo) => {
> +        browser.test.log(`got onUpdated ${JSON.stringify(changeInfo)}`);
> +        if (changeInfo.status === "complete") {
> +          browser.test.notifyPass("onUpdated");
> +        }
> +      }, {urls: ["*://mochi.test/*"]});

Please also test that an extension without the tabs permission doesn't get events when it has a URL filter unless it has the activeTab permission for the tab being changed.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:25
(Diff revision 2)
> +        browser.test.log(`got onUpdated ${JSON.stringify(changeInfo)}`);
> +        if (changeInfo.status === "complete") {
> +          browser.test.notifyPass("onUpdated");
> +        }
> +      }, {urls: ["*://mochi.test/*"]});
> +      browser.test.sendMessage("ready");

Not necessary as long as the background script's initialization does any async work. `startup()` doesn't resolve until the top-level of the background script has finished executing.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:59
(Diff revision 2)
> +    background() {
> +      browser.tabs.onUpdated.addListener((tabId, changeInfo) => {
> +        if (changeInfo.status === "complete") {
> +          browser.test.notifyPass("onUpdated");
> +        }
> +      });

Please test this with an actual tab ID. I guess that means opening a tab, getting its ID, adding the listener, and updating the tab.

Also, I winder if we should add some code to automatically cull listeners when the tab or window it has filters for closes...

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:94
(Diff revision 2)
> +    background() {
> +      browser.tabs.onUpdated.addListener((tabId, changeInfo) => {
> +        if (changeInfo.status === "complete") {
> +          browser.test.notifyPass("onUpdated");
> +        }
> +      }, {windowId: browser.windows.WINDOW_ID_CURRENT});

We should probably test that this works with an explicit window ID, too.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:129
(Diff revision 2)
> +        if (Object.keys(changeInfo).some(p => properties.has(p))) {
> +          browser.test.fail(`received unexpected onUpdated event ${JSON.stringify(changeInfo)}`);
> +        }

We should probably just check that Object.keys(changeInfo) is always ["status"]

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8955375 [details]
Bug 1329507 add filtering to tabs.onUpdated,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224554/diff/2-3/

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8955375 [details]
Bug 1329507 add filtering to tabs.onUpdated,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224554/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c03c9417c77
add filtering to tabs.onUpdated, r=kmag

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2c03c9417c77

Comment 15

[21Naown]

Thank you very much!

Comment 16

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 17

[Shane Caraveo (:mixedpuppy)]

has tests.

Comment 18

[Will Bamberg [:wbamberg]]

I've updated the docs page for this, and added some examples: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onUpdated

Please let me know if we need anything else here.

A few things came up:

* the "properties" filter wants "isarticle" (https://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/tabs.json#300) but the actual property name is "isArticle" (https://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/tabs.json#). This works but it seems inconsistent.

* I wanted to make the page clearer about exactly which changes cause this event to be fired. Is it basically: changes to any property that can exist in changeInfo? There are some properties in tabs.Tab that are not in changeInfo, but I don't really understand why they are not. For example, isInReaderMode, height, width.

* it looks as if "changeInfo" in tabs.json (https://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/tabs.json#1398) would give me the list of properties that can appear in the changeInfo argument. But this omits a few properties which do appear, namely title, hidden, isArticle.

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to Will Bamberg [:wbamberg] from comment #18)
> I've updated the docs page for this, and added some examples:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onUpdated
> 
> Please let me know if we need anything else here.
> 
> A few things came up:
> 
> * the "properties" filter wants "isarticle"

unfortunate, but not a big deal.  bug 1461695

> * I wanted to make the page clearer about exactly which changes cause this
> event to be fired. Is it basically: changes to any property that can exist
> in changeInfo? There are some properties in tabs.Tab that are not in
> changeInfo, but I don't really understand why they are not. For example,
> isInReaderMode, height, width.

Some of these it's expected to just get it off the tab.  reader mode will fire a url change iirc.  the tests are checking the tab url is about:reader.

> * it looks as if "changeInfo" in tabs.json

bug 1461693

Comment 20

[robbendebiene]

Is there a reason one cannot use "url" in the properties array? Since "title" and "favIconUrl" are both possible I would assume it's not related to permissions.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Please don't use old bugs to ask questions, there are various forums or even a new bug if you suspect an issue that needs to be corrected.

allowing a filter without permissions would allow an extension to figure out what tabs are open. the restricted set should not be usable without permission, I'll have to look into that.

Comment 22

[robbendebiene]

Sorry for that. I meant that it's not possible to use the "url" property as a filter even if the right permissions are granted. I've created a new ticket for this: https://bugzilla.mozilla.org/show_bug.cgi?id=1680279