1588616 - ext-menus.js doesn't properly call withLastError (null instead of caller)

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[Mike Kaply [:mkaply]]

ext-menus.js calls withLastError explicitly with a null instead of a caller:

https://searchfox.org/mozilla-central/rev/17756e2a5c180d980a4b08d99f8cc0c97290ae8d/browser/components/extensions/child/ext-menus.js#148

As a result, you can't trace errors in adding menus back from where they came from

Per kmag:

John-Galt
It can probably just pass context.getCaller()
John-Galt
And maybe withLastError should assert that caller is not null :/

Comment 1

[Rob Wu [:robwu]]

To fix this bug, save context.getCaller() in a variable at the start of the create method and pass that value instead of null.

https://searchfox.org/mozilla-central/rev/165e8d8f80c02605c2f3e89c5de462abfce91c32/browser/components/extensions/child/ext-menus.js#131,148

Comment 2

[ítalo]

Hi Rob, I'm an Outreachy applicant. Can I fix this bug? Thank you.

Comment 3

[ítalo]

Attachment: Bug 1588616 - Fix ext-menus.js and tests added. r=robwu

Comment 4

[Rob Wu [:robwu]]

I'll comment on the patch.

Comment 5

[ítalo]

Attachment: Bug 1588616 - Starting to add tests r=robwu

Depends on D93290

Comment 6

[Caitlin Neiman [:caitmuenster]]

Hi italo! Just checking in since we haven't heard from you in awhile. How's it going with this bug? Is there anything we can do to help you get unstuck?

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Cleared needinfo and assignee because of patch stale for longer than a month.

Comment 8

[Sunday Mgbogu [:digitalsimboja]]

Hello,

I am new to Open Source and would like to take on this bug if it is still open and unassigned.
can I work on it?

I would appreciate all guidance.

Thanks

Comment 9

[Caitlin Neiman [:caitmuenster]]

Hi Sunday, go for it! You can find tips for getting started at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp. Rob also describes how to fix the bug in comment #1. :)

Feel free to share your patch as soon as you would like some feedback! We'll assign the bug to you once you have the patch up.

Comment 10

[Sunday Mgbogu [:digitalsimboja]]

Alright, I am taking it on

Comment 11

[Sunday Mgbogu [:digitalsimboja]]

Dear Rob,

I am trying to work on the bug but yet to reproduce the error on my local machine.
Could you direct me if I am mistaking something when I ran the test

./mach test browser/components/extensions/test/browser/browser_ext_menus.js

I could not trace out the error on the output. Kindly point me to the right tests to use so I can be in the right track.
However, I am going over the web extension documentation.

Thanks and appreciate

Comment 12

[Rob Wu [:robwu]]

There are no existing unit tests for this bug. A previous contributor has started a patch at https://phabricator.services.mozilla.com/D93290?id=356450 which shows the correct logic in the test, but I added some review comments that have not been addressed.

To reproduce locally and manually (in the browser), outside of a unit test:

1. Load an extension with the menus permission.
2. Open the global Browser Console (Ctrl-Shift-J).
3. Call the browser.menus.create API with invalid parameters (e.g. by creating a menu with the same ID twice, like the linked unit test does).
4. Look at the global browser console, and see if there is a useful stack trace associated with the printed error (Unchecked lastError value: ID already exists: ...")
   (currently, there is no useful stack trace that references the extension file; the desired result is that there is)

Comment 13

[Sunday Mgbogu [:digitalsimboja]]

Attachment: Bug 1588616 -Test case added Fix ext-menus.js to call withLastError with caller r=robwu

Comment 14

[Rob Wu [:robwu]]

When you update the patch, I'll automatically be notified. There is no need to use needinfo too. It results in two extra emails on top of the one that I get from Phabricator.

Comment 15

[Sunday Mgbogu [:digitalsimboja]]

Okay noted

Comment 16

[Sunday Mgbogu [:digitalsimboja]]

Dear Rob,
Just to let you know that I am applying for this round of Outreachy. I would love to hear if you are planning on proposing a project. I will be very glad to make contributions on your Outreachy project. For this particular bug fix, I shall be sending my unit test with your revisions tonight.

Comment 17

[Rob Wu [:robwu]]

I am currently mentoring an Outreachy project, but am not planning to participate in the next round.

Comment 18

[Sunday Mgbogu [:digitalsimboja]]

I have updated the patch.

Okay I would appreciate if you know of mentors participating so I can also get in touch with them

Thanks

Comment 19

[Sunday Mgbogu [:digitalsimboja]]

I have added your review.
Please check.
Thanks

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea9a50dbf12f
Test case added Fix ext-menus.js to call withLastError with caller  r=robwu

Comment 21

[Sandor Molnar[:smolnar]]

Backed out for causing xpcshell failures in test_ext_menu_caller.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/08ca0cd0024ee299579849d9a14b5e19e21e33b0

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Bf4xgXW9QCGPxosJrF93-Q.0&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=ea9a50dbf12f5f7df755ca5ab08fdf31ddd0dca5

Failure log: https://treeherder.mozilla.org/logviewer?job_id=329331725&repo=autoland&lineNumber=1931

Comment 22

[Sunday Mgbogu [:digitalsimboja]]

Dear Rob,
My attention is drawn to this test by one of the contributors

https://treeherder.mozilla.org/logviewer?job_id=329331725&repo=autoland&lineNumber=1931

Seems the test failed on Andriod. Is this a device restriction? How do I proceed to resolve this?
I appreciate all guides.
Thanks

Comment 23

[Sunday Mgbogu [:digitalsimboja]]

I have skipped the testcase for Andriod since the menus API isn't yet supported on Firefox for Android based on the compatibility table.

Comment 24

[Rob Wu [:robwu]]

I'm following both this bug and the Phabricator revision. Since we're already discussing on Phabricator, there is no need for a needinfo here as well, it just results in extra emails (and some noise here). Comments about the implementation direction can be posted on the Phabricator revision.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b9a479c0705
Test case added Fix ext-menus.js to call withLastError with caller  r=robwu

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3b9a479c0705

Comment 27

[Caitlin Neiman [:caitmuenster]]

Congrats on landing this patch, Sunday! 🎉 Your contribution has been added to our recognition wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition.

Welcome onboard, and good luck with your Outreachy application! While the Add-ons Team isn't running any projects for the upcoming round, you may want to take a look at other projects Mozilla is sponsoring. They are listed on the Outreachy website. :)