949175 - HTTP cache v2:Assertion failure: !mMemoryOnly, at netwerk/cache2/CacheFile.cpp:753

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: log speciment for the CacheFile and related CacheEntry

CacheFile::OnMetadataWritten(nsresult aResult)
{
  CacheFileAutoLock lock(this);

  LOG(("CacheFile::OnMetadataWritten() [this=%p, rv=0x%08x]", this, aResult));

  MOZ_ASSERT(mWritingMetadata);
  mWritingMetadata = false;

> MOZ_ASSERT(!mMemoryOnly);
  MOZ_ASSERT(!mOpeningFile);




>	xul.dll!mozilla::net::CacheFile::OnMetadataWritten(tag_nsresult aResult=NS_OK) Line 753	C++
 	xul.dll!mozilla::net::MetadataListenerHelper::OnMetadataWritten(tag_nsresult aResult=NS_OK) Line 309	C++
 	xul.dll!mozilla::net::CacheFileMetadata::OnDataWritten(mozilla::net::CacheFileHandle * aHandle=0x1fac2368, const char * aBuf=0x23639778, tag_nsresult aResult=NS_OK) Line 479	C++
 	xul.dll!mozilla::net::WriteEvent::Run() Line 763	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x0059eeeb) Line 612	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x001d2a20, bool mayWait=true) Line 263	C++
 	xul.dll!nsThread::Shutdown() Line 465	C++
 	xul.dll!mozilla::dom::EncodingCompleteEvent::Run() Line 59	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x0059f023) Line 612	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x001d2a20, bool mayWait=true) Line 263	C++
 	xul.dll!nsThread::Shutdown() Line 465	C++
 	xul.dll!nsRunnableMethodImpl<enum tag_nsresult (__stdcall nsIThread::*)(void),void,1>::Run() Line 384	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x0059f127) Line 612	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x001d2a20, bool mayWait=true) Line 263	C++
 	xul.dll!nsThread::Shutdown() Line 465	C++
 	xul.dll!nsRunnableMethodImpl<enum tag_nsresult (__stdcall nsIThread::*)(void),void,1>::Run() Line 384	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=false, bool * result=0x0059f22b) Line 612	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x001d2a20, bool mayWait=false) Line 263	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x001c83c0) Line 85	C++
 	xul.dll!MessageLoop::RunInternal() Line 223	C++
 	xul.dll!MessageLoop::RunHandler() Line 216	C++
 	xul.dll!MessageLoop::Run() Line 190	C++
 	xul.dll!nsBaseAppShell::Run() Line 163	C++
 	xul.dll!nsAppShell::Run() Line 113	C++
 	xul.dll!nsAppStartup::Run() Line 268	C++
 	xul.dll!XREMain::XRE_mainRun() Line 3978	C++
 	xul.dll!XREMain::XRE_main(int argc=4, char * * argv=0x001b32e0, const nsXREAppData * aAppData=0x0059f710) Line 4046	C++
 	xul.dll!XRE_main(int argc=4, char * * argv=0x001b32e0, const nsXREAppData * aAppData=0x0059f710, unsigned int aFlags=0) Line 4254	C++
 	firefox.exe!do_main(int argc=4, char * * argv=0x001b32e0, nsIFile * xreDirectory=0x001bb608) Line 280	C++
 	firefox.exe!NS_internal_main(int argc=4, char * * argv=0x001b32e0) Line 648	C++
 	firefox.exe!wmain(int argc=4, wchar_t * * argv=0x00b52490) Line 105	C++
 	firefox.exe!__tmainCRTStartup() Line 552	C
 	firefox.exe!wmainCRTStartup() Line 371	C


Whole log saved.

Comment 1

[Honza Bambas (:mayhemer)]

Based on https://hg.mozilla.org/projects/gum/rev/ec2fcd5a61ae.

Comment 2

[Honza Bambas (:mayhemer)]

Happened again on cnn.com.

