Use C++11 lambdas to construct FileThread::Task objects
Created attachment 231708 [details] Patch
Attachment 231708 [details] did not pass style-queue: ERROR: Source/WebCore/fileapi/FileThread.h:60: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 231708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231708&action=review > Source/WebCore/fileapi/AsyncFileStream.cpp:81 > - fileThread()->postTask(std::make_unique<FileThread::Task>(proxy.get(), &AsyncFileStream::startOnFileThread)); > + AsyncFileStream* proxyPtr = proxy.get(); This code looks very suspicious. Both main thread and file thread have pointers to AsyncFileStream, potentially referencing and dereferencing it concurrently. But it's not even ThreadSafeRefCounted.
Comment on attachment 231708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231708&action=review >> Source/WebCore/fileapi/AsyncFileStream.cpp:81 >> + AsyncFileStream* proxyPtr = proxy.get(); > > This code looks very suspicious. Both main thread and file thread have pointers to AsyncFileStream, potentially referencing and dereferencing it concurrently. But it's not even ThreadSafeRefCounted. Alexey, is that a comment on some new aspect of the code or just on the code as it already was? > Source/WebCore/fileapi/AsyncFileStream.cpp:84 > + if (!proxyPtr->client()) > + return; Why did you delete the comment about a race condition that was in the startOnFileThread function? > Source/WebCore/fileapi/AsyncFileStream.cpp:171 > + URL blobURLCopy = blobURL.copy(); I wonder why we need isolatedCopy for String, but not for URL. Maybe URL needs an isolatedCopy function too. That’s not something that’s changing in this patch, but might be worth thinking through. > Source/WebCore/fileapi/FileThread.h:71 > + Task(Task&& other) > + : m_task(std::move(other.m_task)) > + , m_instance(other.m_instance) > + { > + } Do we need to write this explicitly? Won’t the compiler generate this if we don’t write it?
> Alexey, is that a comment on some new aspect of the code or just on the code as it already was? I think that it was already unsafe, yes.
Comment on attachment 231708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231708&action=review >>> Source/WebCore/fileapi/AsyncFileStream.cpp:81 >>> + AsyncFileStream* proxyPtr = proxy.get(); >> >> This code looks very suspicious. Both main thread and file thread have pointers to AsyncFileStream, potentially referencing and dereferencing it concurrently. But it's not even ThreadSafeRefCounted. > > Alexey, is that a comment on some new aspect of the code or just on the code as it already was? I believe this patch neither fixes this issue nor does it make it worse. I'd prefer to spin this off into a separate bug, if that's fine by both of you. >> Source/WebCore/fileapi/AsyncFileStream.cpp:84 >> + return; > > Why did you delete the comment about a race condition that was in the startOnFileThread function? Unintentional -- I'll re-add it. >> Source/WebCore/fileapi/AsyncFileStream.cpp:171 >> + URL blobURLCopy = blobURL.copy(); > > I wonder why we need isolatedCopy for String, but not for URL. Maybe URL needs an isolatedCopy function too. That’s not something that’s changing in this patch, but might be worth thinking through. URL::copy() does an isolated copy of the internal String object, and should perhaps have the same name. copy() makes it sound like it returns a general copy, not a cross-thread one. >> Source/WebCore/fileapi/FileThread.h:71 >> + } > > Do we need to write this explicitly? Won’t the compiler generate this if we don’t write it? Yes, it's required because the implicit move constructor doesn't get generated due to the copy constructor being declared deleted through WTF_MAKE_NONCOPYABLE.
> I'd prefer to spin this off into a separate bug, if that's fine by both of you. OK.
(In reply to comment #7) > > I'd prefer to spin this off into a separate bug, if that's fine by both of you. > > OK. Bug #133540.
Created attachment 232532 [details] Patch for landing
Attachment 232532 [details] did not pass style-queue: ERROR: Source/WebCore/fileapi/FileThread.h:58: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 232532 [details] Patch for landing Attachment 232532 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5911483581988864 New failing tests: media/W3C/video/networkState/networkState_during_loadstart.html
Created attachment 232565 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
This is not a real failure (and not even a crash). I wish I knew what was going on.
Comment on attachment 232532 [details] Patch for landing Clearing flags on attachment: 232532 Committed r169675: <http://trac.webkit.org/changeset/169675>
All reviewed patches have been landed. Closing bug.