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

Closes #2522: Removed mutable pending intent flags #2532

Merged

Conversation

mavduevskiy
Copy link

Having mutable pending intents is disallowed on android 14+ and results in app crashing on launch. I don't think there was any particular reason why the PendingIntets we are using were set to be mutable instead of immutable – we are creating new PendingIntent for each tab, without intention to alter (or override in this case) existing ones. My hunch, the system was using mutable flags by default prior to API 31, so the behaviour was kept the same.

Pull Request checklist

Having mutable pending intents is disallowed on android 14+ and results in app crashing on launch.
I don't think there was any particular reason why the PendingIntets we are using were set to be
mutable instead of immutable – we are creating new PendingIntent for each tab, without intention
to alter (or override in this case) existing ones. My hunch, the system was using mutable flags
by default prior to API 31, so the behaviour was kept the same.
@mavduevskiy mavduevskiy requested a review from a team as a code owner October 18, 2023 02:29
@mavduevskiy
Copy link
Author

mavduevskiy commented Oct 18, 2023

While testing the behaviour of mutable and immutable flags, I found another bug: If we send the same tab multiple times, only the first one will work. Will create a follow up. Edited: the follow up.

@mavduevskiy
Copy link
Author

@mcarare
Hello, Mihai :) Probably you are the right person to ask to take a look at it

Copy link
Contributor

@mcarare mcarare left a comment

Choose a reason for hiding this comment

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

LGTM!

Not sure either why we were using FLAG_MUTABLE. AFAIK for the same feature we use FLAG_IMMUTABLE in Fenix.

Note that one other way to fix this would have been to use explicit intent.

@mcarare
Copy link
Contributor

mcarare commented Oct 18, 2023

Thanks for tagging me, I will keep this in mind to retest for the Fenix targeting 34 PR.

@mavduevskiy
Copy link
Author

mavduevskiy commented Oct 19, 2023

That's a great point, we should do that. Because right now the tab will be opened by the default browser that might not be the RB. Filled an issue.

Do you think we could avoid changing flags? From what I can see, the pending intent requires a mutability flag, and android 14 requires it to be set to immutable.

btw, I don't have rights to set tags, could you help me merging it? @mcarare

@rvandermeulen rvandermeulen added the needs landing Auto lands approved and green PRs. label Oct 19, 2023
@mergify mergify bot merged commit 7e888c8 into mozilla-mobile:master Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs landing Auto lands approved and green PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants