Adds exception handling for all places rust calls

[tarikeshaq]

Should fix the crash in mozilla/application-services#4990

A little effy about the bookmarks changes because of the default values of the GUIDs, but that seems better than a crash? I haven't tracked the callers yet

Pull Request checklist

• Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry or does not need one
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

• Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
• Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

[mhammond]

I agree with your concerns around things like addItem (and addFolder - although I can't find a caller of that :) - returning an empty GUID isn't ideal, and looking at the one caller I can find, Fenix is still going to try and open a "bookmarks editor", which is probably going to have a bad time. Sadly I don't know what the correct answer there is, but it feels like the right option would be to make these results nullable, which will mean consumers are forced to deal with it in a meaningful way - but that's a fair bit more work as it crosses repos, and really isn't our call to make.

tl;dr - I'm fine with this if the Fenix team is and agree it's better than a crash, but think they should probably take on another issue to make it cleaner - possibly how I describe above, or possibly in some other way. So r=me, but don't take my word for it 😅

[mhammond]

and looking at the one caller I can find

And looking more at that, it already handles PlacesException.UrlParseFailed and records telemetry for it - which this might break?

[tarikeshaq]

And looking more at that, it already handles PlacesException.UrlParseFailed and records telemetry for it - which this might break?

Hmmm I'd like more insight from some Fenix folks on what to do here, I have a couple of options:

1. Just don't add the exception handling for the bookmarks functions, I don't like this, and will most definitely keep crashing for some users
2. For the bookmark functions, rethrow the UrlParseFailed but eat up the rest, i.e not use the handlePlacesExceptions function and instead another try, catch there - I like this, it's a quick change to help us stop the crashes and keeps the same behaviour. Then possibly a cleanup task can be made to make this all "better"
3. Eat up the exception and it'll be a breaking change for fenix - I don't think this is feasible, but 🤷

For the sake of moving forward, I'll do option 2 in this PR, but happy to revert it back before landing

[mergify]

This pull request has conflicts when rebasing. Could you fix it @tarikeshaq? 🙏

[mergify]

This pull request has conflicts when rebasing. Could you fix it @tarikeshaq? 🙏

[mergify]

This pull request has conflicts when rebasing. Could you fix it @tarikeshaq? 🙏

[tarikeshaq]

@jonalmeida have you had the time to take a look at this? 😬 I'll rebase with your feedback if you have some!

[jonalmeida]

Sorry, this review took a while. Please ping me if I'm blocking you in the future. 🙂