Comment 3

[Michal Novotny [:michal]]

Attachment: fix

Problem is in postponing call to CacheFile::SetMemoryOnly() just before opening output stream. Writing to metadata can cause writing to a disk and we cannot switch to memory only operations after this point.

changes in this patch:
- CacheEntry::SetPersistToDisk() calls CacheFile::SetMemoryOnly() immediately
- CacheFile cannot switch back to disk once CacheFile::SetMemoryOnly() was called. Such CacheEntry::SetPersistToDisk(true) call does not fail, but we simply don't store the data to disk.
- CacheFile cannot switch to memory only if some data is already on the disk. This could be either because the file was parsed from an disk entry or because metadata was changed and was already written to disk.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8357098 [details] [diff] [review]
fix

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

::: netwerk/cache2/CacheEntry.cpp
@@ +870,2 @@
>  
> +  return mFile->SetMemoryOnly();

The code should be like:

  if (mUseDisk == aPersistToDisk)
    return NS_OK;

  if (!aPresistToDisk) {
    nsresult rv = mFile->SetMemoryOnly();
    if (NS_FAILED(rv))
      return rv; // should we warn somehow?
  } else {
    // this leads me to change this API, we actually cannot
    // re-persist a mem-only entry...
    return NS_ERROR_NOT_AVAILABLE;
  }

  mozilla::MutexAutoLock lock(CacheStorageService::Self()->Lock());

  mUseDisk = aPersistToDisk;
  CacheStorageService::Self()->RecordMemoryOnlyEntry(
    this, !aPersistToDisk, false /* don't overwrite */);

  return NS_OK;


The nsICacheEntry interface might look rather like:

void setMemoryOnly();
readonly attribute bool isMemoryOnly;

::: netwerk/cache2/CacheFile.cpp
@@ +906,5 @@
> +  if (mHandle && mHandle->FileSize() != 0) {
> +    LOG(("CacheFile::SetMemoryOnly() - Some data is already on disk. [this=%p]",
> +         this));
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }

Can't we have some different, less volatile way to recognize that something has been written (demanded to be written) to disk then these 3 flags testing?  Something best on the handle, like "HasData()" flag that is set when we demand write on it via IOMan::Write() or is an open existing file.

This is IMO overcomplicated and appear fragile and unpredictable.

@@ +912,4 @@
>    mMemoryOnly = true;
> +  if (mHandle) {
> +    mMetadata->ReleaseHandle();
> +    mHandle = nullptr;

I'm a bit worried about this...

::: netwerk/cache2/CacheFileMetadata.cpp
@@ +144,5 @@
> +       mHandle.get()));
> +
> +  MOZ_ASSERT(mHandle);
> +
> +  mHandle = nullptr;

is this (and other places accessing mHandle) ok to be unlocked?

Comment 5

[Michal Novotny [:michal]]

Attachment: patch v2

https://tbpl.mozilla.org/?tree=Try&rev=b361b8dfd0ab


(In reply to Honza Bambas (:mayhemer) from comment #4)
> ::: netwerk/cache2/CacheFile.cpp
> @@ +906,5 @@
> > +  if (mHandle && mHandle->FileSize() != 0) {
> > +    LOG(("CacheFile::SetMemoryOnly() - Some data is already on disk. [this=%p]",
> > +         this));
> > +    return NS_ERROR_NOT_AVAILABLE;
> > +  }
> 
> Can't we have some different, less volatile way to recognize that something
> has been written (demanded to be written) to disk then these 3 flags
> testing?  Something best on the handle, like "HasData()" flag that is set
> when we demand write on it via IOMan::Write() or is an open existing file.

I don't like the idea having it in handle, since this flag (needed only for a single method in CacheFile) would need to be set in CacheFileIOManager::Write() as well as in CacheFile::OnMetadataRead(). I've renamed and used mDataAccessed for this since it is used only in SetMemoryOnly() anyway.


> This is IMO overcomplicated and appear fragile and unpredictable.
> 
> @@ +912,4 @@
> >    mMemoryOnly = true;
> > +  if (mHandle) {
> > +    mMetadata->ReleaseHandle();
> > +    mHandle = nullptr;
> 
> I'm a bit worried about this...

Why?


> ::: netwerk/cache2/CacheFileMetadata.cpp
> @@ +144,5 @@
> > +       mHandle.get()));
> > +
> > +  MOZ_ASSERT(mHandle);
> > +
> > +  mHandle = nullptr;
> 
> is this (and other places accessing mHandle) ok to be unlocked?

