1451079 - runtime.setUninstallURL API does not honor empty string at runtime

Status: VERIFIED FIXED

(WebExtensions :: Compatibility, defect, P5)

Description

[Andrew L]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180323154952

Steps to reproduce:

1. Install an extension, uninstall URL is set
2. Update the extension, changing the uninstall URL string to null (empty)
3. Uninstall the extension


Actual results:

Post uninstall, the original URL string is shown (which was set at install time not at runtime).


Expected results:

The changed URL (to null) is not honored. 

However, IF you change the URL to a different URL it will honor that value. The bug is when the value is set to empty.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/setUninstallURL   states that "Set it to an empty string to not open a new tab upon uninstallation."

Comment 1

[Andrew L]

Any chance someone could confirm this bug and determine next steps? We would like to update our users with a new experience but the value is not being honored. Thanks

Comment 2

[Shailja Agarwala]

I'd like to take up this bug and work on it. Could you please help me the codebase and the files I need to modify for this?

Comment 3

[Tomislav Jovanovic :zombie]

Hi Shaija,

The specific file that implements this method is here:
https://searchfox.org/mozilla-central/rev/c3ebaf6/toolkit/components/extensions/parent/ext-runtime.js#136

And you will probably need to add a test here:
https://searchfox.org/mozilla-central/rev/c3ebaf6/browser/components/extensions/test/browser/browser_ext_runtime_setUninstallURL.js

If this is your first contribution, please take a look at the introduction docs at:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

Let me know if you get stuck or have any questions.

Comment 4

[Shailja Agarwala]

Apologies for such a basic question, but how exactly can I reproduce the bug?

According to the steps to reproduce, I need to set the uninstall URL of the extension to null. I want to understand how I can do this to proceed with the testing.

Also, after adding a test case - https://searchfox.org/mozilla-central/rev/c3ebaf6/browser/components/extensions/test/browser/browser_ext_runtime_setUninstallURL.js , is there a documentation on how to execute this test script?

Comment 5

[Tomislav Jovanovic :zombie]

I haven't tested, but from the original report, it seems just calling the method browser.runtime.setUninstallURL(null) should reproduce the bug.

I think your test will probably end up being the same as test_setuninstall_empty_url with just null passed instead of an empty string here:
https://searchfox.org/mozilla-central/rev/c3ebaf6/browser/components/extensions/test/browser/browser_ext_runtime_setUninstallURL.js#55-73

For getting started with writing and running tests, check out this introduction:
https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Testing

Comment 6

[Shailja Agarwala]

Attachment: Bug 1451079 - Fix runtime.setUninstallURL to honor empty string, r=zombie

The bug indicated that the setUninstallURL did not honor empty url during runtime.
Steps to reproduce:

1. Install an extension, uninstall URL is set
2. Update the extension, changing the uninstall URL string to null (empty)
3. Uninstall the extension

Comment 7

[Shailja Agarwala]

Hi Tomislav, I have submitted the changes. The setUninstallURL with an empty string was working in the first time, however during runtime update, the bug appeared as mentioned. I have made the changes for this and also included a test function to accept null URL.

Have submitted the review in https://phabricator.services.mozilla.com/D18499

Please let me know if changes are required.

Comment 8

[Shailja Agarwala]

review ping?

Comment 9

[Manish [:manishkk]]

Hi shailja,
Check patch on Phabricator, ReviewBot mentioned some error there.
Let me know If you have any other question.

Comment 10

[Shailja Agarwala]

Thanks for the information, Manish. I have fixed the errors mentioned by ReviewBot and updated the Diff. Could you please check and review?

Comment 11

[Shailja Agarwala]

Thanks for the information, Manish. I have fixed the errors mentioned by ReviewBot and updated the Diff. Could you please check and review?

Also, how can I assign this particular bug to myself? It still shows unassigned in the Bug Description.

Comment 12

[Manish [:manishkk]]

Work on a few more bugs after some time, someone from Mozilla can give you privileges by which you can assign bugs to yourself.

