Bug 53104 – Intermittent crash in fast/files/read-blob-async.html on the GTK+ debug bots

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

It seems that didFinishLoading is cleaning up the BlobResourceHandle before the asynchronous tasks finish. Here is the valgrind output:


==19460== Memcheck, a memory error detector
==19460== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==19460== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info
==19460== Command: ./WebKitBuild/Debug/Programs/DumpRenderTree LayoutTests/fast/files/read-blob-async.html
==19460== 
==19476== Warning: invalid file descriptor 1014 in syscall close()
==19476== Warning: invalid file descriptor 1015 in syscall close()
==19476== Warning: invalid file descriptor 1016 in syscall close()
==19476==    Use --log-fd=<number> to select an alternative log fd.
==19476== Warning: invalid file descriptor 1017 in syscall close()
==19476== Warning: invalid file descriptor 1018 in syscall close()
==19460== Invalid write of size 8
==19460==    at 0x5EF3608: WebCore::BlobResourceHandle::readDataAsync(WebCore::BlobDataItem const&) (BlobResourceHandle.cpp:438)
==19460==    by 0x5EF34C7: WebCore::BlobResourceHandle::readAsync() (BlobResourceHandle.cpp:423)
==19460==    by 0x5EF29D5: WebCore::BlobResourceHandle::getSizeForNext() (BlobResourceHandle.cpp:232)
==19460==    by 0x5EF2C2B: WebCore::BlobResourceHandle::didGetSize(long long) (BlobResourceHandle.cpp:279)
==19460==    by 0x5EF2A43: WebCore::BlobResourceHandle::getSizeForNext() (BlobResourceHandle.cpp:240)
==19460==    by 0x5EF28B5: WebCore::BlobResourceHandle::start() (BlobResourceHandle.cpp:214)
==19460==    by 0x5EF1FD1: WebCore::delayedStart(void*) (BlobResourceHandle.cpp:143)
==19460==    by 0x667816F: WTF::dispatchFunctionsFromMainThread() (MainThread.cpp:155)
==19460==    by 0x6677F46: WTF::timeoutFired(void*) (MainThreadGtk.cpp:43)
==19460==    by 0x9660B0A: ??? (in /lib/libglib-2.0.so.0.2705.0)
==19460==    by 0x96600B1: g_main_context_dispatch (in /lib/libglib-2.0.so.0.2705.0)
==19460==    by 0x9664777: ??? (in /lib/libglib-2.0.so.0.2705.0)
==19460==  Address 0x14216db8 is 152 bytes inside a block of size 176 free'd
==19460==    at 0x4C27D71: free (vg_replace_malloc.c:366)
==19460==    by 0x6677C0C: WTF::fastFree(void*) (FastMalloc.cpp:327)
==19460==    by 0x5772D93: WTF::RefCounted<WebCore::ResourceHandle>::operator delete(void*) (RefCounted.h:136)
==19460==    by 0x5EF2730: WebCore::BlobResourceHandle::~BlobResourceHandle() (BlobResourceHandle.cpp:178)
==19460==    by 0x572F86D: WTF::RefCounted<WebCore::ResourceHandle>::deref() (RefCounted.h:141)
==19460==    by 0x572FB22: void WTF::derefIfNotNull<WebCore::ResourceHandle>(WebCore::ResourceHandle*) (PassRefPtr.h:59)
==19460==    by 0x572F93F: WTF::RefPtr<WebCore::ResourceHandle>::operator=(WebCore::ResourceHandle*) (RefPtr.h:135)
==19460==    by 0x5DE74D2: WebCore::SubresourceLoader::didFinishLoading(double) (SubresourceLoader.cpp:183)
==19460==    by 0x5DDE814: WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) (ResourceLoader.cpp:434)
==19460==    by 0x5EF4167: WebCore::BlobResourceHandle::notifyFinish() (BlobResourceHandle.cpp:584)
==19460==    by 0x5EF3472: WebCore::BlobResourceHandle::readAsync() (BlobResourceHandle.cpp:417)
==19460==    by 0x5EF3953: WebCore::BlobResourceHandle::consumeData(char const*, int) (BlobResourceHandle.cpp:502)
==19460== 
==19460== Invalid write of size 8
==19460==    at 0x5EF3608: WebCore::BlobResourceHandle::readDataAsync(WebCore::BlobDataItem const&) (BlobResourceHandle.cpp:438)
==19460==    by 0x5EF34C7: WebCore::BlobResourceHandle::readAsync() (BlobResourceHandle.cpp:423)
==19460==    by 0x5EF3953: WebCore::BlobResourceHandle::consumeData(char const*, int) (BlobResourceHandle.cpp:502)
==19460==    by 0x5EF3849: WebCore::BlobResourceHandle::didRead(int) (BlobResourceHandle.cpp:473)
==19460==    by 0x5BE2752: WebCore::didRead(WebCore::ScriptExecutionContext*, WebCore::FileStreamProxy*, int) (FileStreamProxy.cpp:169)
==19460==    by 0x5BE52D9: WebCore::CrossThreadTask2<WebCore::FileStreamProxy*, WebCore::FileStreamProxy*, int, int>::performTask(WebCore::ScriptExecutionContext*) (CrossThreadTask.h:112)
==19460==    by 0x5A9A23F: WebCore::performTask(void*) (Document.cpp:4722)
==19460==    by 0x667816F: WTF::dispatchFunctionsFromMainThread() (MainThread.cpp:155)
==19460==    by 0x6677F46: WTF::timeoutFired(void*) (MainThreadGtk.cpp:43)
==19460==    by 0x9660B0A: ??? (in /lib/libglib-2.0.so.0.2705.0)
==19460==    by 0x96600B1: g_main_context_dispatch (in /lib/libglib-2.0.so.0.2705.0)
==19460==    by 0x9664777: ??? (in /lib/libglib-2.0.so.0.2705.0)
==19460==  Address 0x14433b88 is 152 bytes inside a block of size 176 free'd
==19460==    at 0x4C27D71: free (vg_replace_malloc.c:366)
==19460==    by 0x6677C0C: WTF::fastFree(void*) (FastMalloc.cpp:327)
==19460==    by 0x5772D93: WTF::RefCounted<WebCore::ResourceHandle>::operator delete(void*) (RefCounted.h:136)
==19460==    by 0x5EF2730: WebCore::BlobResourceHandle::~BlobResourceHandle() (BlobResourceHandle.cpp:178)
==19460==    by 0x572F86D: WTF::RefCounted<WebCore::ResourceHandle>::deref() (RefCounted.h:141)
==19460==    by 0x572FB22: void WTF::derefIfNotNull<WebCore::ResourceHandle>(WebCore::ResourceHandle*) (PassRefPtr.h:59)
==19460==    by 0x572F93F: WTF::RefPtr<WebCore::ResourceHandle>::operator=(WebCore::ResourceHandle*) (RefPtr.h:135)
==19460==    by 0x5DE74D2: WebCore::SubresourceLoader::didFinishLoading(double) (SubresourceLoader.cpp:183)
==19460==    by 0x5DDE814: WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) (ResourceLoader.cpp:434)
==19460==    by 0x5EF4167: WebCore::BlobResourceHandle::notifyFinish() (BlobResourceHandle.cpp:584)
==19460==    by 0x5EF3472: WebCore::BlobResourceHandle::readAsync() (BlobResourceHandle.cpp:417)
==19460==    by 0x5EF3953: WebCore::BlobResourceHandle::consumeData(char const*, int) (BlobResourceHandle.cpp:502)
==19460==