In general, CacheFileMetadata doesn't have its own lock and access is synchronized with CacheFile's lock. The exceptions are OnDataRead() and OnDataWritten(). The former is safe because CacheFile is not ready, so it cannot anyhow manipulate with metadata or handle. OnDataWritten() doesn't do anything danger with members.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #5)
> Created attachment 8364688 [details] [diff] [review]
> patch v2
> 
> https://tbpl.mozilla.org/?tree=Try&rev=b361b8dfd0ab
> 
> 
> (In reply to Honza Bambas (:mayhemer) from comment #4)
> > ::: netwerk/cache2/CacheFile.cpp
> > @@ +906,5 @@
> > > +  if (mHandle && mHandle->FileSize() != 0) {
> > > +    LOG(("CacheFile::SetMemoryOnly() - Some data is already on disk. [this=%p]",
> > > +         this));
> > > +    return NS_ERROR_NOT_AVAILABLE;
> > > +  }
> > 
> > Can't we have some different, less volatile way to recognize that something
> > has been written (demanded to be written) to disk then these 3 flags
> > testing?  Something best on the handle, like "HasData()" flag that is set
> > when we demand write on it via IOMan::Write() or is an open existing file.
> 
> I don't like the idea having it in handle, since this flag (needed only for
> a single method in CacheFile) would need to be set in
> CacheFileIOManager::Write() as well as in CacheFile::OnMetadataRead(). I've
> renamed and used mDataAccessed for this since it is used only in
> SetMemoryOnly() anyway.

That was my point, to set that flag on proper places, but this is not that important.  Let's leave it.

> 
> 
> > This is IMO overcomplicated and appear fragile and unpredictable.
> > 
> > @@ +912,4 @@
> > >    mMemoryOnly = true;
> > > +  if (mHandle) {
> > > +    mMetadata->ReleaseHandle();
> > > +    mHandle = nullptr;
> > 
> > I'm a bit worried about this...
> 
> Why?

Good question :)  I am asking that my self, I'll check that again, maybe it was just a comment I forgot to delete.

> 
> 
> > ::: netwerk/cache2/CacheFileMetadata.cpp
> > @@ +144,5 @@
> > > +       mHandle.get()));
> > > +
> > > +  MOZ_ASSERT(mHandle);
> > > +
> > > +  mHandle = nullptr;
> > 
> > is this (and other places accessing mHandle) ok to be unlocked?
> 
> In general, CacheFileMetadata doesn't have its own lock and access is
> synchronized with CacheFile's lock. The exceptions are OnDataRead() and
> OnDataWritten(). The former is safe because CacheFile is not ready, so it
> cannot anyhow manipulate with metadata or handle. OnDataWritten() doesn't do
> anything danger with members.

OK, sounds reasonable.  I'll check this again with this in mind.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8364688 [details] [diff] [review]
patch v2

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

Thanks for this patch, it's r+ for the concept of IDL changes and all parts w/o negative comments.  r- as a whole, please see the comments.

I usually don't do this, but I answered with a patch of my own...  Hope you don't take it as an offense :)


We should bypass ScheduleMetadataWrite when mMemoryOnly is set on the file as an opt.

