971975 - crash in mozilla::net::CacheFile::IsDirty()

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

This bug was filed from the Socorro interface and is 
report bp-d3c9ddfb-b735-4e88-980b-bb4c02140212.
=============================================================

Apparently at [1] mMetadata can be null.  Null check is sufficient IMO, I think it's possible we release a file w/o metadata.

[1] http://hg.mozilla.org/mozilla-central/annotate/ecf20a2484b6/netwerk/cache2/CacheFile.cpp#l1311

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

Kinda trivial.

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 8375712 [details] [diff] [review]
v1

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

::: netwerk/cache2/CacheFile.cpp
@@ +181,1 @@
>      WriteMetadataIfNeededLocked(true);

We never null out mMetadata once we create a new one or from a the data in the entry file. So mMetadata can be null here only for non-ready CacheFile. It would be IMO better to check mReady here instead of mMetadata. Additional MOZ_ASSERT(mMetadata) in WriteMetadataIfNeededLocked() would be probably also good.

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #2)
> Comment on attachment 8375712 [details] [diff] [review]
> v1
> 
> Review of attachment 8375712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheFile.cpp
> @@ +181,1 @@
> >      WriteMetadataIfNeededLocked(true);
> 
> We never null out mMetadata once we create a new one or from a the data in
> the entry file. So mMetadata can be null here only for non-ready CacheFile.
> It would be IMO better to check mReady here instead of mMetadata. Additional
> MOZ_ASSERT(mMetadata) in WriteMetadataIfNeededLocked() would be probably
> also good.

I don't agree.  This is about a crash non-null check that has to be 100% safe.  If you want a check on the state then we should MOZ_ASSERT(mReady) in WriteMetadataIfNeededLocked() or somewhere.  Sorry, but MOZ_ASSERT(mMetadata) in WriteMetadataIfNeededLocked() is a nonsense regarding fix for this bug.  It has no effect in release at all, don't you think?

Comment 4

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> I don't agree.  This is about a crash non-null check that has to be 100%

I also don't agree. If you want 100% safe check you need to do it in CacheFile::IsDirty() instead of in the destructor.


> safe.  If you want a check on the state then we should MOZ_ASSERT(mReady) in
> WriteMetadataIfNeededLocked() or somewhere.  Sorry, but
> MOZ_ASSERT(mMetadata) in WriteMetadataIfNeededLocked() is a nonsense
> regarding fix for this bug.  It has no effect in release at all, don't you
> think?

Sure, it has no effect in release and that's why I suggest regular if check in the destructor.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v2

- check for mReady in the dtor
- in WriteMetadataIfNeededLocked: non-null check with MOZ_CRASH and return

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8375939 [details] [diff] [review]
v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bae2b5cb32a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4bae2b5cb32a