Rob Wu [:robwu] Reporter
	Description • 7 years ago

When an extension has the "activeTab" permission [1], and the user clicks on a browserAction button (desktop) or menu item (Android, since bug 1331742 landed), the extension should be granted access to the current page. This works on desktop, but not on Android.

To test:
1. In Firefox for Android, install https://addons.mozilla.org/en-US/firefox/addon/display-_anchors/versions/?page=1#version-1.3
2. Visit a page with anchors, e.g. https://jsfiddle.net/2yofLy3a/embedded/result/
2. Click on the "Display #Anchors" menu item to run a content script in the current tab. Upon success, you should see blue text with "#this-is-an-anchor".
   Upon failure, the extension shows an "Access denied" notification.

Expected:
- The content script injection should succeed, because the "activeTab" permission should give access to the current page.

Actual:
- On Firefox for Android 55, the content script injection fails; an "access denied" notification is shown by the extension.



pageAction.onClicked is also affected by the same bug.

pageAction popups: When a pageAction popup is opened on desktop, the popup page is temporarily given permission to the tab from where the pageAction is clicked. On Android, a pageAction popup opens in a new page, but the extension page is not given access to the tab.


browserAction popups: browserAction popups are not supported yet on Android. But when they do (bug 1370333), the activeTab permission should be applied when the popup is opened.


other APIs relevant for activeTab: On desktop the activeTab permission also grants permissions in the commands API and menus API, but these are not implemented on Android


 [1]: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions#activeTab_permission

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 3 • 7 years ago

The attached patches add support for the activeTab permission on the pageAction and browserAction,
and they also add the new related tests for the onClicked scenario (for both).

I also looked at the popup scenario (at least for the pageAction popup which has been already implemented) and I'm pretty convinced that to make the activeTab actually useful (and tested) in that scenario we also have to change the popup behavior on Android (filed as Bug 1383160), e.g. while an "extension popup tab" is selected, the currently active tab (e.g. the one that can be retrieved using `tabs.query({active: true})` or by using `tabs.executeScript` without the tabId parameter) should be the tab that was selected when the user has clicked the pageAction which opened the popup tab.

Fixing the Android extension popup current behavior is not trivial and so I preferred to move it into a follow up patch attached to Bug 1383160, where we can evaluate and discuss the approach that I'm proposing to make it to work as an extension may expect (especially one ported from Firefox for Desktop / Chrome).

	Shane Caraveo (:mixedpuppy)
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8898290 [details]
Bug 1389911 - Support activeTab permission on Firefox for Android pageAction.

https://reviewboard.mozilla.org/r/169660/#review176458

::: mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html:33
(Diff revision 1)
> +    await browser.pageAction.show(tabs[0].id);
> +
> +    browser.pageAction.onClicked.addListener(async (tab) => {
> +      browser.test.log(`pageAction clicked on tab.id ${tab.id}`);
> +
> +      try {

Cant we do .catch().then() rather than the outer catch?

	Shane Caraveo (:mixedpuppy)
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8898291 [details]
Bug 1389911 - Support activeTab permission on Firefox for Android browserAction.

https://reviewboard.mozilla.org/r/169662/#review176460

::: mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html:103
(Diff revision 1)
> +    await browser.tabs.create({url: "http://example.com"});
> +
> +    browser.browserAction.onClicked.addListener(async (tab) => {
> +      browser.test.log(`browserAction clicked on tab.id ${tab.id}`);
> +
> +      try {

Same comment as other patch about .catch().then().

::: mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html:144
(Diff revision 1)
> +
> +  await extension.awaitMessage("background_page.ready");
> +
> +  const uuid = `{${extension.uuid}}`;
> +
> +  ok(BrowserActions.isShown(uuid), "browser action is shown");

Isn't this always the case?

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 8 • 7 years ago
mozreview-review-reply

Comment on attachment 8898290 [details]
Bug 1389911 - Support activeTab permission on Firefox for Android pageAction.

https://reviewboard.mozilla.org/r/169660/#review176458

> Cant we do .catch().then() rather than the outer catch?

yeah, I agree, we can make it simpler with a `.catch(error => {...})`.

I applied this change to both the updated test cases.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 9 • 7 years ago
mozreview-review-reply

Comment on attachment 8898291 [details]
Bug 1389911 - Support activeTab permission on Firefox for Android browserAction.

https://reviewboard.mozilla.org/r/169662/#review176460

> Isn't this always the case?

yeah, that is supposed to be always shown, that assertion is there mostly to make a regression easier to identify (e.g. to be able to notice if the popup has never be opened because the browser action wasn't shown at all or if , after clicking on it, it doesn't open the popup or run the code from the popup).

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 12 • 7 years ago

I've more deeply tested these two patches against intermittency on the android x86 emulator, and I've been able to make it fails intermittently locally using "./mach mochitest ... --repeat N".

The reason for the intermittent failures is related to the way the activeTab permission works:

when the user click on the extension pageAction/browserAction, the activeTab permission is given to the extension on the page that is currently loaded in the tab (based on its innerWindowID), this way the permission is automatically revoked when a new page is loaded in the same tab.

On the android x86 emulator, the new activeTab permission tests are able to run so fast that the pageAction/browserAction could have been clicked when the tab is still loading the initial about:blank document, then the tab loads the actual final URL, but the activeTab permission has been added on the about:blank document and it is not valid for the final URL loaded in the tab, which will make the test to fail (raising the "Missing permission" on the `browser.tabs.executeScript` API call).

The updated version of the tests for the activeTab permission are now waiting the tab to be fully loaded before proceeding to click the pageAction/browserAction, which is going to prevent the test from failing intermittently on android x86.

	Pulsebot
	Comment 13 • 7 years ago

Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/5f0a55c594ee
Support activeTab permission on Firefox for Android pageAction. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/dfd6abc765bc
Support activeTab permission on Firefox for Android browserAction. r=mixedpuppy

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 14 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5f0a55c594ee
https://hg.mozilla.org/mozilla-central/rev/dfd6abc765bc

	CosminB
	Comment 15 • 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 content script is injected into the current active tab. 

Please see the attached video: