RESOLVED FIXED 133079
Use C++11 lambdas to construct FileThread::Task objects
https://bugs.webkit.org/show_bug.cgi?id=133079
Summary Use C++11 lambdas to construct FileThread::Task objects
Zan Dobersek
Reported 2014-05-19 12:00:48 PDT
Use C++11 lambdas to construct FileThread::Task objects
Attachments
Patch (14.08 KB, patch)
2014-05-19 12:14 PDT, Zan Dobersek
no flags
Patch for landing (14.23 KB, patch)
2014-06-05 02:21 PDT, Zan Dobersek
no flags
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (774.26 KB, application/zip)
2014-06-05 11:02 PDT, Build Bot
no flags
Zan Dobersek
Comment 1 2014-05-19 12:14:52 PDT
WebKit Commit Bot
Comment 2 2014-05-19 12:17:51 PDT
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.
Alexey Proskuryakov
Comment 3 2014-05-20 10:21:53 PDT
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.
Darin Adler
Comment 4 2014-05-24 19:10:25 PDT
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 Proskuryakov
Comment 5 2014-05-24 19:56:32 PDT
> 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.
Zan Dobersek
Comment 6 2014-05-25 11:05:33 PDT
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.
Alexey Proskuryakov
Comment 7 2014-05-25 18:20:23 PDT
> I'd prefer to spin this off into a separate bug, if that's fine by both of you. OK.
Zan Dobersek
Comment 8 2014-06-05 02:18:01 PDT
(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.
Zan Dobersek
Comment 9 2014-06-05 02:21:48 PDT
Created attachment 232532 [details] Patch for landing
WebKit Commit Bot
Comment 10 2014-06-05 02:36:20 PDT
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.
Build Bot
Comment 11 2014-06-05 11:02:43 PDT
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
Build Bot
Comment 12 2014-06-05 11:02:47 PDT
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
Alexey Proskuryakov
Comment 13 2014-06-05 11:52:59 PDT
This is not a real failure (and not even a crash). I wish I knew what was going on.
Zan Dobersek
Comment 14 2014-06-07 06:30:00 PDT
Comment on attachment 232532 [details] Patch for landing Clearing flags on attachment: 232532 Committed r169675: <http://trac.webkit.org/changeset/169675>
Zan Dobersek
Comment 15 2014-06-07 06:30:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.