Closed Bug 1451079 Opened 5 years ago Closed 4 years ago

runtime.setUninstallURL API does not honor empty string at runtime

Categories

(WebExtensions :: Compatibility, defect, P5)

Product:
WebExtensions
Component:
Compatibility
Version:
59 Branch
Type:
defect

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
47 bytes, text/x-phabricator-request
Details | Review
testExtensionForSetUninstallUrl.zip
2.66 KB, application/x-zip-compressed
Details 
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."
Component: Untriaged → Extension Compatibility
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
Component: Extension Compatibility → Untriaged
Product: Firefox → WebExtensions
Component: Untriaged → Compatibility
Mentor: tomica
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
Priority: -- → P5
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)

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)

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)

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)

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

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)

review ping?

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

Flags: needinfo?(sagarwala)

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)

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.

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)

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)

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)
Flags: needinfo?(tomica)

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)
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
Flags: needinfo?(tomica)

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.

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)
Flags: needinfo?(tomica)

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)
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
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7d64886307e5
Fix runtime.setUninstallURL to honor empty string, r=zombie

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

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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!

What Caitlin said, thanks and good work!

Flags: needinfo?(tomica)

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!

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

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.

Status: RESOLVED → VERIFIED
status-firefox67: fixed → verified
You need to log in before you can comment on or make changes to this bug.