runtime.setUninstallURL API does not honor empty string at runtime
Categories
(WebExtensions :: Compatibility, defect, P5)
Tracking
(firefox67 verified)
Tracking | Status | |
---|---|---|
firefox67 | --- | verified |
People
(Reporter: andrew.lord, Assigned: shailja.agarwala21, Mentored)
Details
(Keywords: good-first-bug)
Attachments
(2 files)
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•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
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
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?
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.
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?
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
Assignee | ||
Comment 6•5 years ago
|
||
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
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.
Assignee | ||
Comment 8•5 years ago
|
||
review ping?
Comment 9•5 years ago
•
|
||
Hi shailja,
Check patch on Phabricator, ReviewBot mentioned some error there.
Let me know If you have any other question.
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?
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.
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.
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 | ||
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.
Updated•5 years ago
|
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.
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
Assignee | ||
Updated•5 years ago
|
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
backout: https://hg.mozilla.org/integration/autoland/rev/c6ecca7fc9882c9a7dfc91b9095775fb56ff9dad
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.
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
Updated•5 years ago
|
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.
Updated•5 years ago
|
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
Comment 22•5 years ago
|
||
bugherder |
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!
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!
Comment 26•5 years ago
|
||
You're vouched, Shailja! Welcome onboard -- we look forward to seeing you around the project! ✨
Comment 27•5 years 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•5 years ago
|
Description
•