1234086 - tabs.onAttached is defined in the schema but not implemented or marked unsupported

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P4)

Description

[Hans Meyer]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20151220030223

Steps to reproduce:

Call chrome.tabs.onAttached.addListener(<handler fct>)


Actual results:

Exception thrown: TypeError: schemaApi[ns][name] is undefined

Comment 1

[Andreas Wagner [:TheOne] [use NI]]

Kris, as you added the "good first bug" tag, could you please add some details about the scope of the bug and some guidance how and where to fix this?

Comment 2

[Kris Maglione [:kmag]]

Matt, do you mind if I take this? It's a lot more complicated than I expected. It's probably not a good bug for your first month.

Comment 3

[Matthew Wein [:mattw]]

Sure, no problem!

Comment 4

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234086: [webext] Add support for tabs.onAttached/onDetached events.

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

Comment 5

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/34605/#review31313

::: browser/components/extensions/ext-tabs.js:174
(Diff revision 1)
> +          if (window.arguments[0] instanceof window.XULElement) {

This isn't tested in this patch. I have a test for it in an upcoming patch for bug 1247493 (which adds a tabId property to windows.create to convert a tab to a window), since I needed that functionality to test this, and didn't want to try to rebase this on top of it.

Comment 6

[Kris Maglione [:kmag]]

Actually, I guess it will probably be easier to review if I submit all of the related patches in one go. The rest of these have different bug numbers, but I suppose mozreview will dump all of them in this bug.

Comment 7

[Kris Maglione [:kmag]]

Ah. "abort: cannot submit reviews referencing multiple bugs"

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8718571 [details]
MozReview Request: Bug 1234086: [webext] Add support for tabs.onAttached/onDetached events.

https://reviewboard.mozilla.org/r/34605/#review31829

::: browser/components/extensions/ext-tabs.js:188
(Diff revision 1)
> +            tab.addEventListener("TabClose", function listener(event) {
> +              tab.removeEventListener("TabClose", listener);

There are a couple cases in this patch where we fire an event handler a little late. If the extension removed this event listener in the middle, we could end up calling the listener after it was removed.

However, I realize now that this can happen with almost any one of our callbacks. We call most of them asynchronously.

If you don't fix these cases here, can you at least make sure that a solution for the more general problem will fix these cases as well?

::: browser/components/extensions/ext-tabs.js:225
(Diff revision 1)
> +          fire(tabId, {windowId, isWindowClosing});

I see some inconsistent spacing around braces in this patch. Can we add an ESLint rule for that? It looks like "object-curly-spacing" is the one we want. I vote for "never", but I don't care too much as long as everything is consistent.

::: browser/components/extensions/ext-tabs.js:237
(Diff revision 1)
> +        let windowListener = window => {
> +          for (let tab of window.gBrowser.tabs) {
> +            fireForTab(tab, true);
> +          }
> +        };

What happens when we drag the last tab out of a window? It sorta looks like we might close the window before we close the tab (hence causing onRemoved to fire incorrectly), but I'm not sure. Can you please test?

::: browser/components/extensions/ext-utils.js:522
(Diff revision 1)
> +    if (!this._initialized) {

I'd rather we check this in initListener and return immediately.

::: browser/components/extensions/test/browser/browser_ext_tabs_events.js:28
(Diff revision 1)
> +        setTimeout(resolve, 0);

What's this for?

Comment 9

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/34605/#review31829

> There are a couple cases in this patch where we fire an event handler a little late. If the extension removed this event listener in the middle, we could end up calling the listener after it was removed.
> 
> However, I realize now that this can happen with almost any one of our callbacks. We call most of them asynchronously.
> 
> If you don't fix these cases here, can you at least make sure that a solution for the more general problem will fix these cases as well?

Yeah, I actually asked Andy to file a bug about that last week. I'll check if he's filed one yet. If not, I'll file one today.

> I see some inconsistent spacing around braces in this patch. Can we add an ESLint rule for that? It looks like "object-curly-spacing" is the one we want. I vote for "never", but I don't care too much as long as everything is consistent.

Yeah, "never" is fine with me. I think it'll require a fair number of code changes before we can turn it on, though. Our code is pretty inconsistent about this.

> What happens when we drag the last tab out of a window? It sorta looks like we might close the window before we close the tab (hence causing onRemoved to fire incorrectly), but I'm not sure. Can you please test?

Hm. Yeah, I thought I had a test for that already. I think you're right, it will probably need a special case.

> What's this for?

Just paranoia, really. A couple of the event dispatchers delay firing their listeners immediately, so I wanted to avoid the possibility that they'd sometimes fire after the promise resolved. `setTimeout` callbacks handlers always return to the event loop, so they'll always happen after any pending queued promises.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Kris Maglione [:kmag] from comment #9)
> Yeah, "never" is fine with me. I think it'll require a fair number of code
> changes before we can turn it on, though. Our code is pretty inconsistent
> about this.

I've never actually run eslint before, but can't we use --fix and check in a patch? You have an r+ for that from me :-).

Comment 11

[Kris Maglione [:kmag]]

Heh. Yeah, it looks like that rule does support --fix. I'll try it and see how well it works.

Comment 12

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/34605/#review31829

> Hm. Yeah, I thought I had a test for that already. I think you're right, it will probably need a special case.

I'm going to file a follow-up for this. It looks like it currently works with the default settings, because the tab being detached is replaced with a new, blank tab in the old window, just before it closes. So we also get onCreated and onRemoved events for that tab. I'm not sure this is the best way to handle it, so I'll file a new bug to improve that behavior.

Comment 13

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/5ec466af51e171623333ad644a94034b55b73b08
Bug 1234086: [webext] Add support for tabs.onAttached/onDetached events. r=billm

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out because either it or one of the other patches pushed together broke some tests: https://treeherder.mozilla.org/logviewer.html#?job_id=7363470&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/93eec7ef1897

Comment 15

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/a225af1e3baa8c207025b7d4afa06727a32fe40f
Bug 1234086: [webext] Add support for tabs.onAttached/onDetached events. r=billm

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a225af1e3baa