WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch for landing
(14.23 KB, patch)
2014-06-05 02:21 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
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
Details
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2014-05-19 12:14:52 PDT
Created
attachment 231708
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug