Extend nsICacheEntry to include syntheticExperationTime

RESOLVED FIXED in mozilla34

Status

()

enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jaypoulz, Assigned: jaypoulz)

Tracking

unspecified
mozilla34
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 26 obsolete attachments)

19.06 KB, patch
jaypoulz
: review+
Details | Diff | Splinter Review
No description provided.
Blocks: 1016628
Blocks: 1020419
Assignee: nobody → jaypoulz
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Posted patch bug-1020416-fix.patch (obsolete) — Splinter Review
Would you mind looking over the changes I made to the _OldCacheEntryWrapper?

I think I handled the objects correctly, but I'm not 100% sure how this object actually works.
Attachment #8435146 - Flags: feedback?(honzab.moz)
Comment on attachment 8435146 [details] [diff] [review]
bug-1020416-fix.patch

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

Hmm.. something I realize now when I see the code..  The thing is that CacheEntry objects may go away in the middle of browser lifetime when there is a memory pressure or simply don't fit the pool, which is small.  This can happen only when the entry is not used by any consumer - actually, when it is not externally referenced.  I have to think whether to rather persist the data or change the code to prevent purging entry objects for which synth-exp-time > now.  I more tend to persist it to save memory.  That would also mean to just use metadata for this, no need to change the interface or use the new attribute only as a shortcut and make different implementations of the interface simpler in this manner...  I'm thinking here about SW as well, they may need to keep entries "synthesized for the future" a longer time but as well they may have a completely different cache entry implementation.  SW implementation is still vague these days.

::: netwerk/cache2/CacheEntry.cpp
@@ +160,5 @@
>                         const nsACString& aEnhanceID,
>                         bool aUseDisk)
>  : mFrecency(0)
>  , mSortingExpirationTime(uint32_t(-1))
> +, mSyntheticExpirationTime(uint32_t(-1))

Oh, no, please init to 0.  uint32_t(-1) is 0xFFFFFFFF and means like "expire never".  For synthetic exp time this is not an option.

See the IDL comment, which is actually the most important here - defines how this behaves and also what default it has to have to and why.

@@ +939,5 @@
>  }
>  
> +NS_IMETHODIMP CacheEntry::GetSyntheticExpirationTime(uint32_t *aSynthExpTime)
> +{
> +  *aSynthExpTime = mSyntheticExpirationTime;

check the aSynthExpTime is non-null first (NS_ENSURE_ARG())

::: netwerk/cache2/CacheEntry.h
@@ +112,5 @@
>    // Accessed only on the service management thread
>    double mFrecency;
>    uint32_t mSortingExpirationTime;
>  
> +  // Used to force cache load

express also this can be used on any thread, uint32 simple r/w access is thread safe.

::: netwerk/cache2/OldWrappers.cpp
@@ +371,5 @@
> +{
> +  uint32_t time;
> +
> +  // Call the new CacheEntry's getter
> +  nsresult rv = GetSyntheticExpirationTime(&time);

How is this supposed to work?  This will just indefinitely recurse.  I think you need to impl this from scratch on _OldCacheEntryWrapper, same way you do on CacheEntry (don't worry of duplication, _Old* stuff will soon go away).

@@ +384,5 @@
> +_OldCacheEntryWrapper::SetSyntheticExpirationTime(uint32_t aSynthExpTime)
> +{
> +
> +  // Call the new CacheEntry's setter
> +  return SetSyntheticExpirationTime(aSynthExpTime);

same here

::: netwerk/cache2/OldWrappers.h
@@ +33,5 @@
>  
>    NS_IMETHOD AsyncDoom(nsICacheEntryDoomCallback* listener);
>    NS_IMETHOD GetPersistent(bool *aPersistToDisk);
> +  NS_IMETHOD SetSyntheticExpirationTime(uint32_t aSynthExpTime);
> +  NS_IMETHOD GetSyntheticExpirationTime(uint32_t *aSynthExpTime);

I think

::: netwerk/cache2/nsICacheEntry.idl
@@ +63,5 @@
>    void setExpirationTime(in uint32_t expirationTime);
>  
>    /**
> +   * Get the synthetic expiration time of the cache entry (in seconds
> +   * since the Epoch). Can be used to force cache load.

Should be:

Synthetic exp time is intended to override the per-spec cache validation decisions when set to a non-null value in the future relative to unix timestamp in seconds.  Default value is 0 indicating no synthetic time has been set and normal cache validation decision will engage.

@@ +65,5 @@
>    /**
> +   * Get the synthetic expiration time of the cache entry (in seconds
> +   * since the Epoch). Can be used to force cache load.
> +   */
> +  attribute uint32_t syntheticExpirationTime;

mark this [infallible], it can be then used in cpp as |expTime = cacheEntry->GetSyntheticExpirationTime()|.

IMPORTANT: when you are adding/removing/modifying an interface method, you need to regen the IID at the top.  use |./mach uuid| to get some new.
Attachment #8435146 - Flags: feedback?(honzab.moz) → feedback-
nsm, do you have yet a more detailed plan on synthesizing the SW responses?  Are you going to use current CacheEntry (that you get via nsICacheStorageService/nsICacheStorage) or have your own impl?  In the former case - do you need the synthetic expiration time to persist?  In the letter case - does a new attribute on the interface fits your needs more then using cache entry metadata?
Flags: needinfo?(nsm.nikhil)
(In reply to Honza Bambas (:mayhemer) from comment #3)
> nsm, do you have yet a more detailed plan on synthesizing the SW responses? 
> Are you going to use current CacheEntry (that you get via
> nsICacheStorageService/nsICacheStorage) or have your own impl?  In the
> former case - do you need the synthetic expiration time to persist?  In the
> letter case - does a new attribute on the interface fits your needs more
> then using cache entry metadata?

I don't know which one to use yet. I just saw the link today for the new documentation on MDN about Cache2. For intercepting resource loads, I do not need the expiration time to persist across browser sessions.
Flags: needinfo?(nsm.nikhil)
Posted patch bug-1020416-fix.patch (obsolete) — Splinter Review
Attachment #8435146 - Attachment is obsolete: true
Attachment #8437225 - Flags: review?(honzab.moz)
Comment on attachment 8437225 [details] [diff] [review]
bug-1020416-fix.patch

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

::: netwerk/cache2/OldWrappers.h
@@ +49,5 @@
>    _OldCacheEntryWrapper(nsICacheEntryInfo* info);
>  
>    virtual ~_OldCacheEntryWrapper();
>  
> +  uint32_t mSyntheticExpirationTime;

make this private?

::: netwerk/cache2/nsICacheEntry.idl
@@ +15,5 @@
>  interface nsICacheListener;
>  interface nsIFile;
>  interface nsICacheEntryMetaDataVisitor;
>  
> +[scriptable, builtinclass, uuid(607c2a2c-0a48-40b9-a956-8cf2bb9857cf)]

please remove builtinclass, there is no need to forbid JS impls, what reason did you have?

@@ +67,5 @@
> +   * decisions when set to a non-null value in the future relative to unix
> +   * timestamp in seconds.  Default value is 0 indicating no synthetic time
> +   * has been set and normal cache validation decision will engage.
> +   */
> +  [infallible] attribute unsigned long syntheticExpirationTime;

As I'm thinking of it, the API can be better.

Please, let's do it like this:

- void enforceFor(unsigned long aSecondsToTheFuture);
- [infallible] readonly attribute bool isEnforced;

Please add good comments.

This way we will hide the PR_Now() arithmetic from consumers and usage will be simpler.

Let's go with the unpersisted version for now, only thing left to do is to return |true| from CacheEntry::IsReferenced() when isEnforced returns true.  However, when enforcement is set way to the future, we can keep this entry live for a long time and waste the pool limit.  If this becomes a problem, we can turn over to the persistent version (no need to change apis).
Attachment #8437225 - Flags: review?(honzab.moz) → feedback+
I added builtin class because the [infallible] attribute is only enabled for builtin class - as least insofar as the following error message:

xpidl.IDLError: error: [infallible] attributes are only allowed on [builtinclass] interfaces, ../../../dist/idl/nsICacheEntry.idl line 71:15
 0:28.97   [infallible] attribute unsigned long syntheticExpirationTime;
(In reply to Jeremy Poulin [:jerzilla] from comment #8)
> I added builtin class because the [infallible] attribute is only enabled for
> builtin class - as least insofar as the following error message:
> 
> xpidl.IDLError: error: [infallible] attributes are only allowed on
> [builtinclass] interfaces, ../../../dist/idl/nsICacheEntry.idl line 71:15
>  0:28.97   [infallible] attribute unsigned long syntheticExpirationTime;

Hmm.. then don't use [infallible], sad..
BTW, we can always add a {%C++ ... %} section and have a C++ getter there.  See nsILoadContextInfo as an example.
Posted patch bug-1020416-fix.patch (obsolete) — Splinter Review
Builtin class is needed to use the keyword 'infallible' (see comment above). I modified the names on a suggestion from Nick.  I hope the comments make my intentions sufficiently clear.  Lastly, I needed to make a second IsForcedValid in CacheEntry.cpp because of const correctness.

https://tbpl.mozilla.org/?tree=Try&rev=1ff9bdf50ad4
Attachment #8437225 - Attachment is obsolete: true
Attachment #8437870 - Flags: feedback?(honzab.moz)
Never mind the fix above - I didn't see your comment yet! :) I'll fix it right away.
Posted patch bug-1020416-fix.patch (obsolete) — Splinter Review
This a quick fix that removes infallible.
Attachment #8437870 - Attachment is obsolete: true
Attachment #8437870 - Flags: feedback?(honzab.moz)
Attachment #8437874 - Flags: feedback?(honzab.moz)
Seems like Nick and I read ones mind :D (comment 7)  I like his a bit more, they are more descriptive.
Comment on attachment 8437874 [details] [diff] [review]
bug-1020416-fix.patch

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

::: netwerk/cache2/CacheEntry.cpp
@@ +952,5 @@
> +}
> +
> +NS_IMETHODIMP CacheEntry::ForceValidFor(uint32_t aSecondsToTheFuture)
> +{
> +  mForcedValidTimeout = PR_Now() + aSecondsToTheFuture * pow(10, 6);

We have constants for conversions, however...

Use TimeStamp here instead.  Please use mozilla::TimeStamp::NowLoRes() to get the 'now' time.  Search for examples in mxr how this is used and compared, also reading the TimeStamp.h header is wise.

::: netwerk/cache2/CacheEntry.h
@@ +120,5 @@
>  private:
>    virtual ~CacheEntry();
>  
> +  // Tracks the state of forced cache validation
> +  uint32_t mForcedValidTimeout;

maybe mForcedValidUntil ?  or, to correspond with TimeStamp, mForcedValidUntilTimestamp.  Up to you.
Attachment #8437874 - Flags: feedback?(honzab.moz) → feedback+
Posted patch bug-1020416-fix.patch (obsolete) — Splinter Review
Attachment #8437874 - Attachment is obsolete: true
Attachment #8437981 - Flags: review?(honzab.moz)
Comment on attachment 8437981 [details] [diff] [review]
bug-1020416-fix.patch

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

::: netwerk/cache2/CacheEntry.cpp
@@ +179,5 @@
>  {
>    MOZ_COUNT_CTOR(CacheEntry);
>  
>    mService = CacheStorageService::Self();
> +  mForcedValidUntilTimestamp = TimeStamp();

you don't need to initialize it

@@ +947,5 @@
> +}
> +
> +bool CacheEntry::IsForcedValid() const
> +{
> +  if (mForcedValidUntilTimestamp.IsNull()) return false;

if () {
  ...
}

@@ +956,5 @@
> +{
> +  mForcedValidUntilTimestamp = TimeStamp::NowLoRes() +
> +    TimeDuration::FromSeconds(aSecondsToTheFuture);
> +
> +  return NS_OK;

Your code is correct here.

However, I forgot about one important factor.  We must also tell the file backend to not throw the entry away when disk limit is reached.

I need to think a bit and also ask Michal (his area) how to do this the best way.

::: netwerk/cache2/CacheEntry.h
@@ +120,5 @@
>  private:
>    virtual ~CacheEntry();
>  
> +  // Tracks the state of forced cache validation
> +  TimeStamp mForcedValidUntilTimestamp;

Please move this down as the last class member.

@@ +121,5 @@
>    virtual ~CacheEntry();
>  
> +  // Tracks the state of forced cache validation
> +  TimeStamp mForcedValidUntilTimestamp;
> +  bool IsForcedValid() const;

Please, move under TransferCallbacks() declaration with a blank line over it.  Add a short comment what it does/returns or at least what it forwards to.

::: netwerk/cache2/OldWrappers.cpp
@@ +351,5 @@
>  {
>    MOZ_COUNT_CTOR(_OldCacheEntryWrapper);
>    LOG(("Creating _OldCacheEntryWrapper %p for descriptor %p", this, desc));
> +
> +  mForcedValidUntilTimestamp = TimeStamp();

no need to do this this

@@ +360,5 @@
>  {
>    MOZ_COUNT_CTOR(_OldCacheEntryWrapper);
>    LOG(("Creating _OldCacheEntryWrapper %p for info %p", this, info));
> +
> +  mForcedValidUntilTimestamp = TimeStamp();

as well here.

@@ +373,5 @@
> +NS_IMETHODIMP _OldCacheEntryWrapper::GetIsForcedValid(bool *aIsForcedValid)
> +{
> +  NS_ENSURE_ARG(aIsForcedValid);
> +  *aIsForcedValid = mForcedValidUntilTimestamp.IsNull() ?
> +    false : TimeStamp::NowLoRes() <= mForcedValidUntilTimestamp;

please as:

  a = b 
    ? c
    : d;

::: netwerk/cache2/nsICacheEntry.idl
@@ +77,5 @@
> +   * @param aSecondsToTheFuture
> +   *        the number of seconds the default cache validation behavior will be
> +   *        overridden before it retuns to normal
> +   */
> +  void forceValidFor(in unsigned long aSecondsToTheFuture);

maybe turn them over - first introduce and explain forceValidFor, then the isForcedValid attribute.

also mention that forcing this long to future will waste memory since we have to keep the entry alive for the time.  then also mention this information is not persisted and doesn't survive session restart.
Attachment #8437981 - Flags: review?(honzab.moz)
Attachment #8437981 - Flags: review-
Attachment #8437981 - Flags: feedback+
CacheFileIOManager::DoomFileInternal will have to ask CacheStorageService for the force valid state.  This must be optional, a new argument is needed, that will be true only when called from CacheFileIOManager::OverLimitEvictionInternal (via CacheFileIOManager::DoomFileByKeyInternal).  See [1] as an example of how the file manager calls on the cache service.

We only need to modify DoomFileInternal() (and not the "ByKey" version of it) since we keep forced entries in memory, so there will always be CacheFileHandle found.

[1] http://hg.mozilla.org/mozilla-central/annotate/4876594bacaa/netwerk/cache2/CacheFileIOManager.cpp#l2087
Comment on attachment 8437981 [details] [diff] [review]
bug-1020416-fix.patch

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

::: netwerk/cache2/CacheEntry.cpp
@@ +948,5 @@
> +
> +bool CacheEntry::IsForcedValid() const
> +{
> +  if (mForcedValidUntilTimestamp.IsNull()) return false;
> +  return TimeStamp::NowLoRes() <= mForcedValidUntilTimestamp;

One more point here.  TimeStamp is not thread safe.  Since it's called from IsReferenced, that should be kept well performing, please change the code like this:

{
  if (mForcedValidUntilTimestamp.IsNull()) {
    return false;
  }

  mozilla::MutexAutoLock lock(const_cast<CacheEntry*>(this)->mLock);
  return TimeStamp::NowLoRes() <= mForcedValidUntilTimestamp;
}

@@ +954,5 @@
> +
> +NS_IMETHODIMP CacheEntry::ForceValidFor(uint32_t aSecondsToTheFuture)
> +{
> +  mForcedValidUntilTimestamp = TimeStamp::NowLoRes() +
> +    TimeDuration::FromSeconds(aSecondsToTheFuture);

lock over here as well.
Michal, does comment 19 make sense to you?
Flags: needinfo?(michal.novotny)
(In reply to Honza Bambas (:mayhemer) from comment #19)
> One more point here.  TimeStamp is not thread safe.  Since it's called from
> IsReferenced, that should be kept well performing, please change the code
> like this:
> 
> {
>   if (mForcedValidUntilTimestamp.IsNull()) {
>     return false;
>   }
> 
>   mozilla::MutexAutoLock lock(const_cast<CacheEntry*>(this)->mLock);
>   return TimeStamp::NowLoRes() <= mForcedValidUntilTimestamp;
> }

Do we also have to worry about thread safety in the _OldCacheEntryWrapper?
(In reply to Honza Bambas (:mayhemer) from comment #18)
> CacheFileIOManager::DoomFileInternal will have to ask CacheStorageService
> for the force valid state.  This must be optional, a new argument is needed,
> that will be true only when called from
> CacheFileIOManager::OverLimitEvictionInternal (via
> CacheFileIOManager::DoomFileByKeyInternal).  See [1] as an example of how
> the file manager calls on the cache service.
> 
> We only need to modify DoomFileInternal() (and not the "ByKey" version of
> it) since we keep forced entries in memory, so there will always be
> CacheFileHandle found.
> 
> [1]
> http://hg.mozilla.org/mozilla-central/annotate/4876594bacaa/netwerk/cache2/
> CacheFileIOManager.cpp#l2087

Alright so I understand how the file manager eviction flow gets to doomFileInternal (so I have added default arguments to allow us to intercept this special case); however, I have no idea
how to actually get the CacheEntry object associated with the handle. It seems to me as though you can use the CacheStorageService to start an EntryInfoCallback, but this does not give you access to the CacheEntry object itself. The CacheStorageService also has access to an array of registered CacheEntry objects, but this cannot be accessed externally. How should I go about retrieving this object? Do I need to extend this CacheStorageService?
Flags: needinfo?(honzab.moz)
(In reply to Jeremy Poulin [:jerzilla] from comment #21)
> Do we also have to worry about thread safety in the _OldCacheEntryWrapper?

Just don't implement it :)

(In reply to Jeremy Poulin [:jerzilla] from comment #22)
> (In reply to Honza Bambas (:mayhemer) from comment #18)
> > CacheFileIOManager::DoomFileInternal will have to ask CacheStorageService
> > for the force valid state.  This must be optional, a new argument is needed,
> > that will be true only when called from
> > CacheFileIOManager::OverLimitEvictionInternal (via
> > CacheFileIOManager::DoomFileByKeyInternal).  See [1] as an example of how
> > the file manager calls on the cache service.
> > 
> > We only need to modify DoomFileInternal() (and not the "ByKey" version of
> > it) since we keep forced entries in memory, so there will always be
> > CacheFileHandle found.
> > 
> > [1]
> > http://hg.mozilla.org/mozilla-central/annotate/4876594bacaa/netwerk/cache2/
> > CacheFileIOManager.cpp#l2087
> 
> Alright so I understand how the file manager eviction flow gets to
> doomFileInternal (so I have added default arguments to allow us to intercept
> this special case); however, I have no idea
> how to actually get the CacheEntry object associated with the handle. It
> seems to me as though you can use the CacheStorageService to start an
> EntryInfoCallback, but this does not give you access to the CacheEntry
> object itself. 

No, but you can extend the callback to provide that info (a bool aIsForcedValid) - I completely forgot about the GetCacheEntryInfo methods :).  Where you don't have an entry, just set it to false.

> The CacheStorageService also has access to an array of
> registered CacheEntry objects, but this cannot be accessed externally. How
> should I go about retrieving this object? Do I need to extend this
> CacheStorageService?

I think it's clear now.  If not feel free to ask again.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #20)
> However, I forgot about one important factor.  We must also tell the file
> backend to not throw the entry away when disk limit is reached.

Why it must not be evicted? I understand that if we have some synthetic expiration time we should ignore normal expiration time when evicting entries, but it doesn't mean that the entry cannot be evicted.


(In reply to Honza Bambas (:mayhemer) from comment #20)
> Michal, does comment 19 make sense to you?

Do you mean comment 18 or 19? Anyway, I don't understand what is the purpose of the change in this bug and how should we behave when dooming or evicting entries.
Flags: needinfo?(michal.novotny)
I don't know if I can answer these in sufficient detail, but I will try.

(In reply to Michal Novotny (:michal) from comment #24)
> (In reply to Honza Bambas (:mayhemer) from comment #20)
> > However, I forgot about one important factor.  We must also tell the file
> > backend to not throw the entry away when disk limit is reached.
> 
> Why it must not be evicted? I understand that if we have some synthetic
> expiration time we should ignore normal expiration time when evicting
> entries, but it doesn't mean that the entry cannot be evicted.
>

I think the idea is that we should evict the entry until the synthetic expiration time expires.  Since the value doesn't persist to disk, the only way we can guarantee the preservation of its value is to prevent eviction until it is no longer valid. (At least that's my understanding - hopefully Honza can clarify further).
 
> 
> (In reply to Honza Bambas (:mayhemer) from comment #20)
> > Michal, does comment 19 make sense to you?
> 
> Do you mean comment 18 or 19? Anyway, I don't understand what is the purpose
> of the change in this bug and how should we behave when dooming or evicting
> entries.

I believe Honza meant comment 18.
(In reply to Michal Novotny (:michal) from comment #24)
> (In reply to Honza Bambas (:mayhemer) from comment #20)
> > However, I forgot about one important factor.  We must also tell the file
> > backend to not throw the entry away when disk limit is reached.
> 
> Why it must not be evicted? I understand that if we have some synthetic
> expiration time we should ignore normal expiration time when evicting
> entries, but it doesn't mean that the entry cannot be evicted.

The sentence is controversial as I can understand it.

Hmm... actually, since we keep the entry alive, the doomed data file will remain, is that so?  If yes, then there is nothing more to do!  

Note that the synthetic exp time doesn't go down to the CacheFile, it's only held by the entry.

> 
> 
> (In reply to Honza Bambas (:mayhemer) from comment #20)
> > Michal, does comment 19 make sense to you?
> 
> Do you mean comment 18 or 19? Anyway, I don't understand what is the purpose
> of the change in this bug and how should we behave when dooming or evicting
> entries.

Ah, sorry, 18 indeed.  You don't understand it?  I think it's clear enough.  The thing is that some consumers want to force a channel to use the cache entry, regardless load flags/real exp time/validation logic.  The most convenient way is to set a time for which the entry is considered as 'forced to be load from'.  That's all.

I'm trying to find a simple way to do this.  My only concern is we may eat memory with this since forced entries has to be left alive.

I also play with an idea for a completely different implementation (and "make forced valid" API with it) using a hash table where you only add a record to that an entry in a context/URI/eid is considered forced.  But let's wait how this is going to work and how this is going to be performing and eating memory.

Nick, this is by the way the reason I wanted as much details as possible ;)  I didn't go though the whole comment yet but I will need more details soon probably.  This whole project needs careful design decisions NOW, before we get bound by an already written code too much.
(In reply to Honza Bambas (:mayhemer) from comment #26)
> > Why it must not be evicted? I understand that if we have some synthetic
> > expiration time we should ignore normal expiration time when evicting
> > entries, but it doesn't mean that the entry cannot be evicted.
> 
> The sentence is controversial as I can understand it.

Why? Even if we had the synthetic exptime so we wouldn't evict the entry as expired, it might be still evicted by frecency.


> Hmm... actually, since we keep the entry alive, the doomed data file will
> remain, is that so?  If yes, then there is nothing more to do!

The data is not deleted until anybody uses it. But the entry cannot be found by AsyncOpenURI() once it is doomed.


> Ah, sorry, 18 indeed.  You don't understand it?  I think it's clear enough. 
> The thing is that some consumers want to force a channel to use the cache
> entry, regardless load flags/real exp time/validation logic.  The most
> convenient way is to set a time for which the entry is considered as 'forced
> to be load from'.  That's all.

It isn't clear enough just because there is no description of the functionality in this bug. The only description in the patch in nsICacheEntry.idl doesn't say anything about that the entry must not be evicted when forceValidFor() is called. And if I understand your text above correctly, comment #18 is completely irrelevant, so don't be surprised that I'm confused...
Michal, we probably had to CC you sooner.

(In reply to Michal Novotny (:michal) from comment #27)
> (In reply to Honza Bambas (:mayhemer) from comment #26)
> > > Why it must not be evicted? I understand that if we have some synthetic
> > > expiration time we should ignore normal expiration time when evicting
> > > entries, but it doesn't mean that the entry cannot be evicted.
> > 
> > The sentence is controversial as I can understand it.
> 
> Why? Even if we had the synthetic exptime so we wouldn't evict the entry as
> expired, it might be still evicted by frecency.

Got it now.  And truly, there is nothing said about this in the IDL.  But we need to keep the entry...

> 
> 
> > Hmm... actually, since we keep the entry alive, the doomed data file will
> > remain, is that so?  If yes, then there is nothing more to do!
> 
> The data is not deleted until anybody uses it. But the entry cannot be found
> by AsyncOpenURI() once it is doomed.

...because of this.  Good point.

> 
> 
> > Ah, sorry, 18 indeed.  You don't understand it?  I think it's clear enough. 
> > The thing is that some consumers want to force a channel to use the cache
> > entry, regardless load flags/real exp time/validation logic.  The most
> > convenient way is to set a time for which the entry is considered as 'forced
> > to be load from'.  That's all.
> 
> It isn't clear enough just because there is no description of the
> functionality in this bug. The only description in the patch in
> nsICacheEntry.idl doesn't say anything about that the entry must not be
> evicted when forceValidFor() is called. And if I understand your text above
> correctly, comment #18 is completely irrelevant, so don't be surprised that
> I'm confused...

This needs to read the whole bug first, right.  

So, what to do?  I think we still need to let eviction happen for forced valid entries.  

So I'm asking again, does comment 18 make sense to you as we now made clear what we want to do here?
(In reply to Honza Bambas (:mayhemer) from comment #28)
> So, what to do?  I think we still need to let eviction happen for forced
> valid entries.  

I think we still need to *NOT* let the eviction happen...
(In reply to Honza Bambas (:mayhemer) from comment #28)
> So, what to do?  I think we still need to not let eviction happen for forced
> valid entries.  
> 
> So I'm asking again, does comment 18 make sense to you as we now made clear
> what we want to do here?

Comment #18 makes sense but I see a problem when there will be a lot of forced valid entries and the cache size will be small. In case when sum of all forced valid entries is larger than cache size limit, the eviction will run over and over trying to evict some entry.

Also, it's not clear to me why CacheStorageService::CacheFileDoomed() returns early when the entry is referenced. How it works for normal entries? And how will it work for forced valid entries that are not in fact referenced, i.e. there is only the fake reference caused by IsForcedValid().
Posted patch bug-1020416-fix.patch (obsolete) — Splinter Review
I know you guys are still making some overall design decisions, but I'm stuck. How do I get the cache entry I want to doom in OnEntryInfo() in CacheFileIOManager.cpp? (This will become more clear when looking at the code.)

Also, am I generally on the right track with this? Obviously it needs documentation, but it's hard to document something when the spec is still being worked on haha. :)

Thanks!
Attachment #8437981 - Attachment is obsolete: true
Attachment #8441657 - Flags: feedback?(michal.novotny)
Attachment #8441657 - Flags: feedback?(honzab.moz)
(In reply to Jeremy Poulin [:jerzilla] from comment #31)
> Created attachment 8441657 [details] [diff] [review]
> bug-1020416-fix.patch
> 
> I know you guys are still making some overall design decisions, but I'm
> stuck. How do I get the cache entry I want to doom in OnEntryInfo() in
> CacheFileIOManager.cpp? (This will become more clear when looking at the
> code.)
> 
> Also, am I generally on the right track with this? Obviously it needs
> documentation, but it's hard to document something when the spec is still
> being worked on haha. :)
> 
> Thanks!

Have a helper class (local will be enough :)) implementing the OnEntryInfo callback and give it the CacheFileHandle.  In OnEntryInfo it will just call back on the IO manager giving it this handle and aCheckIfForcedValid=false.
Comment on attachment 8441657 [details] [diff] [review]
bug-1020416-fix.patch

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

::: netwerk/cache2/CacheEntry.cpp
@@ +956,5 @@
> +}
> +
> +NS_IMETHODIMP CacheEntry::ForceValidFor(uint32_t aSecondsToTheFuture)
> +{
> +  mozilla::MutexAutoLock lock(const_cast<CacheEntry*>(this)->mLock);

no need to const_cast here.

::: netwerk/cache2/CacheStorageService.h
@@ +99,5 @@
> +                             int64_t aDataSize,
> +                             int32_t aFetchCount,
> +                             uint32_t aLastModifiedTime,
> +                             uint32_t aExpirationTime,
> +                             bool aIsForcedValid = false) = 0;

don't use default arg here
Attachment #8441657 - Flags: feedback?(honzab.moz) → feedback-
(In reply to Honza Bambas (:mayhemer) from comment #33)
> Comment on attachment 8441657 [details] [diff] [review]
> bug-1020416-fix.patch
> 
> Review of attachment 8441657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheEntry.cpp
> @@ +956,5 @@
> > +}
> > +
> > +NS_IMETHODIMP CacheEntry::ForceValidFor(uint32_t aSecondsToTheFuture)
> > +{
> > +  mozilla::MutexAutoLock lock(const_cast<CacheEntry*>(this)->mLock);
> 
> no need to const_cast here.

I think you do need the const cast - when I remove it I get the following build failure:

// -- Build Trace Start -- //

/home/jpoulin/mozilla-central/netwerk/cache2/CacheEntry.cpp:954:26: error: no matching
     constructor for initialization of 'mozilla::MutexAutoLock'
     (aka 'BaseAutoLock<mozilla::Mutex>')

 0:04.74   mozilla::MutexAutoLock lock(this->mLock);
 0:04.74                          ^    ~~~~~~~~~~~
 0:04.74 ../../dist/include/mozilla/Mutex.h:168:5: note: candidate constructor not viable:
          1st argument ('const mozilla::Mutex') would lose const qualifier

 0:04.74     BaseAutoLock(T& aLock MOZ_GUARD_OBJECT_NOTIFIER_PARAM) :
 0:04.74     ^
 0:04.74 ../../dist/include/mozilla/Mutex.h:182:5: note: candidate constructor not viable:
         no known conversion from 'const mozilla::Mutex' to
         'mozilla::BaseAutoLock<mozilla::Mutex> &' for 1st argument

// -- Build Trace Ends -- //

This suggests the mutex lock function is only usuable from a constant class pointer.
Sorry, but NS_IMETHODIMP CacheEntry::ForceValidFor is an interface non-const method.  And we do mozilla::MutexAutoLock lock(mLock); all around the code.  If it fails for you here, you do something else wrong.  There is no need for const_cast here.  Note that you don't need |this->mLock| but just |mLock|.
(In reply to Honza Bambas (:mayhemer) from comment #35)
> Sorry, but NS_IMETHODIMP CacheEntry::ForceValidFor is an interface non-const
> method.  And we do mozilla::MutexAutoLock lock(mLock); all around the code. 
> If it fails for you here, you do something else wrong.  There is no need for
> const_cast here.  Note that you don't need |this->mLock| but just |mLock|.

Yeah I messed up. Changed it in the wrong place. Sorry!
Posted patch bug-1020416-fix.patch (obsolete) — Splinter Review
I still have some documentation work to do, but I figured I'd upload a new version while I have something stable.
Attachment #8441657 - Attachment is obsolete: true
Attachment #8441657 - Flags: feedback?(michal.novotny)
Attachment #8443179 - Flags: feedback?(honzab.moz)
Posted patch bug-1020416-fix.patch (obsolete) — Splinter Review
Attachment #8443179 - Attachment is obsolete: true
Attachment #8443179 - Flags: feedback?(honzab.moz)
Attachment #8444652 - Flags: review?(honzab.moz)
Posted patch bug-1020416-fix.patch (obsolete) — Splinter Review
Deals with the merge conflict introduced in last nightly update. Functionally the same as previous patch.
Attachment #8444652 - Attachment is obsolete: true
Attachment #8444652 - Flags: review?(honzab.moz)
Attachment #8446126 - Flags: review?(honzab.moz)
Comment on attachment 8446126 [details] [diff] [review]
bug-1020416-fix.patch

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

Jeremy, how keen would you be to try a second implementation using a hash table (keyed by a string as generated at [1], that also is aHandle->Key()) in CacheStorageService rather than flagging each entry?  The IO manger will still have to ask the service for the force-valid info, but IMO would be a bit simpler (no need to parse the key, since the very same key would be used for the handle keying).

I can't recall if there were a strong argument against, but I cannot say I'm 100% satisfied with approach we have chosen here.

[1] http://hg.mozilla.org/mozilla-central/annotate/aab918af0f9c/netwerk/cache2/CacheEntry.cpp#l312

::: netwerk/cache2/CacheEntry.cpp
@@ +951,5 @@
> +    return false;
> +  }
> +
> +  mozilla::MutexAutoLock lock(const_cast<CacheEntry*>(this)->mLock);
> +  return TimeStamp::NowLoRes() <= mForcedValidUntilTimestamp;

maybe (to optimize the locking even more) set mForcedValidUntilTimestamp to TimeStamp() when 'expired'.  It will effectively null it out and on next enter of IsForcedValid() you will not need to enter the lock anymore again.  

However, then you need to do a check for mForcedValidUntilTimestamp.IsNull() once more *under the lock* since operator usage on timestamps is not allowed when one of the sides is null (could happen in case of a tight race).

Only fight might be the constness here...  up to you.  Definitely IsReferenced() must remain const.

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2037,5 @@
>  
> +// A helper class used to verify whether an entry is currently being
> +// forced valid, and therefore should not be evicted. If the entry is
> +// safe to evict, DoomFileInternal() is called on the associated entry.
> +class CacheEntryValidationHelper : public CacheStorageService::EntryInfoCallback

put this class to an anonymous namespace or just declare it in the method directly (locally)

namespace { // anon
...
} // anon

@@ +2045,5 @@
> +  CacheFileHandle *mCacheFileHandle;
> +
> +public:
> +  nsresult Init(CacheFileIOManager *aCacheFileIOManager,
> +                CacheFileHandle *aCacheFileHandle)

pass via constructor, no need for nsresult Init.

@@ +2049,5 @@
> +                CacheFileHandle *aCacheFileHandle)
> +  {
> +
> +    NS_ENSURE_ARG(aCacheFileIOManager);
> +    NS_ENSURE_ARG(aCacheFileHandle);

don't do these checks for an internal class.. it's overkill

@@ +2065,5 @@
> +                   uint32_t aExpirationTime, bool aIsForcedValid)
> +  {
> +    if (!aIsForcedValid) {
> +      // It is safe to evict this entry
> +      mCacheFileIOManager->DoomFileInternal(mCacheFileHandle, false);

since the result is synchronous (this callback is invoked from inside GetCacheEntryInfo) maybe just remember the entry is force valid in this class (as a bool member) and then just don't return NS_OK from DoomFileInternal() when the flag is false, just let the code go on and do the job.

Then you don't need to pass the manager and the entry here at all, don't need friendship, the class will be simpler.

@@ +2086,5 @@
>  
> +  // Check if forced valid before evicting
> +  if (aCheckIfForcedValid) {
> +    nsRefPtr<CacheStorageService> storageService = CacheStorageService::Self();
> +    if (storageService) {

new line before if ()

@@ +2087,5 @@
> +  // Check if forced valid before evicting
> +  if (aCheckIfForcedValid) {
> +    nsRefPtr<CacheStorageService> storageService = CacheStorageService::Self();
> +    if (storageService) {
> +

no new line here

@@ +2092,5 @@
> +      nsAutoCString enhanceId;
> +      nsAutoCString uriSpec;
> +
> +      nsRefPtr<nsILoadContextInfo> info =
> +        CacheFileUtils::ParseKey(aHandle->Key(), &enhanceId, &uriSpec);

hmm.. happens twice, not good.  can we somehow optimize?  some kind of a lazy parse helper class or so?

@@ +2096,5 @@
> +        CacheFileUtils::ParseKey(aHandle->Key(), &enhanceId, &uriSpec);
> +
> +      MOZ_ASSERT(info);
> +      if (!info) {
> +        return NS_OK; // should never happen... hopefully

don't return on this failure, just bypass your new code.

@@ +2099,5 @@
> +      if (!info) {
> +        return NS_OK; // should never happen... hopefully
> +      }
> +
> +      CacheEntryValidationHelper *helper = new CacheEntryValidationHelper();

Don't allocate (btw, this way you happily leak).  Just have the class on stack.

@@ +2375,5 @@
>        return NS_OK;
>      }
>  
>      // When we are here, there is no existing entry and we need
> +    // to synchronously load metadata from a disk file.

it's ok now, but in general, when you find a typo, just leave it unless it's close the code you modify.  these changes like this just makes the patch larger, potentially more conflicting with work of others, harder to back out when needed.

::: netwerk/cache2/CacheFileIOManager.h
@@ +311,5 @@
>    friend class RenameFileEvent;
>    friend class CacheIndex;
>    friend class MetadataWriteScheduleEvent;
>    friend class CacheFileContextEvictor;
> +  friend class CacheEntryValidationHelper;

you will probably not need this with my suggested changes.

::: netwerk/cache2/CacheStorageService.cpp
@@ +310,3 @@
>    {
>      nsCOMPtr<nsIURI> uri;
> +

no new line here

::: netwerk/cache2/OldWrappers.cpp
@@ +371,5 @@
> +  NS_ENSURE_ARG(aIsForcedValid);
> +
> +  *aIsForcedValid = mForcedValidUntilTimestamp.IsNull()
> +    ? false
> +    : TimeStamp::NowLoRes() <= mForcedValidUntilTimestamp;

how do you ensure thread safety on access to mForcedValidUntilTimestamp?  As I said before, just don't impl this for old wrappers.

::: netwerk/cache2/nsICacheStorageVisitor.idl
@@ +24,5 @@
>                          in int64_t aDataSize,
>                          in long aFetchCount,
>                          in uint32_t aLastModifiedTime,
> +                        in uint32_t aExpirationTime,
> +                        in bool aIsForcedValid);

I don't see a single reason to expose this here, on this interface.  You only need it on the internal helper abstract class.  Please don't include this unnecessary change in this patch.  If otherwise, please put arguments.
Attachment #8446126 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #40)
> Comment on attachment 8446126 [details] [diff] [review]
> bug-1020416-fix.patch
> 
> Review of attachment 8446126 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Jeremy, how keen would you be to try a second implementation using a hash
> table (keyed by a string as generated at [1], that also is aHandle->Key())
> in CacheStorageService rather than flagging each entry?  The IO manger will
> still have to ask the service for the force-valid info, but IMO would be a
> bit simpler (no need to parse the key, since the very same key would be used
> for the handle keying).
> 
> I can't recall if there were a strong argument against, but I cannot say I'm
> 100% satisfied with approach we have chosen here.
> 
> [1]
> http://hg.mozilla.org/mozilla-central/annotate/aab918af0f9c/netwerk/cache2/
> CacheEntry.cpp#l312

I'll start by making the necessary changes to this implementation, then I'll branch off that to make a second implementation using the hash table. I should have them both submitted for review (hopefully) by tommorow.
Posted patch bug-1020416-fix.patch (obsolete) — Splinter Review
This is the patch you reviewed today with the fixes you mentioned, tomorrow I'll do the other implementation and upload that for review.
Attachment #8446126 - Attachment is obsolete: true
Attachment #8446923 - Flags: review?(honzab.moz)
This is the alternative version you requested which uses hash tables to track cache entries. I am tempted to move the key gen code into the constructor/init, to prevent the case where we try to use it uninitialized, but I figured you would know best where it belongs and might have a good reason for it to be in onLoad. Took a little longer than expected, but I've also being busy with intern-stuff. :)
Attachment #8448287 - Flags: review?(honzab.moz)
(In reply to Jeremy Poulin [:jerzilla] from comment #43)
> Created attachment 8448287 [details] [diff] [review]
> bug-1020416-fix-with-hash.patch

I've run this code, and I'm pretty sure it leaks memory, but I'm not sure where. I'd appreciate it if you could note anywhere this might be happening in your review. Thanks :D
Thanks Jeremy, will look at it tomorrow.
Now with less memory leakage. :D
Attachment #8448287 - Attachment is obsolete: true
Attachment #8448287 - Flags: review?(honzab.moz)
Attachment #8448894 - Flags: review?(honzab.moz)
Comment on attachment 8446923 [details] [diff] [review]
bug-1020416-fix.patch

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

looks good, how was this tested?

::: netwerk/cache2/CacheEntry.cpp
@@ +950,5 @@
> +  if (mForcedValidUntilTimestamp.IsNull()) {
> +    return false;
> +  }
> +
> +  mozilla::MutexAutoLock lock(mLock);

blank like after this

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2034,5 @@
>  
>    return NS_OK;
>  }
>  
> +namespace { // anon

blank like after this

@@ +2041,5 @@
> +  // safe to evict, DoomFileInternal() is called on the associated entry.
> +  class CacheEntryValidationHelper : public CacheStorageService::EntryInfoCallback
> +  {
> +  public:
> +    bool mIsForcedValid = false;

this works? :)  some c++11 thing I'm not yet aware of? :)

@@ +2063,5 @@
> +    nsRefPtr<nsILoadContextInfo> mInfo;
> +
> +    void Parse(CacheStorageService *aCacheStorageService,
> +               const nsCSubstring &aKey) {
> +      mInfo = CacheFileUtils::ParseKey(aKey, &mEnhanceId, &mUriSpec);

bypass when mInfo is already non-null?

what's aCacheStorageService for?

@@ +2066,5 @@
> +               const nsCSubstring &aKey) {
> +      mInfo = CacheFileUtils::ParseKey(aKey, &mEnhanceId, &mUriSpec);
> +    }
> +  };
> +} // anon

blank line before this

@@ +2147,5 @@
>  
>    if (!aHandle->IsSpecialFile()) {
>      nsRefPtr<CacheStorageService> storageService = CacheStorageService::Self();
>      if (storageService) {
> +

no blank line

::: netwerk/cache2/CacheStorageService.cpp
@@ +314,5 @@
>        return;
>  
> +    mCallback->OnCacheEntryInfo(uri, aIdEnhance, aDataSize,
> +                                aFetchCount, aLastModifiedTime,
> +                                aExpirationTime);

leave intact

@@ +374,5 @@
>          return NS_OK;
>  
>        mWalker->mCallback->OnCacheEntryInfo(
> +        uri, mIdEnhance, mDataSize, mFetchCount, mLastModifiedTime,
> +        mExpirationTime);

leave intact

@@ +386,5 @@
>      int64_t mDataSize;
>      int32_t mFetchCount;
>      uint32_t mLastModifiedTime;
>      uint32_t mExpirationTime;
> +    bool mIsForcedValid;

remove

@@ +479,5 @@
>      info->mDataSize = aDataSize;
>      info->mFetchCount = aFetchCount;
>      info->mLastModifiedTime = aLastModifiedTime;
>      info->mExpirationTime = aExpirationTime;
> +    info->mIsForcedValid = aIsForcedValid;

remove

::: netwerk/cache2/OldWrappers.cpp
@@ +267,5 @@
>      lastModified = 0;
>  
>    // Send them to the consumer.
> +  rv = mCB->OnCacheEntryInfo(uri, enhanceId, (int64_t)dataSize, fetchCount,
> +                             lastModified, expirationTime);

leave intact please.

@@ +367,5 @@
>  }
>  
> +NS_IMETHODIMP _OldCacheEntryWrapper::GetIsForcedValid(bool *aIsForcedValid)
> +{
> +  // Used stub

so used or unused?

::: netwerk/cache2/OldWrappers.h
@@ +54,5 @@
>    _OldCacheEntryWrapper() MOZ_DELETE;
>    nsICacheEntryDescriptor* mOldDesc; // ref holded in mOldInfo
>    nsCOMPtr<nsICacheEntryInfo> mOldInfo;
> +
> +  mozilla::TimeStamp mForcedValidUntilTimestamp;

remove this please.
Attachment #8446923 - Flags: review?(honzab.moz) → feedback+
Comment on attachment 8448894 [details] [diff] [review]
bug-1020416-fix-with-hash.patch

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

::: netwerk/cache2/CacheEntry.h
@@ +125,5 @@
>    // Keep the service alive during life-time of an entry
>    nsRefPtr<CacheStorageService> mService;
>  
> +  // Trackable Identifier for this cache entry
> +  nsAutoCString mCacheEntryKey;

don't cache this please

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2056,5 @@
> +    if (storageService) {
> +
> +      nsRefPtr<CacheEntry> cacheEntry;
> +      storageService->mPossiblyForcedValidEntries->Get(aHandle->Key(),
> +                                                       getter_AddRefs(cacheEntry));

have a method, I don't want direct access on members.

::: netwerk/cache2/CacheStorageService.h
@@ +257,5 @@
>  
>    mozilla::Mutex mLock;
>  
> +  // Tracks entries that may be forced valid.
> +  nsRefPtrHashtable<nsCStringHashKey, CacheEntry> *mPossiblyForcedValidEntries;

just use directly as a member, not dynamically allocated

but this is definitely not what I wanted.  The values has to be timestamps.  Those timestamps then will be checked (the other patch keeps in the entry, so we must keep the entry alive to preserve that information, but here I want you to keep only that timestamp information here, so that we don't need to keep the entry alive)

sorry, I probably didn't make my self enough clear.
Attachment #8448894 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #47)
> Comment on attachment 8446923 [details] [diff] [review]
> bug-1020416-fix.patch
> 
> Review of attachment 8446923 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good, how was this tested?
> 
I've been testing all of these cache level patches against my prefetch implementation.


> 
> @@ +2041,5 @@
> > +  // safe to evict, DoomFileInternal() is called on the associated entry.
> > +  class CacheEntryValidationHelper : public CacheStorageService::EntryInfoCallback
> > +  {
> > +  public:
> > +    bool mIsForcedValid = false;
> 
> this works? :)  some c++11 thing I'm not yet aware of? :)
Short answer, yes. Long answer -> http://www.informit.com/articles/article.aspx?p=1852519
(In reply to Honza Bambas (:mayhemer) from comment #47)
> Comment on attachment 8446923 [details] [diff] [review]
> bug-1020416-fix.patch
> 
> Review of attachment 8446923 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good, how was this tested?
> 
> ::: netwerk/cache2/CacheEntry.cpp
> @@ +950,5 @@
> > +  if (mForcedValidUntilTimestamp.IsNull()) {
> > +    return false;
> > +  }
> > +
> > +  mozilla::MutexAutoLock lock(mLock);
> 
> blank like after this
> 
> ::: netwerk/cache2/CacheFileIOManager.cpp
> @@ +2034,5 @@
> >  
> >    return NS_OK;
> >  }
> >  
> > +namespace { // anon
> 
> blank like after this
> 
> @@ +2041,5 @@
> > +  // safe to evict, DoomFileInternal() is called on the associated entry.
> > +  class CacheEntryValidationHelper : public CacheStorageService::EntryInfoCallback
> > +  {
> > +  public:
> > +    bool mIsForcedValid = false;
> 
> this works? :)  some c++11 thing I'm not yet aware of? :)
> 
> @@ +2063,5 @@
> > +    nsRefPtr<nsILoadContextInfo> mInfo;
> > +
> > +    void Parse(CacheStorageService *aCacheStorageService,
> > +               const nsCSubstring &aKey) {
> > +      mInfo = CacheFileUtils::ParseKey(aKey, &mEnhanceId, &mUriSpec);
> 
> bypass when mInfo is already non-null?

??? 

If mInfo is non-null, there is no reason to parse. This is reflected in the logic of the code further down. Would you prefer this nuance to be handled in the helper class?
Flags: needinfo?(honzab.moz)
Posted patch bug-1020416-fix.patch (obsolete) — Splinter Review
Attachment #8446923 - Attachment is obsolete: true
Attachment #8449832 - Flags: feedback?(honzab.moz)
Flags: needinfo?(honzab.moz)
(In reply to Jeremy Poulin [:jerzilla] from comment #50)
> If mInfo is non-null, there is no reason to parse. This is reflected in the
> logic of the code further down. Would you prefer this nuance to be handled
> in the helper class?

Yep, I can see you put it to the patch already.  good.
Attachment #8449832 - Flags: feedback?(honzab.moz) → feedback+
I'm know sure if this is what you wanted, but I think I'm at least moving in the right direction.
Attachment #8448894 - Attachment is obsolete: true
Attachment #8450461 - Flags: feedback?(honzab.moz)
Comment on attachment 8450461 [details] [diff] [review]
bug-1020416-fix-with-hash.patch

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

Yes, you are on the right path!  Thanks.

::: netwerk/cache2/CacheEntry.cpp
@@ +951,5 @@
> +
> +  rv = HashingKeyWithStorage(key);
> +
> +  if (NS_SUCCEEDED(rv)) {
> +    mozilla::MutexAutoLock lock(mLock);

The lock is per-entry (there are many entries, simplified view can be one per each URL).  But the table is a global for all entries, so using this lock is probably not what you want ;)

Please don't access the server's members directly.

Have methods on CacheStorageService:

bool IsForcedValidEntry(CacheEntry* aEntry);
void ForceEntryValid(CacheEntry* aEntry, seconsToFuture);

Those will:
- get the hashing key
- use the service lock over put/get to/from the hashtable
- do all the logic around it (comparing to NowLoRes()/setting to NowLoRes() + SecondsToFuture...) and return value

Put them under RecordMemoryOnlyEntry declaration/definition (there is a section dedicated to method of the service that entries internally use), add comments.


You also need such a method for the IO manager (we still need the don't doom check..), have a method on the service:

bool IsForcedValidEntry(nsACString const & aHashingKey);

- actually, the implementation of the CacheEntry*-taking method should then be (simplified):

bool IsForcedValidEntry(CacheEntry* aEntry)
{
  hashKey = aEntry->HashKey();
  return IsForcedValidEntry(hashKey);
}

Put the declaration/definition close to GetCacheEntryInfo

@@ +954,5 @@
> +  if (NS_SUCCEEDED(rv)) {
> +    mozilla::MutexAutoLock lock(mLock);
> +    storageService->mPossiblyForcedValidEntries.Get(key, &validUntilPtr);
> +
> +    if (validUntilPtr) {

in general, I more prefer a code style like:

if (NS_FAILED(rv))
  return;

do something...

if (!found)
  return;

do something with found...

etc.

It's easier to read, it's performing better and you don't need to endlessly indent :)

Nit: just set *aIsForceValid = false at the beginning as a default ret val.

::: netwerk/cache2/CacheStorageService.h
@@ +258,5 @@
>  
>    mozilla::Mutex mLock;
>  
> +  // Tracks entries that may be forced valid.
> +  nsClassHashtable<nsCStringHashKey, TimeStamp> mPossiblyForcedValidEntries;

just use nsDataHashtable, no need to allocate TimeStamp every time.

mForcedValidEntries might be a sufficient name.

The comment has to be more detailed.  Also mention it's access under the service lock only.
Attachment #8450461 - Flags: feedback?(honzab.moz) → feedback+
Turns out the broken part was from someone else's patch :D
It's not exactly what you requested, but I'm sure you'll see why.
Attachment #8450461 - Attachment is obsolete: true
Attachment #8452548 - Flags: feedback?(honzab.moz)
Comment on attachment 8452548 [details] [diff] [review]
bug-1020416-fix-with-hash.patch

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

So much simpler!  Thanks.  This is getting very close to a production quality patch :)

I think you can ask for review for the next version.

::: netwerk/cache2/CacheEntry.cpp
@@ +966,5 @@
> +    return rv;
> +  }
> +
> +  *aIsForcedValid = false;
> +  *aIsForcedValid = CacheStorageService::Self()->IsForcedValidEntry(key);

just omit *aIsForcedValid = false;

LOG(()) the result.

@@ +968,5 @@
> +
> +  *aIsForcedValid = false;
> +  *aIsForcedValid = CacheStorageService::Self()->IsForcedValidEntry(key);
> +
> +  return rv;

return NS_OK;

@@ +971,5 @@
> +
> +  return rv;
> +}
> +
> +NS_IMETHODIMP CacheEntry::ForceValidFor(uint32_t aSecondsToTheFuture)

please add LOG(()) to this method

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2048,5 @@
>      return NS_OK;
>    }
>  
> +  // Check if forced valid before evicting
> +  if (IsForcedValidEntry(aHandle)) {

and the aCheckIfForcedValid arg?

::: netwerk/cache2/CacheStorageService.cpp
@@ +1025,5 @@
> +// Checks if a cache entry is forced valid (will be loaded directly from cache
> +// without further validation) - see nsICacheEntry.idl for further details
> +bool CacheStorageService::IsForcedValidEntry(nsACString &aCacheEntryKey)
> +{
> +  TimeStamp validUntilPtr;

Ptr no longer needed

@@ +1027,5 @@
> +bool CacheStorageService::IsForcedValidEntry(nsACString &aCacheEntryKey)
> +{
> +  TimeStamp validUntilPtr;
> +
> +  mozilla::MutexAutoLock lock(mLock);

blank line after this

::: netwerk/cache2/CacheStorageService.h
@@ +18,2 @@
>  #include "mozilla/Atomics.h"
> +#include "nsClassHashtable.h"

maybe stick similar header together, also in general: try to produce as minimal as possible patch you can
Attachment #8452548 - Flags: feedback?(honzab.moz) → feedback+
Attachment #8452548 - Attachment is obsolete: true
Attachment #8453913 - Flags: review?(honzab.moz)
Comment on attachment 8453913 [details] [diff] [review]
bug-1020416-fix-with-hash.patch

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

Since there are small nits to yet be done I will ask for one more round.

Also, this needs a test!  I almost forgot.

Add a test to this patch or create a new one, up to you.  Simple check that by default the value is false, then set (for only say 1 second) it's true and after other 2 seconds it's back to false.  It should be there to at least exercise the code path so that we know the basic functionality is covered... However, time-based tests tend to be unreliable, it may happen that when the test box is overloaded, the 1 second time may not be enough to check the value occasionally, we could however well use 5 or even more seconds, xpcshell tests are run in parallel.  We will see.

Then, please file a followup bug to clean the hashtable.  You may get inspired how i clean up a similar telemetry hashtable at patch [1], see CacheStorageService::TelemetryPrune and its usage.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8452048&action=diff#a/netwerk/cache2/CacheStorageService.cpp_sec4

::: netwerk/cache2/CacheEntry.cpp
@@ +987,5 @@
> +    return rv;
> +  }
> +
> +  CacheStorageService::Self()->
> +    ForceEntryValidFor(key, aSecondsToTheFuture);

no need to split on two lines?

@@ +989,5 @@
> +
> +  CacheStorageService::Self()->
> +    ForceEntryValidFor(key, aSecondsToTheFuture);
> +
> +  return rv;

return NS_OK;

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2285,5 @@
> +{
> +  nsCString hashKey = aHandle->Key();
> +  if (!CacheStorageService::Self()) {
> +    return false;
> +  }

nit: maybe check the service before getting the key

@@ +2648,5 @@
>      static uint32_t consecutiveFailures = 0;
>      rv = CacheIndex::GetEntryForEviction(&hash, &cnt);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    rv = DoomFileByKeyInternal(&hash, true);

I see one problem here.  When we are on the limit and everything is forced-valid, we will loop here indefinitely.

Some mechanism to prevent looping is needed.  Michal should look at this code.

::: netwerk/cache2/CacheStorageService.cpp
@@ +1033,5 @@
> +  TimeStamp validUntil;
> +
> +  mozilla::MutexAutoLock lock(mLock);
> +
> +  mForcedValidEntries.Get(aCacheEntryKey, &validUntil);

I missed this the last time, do:

if (!mForcedValidEntries.Get(aCacheEntryKey, &validUntil)) {
  return false;
}

@@ +1056,5 @@
> +                                             uint32_t aSecondsToTheFuture)
> +{
> +  // This will be the timeout
> +  TimeStamp validUntilPtr = TimeStamp::NowLoRes() +
> +    TimeDuration::FromSeconds(aSecondsToTheFuture);

I'm thinking of doing this under the lock.  When a context switches while we have to wait for the lock, it may happen that short time to the future will elapse much sooner.  We should ensure that the entry is forced valid for the required number of seconds after the method returns.

::: netwerk/cache2/nsICacheEntry.idl
@@ +70,5 @@
> +   * will not be evicted from the cache for the duration of forced validity.
> +   * This means that there is a potential problem if the number of forced valid
> +   * entries grows to take up more space than the cache size allows. A cache
> +   * entries current forced valid status is kept in the CacheStorageService, and
> +   * will be under the CacheStorageService lock to prevent race conditions.

A cache entries current forced valid status is kept in the CacheStorageService, and will be under the CacheStorageService lock to prevent race conditions.

..is superfluous.  also, it's good not to explain the implementation in an interface.  please remove that line.
Attachment #8453913 - Flags: review?(michal.novotny)
Attachment #8453913 - Flags: review?(honzab.moz)
Attachment #8453913 - Flags: feedback+
Blocks: 1038357
Now with tests! Wow!

https://tbpl.mozilla.org/?tree=Try&rev=9200ef92b6c0
Attachment #8453913 - Attachment is obsolete: true
Attachment #8453913 - Flags: review?(michal.novotny)
Attachment #8455732 - Flags: review?(michal.novotny)
Attachment #8455732 - Flags: review?(honzab.moz)
Comment on attachment 8455732 [details] [diff] [review]
bug-1020416-fix-with-hash.patch

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

(In reply to Honza Bambas (:mayhemer) from comment #58)
> @@ +2648,5 @@
> >      static uint32_t consecutiveFailures = 0;
> >      rv = CacheIndex::GetEntryForEviction(&hash, &cnt);
> >      NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +    rv = DoomFileByKeyInternal(&hash, true);
> 
> I see one problem here.  When we are on the limit and everything is
> forced-valid, we will loop here indefinitely.

The problem is not only when everything is forced-valid. When CacheIndex::GetEntryForEviction() returns a single existing forced-valid entry it won't be doomed and CacheIndex::GetEntryForEviction() will return the same entry again on the next call.


> Some mechanism to prevent looping is needed.  Michal should look at this
> code.

There is currently no mechanism in the index to exclude some entry from the eviction algorithm. Ideally, the method CacheIndex::GetEntryForEviction() won't return an entry that must not be evicted. The problem is that the handle must be checked on cache IO thread and this method can be called on any thread. But we call it now only from CacheFileIOManager::OverLimitEvictionInternal(), so change it so it can be called only on the IO thread and put the check there.
Attachment #8455732 - Flags: review?(michal.novotny) → review-
Comment on attachment 8455732 [details] [diff] [review]
bug-1020416-fix-with-hash.patch

Canceling the review until I have Michal's changes.
Attachment #8455732 - Flags: review?(honzab.moz)
This now has a test. Also per Michal's suggestion, does the doom-prevention in the CacheIndex::GetEntryForEviction() method.
Attachment #8449832 - Attachment is obsolete: true
Attachment #8455732 - Attachment is obsolete: true
Attachment #8456579 - Flags: review?(michal.novotny)
Attachment #8456579 - Flags: review?(honzab.moz)
I also realize we need to prevent purging memory-only entries from the memory pool (that actually stands as a LRU for memory cache).

CacheStorageService::RemoveEntry() must check in the |if (aOnlyUnreferenced)| branch if the entry is not using disk (i.e. !entry->IsUsingDisk()) and IsForcedValidEntry(entryKey).  In that case we must prevent purging such entry (return false).
Comment on attachment 8456579 [details] [diff] [review]
bug-1020416-fix-with-hash.patch

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

You will also need to incorporate comment 63..

::: netwerk/cache2/CacheIndex.cpp
@@ +1183,5 @@
>    }
>  
> +  nsRefPtr<CacheFileHandle> handle;
> +  CacheFileIOManager::gInstance->mHandles.GetHandle(aHash, false,
> +                                                     getter_AddRefs(handle));

indention, or you can put all the args on a new line indented by 2 spaces, up to you.

::: netwerk/test/unit/test_cache2-27-force-valid-for.js
@@ +13,5 @@
> +    var mem = getCacheStorage("memory");
> +    var disk = getCacheStorage("disk");
> +
> +    do_check_true(disk.exists(createURI("http://m1/"), ""));
> +    do_check_true(mem.exists(createURI("http://m1/"), ""));

what's purpose of this check?  we only want to check the force-valid API with this test.

@@ +19,5 @@
> +    finish_cache2_test();
> +  });
> +
> +  asyncOpenCacheEntry("http://m1/", "memory", Ci.nsICacheStorage.OPEN_NORMALLY, LoadContextInfo.default,
> +    new OpenCallback(NEW | WAITFORWRITE, "meta", "data", function(entry) {

more just a question: why do you need to wait for write?

::: netwerk/test/unit/xpcshell.ini
@@ +65,5 @@
>  # Bug 675039, comment 6: "The difference is that the memory cache is disabled in Armv6 builds."
>  skip-if = os == "android"
>  [test_cache2-25-chunk-memory-limit.js]
>  [test_cache2-26-no-outputstream-open.js]
> +[test_cache2-27-force-valid-for.js]

you are putting this on a wrong line.  the comment and skip-if belongs to the test 26, this is a windows .ini-like file format.

however, you are using memory cache and that is disabled on android, so you need to disable your test on android as well, it should look like this: (you can also fix some of my typos ;))

[test_cache2-26-no-outputstream-open.js]
# GC, that this patch is dependent on, doesn't work well on Android.
skip-if = os == "android"
[test_cache2-27-force-valid-for.js]
# Bug 675039, comment 6: "The difference is that the memory cache is disabled in Armv6 builds."
skip-if = os == "android"
Attachment #8456579 - Flags: review?(honzab.moz) → feedback+
Now with less terrible. :D

https://tbpl.mozilla.org/?tree=Try&rev=d6559f051af8
Attachment #8456579 - Attachment is obsolete: true
Attachment #8456579 - Flags: review?(michal.novotny)
Attachment #8457132 - Flags: review?(michal.novotny)
Attachment #8457132 - Flags: review?(honzab.moz)
Comment on attachment 8457132 [details] [diff] [review]
bug-1020416-fix-with-hash.patch

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

Thanks!  Looks good on my side.  You still need r+ from michal as well.
Attachment #8457132 - Flags: review?(honzab.moz) → review+
Comment on attachment 8457132 [details] [diff] [review]
bug-1020416-fix-with-hash.patch

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

::: netwerk/cache2/CacheIndex.cpp
@@ +1140,5 @@
>  // static
>  nsresult
>  CacheIndex::GetEntryForEviction(SHA1Sum::Hash *aHash, uint32_t *aCnt)
>  {
> +  MOZ_ASSERT(CacheFileIOManager::IsOnIOThread());

To be consistent with the other methods, please move the assertion just before the lock below.

@@ +1186,5 @@
> +  CacheFileIOManager::gInstance->mHandles.GetHandle(
> +    aHash, false, getter_AddRefs(handle));
> +
> +  // Check if forced valid to prevent the same entry from being returned
> +  // repeatedly. Also approriately prevents dooming when forced valid.

The comment is wrong, this doesn't prevent dooming the entry, it just ensures that the entry won't be evicted when the cache size reaches the limit. The entry can be still doomed normally via AsyndDoom().

@@ +1189,5 @@
> +  // Check if forced valid to prevent the same entry from being returned
> +  // repeatedly. Also approriately prevents dooming when forced valid.
> +  nsCString hashKey = handle->Key();
> +  if (CacheStorageService::Self()->IsForcedValidEntry(hashKey)) {
> +    return NS_ERROR_NOT_AVAILABLE;

We need to skip forced-valid entries and not stop returning any entry when the first entry in the array is forced-valid. You have to iterate the arrays until you find first entry that isn't forced-valid.
Attachment #8457132 - Flags: review?(michal.novotny) → review-
This version approaches the Cache Index stuff a little more delicately.
Attachment #8457132 - Attachment is obsolete: true
Attachment #8458374 - Flags: review?(michal.novotny)
Comment on attachment 8458374 [details] [diff] [review]
bug-1020416-fix-with-hash.patch

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

::: netwerk/cache2/CacheIndex.cpp
@@ +1166,2 @@
>    uint32_t now = PR_Now() / PR_USEC_PER_SEC;
> +  if (index->mExpirationArray[i]->mExpirationTime < now) {

You have to check every entry whether it is expired, not just the first one. The logic should be (pseudocode):

var hash = null;
var i=0, j=0;

// find first expired non-forced valid entry
for (i=0; i<expArray.Length(); i++) {
  if (expArray[i].mExpirationTime < now) {
    if (!IsForcedValidEntry(expArray[i])) {
      hash = expArray[i];
      break;
    }
  } else {
    break;
  }
}

if (!hash) {
  if (i==expArray.Length())
    return NOT_AVAIL;

  // find non-forced valid entry with lowest frecency
  for (j=0; j<frecencyArray.Length(); j++) {
    if (!IsForcedValidEntry(frecencyArray[j])) {
      hash = frecencyArray[j];
      break;
    }
  }
}

if (!hash)
  return NOT_AVAIL;

aHash = hash;
// forced valid entries skipped in both arrays could overlap, just use the bigger number
aCnt = expArray.Length() - max(i, j);

return OK;

::: netwerk/cache2/CacheIndex.h
@@ +560,5 @@
>    static nsresult HasEntry(const nsACString &aKey, EntryStatus *_retval);
>  
>    // Returns a hash of the least important entry that should be evicted if the
>    // cache size is over limit and also returns a total number of all entries in
> +  // the index minus the number that are currently forced valid (see below).

This is not exact, we subtract number of forced valid entries that we've seen while searching the entry, not all forced valid entries that are in the array.
Attachment #8458374 - Flags: review?(michal.novotny) → review-
Attachment #8458374 - Attachment is obsolete: true
Attachment #8458772 - Flags: review?(michal.novotny)
Attachment #8458772 - Attachment is obsolete: true
Attachment #8458772 - Flags: review?(michal.novotny)
Attachment #8458775 - Flags: review?(michal.novotny)
Comment on attachment 8458775 [details] [diff] [review]
bug-1020416-fix-with-hash.patch

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

::: netwerk/cache2/CacheIndex.cpp
@@ +1163,5 @@
>      return NS_ERROR_NOT_AVAILABLE;
>  
> +  SHA1Sum::Hash hash;
> +  bool foundEntry = false;
> +  unsigned int i = 0, j = 0;

use uint32_t
Attachment #8458775 - Flags: review?(michal.novotny) → review+
Updated the comment to include the bug number.
Attachment #8459001 - Attachment is obsolete: true
Attachment #8459060 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a232fd36c827
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1041492
backedout since this might have caused bug 1041492
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: checkin-needed
Target Milestone: mozilla33 → ---
sorry had to back this out since this caused merge conflicts when trying to merge mozilla-inbound to mozilla-central in netwerk/cache2/CacheIndex.cpp
(In reply to Carsten Book [:Tomcat] from comment #80)
> sorry had to back this out since this caused merge conflicts when trying to
> merge mozilla-inbound to mozilla-central in netwerk/cache2/CacheIndex.cpp

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f5cad240b3
Does this mean that this latest revision has not yet made it to Nightly? Also, does this mean that Nightly from Mozilla central still contains the bad patch from July 20?
Flags: needinfo?(emorley)
Flags: needinfo?(cbook)
(In reply to Jeremy Poulin [:jerzilla] from comment #82)
> Does this mean that this latest revision has not yet made it to Nightly?

Neither the relanding nor the backout of the relanding have been merged from mozilla-inbound to mozilla-central yet.

> Also, does this mean that Nightly from Mozilla central still contains the
> bad patch from July 20?

No this was backed out directly from mozilla-central, comment 77 (the same as comment 80) should have included the hg URL to be clearer. Tomcat, please can you remember to include it? :-)
Flags: needinfo?(emorley)
mozilla-central has now been merged to mozilla-inbound (was delayed by other conflicts), so this can reland without fear of later conflicts from this :-)
Flags: needinfo?(cbook)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ef2d0dcf77e1
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1043871
This is causing frequent hangs in Nightly for Android (07/24+)
Bad news: This code seems to be causing Firefox for Android to hang after going into the background (again). We may need to back it out (again). Bug 1043871 was filed after this landed back into Nightly.
Honza/Michal, do you guys know what might be causing this? Also, how should I go about fixing this?
Flags: needinfo?(michal.novotny)
Flags: needinfo?(honzab.moz)
backed out https://hg.mozilla.org/mozilla-central/rev/8da875b402fe

Note, that this wasn't a clean back out, CacheStorageService.cpp seems to have changed since. I did build and test on mac before pushing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Brad - the changes for CacheStorageService.cpp were for bug 1038357 which is blocked by this one. It probably would have been best to back out both, seeing as how that bug is blocked by this one.
Flags: needinfo?(blassey.bugs)
(In reply to Jeremy Poulin [:jerzilla] from comment #91)
> Brad - the changes for CacheStorageService.cpp were for bug 1038357 which is
> blocked by this one. It probably would have been best to back out both,
> seeing as how that bug is blocked by this one.

what info do you need? If you think it's best to back out bug 1038357 as well, have at it.
Flags: needinfo?(blassey.bugs)
This seems likely to also be the cause of bug 1044233, where B2G phones lock up completely after a few hours or half a day.
Also, blassey's backout backed out the .cpp changes in bug 1038357 but not the .h changes, so it was effectively backed out in that patch.  (Is it still clear that this bug in particular was the cause?)
Depends on: 1044233
I can see hangs *on Desktop* (OSX and Win, so cross-platform) with the patch.
I verified that the issue is gone in the backout tinderbox build (https://tbpl.mozilla.org/?rev=8da875b402fe) but was still present in the previous build (https://tbpl.mozilla.org/?rev=6c0971104909).

I don't got good steps to reproduce, but I am able to reproduce this fairly reliably by forcing many cachable http connections one way or another (I found this during development of my DownThemAll! download manager extension in conjunction with another add-on that hooks into it and creates a lot of XHRs).

There seems to be some kind of deadlock or mutex corruption on CacheStorageService::mLock, at least that's what lldb on OSX and Visual Studio on Windows tells me: The main thread is waiting to acquire that mutex in CacheStorageService::AddStorageEntry, but apparently never does.
(In reply to Jeremy Poulin [:jerzilla] from comment #89)
> Honza/Michal, do you guys know what might be causing this? Also, how should
> I go about fixing this?

I don't see CacheFileInputStream::ReadSegments anywhere in the log in bug 1044233, but it migth worth to double check whether it could be caused by 1035411. If the STR in bug 1043871 or 1041531 are reliable, try your patch with and without the change from bug 1035411.
Flags: needinfo?(michal.novotny)
Hi Honza - I made some changes to the CacheStorageService to deal with the mutex corruption noted in bug 1044233. Would you mind taking a glance at it to see if my implementation is sane?
Attachment #8459756 - Attachment is obsolete: true
Attachment #8463566 - Flags: review?(honzab.moz)
Flags: needinfo?(honzab.moz)
Posted patch review-1020416.patch (obsolete) — Splinter Review
Just added an internal isforcedvalidentry that doesn't grab the mutex.
Attachment #8464188 - Flags: review?(michal.novotny)
Attachment #8463566 - Flags: review?(honzab.moz) → review+
Attachment #8464188 - Flags: review?(michal.novotny) → review+
Attachment #8464188 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e875c9862afd
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Blocks: 1050613
Depends on: 1206407
You need to log in before you can comment on or make changes to this bug.