1388066 - sender.tab is undefined for tab #0

Status: VERIFIED FIXED

(WebExtensions :: Android, defect, P1)

Description

[Manish Jethani]

Attachment: An example extension

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce:

Install the attached extension, restart the browser, go to http://www.example.com/ in the first tab


Actual results:

The onMessage handler in the background page throws the "Unknown tab" error


Expected results:

The onMessage handler should print "Tab #0 says: Hello, background.js" to the console

Comment 1

[Manish Jethani]

Note that this only happens for the first tab (tab #0). If you open a new tab and visit http://www.example.com/ in it, the background page correctly gets the tabs.Tab object.

Comment 2

[Manish Jethani]

Interestingly there is no tab #0 in Firefox for desktop.

Comment 3

[Tom S. (please needinfo tschuster)]

I haven't actually looked deeply into this, but just looking through the Messenger, we end up calling tabGetSender, which treats 0 tab ids like they don't exist:
http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/browser/components/extensions/ext-browser.js#23
http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/mobile/android/components/extensions/ext-android.js#20

Comment 4

[Shane Caraveo (:mixedpuppy)]

Desktop starts with id=1.  Android uses nativeTab.id which may be zero.  I'm guessing you cannot reproduce this on desktop, correct?

Comment 5

[Wladimir Palant]

Yes, he already said so in comment 2.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1388066 fix when tabId is zero

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

Comment 7

[Shane Caraveo (:mixedpuppy)]

Luca, I'm going to have to pass this off to you for testing.  I put up a patch I think will address the problem.

Comment 8

[Tomislav Jovanovic :zombie]

Working on related code in bug 1377734, Shane suggested this is related, since tabId can be 0 on android:

https://reviewboard.mozilla.org/r/167332/diff/1-2/

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1388066 - Fix message sender.tab when tabId is 0 on Firefox for Android.

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

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1388066 - Add test for message sender.tab when tabId is 0 on Firefox for Android.

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

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8896396 [details]
Bug 1388066 - Fix message sender.tab when tabId is 0 on Firefox for Android.

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

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8896397 [details]
Bug 1388066 - Add test for message sender.tab when tabId is 0 on Firefox for Android.

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

Comment 13

[Tomislav Jovanovic :zombie]

Comment on attachment 8896396 [details]
Bug 1388066 - Fix message sender.tab when tabId is 0 on Firefox for Android.

https://reviewboard.mozilla.org/r/167638/#review172976

Comment 14

[Tomislav Jovanovic :zombie]

Comment on attachment 8896397 [details]
Bug 1388066 - Add test for message sender.tab when tabId is 0 on Firefox for Android.

https://reviewboard.mozilla.org/r/167640/#review172870

This test looks good (a few nits), but based on the code Shane pointed to in bug 1377734, I suspect `tabs.sendMessage(0, ...` doesn't work either without that change, so we should add a test for that too.

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_sendMessage.html:241
(Diff revision 2)
>  
>    yield extension.unload();
>  });
> +
> +
> +add_task(function* tabsSendMessageTabId0() {

I think we can just use async/await everywhere now.

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_sendMessage.html:255
(Diff revision 2)
> +      }
> +
> +      await browser.runtime.onMessage.addListener((msg, sender) => {
> +        browser.test.assertEq(msg, "message from tabId 0",
> +                              "Got the expected message from a content script");
> +        browser.test.assertEq(0, sender.tab && sender.tab.id,

nit: I'd rather you either split `sender.tab` in a separate assert, or just omit it.  Either way, it would be a less ambiguous test failure message/exception.

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_sendMessage.html:264
(Diff revision 2)
> +
> +      await browser.tabs.executeScript(0, {code: `new ${contentScriptCode}`});
> +    },
> +  });
> +
> +  yield Promise.all([

Promise.all() is mostly useful when you have two independent promises.  I don't think it does much here, as both are resolved by running the same code.

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8896397 [details]
Bug 1388066 - Add test for message sender.tab when tabId is 0 on Firefox for Android.

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

Comment 16

[Tomislav Jovanovic :zombie]

Comment on attachment 8896397 [details]
Bug 1388066 - Add test for message sender.tab when tabId is 0 on Firefox for Android.

https://reviewboard.mozilla.org/r/167640/#review173098

Hey Luca, did you miss my ask for the `tabs.sendMessage(0, "msg")` test above?

r=me for this code if you want to leave that for a separate bug (but please file it).

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8896396 [details]
Bug 1388066 - Fix message sender.tab when tabId is 0 on Firefox for Android.

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

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8896397 [details]
Bug 1388066 - Add test for message sender.tab when tabId is 0 on Firefox for Android.

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

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8896397 [details]
Bug 1388066 - Add test for message sender.tab when tabId is 0 on Firefox for Android.

https://reviewboard.mozilla.org/r/167640/#review173612

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8896396 [details]
Bug 1388066 - Fix message sender.tab when tabId is 0 on Firefox for Android.

https://reviewboard.mozilla.org/r/167638/#review173616

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34001544cd69
Fix message sender.tab when tabId is 0 on Firefox for Android. r=mixedpuppy,zombie
https://hg.mozilla.org/integration/autoland/rev/dde62a1a7b0b
Add test for message sender.tab when tabId is 0 on Firefox for Android. r=mixedpuppy,zombie

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34001544cd69
https://hg.mozilla.org/mozilla-central/rev/dde62a1a7b0b

Comment 23

[CosminB]

Attachment: onMessage.gif

This issue is verified as fixed on Fennec 58.0a1 (2017-10-17) and Fennec 57.0b9 under Android 6.0.1

The following message: Tab #0 says: Hello, background.js   background.js:6:3
                       background.js says: Hi, tab #0      content.js:3:3

is displayed in the browser console.

Please see the attached video.