Ensure the disk-cache stays around until all cache-operations have finished

RESOLVED FIXED

Status

()

--
major
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bjarne, Assigned: bjarne)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
Recent improvements to the cache introducing a separate cache-io thread have exposed an issue when shutting down the cache (or the profile), namely that internal data-structures of the disk-cache device (effectively the device itself) may be cleared/reset on the main-thread before all pending cache-events have finished.

I see this clearly and consistently when working on bug #549767 (async deactivate/flush) because deactivating entries are done as part of the shutdown, but I believe this fundamental issue may cause problems from time to time in general (in particular I suspect it to cause bug #614513, but potentially also others).

The same problem may also be present in other cache-devices and in the cache-service itself, but it is clearly seen with the disk-cache.

IMO this is a pretty obvious thing to fix, and it is independent of bug #549767 which is why I'd prefer to have a separate bug for it.
(Assignee)

Updated

8 years ago
blocking2.0: --- → ?
(Assignee)

Comment 1

8 years ago
Created attachment 499023 [details] [diff] [review]
Proposed solution

This is an excerpt from the WIP patch for bug #549767 which I believe will fix the issue described in this bug. The idea is simple: wait for all pending cache-operations to finish before doing anything harmful, at the right places.

It survives simple local testing as well as Michals trick to reproduce bug #614513. Passed to tryserver.
Assignee: nobody → bjarne
Attachment #499023 - Flags: review?(michal.novotny)
(Assignee)

Comment 2

8 years ago
Tryserver run looks reasonable,
(Assignee)

Comment 3

8 years ago
Comment on attachment 499023 [details] [diff] [review]
Proposed solution

Changed reviewer - Michal seems busy.
Attachment #499023 - Flags: review?(michal.novotny) → review?(bzbarsky)
I'm not sure I'm competent to review this; at least not if you want anything like a quick review.  Who're the patch author and reviewer for the existing code?  Seems like they would both be better reviewers.
(In reply to comment #3)
> Comment on attachment 499023 [details] [diff] [review]
> Proposed solution
> 
> Changed reviewer - Michal seems busy.

I could do the review soon since I've already seen the patch when we were discussing #614513...
Attachment #499023 - Flags: review?(bzbarsky) → review?(michal.novotny)
(Assignee)

Updated

8 years ago
Blocks: 549767
Comment on attachment 499023 [details] [diff] [review]
Proposed solution

> +    NS_IMETHOD Run()
> +    {
> +        nsCacheServiceAutoLock lock; // probably not needed

IMO you really don't need it. Also you don't need the mIsFinished variable.


> +    mozilla::Monitor                mMonitor;

I would prefer to move the monitor into the nsBlockOnCacheThreadEvent class.


> +        nsCacheServiceAutoLock lock; // probably not needed
> +        mozilla::MonitorAutoEnter autoMonitor(nsCacheService::gService->mMonitor);
> +
> +#ifdef PR_LOGGING
> +        nsIThread *thread = NS_GetCurrentThread();
> +        CACHE_LOG_DEBUG(("nsBlockOnCacheThreadEvent [%p] running on thread %x\n", this, thread));
> +#endif

80 chars please
Attachment #499023 - Flags: review?(michal.novotny)
(Assignee)

Comment 7

8 years ago
Created attachment 500760 [details] [diff] [review]
Proposed solution v2

Updated with comments from reviewer
Attachment #499023 - Attachment is obsolete: true
Attachment #500760 - Flags: review?(michal.novotny)
(Assignee)

Comment 8

8 years ago
Created attachment 500771 [details] [diff] [review]
Proposed solution v3

Re-introduced the monitor in nsCacheService because holding it in the event itself seems to cause problems when the runnable it deleted by the thread executing it.
Attachment #500760 - Attachment is obsolete: true
Attachment #500771 - Flags: review?(michal.novotny)
Attachment #500760 - Flags: review?(michal.novotny)
Comment on attachment 500771 [details] [diff] [review]
Proposed solution v3

> +    nsBlockOnCacheThreadEvent *event = new nsBlockOnCacheThreadEvent();
> +    if (!event) {
> +        NS_WARNING("Failed creating event!");
> +        return NS_ERROR_OUT_OF_MEMORY;
> +    }

You don't have to check the return value of new. It either successfully allocates memory, or it aborts.


> +    nsresult rv =
> +        gService->mCacheIOThread->Dispatch(event, NS_DISPATCH_NORMAL);
> +    if (NS_FAILED(rv)) {
> +        NS_WARNING("Failed dispatching block-event");
> +        delete event;
> +        return NS_ERROR_UNEXPECTED;
> +    }

This isn't IMO quite correct. Dispatch() internally addrefs and releases the event. So in case event dispatching fails, it could be deleted there. You should do:

nsCOMPtr<nsIRunnable> event = new nsBlockOnCacheThreadEvent();


Otherwise it looks good.
Attachment #500771 - Flags: review?(michal.novotny) → review+
(Assignee)

Comment 10

8 years ago
(In reply to comment #9)
> You don't have to check the return value of new. It either successfully
> allocates memory, or it aborts.

But such checking is surely done all over the place...?  :) See e.g. creation of mTransaction in nsHttpChannel::SetupTransaction(), mResponseHead in nsHtttpChannel::CheckCache() or mObserver in nsCacheService::Init()

I assume this pattern is used for good reasons?

> This isn't IMO quite correct. Dispatch() internally addrefs and releases the
> event. So in case event dispatching fails, it could be deleted there.

Could you point me to the code which does this book-keeping? I cannot find it.
(In reply to comment #10)
> (In reply to comment #9)
> > You don't have to check the return value of new. It either successfully
> > allocates memory, or it aborts.
> 
> But such checking is surely done all over the place...?  :)

Yes, it's only in the somewhat recent months that new has been infallible, so we still have tons of code that still checks. But there's no reason to add more, Michal is right here, you don't need checks like that any more.
(Assignee)

Comment 12

8 years ago
Fair enough. :)  About the Dispatch()-issue: Never mind - I overlooked the connection with the change to COMPtr.
(Assignee)

Comment 13

8 years ago
Created attachment 501111 [details] [diff] [review]
Proposed solution v4

Updated with review-comments. Requesting approval for 2.0
Attachment #500771 - Attachment is obsolete: true
Attachment #501111 - Flags: approval2.0?
Not blocking on this, but I'll approve the patch.
blocking2.0: ? → -
Attachment #501111 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 15

8 years ago
Didn't see the approval...  Patch still applies - requesting check-in.
Status: NEW → ASSIGNED
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/22a635e15db1
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.