sender.tab is undefined for tab #0

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
WebExtensions: Android
P1
normal
RESOLVED FIXED
16 days ago
8 days ago

People

(Reporter: Manish Jethani, Assigned: rpl)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

16 days ago
Created attachment 8894517 [details]
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
(Reporter)

Comment 1

16 days ago
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.

Updated

15 days ago
Component: General → WebExtensions: Android
Product: Firefox for Android → Toolkit

Updated

15 days ago
Blocks: 467520
(Reporter)

Comment 2

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

Updated

15 days ago
Blocks: 1226547
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
No longer blocks: 1226547

Updated

15 days ago
Blocks: 1226547

Updated

12 days ago
Assignee: nobody → mixedpuppy
Priority: -- → P1
Desktop starts with id=1.  Android uses nativeTab.id which may be zero.  I'm guessing you cannot reproduce this on desktop, correct?
Flags: needinfo?(manish.jethani)

Comment 5

12 days ago
Yes, he already said so in comment 2.
Flags: needinfo?(manish.jethani)
Comment hidden (mozreview-request)
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.
Assignee: mixedpuppy → lgreco
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 days ago
Attachment #8896396 - Flags: review?(lgreco)
Attachment #8896397 - Flags: review?(mixedpuppy)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 days ago
Attachment #8896396 - Flags: review?(tomica)
Attachment #8896397 - Flags: review?(tomica)
Attachment #8896018 - Attachment is obsolete: true

Comment 13

11 days ago
mozreview-review
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
Attachment #8896396 - Flags: review?(tomica) → review+

Comment 14

11 days ago
mozreview-review
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.
Attachment #8896397 - Flags: review?(tomica)
Comment hidden (mozreview-request)

Comment 16

10 days ago
mozreview-review
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).
Attachment #8896397 - Flags: review?(tomica) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

8 days ago
mozreview-review
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
Attachment #8896397 - Flags: review?(mixedpuppy) → review+

Comment 20

8 days ago
mozreview-review
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
Attachment #8896396 - Flags: review+

Comment 21

8 days ago
Pushed by luca.greco@alcacoop.it:
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

8 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34001544cd69
https://hg.mozilla.org/mozilla-central/rev/dde62a1a7b0b
Status: UNCONFIRMED → RESOLVED
Last Resolved: 8 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.