1343179 - Permissions pop-up appears every time a sideloaded webextension is enabled

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Vasilica Tamas, QA [:vtamas]]

Attachment: webext.gif

[Note]
This is a follow-up bug for Bug 1334096

[Affected versions]:
Firefox 54.0a1 (2017-02-27)

[Affected platforms]:
Windows 10 64-bit
Ubuntu 16.04 32-bit


[Steps to reproduce]:
1.Launch Firefox with clean profile.
2.Create extensions.webextPermissionPrompts and set it to true.
3.Install via sideloading method a webextension with permissions. 
4.After reopening the browser, navigate to about:add-ons and click on Add-ons Manager “Enable” button.
5.a) Click “Enable” button from permissions doorhanger.
6.a) Disable the extension.
7.a) Click again on Add-ons Manager “Enable” button

5.b) Click “Cancel” button from permissions doorhanger.
6.b) Click again on Add-ons Manager “Enable” button

[Expected Results]:
Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1334096#c2 and https://bugzilla.mozilla.org/show_bug.cgi?id=1334096#c3 , permissions pop-up should be displayed only the first time an extension is enabled avoiding the regular sideloading flow.


[Actual Results]:
Permissions pop-up is displayed every time a sideloaded webextension is enabled from Add-ons Manager.

Comment 1

[:shell escalante]

aswan would need to explain the change, but change itself should be fairly minor

Comment 2

[Andrew Swan [:aswan]]

The object that represents individual addons has a "seen" property that is supposed to get set the first time a prompt is shown for a sideloaded extension.  This property is set when the entry in the hamburger menu is selected but it is not set when the extension is manually enabled from about:addons.  Setting that property (and adding appropriate tests) should address this bug.

Comment 3

[Arshad Kazmi [:arshadkazmi42]]

I am interested to work on this. Is this still available? Also can someone guide me where should I start for this?

Comment 4

[Andrew Swan [:aswan]]

Hi, this is still available, thanks for your interest!

There's an overview of a proposed solution in comment 2.  The code that runs when an extension is manually enabled is here:
https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/toolkit/mozapps/extensions/content/extensions.js#1123-1145

The addon object there has a method called markAsSeen() that will set the seen property so that prompts are not shown in the future.

The larger chunk of work will be writing a test for this.  This will need to be a browser mochitest, the easiest thing here is probably to look at some of the existing tests in toolkit/mozapps/extensions/test/browser/ to get an idea for how these tests are written, then try to construct a new one.

Comment 5

[Arshad Kazmi [:arshadkazmi42]]

ok. cool.
I will have a look into it and will get back to you if I need any more info

Comment 6

[Arshad Kazmi [:arshadkazmi42]]

So I added that change of markAsSeen()

I want to check it now, so wanted to follow these steps

[Steps to reproduce]:
[1].Launch Firefox with clean profile.
[2].Create extensions.webextPermissionPrompts and set it to true.
[3].Install via sideloading method a webextension with permissions. 
[4].After reopening the browser, navigate to about:add-ons and click on Add-ons Manager “Enable” button.
[5].a) Click “Enable” button from permissions doorhanger.
[6].a) Disable the extension.
[7].a) Click again on Add-ons Manager “Enable” button

5.b) Click “Cancel” button from permissions doorhanger.
6.b) Click again on Add-ons Manager “Enable” button

I have few questions,
1. What is [3] points mean? Install via sideloading? What is sideloading?
2. Is there a sample extension available to check this? Or should I go ahead and create a small plugin?

Comment 7

[Andrew Swan [:aswan]]

