Unavailable CanvasRenderingContext2D.drawWindow() with "<all_urls>" in "optional_permissions"

RESOLVED FIXED in Firefox 60

Status

defect
P2
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: yuki, Assigned: zombie)

Tracking

Trunk
mozilla61
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox60 fixed, firefox61 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

a year ago
"<all_urls>" permission is required to capture tab contents by "CanvasRenderingContext2D.drawWindow()"(*), but it still fail if the permission is granted optionally.

(*)https://discourse.mozilla.org/t/drawwindow-in-webextension/13811/4


Steps to reproduce:

 1. Create an addon with "optional_permissions" including "<all_urls>".
 2. Load the addon by "about:debugger".
 3. Call "browser.permissions.request()" to grant "<all_urls>" permission.
 3. Call "CanvasRenderingContext2D.drawWindow()" in a content script.

Expected result:
The operation succeeds and I successfully get a captured image.

Actual result:
"Error: context.drawWindow is not a function" is reported and the operation fails.
Reporter

Comment 1

a year ago
After you loaded attached addons "success.xpi" and "failure.xpi", you'll see two toolbar buttons added by them. Click on the "success case" button opens a tab and its content is immediately replaced to an image generated from captured image. On the other hand, click on the "failure case" button also opens a tab but only an error is reported.

There are differences only around codes to grant optional permissions. Actual diff:

--------
$ diff -ur success failure
diff -ur success/background.js failure/background.js
--- success/background.js       2018-02-08 02:59:57.645307300 +0900
+++ failure/background.js       2018-02-08 02:28:50.465952800 +0900
@@ -1,4 +1,7 @@
 browser.browserAction.onClicked.addListener(async () => {
+  const granted = await browser.permissions.request({ origins: ['<all_urls>'] });
+  if (!granted)
+    return;
   const tab = await browser.tabs.create({
     url:    '/background.js',
     active: true
diff -ur success/manifest.json failure/manifest.json
--- success/manifest.json       2018-02-08 02:56:19.327771900 +0900
+++ failure/manifest.json       2018-02-08 02:14:01.473073700 +0900
@@ -1,12 +1,14 @@
 {
   "manifest_version": 2,
-  "name": "success case",
+  "name": "failure case",
   "version": "1",
   "author": "YUKI \"Piro\" Hiroshi",
-  "description": "success case",
+  "description": "failure case",
   "permissions": [
     "activeTab",
-    "tabs",
+    "tabs"
+  ],
+  "optional_permissions": [
     "<all_urls>"
   ],
   "background": {
@@ -15,6 +17,6 @@
     ]
   },
   "browser_action": {
-    "default_title": "success case"
+    "default_title": "failure case"
   }
 }
--------
Reporter

Comment 2

a year ago
I don't know it is related to this problem, but in such a situation "browser.tabs.captureTab()" and "browser.tabs.captureVisibleTab()" are also unavailable.
Zombie -- do you mind taking a look at this?
Flags: needinfo?(tomica)
Assignee

Comment 4

a year ago
Looking into.
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Flags: needinfo?(tomica)

Updated

a year ago
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8962534 - Flags: review?(aswan)

Comment 8

a year ago
mozreview-review
Comment on attachment 8962534 [details]
Bug 1436450 - Fix <all_urls> as an optional API permission

https://reviewboard.mozilla.org/r/231332/#review237338

::: toolkit/components/extensions/ext-permissions.js:37
(Diff revision 3)
> +          if (origins.includes("<all_urls>")) {
> +            permissions.push("<all_urls>");
> +          }

I guess this just gets ignored when we build strings for the permission prompt since we don't have a string for it?  How about just moving this to after the prompt has been accepted instead.
Attachment #8962534 - Flags: review?(aswan) → review+
Comment hidden (mozreview-request)

Comment 10

a year ago
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3a641d9546c6
Fix <all_urls> as an optional API permission r=aswan
Comment hidden (mozreview-request)
Assignee

Comment 13

a year ago
Bah, sorry about that.  Always run the test, no matter how small the change..
Flags: needinfo?(tomica)

Comment 14

a year ago
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e31b828d6003
Fix <all_urls> as an optional API permission r=aswan

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e31b828d6003
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Reporter

Comment 16

a year ago
The patch seems to need to be uplifted to Firefox 60.

Extensions in Firefox 60 | Mozilla Add-ons Blog
https://blog.mozilla.org/addons/2018/04/02/extensions-firefox-60/

This blog post advertises tabs.captureTab and tabs.captureVisibleTab as:

> The tabs API now supports a tabs.captureTab method which can be passed
> a tabId to capture the visible area of the specified tab (as opposed to
> the tabs.captureVisibleTab API, which only captured the active tab of a
> window).

but my addon cannot use them because this bug blocks to call them with optional permissions. Of course I can add "<all_urls>" to the permanent permissions, but I moved it from the permanent permissions to optional in an old version because I got a feedback like that: "I cannot install this addon due to my company policy, because it requires <all_urls>" permission. If it is not a critical feature, please make the permission optional."
Assignee

Comment 17

a year ago
Hi Piro, I'm not sure that's reason enough to uplift, but feel free to nominate for beta, and Julien (or another release driver) will decide.
Reporter

Comment 18

a year ago
Comment on attachment 8962534 [details]
Bug 1436450 - Fix <all_urls> as an optional API permission

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Addons cannot use some features advertised at https://blog.mozilla.org/addons/2018/04/02/extensions-firefox-60/ on Firefox 60
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: This patch just migrates a special permission from optional permissions. The added permission is defined as a fixed literal in the code, and incoming string is not used.
[String changes made/needed]: None
Attachment #8962534 - Flags: approval-mozilla-beta?
Comment on attachment 8962534 [details]
Bug 1436450 - Fix <all_urls> as an optional API permission

Simple fix with new test coverage for a new feature being advertised in Fx60. Approved for 60.0b12.
Attachment #8962534 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.