Also, how can I assign this particular bug to myself? It still shows unassigned in the Bug Description.

Comment 13

[Tomislav Jovanovic :zombie]

Hi Shaija, sorry for the delay, I assumed you were planning to fix the ReviewBot comments on phabricator.

Just reviewed the latest version, and assigned the bug to you.

Comment 14

[Shailja Agarwala]

Hey Tomislav,

Thanks a lot for the review. I have modified the changes as per your comments and submitted the patch again. Please check.

Comment 15

[Shailja Agarwala]

Hi Tomislav,

Thanks for the review again. Now that the review status is changed to accepted, I believe I need to check the patch against the try server. Could you please give me the level 1 access to try server so that I can test the code and ask for a check-in?

In case my assumption is wrong, please help me with the next steps.

Thank you for your time and help.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56d817208b35
Adding fix for setting empty url to set uninstall url during runtime. r=zombie

Comment 17

[Andrei Ciure[:aciure]]

Backed out changeset 56d817208b35 (bug 1451079) for failing browser_ext_runtime_setUninstallURL.js

push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=230914538&revision=56d817208b35b3b1c16833b3dcd7ad80a7297371

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=230930900&searchStr=linux%2Copt%2Cmochitests%2Cwith%2Ce10s%2Ctest-linux32%2Fopt-mochitest-browser-chrome-e10s-4%2Cm-e10s%28bc4%29

backout: https://hg.mozilla.org/integration/autoland/rev/c6ecca7fc9882c9a7dfc91b9095775fb56ff9dad

Comment 18

[Tomislav Jovanovic :zombie]

Hey Shaija, it looks like we forgot to add

"optional": true,

to the runtime schema for setUninsstallURL method here:

https://searchfox.org/mozilla-central/source/toolkit/components/extensions/schemas/runtime.json#234

It also looks like you didn't manage to run tests locally. After you make the changes, you need to build firefox, and then run the test with:

mach build
mach mochitest browser/components/extensions/test/browser/browser_ext_runtime_setUninstallURL.js

and generally follow the Contribution Onramp docs from comment 5.

Comment 19

[Shailja Agarwala]

Thanks Tomislav. I have updated the code with required changes and local tests have passed. Please review. https://phabricator.services.mozilla.com/D18499

Comment 20

[Shailja Agarwala]

Hey Tomislav,

The copyrights comments have been updated as it is in https://phabricator.services.mozilla.com/D18499 . Also, for the title change, arc diff seems to generate a random revision id. I have added the required message in the comment summary. Please let me know if further changes are required. Thank you for the feedback.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d64886307e5
Fix runtime.setUninstallURL to honor empty string, r=zombie

Comment 22

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7d64886307e5

Comment 23

[Caitlin Neiman [:caitmuenster]]

Woohoo! Thanks so much for the patch, sagarwala! 🎉 Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Comment 24

[Tomislav Jovanovic :zombie]

What Caitlin said, thanks and good work!

Comment 25

[Shailja Agarwala]

Thank you Caitlin and Tomislav! I have created a mozillian account. Please vouch for me here - https://mozillians.org/en-US/u/shailja.agarwala21/

Thanks again!

Comment 26

[Caitlin Neiman [:caitmuenster]]

You're vouched, Shailja! Welcome onboard -- we look forward to seeing you around the project! ✨

Comment 27

[Madalin Cotetiu]

Attachment: testExtensionForSetUninstallUrl.zip

Verified in Win7x64, Ubuntu 14.0.4 x32, MacOS 10.14.1 on FF67.0b5 (20190325125126)
I have created a test webextensions that sets the uninstallUrl to empty string when the extension icon is clicked (see the attachement).

• if the extension is uninstalled before clicking the icon the uninstallUrl set at install is used and a new tab is open.
• if the extension is uninstalled after the icon is clicked the uninstallUrl is set to empty string and no new tab is open.

Marking bug as verified fixed.