1688743 - menus.remove doesn't return an error when removing a non-existing menu item

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P5)

Description

[fluks]

Attachment: menus-remove.tar.gz

menus.remove should return an error when a menu item can't be found for example, but it doesn't. I attached a test case where it should throw an error but it only prints undefined.

Firefox version: 85.0b9
Build ID: 20210114193053
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:85.0) Gecko/20100101 Firefox/85.0

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Shane Caraveo (:mixedpuppy)]

verified that we are not throwing, though docs suggest exactly this scenario should throw

Comment 3

[Shruti Vellanki]

Just getting started with my first bug + setting up my dev env, I would like to work on this!

Comment 4

[Shruti Vellanki]

Hello! So I need a little help proceeding with this bug:

1. I'm pretty sure the fix will be in browser\components\extensions\parent\ext-menus.js
2. It looks like a logical place for its accompanying unit test would be in: browser\components\extensions\test\browser and the filename could be something like browser_ext_themes_menus.js

I was able to run the unit tests in this folder with: ./mach test toolkit/components/extensions/test/browser/browser.ini

Where I'm stuck:

1. I noticed the attached unit test looks more like a geckoview unit test that would belong in the: mobile\android\geckoview\src\androidTest\assets\web_extensions\ folder
2. I can't seem to figure out how to run tests in this folder locally
3. I'm having doubts about whether I'm looking in the wrong spot because of the provided unit test

Comment 5

[Rob Wu [:robwu]]

I'm sorry for not responding to the above message; if you're still interested please ping me and I'll get back to you.

To answer the above questions, this API is not supported on Android, so you don't need to touch any code in mobile/android/.

Note that menus.update has a similar issue: it silently fails to update the menu if the given menu ID is invalid. It should also throw instead. Let's update both for consistency. The relevant parts of the implementation to modify is at https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-menus.js#1409,1424

A good place to add tests would be at https://searchfox.org/mozilla-central/rev/f9139f56bbcc0d587966c007178910ea31df7d58/browser/components/extensions/test/browser/browser_ext_menus_errors.js#90-116

Comment 6

[kernp25]

Attachment: Bug 1688743 - Make menus.update and menus.remove throw when menu item is not found. r?robwu

Comment 7

[kernp25]

Attachment: Bug 1688743 - Make menus.update and menus.remove throw when menu item is not found. r=robwu

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c5db6ac9fcb
Make menus.update and menus.remove throw when menu item is not found. r=robwu

Comment 9

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/2c5db6ac9fcb

Comment 10

[Rob Wu [:robwu]]

The menus.update and menus.remove (and their aliases contextMenus.update and contextMenus.remove) now reject with an error instead ignoring non-existing menu items.

This should be documented in the MDN "Firefox 136 for developers" release notes, the BCD and the reference documentation.

Comment 11

[rbloor]

Doc updates in:

• (content) Nonexistent menu items now return an error message #37720 – added a release note only, as the current reference documentation indicates that an error message should be returned
• (BCD) Menu update and remove error message support note #25710 – querying whether this should document releases up to 134 as partial implementations