Bug 53104 - Intermittent crash in fast/files/read-blob-async.html on the GTK+ debug bots
Summary: Intermittent crash in fast/files/read-blob-async.html on the GTK+ debug bots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Jian Li
URL:
Keywords: Gtk
: 53673 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-01-25 11:14 PST by Martin Robinson
Modified: 2011-02-07 15:56 PST (History)
3 users (show)

See Also:


Attachments
Proposed Patch (11.29 KB, patch)
2011-02-01 18:35 PST, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (11.30 KB, patch)
2011-02-02 11:06 PST, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
3rd patch (8.17 KB, patch)
2011-02-04 13:59 PST, Jian Li
levin: review-
jianli: commit-queue-
Details | Formatted Diff | Diff
4th patch (8.13 KB, patch)
2011-02-04 16:34 PST, Jian Li
levin: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2011-01-25 11:14:58 PST
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==
Comment 1 Jian Li 2011-01-28 15:10:02 PST
Found the cause. Working on the fix.
Comment 2 Jian Li 2011-02-01 18:35:42 PST
Created attachment 80872 [details]
Proposed Patch
Comment 3 Jian Li 2011-02-02 11:06:09 PST
Created attachment 80932 [details]
Proposed Patch
Comment 4 Jian Li 2011-02-04 13:49:20 PST
*** Bug 53673 has been marked as a duplicate of this bug. ***
Comment 5 Jian Li 2011-02-04 13:59:55 PST
Created attachment 81282 [details]
3rd patch
Comment 6 David Levin 2011-02-04 14:22:38 PST
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.
Comment 7 Jian Li 2011-02-04 16:34:52 PST
Created attachment 81319 [details]
4th patch

All fixed.
Comment 8 David Levin 2011-02-07 13:09:56 PST
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.
Comment 9 Jian Li 2011-02-07 15:29:40 PST
Committed as https://trac.webkit.org/changeset/77852.
Comment 10 Martin Robinson 2011-02-07 15:56:28 PST
Thanks for fixing this!