runtime.setUninstallURL API does not honor empty string at runtime

VERIFIED FIXED in Firefox 67

Status

defect
P5
normal
VERIFIED FIXED
a year ago
2 months ago

People

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

Tracking

({good-first-bug})

59 Branch
mozilla67

Firefox Tracking Flags

(firefox67 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

Updated

a year ago
Component: Untriaged → Extension Compatibility
(Reporter)

Comment 1

a year 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
Component: Extension Compatibility → Untriaged
Product: Firefox → WebExtensions
Component: Untriaged → Compatibility
Mentor: tomica
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
Priority: -- → P5
(Assignee)

Comment 2

4 months 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)

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)
(Assignee)

Comment 4

4 months 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)

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)
(Assignee)

Comment 6

4 months ago

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
(Assignee)

Comment 7

4 months 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)
(Assignee)

Comment 8

3 months ago

review ping?

Comment 9

3 months ago

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

Flags: needinfo?(sagarwala)
(Assignee)

Comment 10

3 months 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)
(Assignee)

Comment 11

3 months 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.

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)
(Assignee)

Comment 14

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

Comment 15

3 months 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)

Comment 16

3 months 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
(Assignee)

Updated

3 months ago
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.

(Assignee)

Comment 19

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

Comment 20

2 months 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)
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

Comment 21

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

Comment 22

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
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)
(Assignee)

Comment 25

2 months 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!

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

Comment 27

2 months ago

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.

Updated

2 months ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.