Created attachment 73889 [details] Repro Repro: <script>window.requestFileSystem(16)</script> id: chrome.dll!WebCore::DOMFileSystem::DispatchCallbackTask<...>::performTask ReadAV@NULL (2ef0152de44115d5e3f1de3111aba5dd) description: Attempt to read from unallocated NULL pointer in chrome.dll!WebCore::DOMFileSystem::DispatchCallbackTask<...>::performTask stack: chrome.dll!WebCore::DOMFileSystem::DispatchCallbackTask<...>::performTask chrome.dll!WebCore::performTask chrome.dll!RunnableFunction<void chrome.dll!MessageLoop::RunTask chrome.dll!MessageLoop::DoWork chrome.dll!base::MessagePumpDefault::Run chrome.dll!MessageLoop::RunInternal chrome.dll!MessageLoop::Run chrome.dll!RendererMain chrome.dll!ChromeMain chrome.exe!MainDllLoader::Launch chrome.exe!wWinMain chrome.exe!__tmainCRTStartup ...
Code is not checking if errorCallback is NULL before scheduling a callback on error. Suggested patch: void DOMWindow::requestFileSystem(int type, long long size, PassRefPtr<FileSystemCallback> successCallback, PassRefPtr<ErrorCallback> errorCallback) { Document* document = this->document(); if (!document) return; if (!AsyncFileSystem::isAvailable() || !document->securityOrigin()->canAccessFileSystem()) { - DOMFileSystem::scheduleCallback(document, errorCallback, FileError::create(FileError::SECURITY_ERR)); + if (errorCallback) + DOMFileSystem::scheduleCallback(document, errorCallback, FileError::create(FileError::SECURITY_ERR)); return; } AsyncFileSystem::Type fileSystemType = static_cast<AsyncFileSystem::Type>(type); if (fileSystemType != AsyncFileSystem::Temporary && fileSystemType != AsyncFileSystem::Persistent) { - DOMFileSystem::scheduleCallback(document, errorCallback, FileError::create(FileError::INVALID_MODIFICATION_ERR)); + if (errorCallback) + DOMFileSystem::scheduleCallback(document, errorCallback, FileError::create(FileError::INVALID_MODIFICATION_ERR)); return; } LocalFileSystem::localFileSystem().requestFileSystem(document, fileSystemType, size, FileSystemCallbacks::create(successCallback, errorCallback, document), false); }
I'm implementing a patch
Created attachment 84761 [details] Patch
Attachment 84761 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/page/DOMWindow.cpp:746: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/page/DOMWindow.cpp:753: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 84763 [details] Patch
Variation: <script>setTimeout(resolveLocalFileSystemURI,1)</script> I'll create a patch for both issues
Created attachment 85146 [details] Patch for both requestFileSystem and resolveLocalFileSystemURI
Adam/Kinuko: you seem to have implemented/reviewed these functions originally - any chance you can review this patch?
Comment on attachment 85146 [details] Patch for both requestFileSystem and resolveLocalFileSystemURI View in context: https://bugs.webkit.org/attachment.cgi?id=85146&action=review > Source/WebCore/page/DOMWindow.cpp:746 > + if (errorCallback) > + DOMFileSystem::scheduleCallback(document, errorCallback, FileError::create(FileError::SECURITY_ERR)); Random drive-by nit: Seems it might be time to think about making a helper function for this. Something like: scheduleFileCallback(document, errorCallback, SECURITY_ERR); which includes the if (!errorCallback) return, and the FileError creation. It may not feel worth it. Unsure.
The patch does not appear marked for review (unless I cleared it accidentally?)
Thanks for fixing it. I think we could just remove the ASSERT(callback) in DOMFileSystem::scheduleCallback and schedule a task iff callback is given and not null. Otherwise it seems we have to put such an if condition whenever we call it. diff --git a/Source/WebCore/fileapi/DOMFileSystem.h b/Source/WebCore/fileapi/DOMFileSystem.h index 1d820f1..38a054a 100644 --- a/Source/WebCore/fileapi/DOMFileSystem.h +++ b/Source/WebCore/fileapi/DOMFileSystem.h @@ -101,8 +101,8 @@ template <typename CB, typename CBArg> void DOMFileSystem::scheduleCallback(ScriptExecutionContext* scriptExecutionContext, PassRefPtr<CB> callback, PassRefPtr<CBArg> arg) { ASSERT(scriptExecutionContext->isContextThread()); - ASSERT(callback); - scriptExecutionContext->postTask(new DispatchCallbackTask<CB, CBArg>(callback, arg)); + if (callback) + scriptExecutionContext->postTask(new DispatchCallbackTask<CB, CBArg>(callback, arg)); } } // namespace
Created attachment 85462 [details] Patch for DOMFileSystem::scheduleCallback
Comment on attachment 85462 [details] Patch for DOMFileSystem::scheduleCallback Thanks, this patch looks good to me (though I'm not a reviewer).
Comment on attachment 85462 [details] Patch for DOMFileSystem::scheduleCallback The patch doesn't have filesystem-no-callback-null-ptr-crash-expected.txt.
Created attachment 95445 [details] Patch Same patch, but with fixed layout test and added "-expected.txt" file.
Created attachment 95446 [details] Same patch, but updated changelog date
@Kent Tamura: can you review+/commit+?
Comment on attachment 95446 [details] Same patch, but updated changelog date Clearing flags on attachment: 95446 Committed r87758: <http://trac.webkit.org/changeset/87758>
All reviewed patches have been landed. Closing bug.
Thanks! Chromium: https://code.google.com/p/chromium/issues/detail?id=63204
Revision r87758 cherry-picked into qtwebkit-2.2 with commit f1148b9 <http://gitorious.org/webkit/qtwebkit/commit/f1148b9>
bug 61936 was opened because the test is flaky and I don't know why - can somebody help me find out?