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

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[YUKI "Piro" Hiroshi]

Attachment: failure.xpi: an example for the failure case

"<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.

Comment 1

[YUKI "Piro" Hiroshi]

Attachment: success.xpi: an example for the success case

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"
   }
 }
--------

Comment 2

[YUKI "Piro" Hiroshi]

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.

Comment 3

[David Durst [:ddurst]]

Zombie -- do you mind taking a look at this?

Comment 4

[Tomislav Jovanovic :zombie]

Looking into.

Comment 5

[Tomislav Jovanovic :zombie]

Attachment: Bug 1436450 - Fix <all_urls> as an optional API permission

Review commit: https://reviewboard.mozilla.org/r/231332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231332/

Comment 6

[Tomislav Jovanovic :zombie]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231332/diff/1-2/

Comment 7

[Tomislav Jovanovic :zombie]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231332/diff/2-3/

Comment 8

[Andrew Swan [:aswan]]

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.

Comment 9

[Tomislav Jovanovic :zombie]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231332/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a641d9546c6
Fix <all_urls> as an optional API permission r=aswan

Comment 11

[Cosmin Sabou [:CosminS]]

Backed out for xpcshell failure on xpcshell/test_ext_permissions.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=3a641d9546c6518263b55ae5e32cf9150cf47629&tochange=e67d08c731d74f5eae9fc2b20fe564121dba9382&filter-searchStr=xpcshell%20x6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&selectedJob=170801342

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170801342&repo=autoland&lineNumber=3129

Backout link: https://hg.mozilla.org/integration/autoland/rev/e67d08c731d74f5eae9fc2b20fe564121dba9382

Comment 12

[Tomislav Jovanovic :zombie]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231332/diff/4-5/

Comment 13

[Tomislav Jovanovic :zombie]

Bah, sorry about that.  Always run the test, no matter how small the change..

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e31b828d6003
Fix <all_urls> as an optional API permission r=aswan

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/e31b828d6003

Comment 16

[YUKI "Piro" Hiroshi]

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."

Comment 17

[Tomislav Jovanovic :zombie]

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.

Comment 18

[YUKI "Piro" Hiroshi]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5dc2bed05d2a