Bugfix FXIOS-9830 Do not duplicate favicon URL and image requests on UITableViewCell reload [Favicon Refactor]

[ih-codes]

📜 Tickets

Jira ticket
Github issue

💡 Description

Note I have the merge destination currently as my other favicon refactor branch so the changes can be more clearly reviewed. But merge in FXIOS-9667 fix first.

---

Revises the solution for this FXIOS-9427 bugfix as the solution did not prevent duplicate network calls when loading top site tiles on the Home screen (if the DefaultSiteImageHandler was destroyed on cell reload, for example).

Ideally the fix would be a larger architectural change, but this is a stopgap measure for now. But the TopSiteItemCell really should share a common reference to oneDefaultSiteImageHandler so we can better orchestrate multiple requests to the same resource (i.e. queue, throttle, etc.).

As it stands, the multiple reloads of things on the Home tab cause the cells to reload multiple times and trigger multiple fetches of the same favicon URL and image. If a new image is not yet in the cache, for example, that might trigger 3 network calls to download the image for a single tile.

📝 Checklist

You have to check all boxes before merging

• Filled in the above information (tickets numbers and description of your work)
• Updated the PR name to follow our PR naming guidelines
• Wrote unit tests and/or ensured the tests suite is passing
• When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
• If needed, I updated documentation / comments for complex code and public methods
• If needed, added a backport comment (example @Mergifyio backport release/v120)

[mattreaganmozilla]

@ih-codes I think I understand the intent of this change though I'll probably need to take a deeper dive to grok it fully because I'm not immediately clear on all of the implications. We've changed this class to an actor but I'm not really seeing what this gets us since most of the funcs are marked nonisolated which is going to remove the state synchronization benefits in those functions AFAIU. Are we using an actor here solely to synchronize changes to requestQueue? That is confusing to me because I believe that actors don't protect static state since it's effectively global, but maybe that has changed or I'm misunderstanding something.

[ih-codes]

@mattreaganmozilla Here's a fun example of the pattern I was borrowing (might be easier to grok): https://x.com/cocoawithlove/status/1402762956211769347

Basically I only wanted to isolate getFaviconImage. My concern was that multiple calls could trigger and I didn't want more than one call to drop past the check to requestQueue on line 114. There's no need to protect access elsewhere.

Maybe a better solution would be making a custom actor annotation (something like @MainActor) for just that method rather than making the whole class an actor and using a bunch of nonisolated. Haven't tried that before.

I'm still a bit new to actors myself if you have any alternative suggestions. Or maybe actors isn't the best way to solve this situation. 🤔

[mattreaganmozilla]

My initial thinking was that an actor with most funcs marked nonisolated is a potential code smell, but I think this is one of the areas of our app architecture where we don't have an official guideline / policy, though I worry that perhaps we're using the wrong tool.

If we only want to synchronize/lock around getFaviconImage there may be better synchronization primitives (an internal lock, queue, or even something like objc_sync_enter/exit) we could leverage that would be more clear, as it stands right now I think actor is a bit confusing given that there's not really any mutable state on the actor to protect, and DefaultSiteImageHandler being Sendable doesn't seem to (AFAICS) currently matter much elsewhere.

Having said all that 😅, I still need to dig in further to make sure I'm fully understanding the change here; I'll defer to you (and Orla) in the meantime. Thanks for investigating all of this, this area of the codebase will benefit greatly from improvements. 🙏

[ih-codes]

Cool, I think I'll go with a synchronization primitive or... I could just annotate the method with @MainActor, honestly. 🤔 That's what I had originally.

I will test this a bit more because I'm not even sure synchronization is a problem to fix in the first place, I was just guessing there was potential a race condition.

Regarding the change as a whole -- the idea is that we keep a queue (or rather, a dictionary) of active favicon requests (which are Tasks to await). Any other code that attempts to get the same favicon will instead await the current Task that's already in the queue. Once the Task completes, all awaiting threads also complete with the same return value from the Task (i.e. the favicon image). Hope that helps clarify a bit! I was testing this in a sample project earlier just for my sanity's sake haha.

[mattreaganmozilla]

Regarding the change as a whole -- the idea is that we keep a queue (or rather, a dictionary) of active favicon requests (which are Tasks to await). Any other code that attempts to get the same favicon will instead await the current Task that's already in the queue

I can't remember the details now, but when I had originally looked into this I recall thinking there was a reason this wouldn't work, because that was what I had originally tried also (but I could have been mistaken). I think the previous hack I implemented was -- effectively -- doing the same thing, except instead of suspending the duplicate task via return await activeRequest.value, it was sleeping them (which should also suspend and not block, but it's definitely not a great approach). In any case, if this is working then it's an improvement over my solution so I'm all for it. Thanks for digging into this.

[ih-codes]

This is ready for another look whenever you have time @mattreaganmozilla (no rush).

I removed the actor to instead annotate the method in question with @MainThread. Hopefully that resolves your concerns about code smell.

I saw some crashes while running the code in the sim, but that was after I removed actor and before I added the @MainActor annotation. 🤞 🙏

I also added a unit test which replicates the behaviour of TopSiteItemCells reloading on the home screen (which was causing multiple requests to be made to new instances of SiteImageHandlers). Overall, I think this code is an improvement simply because I can get my test to pass reliably after hundreds of iterations, whereas on main I get crashes and duplicated network calls. 🤔

[mattreaganmozilla]

I think my only concern with this is the introduction of this global state; it's somewhat obscured that all of these instances will be potentially checking against the same shared request queue, even if they are initialized with different url handlers or image handlers. Right now I don't think that matters much in practice, and it's hard to imagine how it would realistically cause problems but it might be something nice for us to revisit at some point.

[ih-codes]

I totally understand. Ideally this would all be refactored to be using a single instance of DefaultSiteImageHandler passed in to each cell, but this would entail a big refactor...

I'm ok with leaving the current solution in as well, as long as we can get my new unit test to pass reliably for it. Since that (I believe) mimics the current state of affairs with multiple instances of DefaultSiteImageHandler being created for a single cell/site URL.

I'll experiment with it a bit this afternoon.

[OrlaM]

We went back and forth on this a bunch when originally building it if this should be a singleton or not. In the end we opted to go with the simpler solution and live with the fact there would occasionally be the odd unnecessary request but obviously that has turned out to be a worse problem than we anticipated.

[OrlaM]

I'm not too worried about the global state from an app stability perspective because it's private and it's within a pretty contained submodule but it's a little concerning from a test perspective because of the possibility of one test colliding with another. It's not a major concern tho and seems like a reasonable solution that limits the scope of the change. Just worth keeping an eye on if we start getting flaky tests in this area in the future we might want to revisit.

[ih-codes]

Thanks @OrlaM! That makes sense. This is definitely something to keep an eye on in the future... I was browsing Sentry to see if I could find any EXC_BAD_ACCESS type crashes related to favicons and saw this one. Made me wonder if it was something related to this recreation of the SiteImageHandler.

[OrlaM]

LGTM

[mergify]

This pull request has conflicts when rebasing. Could you fix it @ih-codes? 🙏

[mobiletest-ci-bot]

	Messages
📖	Edited 3 files
📖	Created 0 files

Generated by 🚫 Danger Swift against f70af63