Chromium Code Reviews
|
|
DescriptionIntroduce CompositeDataConsumerHandle.
This CL introduces CompositeDataConsumerHandle, a utility class to composite
multiple WebDataConsumerHandle objects. This class is planned to be used in
the following use case, for example.
Imagine we want to provide a WebDataConsumerHandle representing a response body
gotten from a ThreadableLoader, but we need to create the handle BEFORE the
response body arrives. In such a case, we can create a
CompositeDataConsumerHandle initialized with a handle gotten from
CompositeDataConsumerHandle::createWaitingHandle. When the response body
arrives, calling CompositeDataConsumerHandle::update will update the handle so
that it represents the response body.
BUG=480746
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197174
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 14
Patch Set 6 : #
Total comments: 16
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #
Total comments: 9
Patch Set 9 : #
Total comments: 2
Patch Set 10 : #
Total comments: 20
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #
Messages
Total messages: 44 (16 generated)
yhirano@chromium.org changed reviewers: + hiroshige@chromium.org
Sorry, I misunderstood WeakPtr requirements and that leads to test failures. Please wait for a while.
It's ready now. PTAL.
> I misunderstood WeakPtr requirements and that leads to test failures. In my understandings, this is that WeakPtr shouldn't pass across threads (I also missed this requirement) and you implemented a kind of thread-safe weak pointer in this CL. It might be better to implement a thread-safe weak pointer in WTF and use it in this CL and other code locations that do similar things, but I'm not sure this is a good idea. How do you think? Anyway, thinking about this cross-thread WeakPtr issue would require some time, so keeping the current implementation of Patch Set 5 (and doing WeakPtr-related things later) would be reasonable. (I think I also should disable WeakPtr + AllowCrossThreadAccess and createCrossThreadTask because it is not thread-safe in general, but this is not related to this CL.)
https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.cpp:88: Result CompositeDataConsumerHandle::read(void* data, size_t size, Flags flags, size_t* readSize) How about adding ASSERT(!m_client || m_clientThread == Platform::current()->currentThread()) (or RELEASE_ASSERT) in read() and other methods that have this thread restriction? https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.cpp:129: void CompositeDataConsumerHandle::updateInternal(PassOwnPtr<WebDataConsumerHandle> handle) I think |handle| shouldn't have any client here, right? If so, it would be good to mention this in the comment of update() and add ASSERT()'s. https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.cpp:138: // ... and it is the curren thread. nit: current. https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... File Source/modules/fetch/CompositeDataConsumerHandle.h (right): https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.h:18: // ownes a web data consumer handle and delegates methods. A user can update nit: s/ownes/owns/ https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.h:35: // was created. Why does this thread restriction exist? From the code, update() seems callable from any thread. (Also, I wonder if we can remove some mutexes by limiting threads where update() is called, but this current thread restriction doesn't remove mutexes...) https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.h:40: static PassOwnPtr<WebDataConsumerHandle> createWaitingHandle(); It would be better to add a comment explaining why createWaitingHandle(), createDoneHandle() and related classes are placed here (as explained in the CL description), because there are no direct dependencies bewteen them and CompositeDataConsumerHandle.
https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.cpp:123: void CompositeDataConsumerHandle::update(PassOwnPtr<WebDataConsumerHandle> handle) Multiple calls to update() might be executed out-of-order. For example, 0. registerClient() on Thread A. 1. update(handle1) on Thread B. -> postTask to Thread A. 2. unregisterClient() on Thread A. 3. update(handle2) on Thread B, and m_handle = handle2 because |m_client| is null here. 4. updateInternal(handle1) is called on Thread A (posted on Step 1), and m_handle = handle1. or 0. registerClient() on Thread A. 1. update(handle1) on Thread B. -> postTask to Thread A. 2. unregisterClient() on Thread A. 3. registerClient() on Thread B. 4. updateInternal(handle1) is called on Thread A (posted on Step 1) -> postTask to Thread B again. 5. update(handle2) on Thread B, and m_handle = handle2. 6. updateInternal(handle1) on Thread B )posted on Step 4), and m_handle = handle1. The nature of thread restriction of WebDataConsumerHandle might make the semantics complex, because "the thread that calls registerClient()" is dynamically defined after handle's construction, can be changed, can be non-existing e.g. before the first call to registerClient() / after the last call to unregisterClient().
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
I'm not sure if thread-safe weak ptr is generally needed. Let's talk offline. https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.cpp:88: Result CompositeDataConsumerHandle::read(void* data, size_t size, Flags flags, size_t* readSize) On 2015/06/03 08:45:05, hiroshige wrote: > How about adding ASSERT(!m_client || m_clientThread == > Platform::current()->currentThread()) (or RELEASE_ASSERT) in read() and other > methods that have this thread restriction? Done. https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.cpp:123: void CompositeDataConsumerHandle::update(PassOwnPtr<WebDataConsumerHandle> handle) On 2015/06/03 10:35:56, hiroshige wrote: > Multiple calls to update() might be executed out-of-order. > For example, > > 0. registerClient() on Thread A. > 1. update(handle1) on Thread B. -> postTask to Thread A. > 2. unregisterClient() on Thread A. > 3. update(handle2) on Thread B, and m_handle = handle2 because |m_client| is > null here. > 4. updateInternal(handle1) is called on Thread A (posted on Step 1), and > m_handle = handle1. > > or > > 0. registerClient() on Thread A. > 1. update(handle1) on Thread B. -> postTask to Thread A. > 2. unregisterClient() on Thread A. > 3. registerClient() on Thread B. > 4. updateInternal(handle1) is called on Thread A (posted on Step 1) -> postTask > to Thread B again. > 5. update(handle2) on Thread B, and m_handle = handle2. > 6. updateInternal(handle1) on Thread B )posted on Step 4), and m_handle = > handle1. > > The nature of thread restriction of WebDataConsumerHandle might make the > semantics complex, because "the thread that calls registerClient()" is > dynamically defined after handle's construction, can be changed, can be > non-existing e.g. before the first call to registerClient() / after the last > call to unregisterClient(). You're right, thanks. I fixed the problem by introducing a request token. There is another issue that update will never take effect if client registration / unregistration is made multiple time on multiple threads (register on A -> update on C -> unregister on A -> register on B -> unregister on B -> ...), but I don't care about that pathological case. https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.cpp:129: void CompositeDataConsumerHandle::updateInternal(PassOwnPtr<WebDataConsumerHandle> handle) On 2015/06/03 08:45:05, hiroshige wrote: > I think |handle| shouldn't have any client here, right? > If so, it would be good to mention this in the comment of update() and add > ASSERT()'s. Yes. Added a comment on the header. I cannot put an assertion because we don't have such a predicate. https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.cpp:138: // ... and it is the curren thread. On 2015/06/03 08:45:05, hiroshige wrote: > nit: current. Done. https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... File Source/modules/fetch/CompositeDataConsumerHandle.h (right): https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.h:18: // ownes a web data consumer handle and delegates methods. A user can update On 2015/06/03 08:45:05, hiroshige wrote: > nit: s/ownes/owns/ Done. https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.h:35: // was created. On 2015/06/03 08:45:05, hiroshige wrote: > Why does this thread restriction exist? > From the code, update() seems callable from any thread. > > (Also, I wonder if we can remove some mutexes by limiting threads where update() > is called, but this current thread restriction doesn't remove mutexes...) I would like to leave a future optimization opportunity by limiting thread. Currently I don't think of a use case that needs calling |update| on arbitrary threads, but if it exists, loosing the restriction will be OK. https://codereview.chromium.org/1162043007/diff/80001/Source/modules/fetch/Co... Source/modules/fetch/CompositeDataConsumerHandle.h:40: static PassOwnPtr<WebDataConsumerHandle> createWaitingHandle(); On 2015/06/03 08:45:05, hiroshige wrote: > It would be better to add a comment explaining why createWaitingHandle(), > createDoneHandle() and related classes are placed here (as explained in the CL > description), because there are no direct dependencies bewteen them and > CompositeDataConsumerHandle. > Done.
Patchset #8 (id:180001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #6 (id:140001) has been deleted
I uploaded PS6 which uses the Reader API (https://codereview.chromium.org/1149563007/). Can you take a look?
https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:77: ++m_token; Assert that there is no reader (e.g. |!m_reader|). https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:85: MutexLocker locker(m_mutex); Assert that m_readerThread is the current thread. https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:131: m_readerThread->postTask(FROM_HERE, new Task(threadSafeBind(&Context::updateReader, this, m_token))); |this| might destructed before this task is executed: 1. handle->update() is called, and postTask() here is called. 2. handle is destructed. 3. reader is destructed, Context is destructed. 4. updateReader() is called on the invalidated Context. How about passing RefPtr to |this|? (I think RefPtr/PassRefPtr to this can be passed to threadSafeBind() because Context is ThreadSafeRefCounted, and we can call static void updateReader(PassRefPtr<Context> that, Token token) via postTask(), but not so sure. ping me offline if necessary) https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.h (right): https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:20: // |handle| must not be locked. Also |handle| must be non-null (according to the current impl)? If so, please add comments and assertions. https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:28: // was created. |handle| must not be locked. ditto. https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:38: // Returns a handle that returns ShouldWait for read / beginRead / endRead Actually this returns UnexpectedError for endRead. https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:42: // Returns a handle that returns Done for read / beginRead / endRead ditto. https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:51: explicit CompositeDataConsumerHandle(PassOwnPtr<WebDataConsumerHandle>); The handle must be non-null?
https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:131: m_readerThread->postTask(FROM_HERE, new Task(threadSafeBind(&Context::updateReader, this, m_token))); Correction: The current code is fine. CrossThreadCopier wraps pointers to ThreadSafeRefCounted by PassRefPtr, which is stored as RefPtr in WTF::Function, thus the reference is retained until the task execution.
Patchset #7 (id:210001) has been deleted
https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:77: ++m_token; On 2015/06/09 05:37:16, hiroshige wrote: > Assert that there is no reader (e.g. |!m_reader|). Done. https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:85: MutexLocker locker(m_mutex); On 2015/06/09 05:37:15, hiroshige wrote: > Assert that m_readerThread is the current thread. Done. https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.h (right): https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:20: // |handle| must not be locked. On 2015/06/09 05:37:16, hiroshige wrote: > Also |handle| must be non-null (according to the current impl)? > If so, please add comments and assertions. Done, though I'm not sure if we should always write whether we accept a null. https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:28: // was created. |handle| must not be locked. On 2015/06/09 05:37:16, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:38: // Returns a handle that returns ShouldWait for read / beginRead / endRead On 2015/06/09 05:37:16, hiroshige wrote: > Actually this returns UnexpectedError for endRead. Done. https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:42: // Returns a handle that returns Done for read / beginRead / endRead On 2015/06/09 05:37:16, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/1162043007/diff/200001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:51: explicit CompositeDataConsumerHandle(PassOwnPtr<WebDataConsumerHandle>); On 2015/06/09 05:37:16, hiroshige wrote: > The handle must be non-null? I don't want to write comments because this is a private function.
https://codereview.chromium.org/1162043007/diff/230001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/230001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:133: m_reader = m_handle->obtainReader(m_client); From offline discussion: this is not safe if updateReaderNoLock() is called after beginRead() and before the corresponding endRead(), because the old |m_reader| is destructed after beginRead() without endRead(), and the new reader's endRead() is called without beginRead(). https://codereview.chromium.org/1162043007/diff/230001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.h (right): https://codereview.chromium.org/1162043007/diff/230001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:43: // Returns a handle that returns ShouldWait for read / beginRead and s/ShouldWait/Done/ ?
https://codereview.chromium.org/1162043007/diff/230001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/230001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:133: m_reader = m_handle->obtainReader(m_client); On 2015/06/09 10:35:30, hiroshige wrote: > From offline discussion: this is not safe if updateReaderNoLock() is called > after beginRead() and before the corresponding endRead(), because the old > |m_reader| is destructed after beginRead() without endRead(), and the new > reader's endRead() is called without beginRead(). You're right, fixed. https://codereview.chromium.org/1162043007/diff/230001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.h (right): https://codereview.chromium.org/1162043007/diff/230001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:43: // Returns a handle that returns ShouldWait for read / beginRead and On 2015/06/09 10:35:30, hiroshige wrote: > s/ShouldWait/Done/ ? Sorry, done.
https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:86: { Add ASSERT(!m_isInTwoPhaseRead) and ASSERT(!m_isUpdateWaitingForEndRead). Also, add a destructor with: ASSERT(!m_readerThread); ASSERT(!m_reader); ASSERT(!m_client); https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:90: ASSERT(m_reader); We assume obtainReader() returns non-null pointer here, but WebDataConsumerHandle::obtainReaderInternal() returns nullptr. So how about making WebDataConsumerHandle::obtainReaderInternal() abstract (in a separate CL)? https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:126: if (m_isUpdateWaitingForEndRead) { We need MutexLocker(m_mutex) here, because we should protect |m_handle| (which can be modified on the writer thread). https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:165: m_reader = m_handle->obtainReader(m_client); Should we post a task for didGetReadable() here because the old reader might have returned ShouldWait and the new reader might return non-ShouldWait? Or is it |m_handle->obtainReader()|'s responsibility to post such task if it returns non-ShouldWait? If the latter, we have to update DoneHandle::obtainReaderInternal(). https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.h (right): https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.h:46: FYI: createUnexpectedErrorHandle() might be helpful for FetchBlobConsumerHandle implementation (I'm adding it locally and https://codereview.chromium.org/1171913003/).
Patchset #9 (id:270001) has been deleted
https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:86: { On 2015/06/10 06:55:25, hiroshige wrote: > Add ASSERT(!m_isInTwoPhaseRead) and ASSERT(!m_isUpdateWaitingForEndRead). > > Also, add a destructor with: > ASSERT(!m_readerThread); > ASSERT(!m_reader); > ASSERT(!m_client); > Done. https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:90: ASSERT(m_reader); On 2015/06/10 06:55:26, hiroshige wrote: > We assume obtainReader() returns non-null pointer here, but > WebDataConsumerHandle::obtainReaderInternal() returns nullptr. > So how about making WebDataConsumerHandle::obtainReaderInternal() abstract (in a > separate CL)? The implementation in WebDataConsumerHandle is not intended to be used. They are implemented just for make refactoring easy across the repository boundary. Added assertions in another CL. https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:126: if (m_isUpdateWaitingForEndRead) { On 2015/06/10 06:55:25, hiroshige wrote: > We need MutexLocker(m_mutex) here, because we should protect |m_handle| (which > can be modified on the writer thread). Done. https://codereview.chromium.org/1162043007/diff/250001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:165: m_reader = m_handle->obtainReader(m_client); On 2015/06/10 06:55:26, hiroshige wrote: > Should we post a task for didGetReadable() here because the old reader might > have returned ShouldWait and the new reader might return non-ShouldWait? Or is > it |m_handle->obtainReader()|'s responsibility to post such task if it returns > non-ShouldWait? > If the latter, we have to update DoneHandle::obtainReaderInternal(). Done.
https://codereview.chromium.org/1162043007/diff/290001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/290001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:157: m_isInTwoPhaseRead = true; We shouldn't set m_isInTwoPhaseRead if m_reader->beginRead() is not successful (in that case endRead() is never called). Patching locally: Result result = m_reader->beginRead(buffer, flags, available); if (result == Ok) m_isInTwoPhaseRead = true; return result;
https://codereview.chromium.org/1162043007/diff/290001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right): https://codereview.chromium.org/1162043007/diff/290001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandle.cpp:157: m_isInTwoPhaseRead = true; On 2015/06/10 13:39:52, hiroshige wrote: > We shouldn't set m_isInTwoPhaseRead if m_reader->beginRead() is not successful > (in that case endRead() is never called). > > Patching locally: > Result result = m_reader->beginRead(buffer, flags, available); > if (result == Ok) > m_isInTwoPhaseRead = true; > return result; Done.
https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:85: m_readingThread = adoptPtr(Platform::current()->createThread("reading thread")); nit optional: can this be in the initializer list? https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:88: String threadName() nit optional: currentThreadName() might be a better name. https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:107: m_context->recordAttach(m_name); Please take m_name.isolatedCopy() here (rather than in recordAttach()), so that all the code for thread-safe handling of |m_name| is in ReaderImpl class. https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:109: ~ReaderImpl() override { m_context->recordDetach(m_name); } ditto. https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:199: class ThreadingRegistrationDeleteReaderTest : public ThreadingTestBase { Could you add tests for the case where a reader is deleted and then a new one is created (on readingThread(), and on another thread e.g. updatingThread()) while updating? https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:257: class ThreadingDoneHandleNotificationTest : public ThreadingTestBase, public WebDataConsumerHandle::Client { Could you add a test similar to ThreadingDoneHandleNotificationTest but: void obtainReader() { m_reader = m_handle->obtainReader(this); resetReader(); postTask signalDone; } void didGetReadable() override { ASSERT_NOT_REACHED(); } i.e. reader is destructed before didGetReadable() notification and thus didGetReadable() is not called (and the test doesn't crash by use-after-free of the reader) https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:279: readingThread()->postTask(FROM_HERE, new Task(threadSafeBind(&Self::signalDone, this))); These tasks are same-thread tasks so please use bind instead of threadSafeBind(). (in these cases, bind(&Self::..., this) is sufficient, rather than bind(&Self::..., PassRefPtr<Self>(this)), because a reference to this is retained by the caller of run().) https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:307: (void)kOk; What is this line? https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:395: TEST(CompositeDataConsumerHandleTest, HangingTwoPhaseRead) Could you add a test similar to HangingTwoPhaseRead but the first beginRead() returns non-OK (and thus corresponding endRead() is not called)?
https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:291: EXPECT_EQ(kShouldWait, reader->read(buffer, sizeof(buffer), kNone, &size)); Should we check |size| for read() and beginRead(), here and CreateDoneHandle?
https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:291: EXPECT_EQ(kShouldWait, reader->read(buffer, sizeof(buffer), kNone, &size)); Should we check |size| for read() and beginRead(), here and CreateDoneHandle?
Patchset #11 (id:330001) has been deleted
Patchset #12 (id:370001) has been deleted
PS11 addresses hiroshige@'s comments. PS12 decouples "Updater" from the handle. PTAL https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... File Source/modules/fetch/CompositeDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:85: m_readingThread = adoptPtr(Platform::current()->createThread("reading thread")); On 2015/06/15 07:29:37, hiroshige wrote: > nit optional: can this be in the initializer list? Done. https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:88: String threadName() On 2015/06/15 07:29:37, hiroshige wrote: > nit optional: currentThreadName() might be a better name. Done. https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:107: m_context->recordAttach(m_name); On 2015/06/15 07:29:37, hiroshige wrote: > Please take m_name.isolatedCopy() here (rather than in recordAttach()), so that > all the code for thread-safe handling of |m_name| is in ReaderImpl class. Done. https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:109: ~ReaderImpl() override { m_context->recordDetach(m_name); } On 2015/06/15 07:29:37, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:199: class ThreadingRegistrationDeleteReaderTest : public ThreadingTestBase { On 2015/06/15 07:29:37, hiroshige wrote: > Could you add tests for the case where a reader is deleted and then a new one is > created (on readingThread(), and on another thread e.g. updatingThread()) while > updating? Added: ThreadingUpdatingReaderWhileUpdatingTest https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:257: class ThreadingDoneHandleNotificationTest : public ThreadingTestBase, public WebDataConsumerHandle::Client { On 2015/06/15 07:29:37, hiroshige wrote: > Could you add a test similar to ThreadingDoneHandleNotificationTest but: > void obtainReader() > { > m_reader = m_handle->obtainReader(this); > resetReader(); > postTask signalDone; > } > void didGetReadable() override > { > ASSERT_NOT_REACHED(); > } > > i.e. reader is destructed before didGetReadable() notification and thus > didGetReadable() is not called (and the test doesn't crash by use-after-free of > the reader) Added: ThreadingDoneHandleNoNotificationTest https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:279: readingThread()->postTask(FROM_HERE, new Task(threadSafeBind(&Self::signalDone, this))); On 2015/06/15 07:29:37, hiroshige wrote: > These tasks are same-thread tasks so please use bind instead of > threadSafeBind(). > > (in these cases, bind(&Self::..., this) is sufficient, rather than > bind(&Self::..., PassRefPtr<Self>(this)), because a reference to this is > retained by the caller of run().) Discussed offline. We use threadSafeBind for these use-cases, at least for now. https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:291: EXPECT_EQ(kShouldWait, reader->read(buffer, sizeof(buffer), kNone, &size)); On 2015/06/15 09:28:57, hiroshige wrote: > Should we check |size| for read() and beginRead(), here and CreateDoneHandle? The behavior is not specified, so we don't have expectation. https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:307: (void)kOk; On 2015/06/15 07:29:37, hiroshige wrote: > What is this line? Deleted. https://codereview.chromium.org/1162043007/diff/310001/Source/modules/fetch/C... Source/modules/fetch/CompositeDataConsumerHandleTest.cpp:395: TEST(CompositeDataConsumerHandleTest, HangingTwoPhaseRead) On 2015/06/15 07:29:37, hiroshige wrote: > Could you add a test similar to HangingTwoPhaseRead but the first beginRead() > returns non-OK (and thus corresponding endRead() is not called)? Done.
lgtm for Patch Set 11. Could you make Patch Set 12 a separate CL? (I'd like to create some CLs that depend createDone/WaitHandle() in this CL, which is independent from Patch Set 12.)
On 2015/06/16 09:18:20, hiroshige wrote: > lgtm for Patch Set 11. > > Could you make Patch Set 12 a separate CL? > (I'd like to create some CLs that depend createDone/WaitHandle() in this CL, > which is independent from Patch Set 12.) OK, I'll land PS13 which is equivalent to PS11.
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org Link to the patchset: https://codereview.chromium.org/1162043007/#ps410001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162043007/410001
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org Link to the patchset: https://codereview.chromium.org/1162043007/#ps420001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162043007/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/66781)
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162043007/420001
Message was sent while issue was closed.
Committed patchset #14 (id:420001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197174 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
