957404 - FaviconCache apparently unnecessarily takes a lock when handling permanent favicons

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Chris Kitching [:ckitching]]

Consider:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/cache/FaviconCache.java#590


Before the try block, the reordering semaphore (intended to protect access to the ordering list) is taken. Since, in the case that `permanently` is true, we never touch that structure, we should be able to not bother locking in that case. Slight performance improvement may ensue.

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Don't lock things when we (Seemingly) don't need to.

Hopefully I didn't miss something about the permanence logic that makes this claim false...

Comment 2

[Richard Newman [:rnewman]]

I left that lock in place to avoid having to reason about whether it would be correct -- that is, whether the reordering semaphore was also protecting some other assumption. If you want to fix it, you need to be confident :D

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8356894 [details] [diff] [review]
Don't lock things when we (Seemingly) don't need to.

Review of attachment 8356894 [details] [diff] [review]:
-----------------------------------------------------------------

You should also conditionalize each release of the semaphore (597, 604).

---
There is no requirement that a thread that releases a permit must have acquired that permit by calling acquire(). Correct usage of a semaphore is established by programming convention in the application.
---

Yes, we're using a single-permit semaphore... but no sense in being belligerently incorrect, and there's probably still a risk that, because we don't take the semaphore in the first place, someone else will do so and then we'll release it.

You could argue that the release inside the catch block will never be called if permanent, so it doesn't need to be touched.

But more broadly: why not do this?

---
        try {
            if (permanently) {
                upgradeReadToWrite();
                mPermanentBackingMap.put(faviconURL, toInsert);
            } else {
                mReorderingSemaphore.acquireUninterruptibly();
                try {
                    for (FaviconCacheElement newElement : toInsert.mFavicons) {
                        mOrdering.offer(newElement);
                    }
                } catch (Exception e) {
                    abortingRead = true;
                    mReorderingSemaphore.release();
                    finishRead();

                    Log.e(LOGTAG, "Favicon cache exception!", e);
                    return;
                } finally {
                    if (!abortingRead) {
                        mReorderingSemaphore.release();
                        upgradeReadToWrite();
                    }
                }
                mCurrentSize.addAndGet(sizeGained);

                // Update the value in the LruCache...
                recordRemoved(mBackingMap.put(faviconURL, toInsert));
            }
        } finally {
            finishWrite();
        }

        cullIfRequired();

---

Before you request re-review, please make sure that you're confident in the safety of this removal!

Comment 4

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.