Closed Bug 1006217 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Networking: Cache, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: dbaron, Assigned: michal)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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?
Assignee: nobody → michal.novotny
Flags: needinfo?(michal.novotny)
Flags: needinfo?(dbaron)
I cannot reproduce it, so I'll go through the code and try to find the problem.
Flags: needinfo?(michal.novotny)
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
Flags: needinfo?(dbaron)
Attachment #8418052 - Attachment mime type: text/x-log → text/plain
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.
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #8418420 - Flags: review?(honzab.moz)
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 ?
Attachment #8418420 - Flags: review?(honzab.moz) → review+
(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.
Attached patch patch v2Splinter Review
(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.
Attachment #8418420 - Attachment is obsolete: true
Attachment #8419408 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2c8666fc34b3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.