[Jian Li]

Found the cause. Working on the fix.

[Jian Li]

Created attachment 80872 [details]
Proposed Patch

[Jian Li]

Created attachment 80932 [details]
Proposed Patch

[Jian Li]

*** Bug 53673 has been marked as a duplicate of this bug. ***

[Jian Li]

Created attachment 81282 [details]
3rd patch

[David Levin]

Comment on attachment 81282 [details]
3rd patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81282&action=review

> Source/WebCore/ChangeLog:11
> +        a standalone function because the client might choose to dispose the

Function level comment.

> Source/WebCore/ChangeLog:14
> +        period. 

Consider moving to be a function level comment.

> Source/WebCore/ChangeLog:16
> +        BlobResourceHandle::cancel can be triggered when we abort a FileReader.

Remove this bullet and put it at the function level below.

> Source/WebCore/ChangeLog:33
> +        * platform/network/ResourceHandle.h: Change cancel() to virtual.

so that BlobResourceHandle::cancel will be called when we abort a FileReader.

> Source/WebCore/platform/network/BlobRegistryImpl.cpp:73
> +    return handle;

return handle.release();

> Source/WebCore/platform/network/BlobResourceHandle.cpp:189
> +    BlobResourceHandle* handle = static_cast<BlobResourceHandle*>(context);

Make this a RefPtr like this (and remove the deref below).
 RefPtr<BlobResourceHandle> handle = adoptRef(static_cast<BlobResourceHandle*>(context));

> Source/WebCore/platform/network/BlobResourceHandle.cpp:196
> +    // For async version, we need to delay the start so that DocumentThreadableLoader that is in the stack can finish the initialization.

Consider moving this comment to be in the if (m_async) and get rid of "For async version".

// Delay the start so ... (I don't understand the what this comment is talking about, so we should figure out something here.  For example, why is this true for the async version and not the sync version?)

> Source/WebCore/platform/network/BlobResourceHandle.cpp:197
> +    // Note that we also need to add a ref here to guard it until the delay function is called.

Move to where the ref is and shorten (see below).

> Source/WebCore/platform/network/BlobResourceHandle.cpp:198
> +    if (m_async) {

// Keep BlobResourceHandle alive until delayedStartBlobResourceHandle runs.

> Source/WebCore/platform/network/BlobResourceHandle.cpp:200
> +        callOnMainThread(delayedStartBlobResourceHandle, this);

Consider doing a "return;" here and get rid of the else.

> Source/WebCore/platform/network/BlobResourceHandle.cpp:603
> +    // For async version, we will have to notify the client from a standalone function because the client might choose to dispose the handle immediately.

Why is that a problem (since after this statement, it just returns)?
Also consider moving the comment inside of "if (m_async)".

> Source/WebCore/platform/network/BlobResourceHandle.cpp:607
> +        if (client())

Why not just call "delayedFinishLoading" here? (Of course, the name will need to be improved.)  This may also be a good idea for the other place where we do something similar.

[Jian Li]

Created attachment 81319 [details]
4th patch

All fixed.

[David Levin]

Comment on attachment 81319 [details]
4th patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81319&action=review

> Source/WebCore/ChangeLog:27
> +        immediately, as in the GTK+ case.

This line looks like a left over from some editting.

> Source/WebCore/platform/network/BlobResourceHandle.cpp:199
> +        // Do the start later because this method is part of the async creation and we want it to finish quickly.

I think you only need to explain why like this:
  "Finish this async call quickly and return."

> Source/WebCore/platform/network/BlobResourceHandle.cpp:201
> +

Remove extra blank line.

> Source/WebCore/platform/network/BlobResourceHandle.cpp:608
> +        // If we notify the client now, BlobResourceHandle will get disposed while we still have BlobResourceHandle calls in the stack.

The second sentence is sufficient.

[Jian Li]

Committed as https://trac.webkit.org/changeset/77852.

[Martin Robinson]

Thanks for fixing this!