Bug 61669 - Ref BlobResourceHandle before calling doNotifyFinish() on main thread
Summary: Ref BlobResourceHandle before calling doNotifyFinish() on main thread
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
Depends on:
Reported: 2011-05-27 14:40 PDT by Nate Chapin
Modified: 2011-05-27 19:24 PDT (History)
2 users (show)

See Also:

patch #2 (2.11 KB, patch)
2011-05-27 14:42 PDT, Nate Chapin
jianli: review-
Details | Formatted Diff | Diff
Addressing comments (2.35 KB, patch)
2011-05-27 15:14 PDT, Nate Chapin
jianli: review-
Details | Formatted Diff | Diff
Moved test line in ChangeLog (2.35 KB, patch)
2011-05-27 15:18 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2011-05-27 14:40:34 PDT
In another patch, I changed WebCore timings slightly and found that we don't seem to be guaranteeing that a BlobResourceHandle will survive until doNotfyFinish() gets called on the appropriate thread.

I haven't found a way to trigger this without modifying the loader rather substantially, but I do have a fix for the issue I saw.
Comment 1 Nate Chapin 2011-05-27 14:42:07 PDT
Created attachment 95217 [details]
patch #2
Comment 2 Jian Li 2011-05-27 14:52:37 PDT
Comment on attachment 95217 [details]
patch #2

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

> Source/WebCore/ChangeLog:5
> +        ref() BlobResourceHandle before calling doNotifyFinish()

The title is somewhat confusing to me. How about: Keep a reference to BlobResourceHandle before calling doNotifyFinish asynchronously to ensure it's still safe in the main thread.

Which test covers this? Please point it out here.

> Source/WebCore/platform/network/BlobResourceHandle.cpp:599
> +    if (!handle->aborted())

We should say:
    if (!handle->aborted() && handle->client())

> Source/WebCore/platform/network/BlobResourceHandle.cpp:601
> +    handle->deref();

Please add a comment for deref, like: Unbalance the deref in notifyFinish.

> Source/WebCore/platform/network/BlobResourceHandle.cpp:607
> +    ref();

Please add an empty line here.
Comment 3 Nate Chapin 2011-05-27 15:14:05 PDT
Created attachment 95220 [details]
Addressing comments
Comment 4 Jian Li 2011-05-27 15:17:04 PDT
Comment on attachment 95220 [details]
Addressing comments

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

> Source/WebCore/ChangeLog:7
> +        I triggered this crash in fast/files/file-reader-abort.html during a

Please move this line and the line below to place after the bug link.
Comment 5 Nate Chapin 2011-05-27 15:18:33 PDT
Created attachment 95221 [details]
Moved test line in ChangeLog
Comment 6 WebKit Commit Bot 2011-05-27 19:24:32 PDT
Comment on attachment 95221 [details]
Moved test line in ChangeLog

Clearing flags on attachment: 95221

Committed r87595: <http://trac.webkit.org/changeset/87595>
Comment 7 WebKit Commit Bot 2011-05-27 19:24:37 PDT
All reviewed patches have been landed.  Closing bug.