(In reply to Arshad Kazmi from comment #6)
> [2].Create extensions.webextPermissionPrompts and set it to true.

this step is no longer necessary, this has been the default for some time

> 1. What is [3] points mean? Install via sideloading? What is sideloading?

See:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Alternative_distribution_options/Sideloading_add-ons#Installation_using_the_standard_extension_folders

> 2. Is there a sample extension available to check this? Or should I go ahead
> and create a small plugin?

I don't think you need to creat a new one, any existing extension (that has required permissions) should be fine.

Comment 8

[Arshad Kazmi [:arshadkazmi42]]

Do you know any such extension which I can get from Mozilla extension store?

Comment 9

[Andrew Swan [:aswan]]

(In reply to Arshad Kazmi from comment #8)
> Do you know any such extension which I can get from Mozilla extension store?

Any extension on AMO that prompts you for permissions when you install it will do.

Comment 10

[Arshad Kazmi [:arshadkazmi42]]

I tried with this addon https://addons.mozilla.org/en-GB/firefox/addon/turbo-download-manager/?src=search

I am not able to reproduce the issue.

1. Installed the addon, during installation it asked for permission. I Accepted it
2. Disabled extension
3. Closed browser
4. Reopened browser and enable the extension.

I didn't got permission popup again
I repeated 3-4 steps 2-3 times. But I didn't get the permission popup again.

Is this the correct way to reproduce this?

I tried this on latest mozilla-central build

Comment 11

[Andrew Swan [:aswan]]

The steps in comment 10 are not the same as the original steps from comment 0.
Specifically, the original step 3 was to sideload the extension, you omitted that step here.

Comment 12

[Arshad Kazmi [:arshadkazmi42]]

How to sideload the extension?

Comment 13

[Andrew Swan [:aswan]]

See comment 7.
You can also drop the extension into the profile directory, following the same rules for naming the file as described on the MDN page linked from that comment.

Comment 14

[Arshad Kazmi [:arshadkazmi42]]

Tried following the step from Comment 7.

I am  not getting any installation confirmation about the addon.

So I donwloaded xpi file from Comment 10

renamed it to "[e-mail]"

copied it to this location

(I am on MacOS)
/Library/Application Support/Mozilla/Extensions/{ec8030f7-c20a-464f-9b0e-13a3a9e97384}


Then restarted firefox.

Also how can I install by sideloading in my debug version of firefox, which runs from Mozilla Central repo?

Comment 15

[Andrew Swan [:aswan]]

The filename needs to match the id in the extension, not something you made up.
The extension linked in comment 10 has the id "jid0-dsq67mf5kjjhiiju2dfb6kk8dfw@jetpack"

Comment 16

[Arshad Kazmi [:arshadkazmi42]]

So for that extension, this should be the filename 

"[e-mail]"

?

Comment 17

[Arshad Kazmi [:arshadkazmi42]]

ok. got it. fixed issue also.

Just need to verify with one more addon. So got one question.

How can I find id of an addon from addon store?

Comment 18

[Arshad Kazmi [:arshadkazmi42]]

need info

Comment 19

[Arshad Kazmi [:arshadkazmi42]]

No issues. Found it. Just extracted the xpi file using unarchiver and got manifest.json file

Comment 20

[Arshad Kazmi [:arshadkazmi42]]

I am done with the fix. 
Was trying to write test script for it.

Can you help me with getting started in sideloading addon using script & enable disable addon

Comment 21

[Arshad Kazmi [:arshadkazmi42]]

Test Script**

Comment 22

[Andrew Swan [:aswan]]

Take a look at the existing test browser/base/content/test/webextensions/browser_extension_sideloading.js

In particular, you can use something like this to create and sideload an extension:
https://searchfox.org/mozilla-central/rev/6201a9e0067cf6af118c6a99ae9314b8ceb2c4d5/browser/base/content/test/webextensions/browser_extension_sideloading.js#8-26

Followed by this to get the browser to notice that it is there:
https://searchfox.org/mozilla-central/rev/6201a9e0067cf6af118c6a99ae9314b8ceb2c4d5/browser/base/content/test/webextensions/browser_extension_sideloading.js#120

Comment 23

[Arshad Kazmi [:arshadkazmi42]]

Thanks for these details Andrew. 
Will look into it today :)

Comment 24

[Arshad Kazmi [:arshadkazmi42]]

Started working on the test script.

This is what I got till now

```

const {AddonTestUtils} = ChromeUtils.import("resource://testing-common/AddonTestUtils.jsm", {});

AddonTestUtils.initMochitest(this);

async function createWebExtension(details) {
  let options = {
    manifest: {
      applications: {gecko: {id: details.id}},

      name: details.name,

      permissions: details.permissions,
    },
  };

  if (details.iconURL) {
    options.manifest.icons = {"64": details.iconURL};
  }

  let xpi = AddonTestUtils.createTempWebExtensionFile(options);

  await AddonTestUtils.manuallyInstall(xpi);
}

async function test() {

  const ID4 = "[e-mail]";
  await createWebExtension({
    id: ID4,
    name: "Test 4",
    permissions: ["<all_urls>"],
  });

  await ExtensionsUI._checkForSideloaded();

}

```

I am stuck at Enable / Disable Button CLicks on addons page
And Checking the permission popup.

Is there some doc or old test case which does something like this which I can refer?

Comment 25

[Arshad Kazmi [:arshadkazmi42]]

figured it out. working on the test script

Comment 26

[Arshad Kazmi [:arshadkazmi42]]

Attachment: Bug 1343179 - Permission popup appears once when sideloaded webextension is enabled

Comment 27

[Arshad Kazmi [:arshadkazmi42]]

Pushed code to phrabricator for review.

@Andrew add you as reviewer for it.

Comment 28

[Andrew Swan [:aswan]]

I get notified by phabricator about the review request, there's no need to set needinfo here

Comment 29

[Andrew Swan [:aswan]]

Comment on attachment 9007531 [details]
Bug 1343179 - Permission popup appears once when sideloaded webextension is enabled

Andrew Swan [:aswan] has approved the revision.

Comment 30

[Cristina Coroiu [:ccoroiu]]

This failed to land: 
Error: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmpdvGho7\npatching file toolkit/mozapps/extensions/test/browser/browser_dragdrop.js\nHunk #1 FAILED at 13\n1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/browser/browser_dragdrop.js.rej\nabort: patch failed to apply', '')

Comment 31

[Arshad Kazmi [:arshadkazmi42]]

Cristina, I will look into it asap.

Andrew, Is the sideloaded addon creating any issue? As its not manually removed before ending of test.
Also, it does not fails in my local machine.

I just tried now and in my local machine this test case failed

```

toolkit/mozapps/extensions/test/browser/browser_getmorethemes.js
  FAIL A promise chain failed to handle a rejection: Error while loading 'jar:file:///var/folders/5l/crj8l2qd1_s6dz5jmz5gpkxr0000gn/T/tmp0SjRM9.mozrunner/extensions/[e-mail]!/manifest.json' (NS_ERROR_FILE_NOT_FOUND) - stack: readJSON/</<@resource://gre/modules/Extension.jsm:445:18

```

I feel likes its happening due to sideloading extension and its not removed manually.

I found this comment in a test

https://searchfox.org/mozilla-central/source/browser/base/content/test/webextensions/browser_extension_sideloading.js#95

Comment 32

[Andrew Swan [:aswan]]

No, the problem here is that your patches are based on and old checkout from mozilla-central and the patches don't apply cleanly to a recent source tree.  There are various ways you could handle this but I think the simplest is probably to do "hg pull" to get an updated copy of mozilla-central, then use hg rebase (ie, if you have your patch checked out, run "hg rebase -s . -d central"), this is likely going to generate some conflicts that you'll need to resolve.  Once the conflicts are resolved, run eslint and tests again to make sure everything is still working, then upload the revised patches to phabricator.

I glossed over a bit of detail there, if you get stuck feel free to ask for help.  Good luck!

Comment 33

[Arshad Kazmi [:arshadkazmi42]]

Hi Andrew,
Thank you for the detailed information.
I did merge my current feature branch with latest mozilla-central code.
in my local all test at these tests are passed

mach test toolkit/mozapps/extensions/test/browser/

----

So, I was going through my code and found that during merge this was removed.

addon = await AddonManager.getAddonByID(ADDON_ID);
await addon.uninstall();

So I added it back and my this test is failing

toolkit/mozapps/extensions/test/browser/browser_getmorethemes.js

its giving this error

A promise chain failed to handle a rejection: Error while loading 'jar:file:///var/folders/5l/crj8l2qd1_s6dz5jmz5gpkxr0000gn/T/tmpsHxziv.mozrunner/extensions/[e-mail]!/manifest.json' (NS_ERROR_FILE_NOT_FOUND) - stack: readJSON/</<@resource://gre/modules/Extension.jsm:445:18


on running this

mach test toolkit/mozapps/extensions/test/browser/


and on removing the addon.uninstall code, everything is getting passed.

Comment 34

[Andrew Swan [:aswan]]

Great, if you're sure everything is passing, you can now re-set the checkin-needed keyword.
Thanks!

Comment 35

[Eliza Balazs [:ebalazs_]]

This failed to land with the error:

Error: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmptP93k5\npatching file toolkit/mozapps/extensions/test/browser/browser_dragdrop.js\nHunk #1 FAILED at 13\n1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/browser/browser_dragdrop.js.rej\nabort: patch failed to apply', '')

Comment 36

[Arshad Kazmi [:arshadkazmi42]]

Oh. failed again!

Andrew, I think I did something wrong while rebasing.

I did following:

1. I am at my feature branch
2. hg pull
3. hg rebase -s . -d central

Did I missed something?
Do I have to do these steps in default branch?

Comment 37

[Andrew Swan [:aswan]]

That looks correct, can you upload the result of doing that do phabricator?

Comment 38

[Arshad Kazmi [:arshadkazmi42]]

So I found 1 thing,

I executed these commands in my feature branch

1. I am at my feature branch
2. hg pull
3. hg rebase -s . -d central

but after executing all these commands when I check my current branch using `hg branch`

it shows that I am at `default branch`

Also I have pushed the code after merge also.
and I haven't got any conflicts while doing this

I tried these also

1. hg pull
2. hg merge (rev: latest)
3. I got conflicts doing this.
4. I resolved all the conflicts
5. after resolving while building code I am getting some error saying unable to make Telementory file and build is failing

Comment 39

[Andrew Swan [:aswan]]

I'm sorry I don't really understand, it would help if you could show the exact output you get when something doesn't work as you're expecting.

However, what is the output of "hg log -r central"?  What about "hg parent" after you rebase your changes?  You say you "pushed the code", but I don't see anything new on phabricator, where did you push it?

Comment 40

[Arshad Kazmi [:arshadkazmi42]]

Logs of the commands

hg log -r central

changeset:   437752:bb920e419166
fxtree:      central
user:        Ehsan Akhgari <[e-mail]>
date:        Fri Sep 21 13:59:25 2018 -0400
summary:     Backout changeset c5b3caf36ddf (bug 1490297) for causing bug 1493081 and making Nightly unusable; a=Aryx

===============


hg parent


changeset:   437753:5679add2f5b3
tag:         tip
user:        Arshad Kazmi <[e-mail]>
date:        Sun Sep 16 11:38:45 2018 +0530
summary:     setTimeout removed and addon id changed to remove conflicts with legacy test


> You say you "pushed the code", but I don't see anything new on phabricator, where did you push it?

I checked that too, I thougt I pushed it, but nothing was pushed.

Comment 41

[Arshad Kazmi [:arshadkazmi42]]

Finally I was able to merge it.

Tested all the test cases for that component using this command

./mach lint toolkit/mozapps/extensions/test/browser/browser_*.js

all test cases are passed.

pushed my code to phabricator

@Andrew, can you check it once on phabricator, whether its merged propertly, after that I will add check-in needed flag

Comment 42

[Arshad Kazmi [:arshadkazmi42]]

When I pushed it, it created a new seperate diff here

https://phabricator.services.mozilla.com/D6568

is there a way to merge this diff with the older one here

https://phabricator.services.mozilla.com/D5376

Comment 43

[Arshad Kazmi [:arshadkazmi42]]

I think, I managed to update the existing diff with the changes using `arc diff --update rev` command

@Andrew, can just check it once if I have missed something.

Comment 44

[Arshad Kazmi [:arshadkazmi42]]

Hi andrew,
Just a reminder if you have missed my previous comments
Have you got any change to check my merged code once on phabricator?

Comment 45

[Andrew Swan [:aswan]]

Comment on attachment 9007531 [details]
Bug 1343179 - Permission popup appears once when sideloaded webextension is enabled

Andrew Swan [:aswan] has requested changes to the revision.

Comment 46

[Andrew Swan [:aswan]]

No reminders are necessary, the needinfo is enough.  I've been very busy the past few days.
In any case, answered over on phabricator.

Comment 47

[Andrew Swan [:aswan]]

Comment on attachment 9007531 [details]
Bug 1343179 - Permission popup appears once when sideloaded webextension is enabled

Andrew Swan [:aswan] has approved the revision.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c208d905a9d
Permission popup appears once when sideloaded webextension is enabled r=aswan

Comment 49

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87256a1f66d2
Backed out changeset 4c208d905a9d for TV failures in  toolkit/mozapps/extensions/test/browser/browser_extension_sideloading_permission.js

Comment 50

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 4c208d905a9d (Bug 1343179) for TV failures in toolkit/mozapps/extensions/test/browser/browser_extension_sideloading_permission.js

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&selectedJob=202356450

https://treeherder.mozilla.org/logviewer.html#?job_id=202356450&repo=autoland&lineNumber=8079

Comment 51

[Arshad Kazmi [:arshadkazmi42]]

Andrew, I tried running this in my local machine

mach test toolkit/mozapps/extensions/test/browser/browser_*.js

all the tests are passing.

What could be the issue here?

Comment 52

[Arshad Kazmi [:arshadkazmi42]]

I tried running this also

./mach test toolkit/mozapps/extensions/test

everything is passed.

I think its failing in Windows as per the log Comment 50

Did I missed something in my test script for Windows?

Comment 53

[Andrew Swan [:aswan]]

Two things:
1. You removed the `addon.uninstall()` line from the test
2. You didn't make the change to the assertion message I requested in my last comment on phabricator

Comment 54

[Arshad Kazmi [:arshadkazmi42]]

(In reply to Andrew Swan [:aswan] from comment #53)
> Two things:
> 1. You removed the `addon.uninstall()` line from the test

Just checked the code, I think I missed it during merging.

> 2. You didn't make the change to the assertion message I requested in my
> last comment on phabricator

I missed your that comment, saw your comment after the test fail notification, I have made that change. 


I will make that change and re-run the tests

Comment 55

[Arshad Kazmi [:arshadkazmi42]]

I added `addon.uninstall()` and started tests using this command


`mach test toolkit/mozapps/extensions/test/browser/browser_*.js`


But its giving this error


`toolkit/mozapps/extensions/test/browser/browser_getmorethemes.js
  FAIL A promise chain failed to handle a rejection: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [amIAddonManagerStartup.enumerateZipFile] - stack: readDirectory@resource://gre/modules/Extension.jsm:417:22
async*_promiseLocaleMap@resource://gre/modules/Extension.jsm:943:25
async*promiseLocales/locales<@resource://gre/modules/Extension.jsm:1480:46
get@resource://gre/modules/ExtensionParent.jsm:1727:22
async*promiseLocales@resource://gre/modules/Extension.jsm:1479:25
async*initLocale@resource://gre/modules/Extension.jsm:1723:27
async*startup@resource://gre/modules/Extension.jsm:1807:15
async*startup@resource://gre/modules/Extension.jsm:1222:12
callBootstrapMethod@resource://gre/modules/addons/XPIProvider.jsm:1606:20
async*startup@resource://gre/modules/addons/XPIProvider.jsm:1721:27
async*updateAddonDisabledState@resource://gre/modules/addons/XPIDatabase.jsm:2250:15
async*setUserDisabled@resource://gre/modules/addons/XPIDatabase.jsm:593:13
async*enable@resource://gre/modules/addons/XPIDatabase.jsm:985:27
doCommand@chrome://mozapps/content/extensions/extensions.js:1150:9
set_userDisabled@chrome://mozapps/content/extensions/extensions.xml:883:13
oncommand@about:addons:1:1
synthesizeMouseAtPoint@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:471:7
synthesizeMouse@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:405:10
synthesizeMouseAtCenter@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:506:10
test@chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_extension_sideloading_permission.js:108:3
Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1102:34
async*Tester_execTest@chrome://mochikit/content/browser-test.js:1093:16
nextTest/<@chrome://mochikit/content/browser-test.js:995:9
SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
Rejection date: Sat Sep 29 2018 21:42:13 GMT+0530 (India Standard Time) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 257
Stack trace:
resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:257
chrome://mochikit/content/browser-test.js:Tester_execTest/<:1127
chrome://mochikit/content/browser-test.js:Tester_execTest:1093
chrome://mochikit/content/browser-test.js:nextTest/<:995
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795`

Comment 56

[Andrew Swan [:aswan]]

Where is the latest version of your patch?

Comment 57

[Arshad Kazmi [:arshadkazmi42]]

Andrew, just pushed it to phabricator

https://phabricator.services.mozilla.com/D5376

Comment 58

[Andrew Swan [:aswan]]

Comment on attachment 9007531 [details]
Bug 1343179 - Permission popup appears once when sideloaded webextension is enabled

Andrew Swan [:aswan] has requested changes to the revision.

Comment 59

[Arshad Kazmi [:arshadkazmi42]]

Andrew, The revision is approved in phabricator, but I don't think any comment is added for that here.
Should I add checkin-in need flag to this?

Comment 60

[Andrew Swan [:aswan]]

Assuming you've verified that tests are all passing, yes.
Or if you prefer to run and try and don't have level 1 commit access, I can start one for you.

Comment 61

[Arshad Kazmi [:arshadkazmi42]]

Running it on try server will be good. I don't have access, it will be great if you can start for me.
I have tested it in my local machine, all tests are passing

Comment 62

[Andrew Swan [:aswan]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=736b7a776630df286f4dd8d5c76fac347ac3b7b5

Comment 63

[Arshad Kazmi [:arshadkazmi42]]

This test is completed, but I see 1 red bar, something fails, but can't figure it out what exactly it is.

Can you have a look at it once?

Comment 64

[Andrew Swan [:aswan]]

You can ignore the L10n failure.  This looks good to set checkin-needed.  Thanks!

Comment 65

[Arshad Kazmi [:arshadkazmi42]]

Cool. thanks.

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55fe54a19a99
Permission popup appears once when sideloaded webextension is enabled r=aswan

Comment 67

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/55fe54a19a99