[webkit-reviews] review denied: [Bug 53104] Intermittent crash in fast/files/read-blob-async.html on the GTK+ debug bots : [Attachment 81282] 3rd patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 53104: Intermittent crash in fast/files/read-blob-async.html on the GTK+
debug bots
https://bugs.webkit.org/show_bug.cgi?id=53104

Attachment 81282: 3rd patch
https://bugs.webkit.org/attachment.cgi?id=81282&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.