::: netwerk/cache2/CacheFile.cpp
@@ +539,5 @@
>      if (mDataSize == 0 && mMetadata->ElementsSize() == 0) {
>        isNew = true;
>        mMetadata->MarkDirty();
> +    } else {
> +      mCanSetMemoryOnly = false;

As I understand, no one can have the file and entry at this time, right?

@@ +630,5 @@
>  
>    mInputs.AppendElement(input);
>    NS_ADDREF(input);
>  
> +  mCanSetMemoryOnly = false;

Not sure this makes sense, why opening the input stream would disallow setting to mem-only?

@@ +695,3 @@
>    mMemoryOnly = true;
> +  if (mHandle) {
> +    mMetadata->ReleaseHandle();

so, this can remove handle from metadata at any time, on any thread while CacheFileMetadata can pass points of a null check (while there actually are none, whatsoever) on it..

as I looked into the code, I don't think CacheFileMetadata needs to keep the handle as a member.  It can always be passed in via an argument to methods those use it.

CacheFileMetadata::ReadMetadata, called from the file
CacheFileMetadata::WriteMetadata, as well
CacheFileMetadata::OnDataRead, already passed aHandle that is the same handle it was read for

This way metadata are independent and you don't need to have this dangerous construction.

@@ +1362,5 @@
>    rv = mMetadata->WriteMetadata(mDataSize, aFireAndForget ? nullptr : this);
>    if (NS_SUCCEEDED(rv)) {
>      mWritingMetadata = true;
>      mDataIsDirty = false;
> +    mCanSetMemoryOnly = false;

First thought:

----

This is kinda racy.  This method can be called at any time, so it depends on the caller when it manages to call SetMemoryOnly - before we get here or after it.  This may lead to an unpredictable hard to track failures of SetMemoryOnly.

Hard to say how to do it right and if it's that critical.

I'd rather drop this flag at CacheFile::PostWriteTimer (and rename that method to e.g. ScheduleMetadataWrite).  So that whenever whoever sets some metadata on the entry, he cannot set memory-only any more.  This is IMO more consistent.

----


So I told my self to rather try this.  And it failed - the writetimer is scheduled very very soon the entry is created and given to consumers.  It just revealed that setting the flag at any place (either here or at PostWriteTimer()) was not the right choice.

Hence, I think this obstruction of impossibility to disallow store of the entry should be removed altogether, as it's not that simple to decide on 'the point of no return'. 

I think SetMemoryOnly should do the following:
- check mReady as does now
- if mMemOnly already set, return
- set mMemOnly = true
- doom the handle
- throw away the handle reference

This will simply delete the file, regardless whether there are some data already stored or not.


What do you think?

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v3

As described in the last hunk comment + some tuneup of the IDL.  Only locally tested (modified tests + all net xpc + few moments of browsing).  Try is kinda overloaded this time.

Please mainly check the decision to remove mHandle from CacheFileMetadata is correct.

Comment 9

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> We should bypass ScheduleMetadataWrite when mMemoryOnly is set on the file
> as an opt.

Yes, that makes sense.


> ::: netwerk/cache2/CacheFile.cpp
> @@ +539,5 @@
> >      if (mDataSize == 0 && mMetadata->ElementsSize() == 0) {
> >        isNew = true;
> >        mMetadata->MarkDirty();
> > +    } else {
> > +      mCanSetMemoryOnly = false;
> 
> As I understand, no one can have the file and entry at this time, right?

CacheEntry has the file but it must not use it since OnFileReady() was not called yet.


> @@ +630,5 @@
> >  
> >    mInputs.AppendElement(input);
> >    NS_ADDREF(input);
> >  
> > +  mCanSetMemoryOnly = false;
> 
> Not sure this makes sense, why opening the input stream would disallow
> setting to mem-only?

This isn't probably necessary, but also I don't see a valid use case where we would open input stream and after that we would need to switch to mem-only.


> @@ +695,3 @@
> >    mMemoryOnly = true;
> > +  if (mHandle) {
> > +    mMetadata->ReleaseHandle();
> 
> so, this can remove handle from metadata at any time, on any thread while
> CacheFileMetadata can pass points of a null check (while there actually are
> none, whatsoever) on it..
> 
> as I looked into the code, I don't think CacheFileMetadata needs to keep the
> handle as a member.  It can always be passed in via an argument to methods
> those use it.
> 
> CacheFileMetadata::ReadMetadata, called from the file
> CacheFileMetadata::WriteMetadata, as well
> CacheFileMetadata::OnDataRead, already passed aHandle that is the same
> handle it was read for
> 
> This way metadata are independent and you don't need to have this dangerous
> construction.

I agree that it is not necessary to store handle as a member in CacheFileMetadata, but I disagree that call to ReleaseHandle() in CacheFile::SetMemoryOnly() is dangerous. Could you provide any scenario where it can be a problem?


> @@ +1362,5 @@
> >    rv = mMetadata->WriteMetadata(mDataSize, aFireAndForget ? nullptr : this);
> >    if (NS_SUCCEEDED(rv)) {
> >      mWritingMetadata = true;
> >      mDataIsDirty = false;
> > +    mCanSetMemoryOnly = false;
> 
> First thought:
> 
> ----
> 
> This is kinda racy.  This method can be called at any time, so it depends on
> the caller when it manages to call SetMemoryOnly - before we get here or
> after it.  This may lead to an unpredictable hard to track failures of
> SetMemoryOnly.
> 
> Hard to say how to do it right and if it's that critical.
> 
> I'd rather drop this flag at CacheFile::PostWriteTimer (and rename that
> method to e.g. ScheduleMetadataWrite).  So that whenever whoever sets some
> metadata on the entry, he cannot set memory-only any more.  This is IMO more
> consistent.

I'm not against setting this flag to false in CacheFile::PostWriteTimer(). But I don't agree that it is "racy". Nobody should call SetMemoryOnly() after setting some metadata. If there is such call, then it is a wrong usage of CacheFile.


> So I told my self to rather try this.  And it failed - the writetimer is
> scheduled very very soon the entry is created and given to consumers.  It
> just revealed that setting the flag at any place (either here or at
> PostWriteTimer()) was not the right choice.

Could you provide more information? In what cases we first get input/output stream or set metadata and after that we decide to set the entry memory only?


> Hence, I think this obstruction of impossibility to disallow store of the
> entry should be removed altogether, as it's not that simple to decide on
> 'the point of no return'. 
> 
> I think SetMemoryOnly should do the following:
> - check mReady as does now
> - if mMemOnly already set, return
> - set mMemOnly = true
> - doom the handle
> - throw away the handle reference

This won't work. What if the CacheFile is in use there are some input/output streams which already created some chunks etc? I think we need to find out why we call SetMemoryOnly() so late and if it is a valid use case, the caller should throw away the CacheFile reference and create a new one with memory only flag passed to Init().

Comment 10

[Michal Novotny [:michal]]

Comment on attachment 8364826 [details] [diff] [review]
v3

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

See end of the comment #9.

Comment 11

[Honza Bambas (:mayhemer)]

Based on a quick IRC conversation we decided to remove the "unpersist" delayed API altogether.

Reasons:
- as Michal points out, we cannot just easily throw the handle away at any time, it would need a set of not that clear to find conditions to allow/disallow that
- in general it's hard to find the point-of-no-return for any mechanism to de-persist an entry after it has already been open
- also, it doesn't make sense to unpersist an existing entry (it would actually be just a shortcut for doom+reopen with memory/only), so it mostly can been used only on entries that are very fresh

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v4

- "persistent" r/o attribute
- recreate([optional] bool memOnly)


https://tbpl.mozilla.org/?tree=Try&rev=4eb5a0b68d9f

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8367344 [details] [diff] [review]
v4

https://hg.mozilla.org/integration/mozilla-inbound/rev/47bd8742ee3c

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47bd8742ee3c