945945 - Crash in CloseHandleEvent in the new cache

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[Vladan Djeric (:vladan)]

I'm using the new HTTP cache in Nightly on Windows 7

This bug was filed from the Socorro interface and is 
report bp-99578b97-9cfa-460f-901f-360b52131202.
=============================================================

Comment 1

[Honza Bambas (:mayhemer)]

Apparently the given handle is dead/broken.  This is michal's code but I'll take a look, since he is fully in the index currently.

Comment 2

[Honza Bambas (:mayhemer)]

BTW, Vladan, any STR?

Comment 3

[Vladan Djeric (:vladan)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> BTW, Vladan, any STR?

No, sorry

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: wip1

Preliminary version, xpcshell tests work, no crashes, asserts, leaks.  Needs more careful look tho.

Michal if you have time, check on this patch.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8346999 [details] [diff] [review]
wip1

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

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: wip2

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

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Created attachment 8347340 [details] [diff] [review]
> wip2
> 
> https://tbpl.mozilla.org/?tree=Try&rev=c0743a4afc7a

Leaks on OSX M(3,4): 392 bytes leaked (CacheFileHandle, nsLocalFile, nsRunnable, nsStringBuffer)

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8347340 [details] [diff] [review]
wip2

Apparently unfinished...

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: wip4

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

Looks a bit better.  This one may be worth of a review.  I'll recheck this patch ones more and ask for r if found to be.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> Created attachment 8348814 [details] [diff] [review]
> wip4
> 
> https://tbpl.mozilla.org/?tree=Try&rev=b8d70afd4a77

13128 INFO B2GRunner TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/forms/test_max_attribute.html | application timed out after 330.0 seconds with no output
Return code: 2304
https://tbpl.mozilla.org/php/getParsedLog.php?id=31965254&tree=Try

Not sure this is not just something random and completely unrelated...  I will for now ignore it.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8348814 [details] [diff] [review]
wip4

Michal, please check on this patch.  I think it's closing to get to the tree.  Best before christmas.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: followup1, v1

Fixes the ASAN use-after-free.

https://hg.mozilla.org/projects/gum/rev/dd52ceafaf81

Comment 14

[Honza Bambas (:mayhemer)]

Checking whether this patch alone leaks:
https://tbpl.mozilla.org/?tree=Try&rev=112663f88743

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: v1

Looks like this one is ready for review.

Comment 16

[Michal Novotny [:michal]]

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

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

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +51,5 @@
> +  nsRefPtr<nsRunnableMethod<CacheFileHandle, nsrefcnt, false> > event =
> +    NS_NewNonOwningRunnableMethod(this, &CacheFileHandle::Release);
> +  nsresult rv = ioTarget->Dispatch(event, nsIEventTarget::DISPATCH_NORMAL);
> +  if (NS_FAILED(rv))
> +    return false;

I think that it would be better to not call release at all rather than calling it on a wrong thread in case dispatching fails.

@@ +199,5 @@
> +  MOZ_ASSERT(found);
> +}
> +
> +already_AddRefed<CacheFileHandle>
> +CacheFileHandles::HandleHashKey::GetLastHandle()

It seems to me that name GetLatestHandle or GetNewestHandle would describe better what this method does.

@@ +1081,5 @@
>  CacheFileIOManager::ShutdownInternal()
>  {
>    LOG(("CacheFileIOManager::ShutdownInternal() [this=%p]", this));
>  
> +  MOZ_ASSERT(CacheFileIOManager::IsOnIOThread());

Non-static methods should probably do
MOZ_ASSERT(mIOThread->IsCurrentThread());

@@ +1191,5 @@
> +bool
> +CacheFileIOManager::IsOnIOThread(bool aResultWhenUnknown)
> +{
> +  if (gInstance && gInstance->mIOThread)
> +    return gInstance->mIOThread->IsCurrentThread();

This is not always safe since gInstance is nulled out during shutdown. This should look like:

nsRefPtr<CacheFileIOManager> ioMan = gInstance;
if (ioMan && ioMan->mIOThread)
  return ioMan->mIOThread->IsCurrentThread();

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: v2 [healthy]

Healthy! :)

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

The oranges are tracked for plain gum as well.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 8360500 [details] [diff] [review]
v2 [healthy]

Grr... the review comments has not been addressed.  Hope I won't break it...

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #16)
> Comment on attachment 8357513 [details] [diff] [review]
> v1
> 
> Review of attachment 8357513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheFileIOManager.cpp
> @@ +51,5 @@
> > +  nsRefPtr<nsRunnableMethod<CacheFileHandle, nsrefcnt, false> > event =
> > +    NS_NewNonOwningRunnableMethod(this, &CacheFileHandle::Release);
> > +  nsresult rv = ioTarget->Dispatch(event, nsIEventTarget::DISPATCH_NORMAL);
> > +  if (NS_FAILED(rv))
> > +    return false;
> 
> I think that it would be better to not call release at all rather than
> calling it on a wrong thread in case dispatching fails.

Hmm... big assert has to be here.  I believe there will be leaks.  And this needs to somehow handle bug 957707.  I'll do it.

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: v3

- IsOnIOThread renamed properly
- release permitted off the IO thread, using fact that that CloseHandleInternal is no-op after mShuttingDown has been set
- clearing all handles during shutdown internal, using fact that no new handles can be created after mShuttingDown has been set while we don't want to keep any bad handles after that point (this step is probably not necessary after I've rechecked the code, will be removed in next patch version ; Michal, is that so?)

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

I'm a bit worried that the media tests go crazy with this patch.  But more try retriggers will say.

Comment 21

[Honza Bambas (:mayhemer)]

Attachment: v3.1 (to apply before index)

As comment 20, just merged to by applied before the index patch.

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

Comment 22

[Michal Novotny [:michal]]

Comment on attachment 8363274 [details] [diff] [review]
v3.1 (to apply before index)

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

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +858,5 @@
> +  // Clear out the table, when we are here, no new handles can be added
> +  // and handles will no longer remove them self from this table and we
> +  // don't want to keep invalid handles here. Also, there is no lookup
> +  // after this point to happen.
> +  mHandles.ClearAll();

We get all handles from the hashtable and we remove them one by one in the for cycle above. So how we can end up having non-empty hashtable here?

Comment 23

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #22)
> We get all handles from the hashtable and we remove them one by one in the
> for cycle above. So how we can end up having non-empty hashtable here?

Yeah, I want to remove it.  You are actually answering my question from comment 20.  Patch will update.

Comment 24

[Honza Bambas (:mayhemer)]

Attachment: v3.2

Here it is, I converted to assert (just to be sure we are empty).

Comment 25

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/mozilla-central/rev/86ce91bee036

Comment 27

[Honza Bambas (:mayhemer)]

Verified by cache2 trial (bug 967693)

Comment 28

[Honza Bambas (:mayhemer)]

Note: the rational for the "OrCeased" is at:

http://hg.mozilla.org/mozilla-central/file/86ce91bee036/netwerk/cache2/CacheFileIOManager.cpp#l819

We are nullifying gInstance before the thread has a chance to completely finish all events that may need it.  The thread lives, but gInstance is already null.

However, since bug 949250, we may not need that anymore.  The order has been swapped: https://bugzilla.mozilla.org/show_bug.cgi?id=949250#attach_8357509.  Unfortunately, I didn't write why :(  bad Honza...