1006217 - Assertion failure: mValidityMap.Length() == 0, at /home/dbaron/builds/ssd/mozilla-central/mozilla/netwerk/cache2/CacheFileChunk.cpp:392

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

While debugging bug 1005964 I ran into:

Assertion failure: mValidityMap.Length() == 0, at /home/dbaron/builds/ssd/mozilla-central/mozilla/netwerk/cache2/CacheFileChunk.cpp:392

and kept hitting it, such that I had to turn off new cache to actually debug the problem.


I'm not going to bother giving debugging details, because I figure if you actually wanted them, you wouldn't have tried landing a major new feature on trunk while disabling it for all tests.

Comment 1

[Honza Bambas (:mayhemer)]

David, most of us (me and Michal + other members of the net team) are using and debugging cache2 daily.  No one has hit this.  If you take a look at https://tbpl.mozilla.org/?tree=Gum you can check non of the tests have hit this too.  So it's new and we do these trials exactly to catch these problems.

So, please, if you "would bother" with providing us a log with nsHttp:4,cache2:5 it would be much appreciated and help us fix it.  Thanks!

Michal, can you take care of this please?

Comment 2

[Michal Novotny [:michal]]

I cannot reproduce it, so I'll go through the code and try to find the problem.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nspr log with NSPR_LOG_MODULES=nsHttp:4,cache2:5

Note that:
 (a) the assertion itself doesn't show up in the NSPR log (since it's MOZ_ASSERT rather than NS_ABORT_IF_FALSE, I presume)
 (b) the main thread continues after the background thread assertion/crash, so the messages at the end of the log are probably after it

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Steps to reproduce (not simplified):
 1. save http://mollyrocket.com/casey/stream_0006.html to a file
 2. create a file next to it called stream_0006_link.html with the contents:
       <a href="stream_0006.html">link</a>
 3. create an empty directory called, say, profiledir
 4. in that empty directory, add a user.js file with the contents:
     user_pref("browser.cache.use_new_backend_temp", true);
 5. run "./firefox -profile path/to/profiledir/ -no-remote file:///path/to/stream_0006_link.html"
 6. middle click on the word "link" to open it in a new tab (not sure if this works on platforms other than Linux)
 7. close the Firefox window (say "Close tabs" to the dialog)
 8. run ./firefox -profile path/to/profiledir/ -no-remote http://mollyrocket.com/casey/
 9. middle click the top link ("Working on the Witness, part 7") to open it in a new tab
 10. the load will hang.  When it does, close the browser window by clicking the [X] in the titlebar
 11. repeat step 8
 12. repeat step 9

At this point repeating only steps 8 and 9 will reproduce the assertion reliably.

Comment 5

[Michal Novotny [:michal]]

Attachment: patch v1

Thanks for STR, David!

There was a stupid bug that we didn't clear the validity map after merging the the buffers in CacheFileChunk::OnDataRead(). I found another few problem while debugging this bug:

- bug in if statement in ValidityPair::Overlaps()
- bug in adding a new pair into validity map where the new pair could be added even if it was merger with another existing pair

I've moved the logic from CacheFileChunk into a separate ValidityMap class in CacheFileUtils.h|cpp.

Comment 6

[Michal Novotny [:michal]]

Comment on attachment 8418420 [details] [diff] [review]
patch v1

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

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8418420 [details] [diff] [review]
patch v1

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

r=honzab

I wrote a small manual test for the validity map (unfortunately cannot be easily turned to a CPP test...  that starts to be a problem and we should find a solution for it!)  All scenarios that occurred to me have passed, so the new map code is correct.

::: netwerk/cache2/CacheFileUtils.cpp
@@ +301,5 @@
> +bool
> +ValidityPair::CanBeMerged(const ValidityPair& aOther) const
> +{
> +  if ((mOffset <= aOther.mOffset && mOffset + mLen >= aOther.mOffset) ||
> +      (aOther.mOffset <= mOffset && aOther.mOffset + aOther.mLen >= mOffset)) {

it might be more useful to instead of Offset() and Len() methods rather have Begin(), End(), Length().  This is only a suggestion to make the arithmetic more readable - not a review req.

::: netwerk/cache2/CacheFileUtils.h
@@ +42,5 @@
> +  // Returns true when two pairs can be merged, i.e. they do overlap or the one
> +  // ends exactly where the other begins.
> +  bool CanBeMerged(const ValidityPair& aOther) const;
> +
> +  // Returns true when this pair has lower offset that the other pair. In case

.. offset *than* the .. ?

@@ +55,5 @@
> +  uint32_t Len() const    { return mLen; }
> +
> +private:
> +  uint32_t mOffset;
> +  uint32_t mLen;

shouldn't these be uint64_t ?

Comment 8

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> @@ +55,5 @@
> > +  uint32_t Len() const    { return mLen; }
> > +
> > +private:
> > +  uint32_t mOffset;
> > +  uint32_t mLen;
> 
> shouldn't these be uint64_t ?

uint32_t is more than enough. We use the map in chunk, so the values are limited by chunk size.

Comment 9

[Michal Novotny [:michal]]

Attachment: patch v2

(In reply to Honza Bambas (:mayhemer) from comment #7)
> ::: netwerk/cache2/CacheFileUtils.cpp
> @@ +301,5 @@
> > +bool
> > +ValidityPair::CanBeMerged(const ValidityPair& aOther) const
> > +{
> > +  if ((mOffset <= aOther.mOffset && mOffset + mLen >= aOther.mOffset) ||
> > +      (aOther.mOffset <= mOffset && aOther.mOffset + aOther.mLen >= mOffset)) {
> 
> it might be more useful to instead of Offset() and Len() methods rather have
> Begin(), End(), Length().  This is only a suggestion to make the arithmetic
> more readable - not a review req.

I've added a comment and changed it a bit to be more readable.

Comment 10

[Michal Novotny [:michal]]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8666fc34b3

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c8666fc34b3