Closed
Bug 1388066
Opened 7 years ago
Closed 7 years ago
sender.tab is undefined for tab #0
Categories
(WebExtensions :: Android, defect, P1)
WebExtensions
Android
Tracking
(firefox57 verified, firefox58 verified)
VERIFIED
FIXED
mozilla57
People
(Reporter: manish.jethani, Assigned: rpl)
References
Details
Attachments
(4 files, 1 obsolete file)
onMessage.gif
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•7 years 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•7 years ago
|
Component: General → WebExtensions: Android
Product: Firefox for Android → Toolkit
|
|
Reporter | |
Comment 2•7 years ago
|
||
Interestingly there is no tab #0 in Firefox for desktop.
|
||
Updated•7 years ago
|
Blocks: webext-port-abp
|
||
Comment 3•7 years ago
|
||
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: webext-port-abp
|
|
||
Updated•7 years ago
|
Blocks: webext-port-abp
|
||
Updated•7 years ago
|
Assignee: nobody → mixedpuppy
Priority: -- → P1
|
||
Comment 4•7 years ago
|
||
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 hidden (mozreview-request) |
|
|
||
Comment 7•7 years ago
|
||
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
|
||
Comment 8•7 years ago
|
||
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•7 years ago
|
Attachment #8896396 -
Flags: review?(lgreco)
Attachment #8896397 -
Flags: review?(mixedpuppy)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8896396 -
Flags: review?(tomica)
Attachment #8896397 -
Flags: review?(tomica)
|
|
||
Updated•7 years ago
|
Attachment #8896018 -
Attachment is obsolete: true
|
|
||
Comment 13•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/34001544cd69 https://hg.mozilla.org/mozilla-central/rev/dde62a1a7b0b
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
|
||
Comment 23•7 years ago
|
||
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.
|
||
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•