Closed Bug 1451079 Opened 6 years ago Closed 5 years ago

runtime.setUninstallURL API does not honor empty string at runtime

Tracking

(firefox67 verified)

Status:

VERIFIED FIXED

Milestone:

mozilla67

Tracking Flags:

Tracking

Status

firefox67

---

verified

People

(Reporter: andrew.lord, Assigned: shailja.agarwala21, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

Bug 1451079 - Fix runtime.setUninstallURL to honor empty string, r=zombie 5 years ago Shailja Agarwala 47 bytes, text/x-phabricator-request		Details \| Review
testExtensionForSetUninstallUrl.zip 5 years ago Madalin Cotetiu 2.66 KB, application/x-zip-compressed		Details

Andrew L

Reporter

Description

•

6 years ago

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

Kanchan Kumari QA

Updated

•

6 years ago

Component: Untriaged → Extension Compatibility

Andrew L

Reporter

Comment 1

•

6 years ago

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

Jorge Villalobos [:jorgev] (he/him)

Updated

•

5 years ago

Component: Extension Compatibility → Untriaged

Product: Firefox → WebExtensions

Daniel Bodea [:danibodea]

Updated

•

5 years ago

Component: Untriaged → Compatibility

Kris Maglione [:kmag]

Updated

•

5 years ago

Mentor: tomica

Status: UNCONFIRMED → NEW

Ever confirmed: true

Keywords: good-first-bug

Priority: -- → P5

Shailja Agarwala

Assignee

Comment 2

•

5 years ago

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?

Flags: needinfo?(tomica)

Tomislav Jovanovic :zombie

Comment 3

•

5 years ago

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.

Flags: needinfo?(tomica)

Shailja Agarwala

Assignee

Comment 4

•

5 years ago

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?

Flags: needinfo?(tomica)

Tomislav Jovanovic :zombie

Comment 5

•

5 years ago

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

Flags: needinfo?(tomica)

Shailja Agarwala

Assignee

Comment 6

•

5 years ago

Attached file Bug 1451079 - Fix runtime.setUninstallURL to honor empty string, r=zombie — Details

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

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

Shailja Agarwala

Assignee

Comment 7

•

5 years ago

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.

Flags: needinfo?(tomica)

Shailja Agarwala

Assignee

Comment 8

•

5 years ago

review ping?

Manish [:manishkk]

Comment 9

•

5 years ago

•

Edited

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

Flags: needinfo?(sagarwala)

Shailja Agarwala

Assignee

Comment 10

•

5 years ago

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

Flags: needinfo?(1991manish.kumar)

Shailja Agarwala

Assignee

Comment 11

•

5 years ago

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.

Manish [:manishkk]

Comment 12

•

5 years ago

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.

Flags: needinfo?(sagarwala)

Flags: needinfo?(1991manish.kumar)

Tomislav Jovanovic :zombie

Comment 13

•

5 years ago

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.

Assignee: nobody → shailja.agarwala21

Flags: needinfo?(tomica)

Shailja Agarwala

Assignee

Comment 14

•

5 years ago

Hey Tomislav,

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

Flags: needinfo?(tomica)

Tomislav Jovanovic :zombie

Updated

•

5 years ago

Flags: needinfo?(tomica)

Shailja Agarwala

Assignee

Comment 15

•

5 years ago

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.

Flags: needinfo?(tomica)

Pulsebot

Comment 16

•

5 years ago

Pushed by wkocher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/56d817208b35
Adding fix for setting empty url to set uninstall url during runtime. r=zombie

Shailja Agarwala

Assignee

Updated

•

5 years ago

Flags: needinfo?(tomica)

Andrei Ciure[:aciure]

Comment 17

•

5 years ago

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

Flags: needinfo?(shailja.agarwala21)

Tomislav Jovanovic :zombie

Comment 18

•

5 years ago

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.

Shailja Agarwala

Assignee

Comment 19

•

5 years ago

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

Flags: needinfo?(shailja.agarwala21) → needinfo?(tomica)

Tomislav Jovanovic :zombie

Updated

•

5 years ago

Flags: needinfo?(tomica)

Shailja Agarwala

Assignee

Comment 20

•

5 years ago

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.

Flags: needinfo?(tomica)

Phabricator Automation

Updated

•

5 years ago

Attachment #9041030 - Attachment description: Adding fix for setting empty url to set uninstall url during runtime. → Bug 1451079 - Fix runtime.setUninstallURL to honor empty string, r=zombie

Pulsebot

Comment 21

•

5 years ago

Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7d64886307e5
Fix runtime.setUninstallURL to honor empty string, r=zombie

Bogdan Tara[:bogdan_tara | bogdant]

Comment 22

•

5 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 5 years ago

status-firefox67: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla67

Caitlin Neiman [:caitmuenster]

Comment 23

•

5 years ago

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!

Tomislav Jovanovic :zombie

Comment 24

•

5 years ago

What Caitlin said, thanks and good work!

Flags: needinfo?(tomica)

Shailja Agarwala

Assignee

Comment 25

•

5 years ago

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!

Caitlin Neiman [:caitmuenster]

Comment 26

•

5 years ago

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

Madalin Cotetiu

Comment 27

•

5 years ago

Attached file testExtensionForSetUninstallUrl.zip — Details

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.

Madalin Cotetiu

Updated

•

5 years ago

Status: RESOLVED → VERIFIED

status-firefox67: fixed → verified

You need to log in before you can comment on or make changes to this bug.