Closed Bug 1389911 Opened 3 years ago Closed 3 years ago

activeTab permission not implemented on Android (browserAction.onClicked, pageAction.onClicked, pageAction popup)

Categories

(WebExtensions :: Android, defect, P1)

55 Branch
defect

Tracking

(firefox57 verified, firefox58 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: robwu, Assigned: rpl)

References

Details

Attachments

(3 files)

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
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Priority: -- → P1
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).
Attachment #8898290 - Flags: review?(mixedpuppy)
Attachment #8898291 - Flags: review?(mixedpuppy)
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?
Attachment #8898290 - Flags: review?(mixedpuppy) → 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?
Attachment #8898291 - Flags: review?(mixedpuppy) → review+
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.
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).
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.
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
Blocks: 1370333
No longer depends on: 1370333
https://hg.mozilla.org/mozilla-central/rev/5f0a55c594ee
https://hg.mozilla.org/mozilla-central/rev/dfd6abc765bc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Attached image Inject.gif
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:
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.