Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ignore errors from menus.remove and menus.update #1250

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ahal
Copy link

@ahal ahal commented Feb 4, 2025

Fixes #1242

I'm not sure if this is the whole fix, but it seems to get things back to normal.

@ahal
Copy link
Author

ahal commented Feb 4, 2025

Note that this bug is about to hit Firefox beta, and will be affecting all users in ~1 month.

Copy link

@philg-dev philg-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising to me. Maybe a reference to the upstream change / bugfix either in a code comment or at least the commit message wouldn't hurt.

https://bugzilla.mozilla.org/show_bug.cgi?id=1688743

Not sure how this is usually handled in the STG codebase, but maybe it would be wise to at least write a debug / info / warning log in those error cases. To me it appears that removing / updating a menu that doesn't exist should be avoided as much as possible... But it's up to the maintainers to decide whether they want a log.

This fixes a regression caused by:
https://bugzilla.mozilla.org/show_bug.cgi?id=1688743

Whereupon menus.remove and menus.update started raising
an error when operating on a menu item that doesn't
exist.

Fixes Drive4ik#1242
@ahal ahal force-pushed the ahal/push-zxsrmtprvzuy branch from 4b5b21e to 317d9d8 Compare February 4, 2025 21:36
@ahal
Copy link
Author

ahal commented Feb 4, 2025

Good call, updated the commit message at least.

I thought about adding some logging, but this way there's less behaviour change (previously there was also no logging). I also noticed a couple other blank try/catches in the code base so figured it was ok to do here as well.

Happy to add some if @Drive4ik wants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

STG keeps crashing on FF Nightly 136.0a1
2 participants