1240631 - Callback not called for chrome.tabs.move when tab is invalid

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P5)

Description

[Andy McKay]

If you pass through in an invalid id to chrome.tabs.move the invalid tags are silently ignored. In chrome the callback is called with undefined as the tab parameter. Thanks rpl for pointing this out.

Comment 1

[Kris Maglione [:kmag]]

Unless it's also supposed to set lastError, I think this is fixed.

Comment 2

[Andy McKay]

In Chrome I get null passed through to the callback and lastError is set: "No tab with id: 129731629.".

In Firefox I get an empty array passed through to the callback and lastError is not set.

Comment 3

[Andy McKay]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f17208906b8b

Comment 4

[Andy McKay]

Attachment: bug 1240631 so that both invalid windows and tabs hit runtime.lastError

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

Comment 5

[Andy McKay]

Note that this implementation follows the slightly inferior Chrome implementation, if you pass a list of tabIds it will go through them in order, moving them until it hits an invalid tab and then it fails and sets last error.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Andy McKay [:andym] from comment #5)
> Note that this implementation follows the slightly inferior Chrome
> implementation, if you pass a list of tabIds it will go through them in
> order, moving them until it hits an invalid tab and then it fails and sets
> last error.

Hmm... It seems like if we're rejecting, it should be all or nothing... I don't like the idea of the call failing but still having side-effects.

Comment 7

[Andy McKay]

(In reply to Kris Maglione [:kmag] from comment #6)
> (In reply to Andy McKay [:andym] from comment #5)
> Hmm... It seems like if we're rejecting, it should be all or nothing... I
> don't like the idea of the call failing but still having side-effects.

I was surprised Chrome does it that way and I definitely prefer a more all or nothing approach. Especially since the tabs that do move don't even come in the callback. This feels like a pretty small edge case where we should diverge from Chrome.

Comment 8

[Andy McKay]

Comment on attachment 8800509 [details]
bug 1240631 so that both invalid windows and tabs hit runtime.lastError

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

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8800509 [details]
bug 1240631 so that both invalid windows and tabs hit runtime.lastError

https://reviewboard.mozilla.org/r/85422/#review86304

r=me with the changes mentioned below.

::: browser/components/extensions/ext-tabs.js:853
(Diff revision 2)
> -        for (let tab of tabs) {
> +        for (let [tabId, tab] of tabs.entries()) {
> +            if (!tab) {
> +              return Promise.reject({message: `Invalid tab ID: ${tabId}`});
> +            }
> +        }

`TabManager.getTab` will throw if any of these tab IDs don't exist, so this check isn't necessary. Also means that this code should have the same behavior as the current code...

So we should just need the invalid window handling and test changes.

::: browser/components/extensions/test/browser/browser_ext_tabs_move.js:74
(Diff revision 2)
> -          browser.tabs.move([12345, tab.id], {index: 0});
> +          browser.tabs.move([tab.id, 12345], {index: 0}, (tabs) => {
> +            browser.test.assertEq(tabs, undefined);
> +            browser.test.assertTrue(/Invalid tab/.test(browser.runtime.lastError.message),
> +                                    "runtime.lastError should report the expected error message");
> +          });

Please use the promise-based variant instead. `browser.test.fail` if the promise resolves, and chain the `browser.tabs.query` call after the rejection handler.

::: browser/components/extensions/test/browser/browser_ext_tabs_move_window.js:31
(Diff revision 2)
> +        // Assuming that this windowId does not exist.
> +        browser.tabs.move(source.id, {windowId: 123144576, index: 0}, tabs => {
> +            browser.test.assertEq(tabs, undefined);
> +            browser.test.assertTrue(/Invalid window/.test(browser.runtime.lastError.message),
> +                                    "runtime.lastError should report the expected error message");
> +        });

Same as above, please use the promise variant.

::: browser/components/extensions/test/browser/browser_ext_tabs_move_window.js:37
(Diff revision 2)
> +
> +

ESLint will fail for padding at start or end of blocks.

Comment 10

[Andy McKay]

Comment on attachment 8800509 [details]
bug 1240631 so that both invalid windows and tabs hit runtime.lastError

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a52b9538af4
so that both invalid windows and tabs hit runtime.lastError r=kmag

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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