Summary: | Intermittent crash in fast/files/read-blob-async.html on the GTK+ debug bots | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||||
Component: | WebKitGTK | Assignee: | Jian Li <jianli> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | jianli, levin, pnormand | ||||||||||
Priority: | P3 | Keywords: | Gtk | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Martin Robinson
2011-01-25 11:14:58 PST
Found the cause. Working on the fix. Created attachment 80872 [details]
Proposed Patch
Created attachment 80932 [details]
Proposed Patch
*** Bug 53673 has been marked as a duplicate of this bug. *** Created attachment 81282 [details]
3rd patch
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. Created attachment 81319 [details]
4th patch
All fixed.
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. Committed as https://trac.webkit.org/changeset/77852. Thanks for fixing this! |