994574 - HTTP cache v2: CacheFileMetadata::ParseMetadata() fails to parse any entry

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

Attachment: fix

This is a regression caused by bug 988186. All members of CacheFileMetadataHeader are now stored in network order but we use keySize in CacheFileMetadata::ParseMetadata() without converting it back to host order.

Comment 1

[Honza Bambas (:mayhemer)]

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

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

locally tested
r=honzab

sad we don't have a persistence tests to catch this, need to think of some.

::: netwerk/cache2/CacheFileMetadata.cpp
@@ +736,5 @@
> +
> +  if (mMetaHdr.mVersion != kCacheEntryVersion) {
> +    LOG(("CacheFileMetadata::ParseMetadata() - Not a version we understand to. "
> +         "[version=0x%x, this=%p]", mMetaHdr.mVersion, this));
> +    return NS_ERROR_UNEXPECTED;

is UNEXPECTED really the best error here?  (no idea for a better one tho)

Comment 2

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #1)
> ::: netwerk/cache2/CacheFileMetadata.cpp
> @@ +736,5 @@
> > +
> > +  if (mMetaHdr.mVersion != kCacheEntryVersion) {
> > +    LOG(("CacheFileMetadata::ParseMetadata() - Not a version we understand to. "
> > +         "[version=0x%x, this=%p]", mMetaHdr.mVersion, this));
> > +    return NS_ERROR_UNEXPECTED;
> 
> is UNEXPECTED really the best error here?  (no idea for a better one tho)

I'm curious about your reply because you chose this error in bug 941698 ;) Seriously, I thought about this while doing review of 941698 and I think UNEXPECTED is OK. Error NS_ERROR_FILE_CORRUPTED is returned elsewhere in CacheFileMetadata::ParseMetadata() but it is IMO not appropriate in this case.

Comment 3

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f19a8c881401

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #2)
> I'm curious about your reply because you chose this error in bug 941698 ;)
> Seriously, I thought about this while doing review of 941698 and I think
> UNEXPECTED is OK. Error NS_ERROR_FILE_CORRUPTED is returned elsewhere in
> CacheFileMetadata::ParseMetadata() but it is IMO not appropriate in this
> case.

You got me :D  OK, yeah, I probably was not able to find a better one that time too.  And just forgot about that dilemma...

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f19a8c881401

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Honza, is this something you may want to write a test for (I would not want you to be sad!)
I can also ask Henrik if it's something he can figure out how to cover in the QA testsuite.

Thanks!

Comment 7

[Honza Bambas (:mayhemer)]

We have some plans with Michal to add test for our persisting format.  It's a bit tricky and will be covered in a different bug.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Honza, when will this happen? I don't think it's worth our time to create a mozmill test if an in-tree test comes up in a bit.

Comment 9

[Honza Bambas (:mayhemer)]

Delegating to Michal.

Comment 10

[Michal Novotny [:michal]]

I'll try to come up with some test for this in bug 994606 in a reasonable time...

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thank you Michal. I will remove in-qa-testsuite to get it out of our todo list.