Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1176403006:
[1b] Implement FetchBlobDataConsumerHandle (Closed)
DescriptionImplement FetchBlobDataConsumerHandle
Unit tests will be added in https://codereview.chromium.org/1199043004/.
FetchInitiatorTypeNames.in is modified to use FetchInitiatorTypeNames::internal from modules/.
BUG=480746
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198220
Patch Set 1 #Patch Set 2 : Refactoring. #Patch Set 3 : Bug fixed. #Patch Set 4 : Rebase. #
Total comments: 13
Patch Set 5 : Imported. #Patch Set 6 : #
Total comments: 12
Patch Set 7 : Fix import error. #Patch Set 8 : Reflect comments. #
Total comments: 6
Patch Set 9 : #Patch Set 10 : Remove unittests. #Patch Set 11 : Comment fix #Patch Set 12 : gypi fix. #
Total comments: 4
Patch Set 13 : Remove virtual #
Total comments: 16
Patch Set 14 : Rebase. #Patch Set 15 : Update comments. #Patch Set 16 : Remove strong reference cycles. test fails due to dependencies. #Patch Set 17 : Rebase, build break fix and bug fix about postTask(). #
Total comments: 4
Patch Set 18 : Reflected comments #25. #
Messages
Total messages: 42 (9 generated)
hiroshige@chromium.org changed reviewers: + yhirano@chromium.org
PTAL (except for unittests).
Are you OK with my change at https://codereview.chromium.org/1199043004/ between PS1 and PS2? If so, can you include the diff in this CL?
> Are you OK with my change at > https://codereview.chromium.org/1199043004/ between PS1 and PS2? > If so, can you include the diff in this CL? Basically fine, I'll include it in this CL (probably together with some refactoring). https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:187: class LoaderContext { I used OwnPtr<LoaderContext> in this Patch Set. Another candidate was: - Make LoaderContext GarbageCollectedFinalized and use Member<LoaderContext> in CrossThreadHolder, - Make |m_loaderFactory| a Member, - Define LoaderContext::stop() to call m_loader->cancel(), - Call |m_obj->stop()| in CrossThreadHolder::Bridge::stop() (because it seems that we must m_loader->cancel() in ActiveDOMObject::stop(); calling it in ~BlobLoaderContext() is too late and layout tests crash with assertion failures), - Move ThreadableLoaderClient to CommonContext or a new class, because ThreadableLoaderClient cannot be GarbageCollectedFinalized. pros: - Threading restriction of LoaderContext is clearer cons: - I slightly preferred managing |m_loader| by the object lifetime rather than stop(). How do you think?
https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:48: // |executionContext| is stopped. It would be worth noting that |task| will be destructed on the calling thread in such a case. https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:156: // mutex is unlocked, but we can still use |loaderContext| here because Please wrap comments in 80 columns. Ditto for below ones. https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:187: class LoaderContext { On 2015/06/23 06:17:08, hiroshige wrote: > I used OwnPtr<LoaderContext> in this Patch Set. > > Another candidate was: > - Make LoaderContext GarbageCollectedFinalized and use Member<LoaderContext> in > CrossThreadHolder, > - Make |m_loaderFactory| a Member, > - Define LoaderContext::stop() to call m_loader->cancel(), > - Call |m_obj->stop()| in CrossThreadHolder::Bridge::stop() (because it seems > that we must m_loader->cancel() in ActiveDOMObject::stop(); calling it in > ~BlobLoaderContext() is too late and layout tests crash with assertion > failures), > - Move ThreadableLoaderClient to CommonContext or a new class, because > ThreadableLoaderClient cannot be GarbageCollectedFinalized. > > pros: > - Threading restriction of LoaderContext is clearer > > cons: > - I slightly preferred managing |m_loader| by the object lifetime rather than > stop(). > > How do you think? I don't have a strong preference at this moment, though I may change my mind after decoupling Updater from CompositeDataConsumerHandle. Can we land this class for now, and think more in July? https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:320: if (m_receivedResponse) Is just ASSERT(!m_receivedResponse) fine? Or just ASSERT_NOT_REACHED is fine? I'm not sure, but it looks unrealistic that any redirect is made for blob loading... https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:337: class FetchBlobDataConsumerHandle::ReaderContext : public ThreadSafeRefCounted<ReaderContext> { +final https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:393: NotifyOnReaderCreationHelper m_notifier; Is this member used? https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:96: visitor->trace(m_obj); Not needed https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:116: public: [optional] I like creating |static PassRefPtr<Peer> create()| and making the default constructor private. https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:238: class BlobLoaderContext This class can be final now. https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:241: WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED(LoaderContext); I assume this is oilpan related, but this class is not oilpan related. https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:351: // FetchDataConsumerHandle::Reader. s/FetchDataConsumerHandle::Reader/ReaderContext::ReaderImpl/ https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:440: // Must be called on the reader thread. [optional] It looks all functions / members are bound to the reader thread except for the following ones: - ReaderContext constructor - ReaderContext destructor - ReaderContext::obtainReader - ReaderContext::m_commonContext Removing "... must be accessed / called only on the reader thread" comments and adding the above comments at the class head makes the class a bit cleaner, I think.
https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:48: // |executionContext| is stopped. On 2015/06/23 07:32:05, yhirano wrote: > It would be worth noting that |task| will be destructed on the calling thread in > such a case. Done. https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:156: // mutex is unlocked, but we can still use |loaderContext| here because On 2015/06/23 07:32:05, yhirano wrote: > Please wrap comments in 80 columns. Ditto for below ones. Done. https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:187: class LoaderContext { On 2015/06/23 07:32:06, yhirano wrote: > On 2015/06/23 06:17:08, hiroshige wrote: > > I used OwnPtr<LoaderContext> in this Patch Set. > > > > Another candidate was: > > - Make LoaderContext GarbageCollectedFinalized and use Member<LoaderContext> > in > > CrossThreadHolder, > > - Make |m_loaderFactory| a Member, > > - Define LoaderContext::stop() to call m_loader->cancel(), > > - Call |m_obj->stop()| in CrossThreadHolder::Bridge::stop() (because it seems > > that we must m_loader->cancel() in ActiveDOMObject::stop(); calling it in > > ~BlobLoaderContext() is too late and layout tests crash with assertion > > failures), > > - Move ThreadableLoaderClient to CommonContext or a new class, because > > ThreadableLoaderClient cannot be GarbageCollectedFinalized. > > > > pros: > > - Threading restriction of LoaderContext is clearer > > > > cons: > > - I slightly preferred managing |m_loader| by the object lifetime rather than > > stop(). > > > > How do you think? > > I don't have a strong preference at this moment, though I may change my mind > after decoupling Updater from CompositeDataConsumerHandle. Can we land this > class for now, and think more in July? Fine with me. https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:320: if (m_receivedResponse) On 2015/06/23 07:32:06, yhirano wrote: > Is just ASSERT(!m_receivedResponse) fine? > > Or just ASSERT_NOT_REACHED is fine? I'm not sure, but it looks unrealistic that > any redirect is made for blob loading... Done. https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:337: class FetchBlobDataConsumerHandle::ReaderContext : public ThreadSafeRefCounted<ReaderContext> { On 2015/06/23 07:32:06, yhirano wrote: > +final Done. https://codereview.chromium.org/1176403006/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:393: NotifyOnReaderCreationHelper m_notifier; On 2015/06/23 07:32:05, yhirano wrote: > Is this member used? Yes, just having as a member and its initialization at constructor has an effect (calling didGetReadable() asynchronously). https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:96: visitor->trace(m_obj); On 2015/06/23 07:32:06, yhirano wrote: > Not needed Done. https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:116: public: On 2015/06/23 07:32:06, yhirano wrote: > [optional] I like creating |static PassRefPtr<Peer> create()| and making the > default constructor private. Done. https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:238: class BlobLoaderContext On 2015/06/23 07:32:06, yhirano wrote: > This class can be final now. Done. https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:241: WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED(LoaderContext); On 2015/06/23 07:32:06, yhirano wrote: > I assume this is oilpan related, but this class is not oilpan related. Done. https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:351: // FetchDataConsumerHandle::Reader. On 2015/06/23 07:32:06, yhirano wrote: > s/FetchDataConsumerHandle::Reader/ReaderContext::ReaderImpl/ Done. https://codereview.chromium.org/1176403006/diff/90001/Source/modules/fetch/Fe... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:440: // Must be called on the reader thread. On 2015/06/23 07:32:06, yhirano wrote: > [optional] It looks all functions / members are bound to the reader thread > except for the following ones: > > - ReaderContext constructor > - ReaderContext destructor > - ReaderContext::obtainReader > - ReaderContext::m_commonContext > > Removing "... must be accessed / called only on the reader thread" comments and > adding the above comments at the class head makes the class a bit cleaner, I > think. Done.
https://codereview.chromium.org/1176403006/diff/130001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/130001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:333: Persistent<FetchBlobDataConsumerHandle::LoaderFactory> m_loaderFactory; I made this Persistent rather than CrossThreadPersistent because LoaderContext is bound to the loader thread.
https://codereview.chromium.org/1176403006/diff/130001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.h (right): https://codereview.chromium.org/1176403006/diff/130001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.h:20: public: You need to declare the empty destructor in the header file and define it in the source file, because ReaderContext is incomplete. https://codereview.chromium.org/1176403006/diff/130001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.h:21: class LoaderFactory : public GarbageCollected<LoaderFactory> { Sorry, I need GarbageCollectedFinalized, because gmock requires it. Can you fix it?
https://codereview.chromium.org/1176403006/diff/130001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.h (right): https://codereview.chromium.org/1176403006/diff/130001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.h:20: public: On 2015/06/23 08:44:08, yhirano wrote: > You need to declare the empty destructor in the header file and define it in the > source file, because ReaderContext is incomplete. Done. https://codereview.chromium.org/1176403006/diff/130001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.h:21: class LoaderFactory : public GarbageCollected<LoaderFactory> { On 2015/06/23 08:44:08, yhirano wrote: > Sorry, I need GarbageCollectedFinalized, because gmock requires it. Can you fix > it? Done.
hiroshige@chromium.org changed reviewers: + haraken@chromium.org
+haraken@, could you take a look at CrossThreadHolder, especially the use of CrossThreadPersistent?
LGTM I'm adding tests in https://codereview.chromium.org/1199043004/ and I found no defects of this implementation. https://codereview.chromium.org/1176403006/diff/130001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/130001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:333: Persistent<FetchBlobDataConsumerHandle::LoaderFactory> m_loaderFactory; On 2015/06/23 08:38:59, hiroshige wrote: > I made this Persistent rather than CrossThreadPersistent because LoaderContext > is bound to the loader thread. Acknowledged. https://codereview.chromium.org/1176403006/diff/210001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/210001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:278: virtual PassRefPtr<ThreadableLoader> createLoader(ExecutionContext* executionContext, ThreadableLoaderClient* client) const -virtual? https://codereview.chromium.org/1176403006/diff/210001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.h (right): https://codereview.chromium.org/1176403006/diff/210001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.h:39: virtual const char* debugName() const override { return "FetchBlobDataConsumerHandle"; } No virtual is needed
https://codereview.chromium.org/1176403006/diff/210001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/210001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:278: virtual PassRefPtr<ThreadableLoader> createLoader(ExecutionContext* executionContext, ThreadableLoaderClient* client) const On 2015/06/24 11:15:14, yhirano wrote: > -virtual? Done. https://codereview.chromium.org/1176403006/diff/210001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.h (right): https://codereview.chromium.org/1176403006/diff/210001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.h:39: virtual const char* debugName() const override { return "FetchBlobDataConsumerHandle"; } On 2015/06/24 11:15:14, yhirano wrote: > No virtual is needed Done.
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org
https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:70: // T <-OwnPtr- |Bridge| ---------RefPtr--------> |Peer| <-(RefPtr)- CTH Is it guaranteed that Peer is allocated on a thread that is attached to Oilpan? It's sometimes problematic to use CrossThreadPersistent from a thread that is not attached to Oilpan. Also it is guaranteed that Bridge is created and destructed in the same thread, and that the CrossThreadPersistent does not outlive the thread, right? Oilpan does not support an object that outlive the thread that created the object. https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:77: // [2] or when CrossThreadHolder is destructed: It seems that CrossThreadHolder must be always explicitly destructed. If there is a class like: class X { // Expecting that m_holder is cleared in ~X() OwnPtr<CrossThreadHolder> m_holder; }; then it will cause a leak. Maybe it's worth having a comment and mention that CrossThreadHolder must be always explicitly cleared. Also it would be a good idea to support CrossThreadHolder in the leak detector (not in this CL but in a follow-up CL). It looks easy to leak :)
Thanks for reviewing! https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:70: // T <-OwnPtr- |Bridge| ---------RefPtr--------> |Peer| <-(RefPtr)- CTH On 2015/06/24 13:37:53, haraken wrote: > > Is it guaranteed that Peer is allocated on a thread that is attached to Oilpan? Yes, allocated on threads attached to Oilpan (main thread, workers, etc. added comments). But Peer and its CrossThreadPersistent can be accessed and cleared from any other thread (according to the current WebDataConsumerHandle design). Is this problematic? > Also it is guaranteed that Bridge is created and destructed in the same thread, > and that the CrossThreadPersistent does not outlive the thread, right? CrossThreadPersistent::clear() is called in Bridge::stop() so the cross-thread reference to Bridge does not outlive the thread. (CrossThreadPersistent object itself remains after that with nullptr) https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:77: // [2] or when CrossThreadHolder is destructed: On 2015/06/24 13:37:53, haraken wrote: > > It seems that CrossThreadHolder must be always explicitly destructed. If there > is a class like: > > class X { // Expecting that m_holder is cleared in ~X() > OwnPtr<CrossThreadHolder> m_holder; > }; > > then it will cause a leak. Could you explain what is leaking (and how)? I expected class X works (|m_holder| is cleared in ~X(), CrossThreadHolder is destructed and everything is cleared as in [2]). I updated the comments.
https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:70: // T <-OwnPtr- |Bridge| ---------RefPtr--------> |Peer| <-(RefPtr)- CTH On 2015/06/25 06:25:37, hiroshige wrote: > On 2015/06/24 13:37:53, haraken wrote: > > > > Is it guaranteed that Peer is allocated on a thread that is attached to > Oilpan? > Yes, allocated on threads attached to Oilpan (main thread, workers, etc. added > comments). > But Peer and its CrossThreadPersistent can be accessed and cleared from any > other thread (according to the current WebDataConsumerHandle design). Is this > problematic? Just to clarify: Who allocates the Bridge and the Peer and who uses the Bridge and the Peer? > > > Also it is guaranteed that Bridge is created and destructed in the same > thread, > > and that the CrossThreadPersistent does not outlive the thread, right? > CrossThreadPersistent::clear() is called in Bridge::stop() so the cross-thread > reference to Bridge does not outlive the thread. > (CrossThreadPersistent object itself remains after that with nullptr) Sounds good. https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:77: // [2] or when CrossThreadHolder is destructed: On 2015/06/25 06:25:37, hiroshige wrote: > On 2015/06/24 13:37:53, haraken wrote: > > > > It seems that CrossThreadHolder must be always explicitly destructed. If there > > is a class like: > > > > class X { // Expecting that m_holder is cleared in ~X() > > OwnPtr<CrossThreadHolder> m_holder; > > }; > > > > then it will cause a leak. > Could you explain what is leaking (and how)? > I expected class X works (|m_holder| is cleared in ~X(), CrossThreadHolder is > destructed and everything is cleared as in [2]). > I updated the comments. My point is that the CrossThreadPersistent must be explicitly cleared without relying on some destructor. In other words, the reference cycle must be explicitly cleared without relying on some destructor, because the destructor may not be called because of the reference cycle. Otherwise, we have a risk of leaking the reference cycle. You're calling m_peer->clear() (which clears the CrossThreadPersistent) in ~CrossThreadHolder(). Thus we must make sure that ~CrossThreadHolder() is explicitly called from somewhere other than destructors.
https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:77: // [2] or when CrossThreadHolder is destructed: On 2015/06/26 00:15:08, haraken wrote: > On 2015/06/25 06:25:37, hiroshige wrote: > > On 2015/06/24 13:37:53, haraken wrote: > > > > > > It seems that CrossThreadHolder must be always explicitly destructed. If > there > > > is a class like: > > > > > > class X { // Expecting that m_holder is cleared in ~X() > > > OwnPtr<CrossThreadHolder> m_holder; > > > }; > > > > > > then it will cause a leak. > > Could you explain what is leaking (and how)? > > I expected class X works (|m_holder| is cleared in ~X(), CrossThreadHolder is > > destructed and everything is cleared as in [2]). > > I updated the comments. > > My point is that the CrossThreadPersistent must be explicitly cleared without > relying on some destructor. In other words, the reference cycle must be > explicitly cleared without relying on some destructor, because the destructor > may not be called because of the reference cycle. Otherwise, we have a risk of > leaking the reference cycle. > > You're calling m_peer->clear() (which clears the CrossThreadPersistent) in > ~CrossThreadHolder(). Thus we must make sure that ~CrossThreadHolder() is > explicitly called from somewhere other than destructors. CrossThreadHolder is owned by CommonContext, so it is called when clearLoaderContext is called.
https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:70: // T <-OwnPtr- |Bridge| ---------RefPtr--------> |Peer| <-(RefPtr)- CTH > Just to clarify: Who allocates the Bridge and the Peer and who uses the Bridge > and the Peer? CrossThreadHolder. Bridge and Peer are internal classes of CrossThreadHolder and accessed via CrossThreadHolder. https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:77: // [2] or when CrossThreadHolder is destructed: > My point is that the CrossThreadPersistent must be explicitly cleared without > relying on some destructor. In other words, the reference cycle must be > explicitly cleared without relying on some destructor, because the destructor > may not be called because of the reference cycle. Otherwise, we have a risk of > leaking the reference cycle. Understood, thanks. I think we can depend destructor outside the reference cycle to clear the cycle. So the comment should be "If T has (direct or indirect) references to CrossThreadHolder, it forms another reference cycle that should be explicitly cleared"? And this is the case here; Comments around L183 shows the outer reference cycle which is cleared by clearLoaderContext(): > CrossThreadHolder is owned by CommonContext, so it is called when > clearLoaderContext is called. and clearLoaderContext() is called when: a: Loading finished, or b: ReaderContext is destructed. So it still depends on destructor, but I expect we don't have references from inside reference cycles to ReaderContext. I agree to have leak detection to check the expectation above. How about inserting leak detection to Peer (for the inner reference cycle) and CommonContext (for the outer reference cycle)? Those classes are expected to be destructed at last in each of the reference cycles.
https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:70: // T <-OwnPtr- |Bridge| ---------RefPtr--------> |Peer| <-(RefPtr)- CTH On 2015/06/26 03:07:56, hiroshige wrote: > > Just to clarify: Who allocates the Bridge and the Peer and who uses the Bridge > > and the Peer? > CrossThreadHolder. Bridge and Peer are internal classes of CrossThreadHolder and > accessed via CrossThreadHolder. Thanks, understood. - The thread that allocates the CrossThreadHandler (and Bridge and Peer) is guaranteed to be attached to Oilpan. - Is it guaranteed that the thread that uses the Bridge and the Peer via CrossTheadHandler is attached to Oilpan? If the answer is no, it is problematic because a non-oilpan thread should not touch an on-heap object. (For example, it can happen that the non-oilpan thread touches the on-heap object while Oilpan is doing a GC (where all oilpan threads are stopped).) https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:77: // [2] or when CrossThreadHolder is destructed: On 2015/06/26 03:07:57, hiroshige wrote: > > My point is that the CrossThreadPersistent must be explicitly cleared without > > relying on some destructor. In other words, the reference cycle must be > > explicitly cleared without relying on some destructor, because the destructor > > may not be called because of the reference cycle. Otherwise, we have a risk of > > leaking the reference cycle. > Understood, thanks. > I think we can depend destructor outside the reference cycle to clear the cycle. > So the comment should be "If T has (direct or indirect) references to > CrossThreadHolder, it forms another reference cycle that should be explicitly > cleared"? > > And this is the case here; > Comments around L183 shows the outer reference cycle which is cleared by > clearLoaderContext(): > > CrossThreadHolder is owned by CommonContext, so it is called when > > clearLoaderContext is called. > and clearLoaderContext() is called when: > a: Loading finished, or > b: ReaderContext is destructed. > So it still depends on destructor, but > I expect we don't have references from inside reference cycles to ReaderContext. > > I agree to have leak detection to check the expectation above. > How about inserting leak detection to Peer (for the inner reference cycle) and > CommonContext (for the outer reference cycle)? > Those classes are expected to be destructed at last in each of the reference > cycles. Hmm, this sounds a bit concerning. How is it guaranteed that the following scenario doesn't happen? - An object X calls clearLoaderContext() in ~X(). - The object X is reachable from the reference cycle. If this happen, we will have a memory leak. That's why I'm saying that clearLoaderContext() must not depend on any destructor.
https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:77: // [2] or when CrossThreadHolder is destructed: On 2015/06/26 03:57:36, haraken wrote: > On 2015/06/26 03:07:57, hiroshige wrote: > > > My point is that the CrossThreadPersistent must be explicitly cleared > without > > > relying on some destructor. In other words, the reference cycle must be > > > explicitly cleared without relying on some destructor, because the > destructor > > > may not be called because of the reference cycle. Otherwise, we have a risk > of > > > leaking the reference cycle. > > Understood, thanks. > > I think we can depend destructor outside the reference cycle to clear the > cycle. > > So the comment should be "If T has (direct or indirect) references to > > CrossThreadHolder, it forms another reference cycle that should be explicitly > > cleared"? > > > > And this is the case here; > > Comments around L183 shows the outer reference cycle which is cleared by > > clearLoaderContext(): > > > CrossThreadHolder is owned by CommonContext, so it is called when > > > clearLoaderContext is called. > > and clearLoaderContext() is called when: > > a: Loading finished, or > > b: ReaderContext is destructed. > > So it still depends on destructor, but > > I expect we don't have references from inside reference cycles to > ReaderContext. > > > > I agree to have leak detection to check the expectation above. > > How about inserting leak detection to Peer (for the inner reference cycle) and > > CommonContext (for the outer reference cycle)? > > Those classes are expected to be destructed at last in each of the reference > > cycles. > > Hmm, this sounds a bit concerning. How is it guaranteed that the following > scenario doesn't happen? > > - An object X calls clearLoaderContext() in ~X(). > - The object X is reachable from the reference cycle. > > If this happen, we will have a memory leak. That's why I'm saying that > clearLoaderContext() must not depend on any destructor. > Thanks, I didn't know that. As currently all users are on oilpan-enabled threads, I would add a restriction: https://codereview.chromium.org/1214613003/. However I hope we will be able to get rid of it.
https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:70: // T <-OwnPtr- |Bridge| ---------RefPtr--------> |Peer| <-(RefPtr)- CTH > a non-oilpan thread should not touch an on-heap object. Handled in yhirano@'s comment. For clarification: we should not do all of the following from non-Oilpan threads? - Calling CrossThreadPersistent::clear(). - Destructing CrossThreadPersistent. - Dereferencing CrossThreadPersistent. https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:77: // [2] or when CrossThreadHolder is destructed: > Hmm, this sounds a bit concerning. How is it guaranteed that the following > scenario doesn't happen? > > - An object X calls clearLoaderContext() in ~X(). > - The object X is reachable from the reference cycle. I think this is responsibility of the users (owners) of FetchBlobDataConsumerHandle. The object X is always ReaderContext (because CommonContext is internal class in this .cpp). A reference cycle is created if: ThreadableLoader contains an indirect reference to the owner of FetchBlobDataConsumerHandle or its reader. FetchBlobDataConsumerHandle conceptually creates ThreadableLoader that loads data from a Blob. So if the ThreadableLoader has an inderect reference to the owner of FetchBlobDataConsumerHandle, then there would be a reference cycle regardless of the implementation of FetchBlobDataConsumerHandle.
https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:70: // T <-OwnPtr- |Bridge| ---------RefPtr--------> |Peer| <-(RefPtr)- CTH > For clarification: we should not do all of the following from non-Oilpan > threads? > - Calling CrossThreadPersistent::clear(). This is currently not safe but is easy to make it safe. (We need to add locks to all CrossThreadPersistent->m_raw accesses.) > - Destructing CrossThreadPersistent. This is safe. > - Dereferencing CrossThreadPersistent. This is unsafe, because it touches an on-heap object. https://codereview.chromium.org/1176403006/diff/220001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:77: // [2] or when CrossThreadHolder is destructed: On 2015/06/26 06:52:29, hiroshige wrote: > > Hmm, this sounds a bit concerning. How is it guaranteed that the following > > scenario doesn't happen? > > > > - An object X calls clearLoaderContext() in ~X(). > > - The object X is reachable from the reference cycle. > > I think this is responsibility of the users (owners) of > FetchBlobDataConsumerHandle. > > The object X is always ReaderContext (because CommonContext is internal class in > this .cpp). > A reference cycle is created if: > ThreadableLoader contains an indirect reference to the owner of > FetchBlobDataConsumerHandle or its reader. > > FetchBlobDataConsumerHandle conceptually creates ThreadableLoader that loads > data from a Blob. > So if the ThreadableLoader has an inderect reference to the owner of > FetchBlobDataConsumerHandle, then there would be a reference cycle regardless of > the implementation of FetchBlobDataConsumerHandle. My concern is that it's hard to reason about the correctness. Would it be hard to design things so that there is an explicit point that calls clearLoaderContext()? For example, Document::detach() is explicitly called without relying on any destructor.
According to the offline discussion, - The reference cycle between Bridge-Peer is removed by replacing RefPtr from Bridge to Peer with a raw pointer. - Also I removed the outer reference cycle of CommonContext->CrossThreadHolder->LoaderContext->CommonContext because it might cause deadlocks (not only leaks). - Merged CrossThreadHolder::Peer into CrossThreadHolder, and merged CommonContext into ReaderContext, because separate objects are no longer needed.
Still LGTM https://codereview.chromium.org/1176403006/diff/300001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/300001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:31: // bounded to the thread of |executionContext| where |obj| is created. bound to https://codereview.chromium.org/1176403006/diff/300001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:146: m_holder->clearInternal(); ASSERT(!m_holder);
haraken@, thanks you for useful comments at the previous offline discussion, and I removed strong reference cycles in Patch Set 18. Could you take a look at again? https://codereview.chromium.org/1176403006/diff/300001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right): https://codereview.chromium.org/1176403006/diff/300001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:31: // bounded to the thread of |executionContext| where |obj| is created. On 2015/07/01 11:27:50, yhirano wrote: > bound to Done. https://codereview.chromium.org/1176403006/diff/300001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:146: m_holder->clearInternal(); On 2015/07/01 11:27:50, yhirano wrote: > ASSERT(!m_holder); Done.
In terms oilpan, LGTM. (I feel that the threading design is unacceptably complicated, but I'm not understanding things enough to comment on the design.)
On 2015/07/02 10:52:45, haraken wrote: > In terms oilpan, LGTM. > > (I feel that the threading design is unacceptably complicated, but I'm not > understanding things enough to comment on the design.) Thanks for reviewing! I hope we can make the threading design better in the future.
hiroshige@chromium.org changed reviewers: + mkwst@chromium.org
+mkwst@, could you take a look at the change in FetchInitiatorTypeNames.in?
Adding the export is fine. LGTM. The windows compilation error looks legit, though.
Thanks! > The windows compilation error looks legit, though. This will be resolved by a separate CL: https://codereview.chromium.org/1222663002/.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1176403006/#ps320001 (title: "Reflected comments #25.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1176403006/320001
The CQ bit was unchecked by hiroshige@chromium.org
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1176403006/320001
Message was sent while issue was closed.
Committed patchset #18 (id:320001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198220
Message was sent while issue was closed.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Message was sent while issue was closed.
This might have broken cast_shell_linux: http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu... The failing test is content_browsertests --gtest_filter=IndexedDBBrowserTestWithGCExposed.BlobDidAck [ RUN ] IndexedDBBrowserTestWithGCExposed.BlobDidAck [32019:32019:0702/100121:9802764899:INFO:audio_manager_pulse.cc(258)] Failed to connect to the context. Error: Connection refused [32019:32019:0702/100121:9802768882:INFO:indexed_db_browsertest.cc(78)] Navigating to URL and blocking. [32019:32019:0702/100121:9802968295:INFO:indexed_db_browsertest.cc(80)] Navigation done. ../../content/browser/indexed_db/indexed_db_browsertest.cc:451: Failure Value of: blob_context->context()->blob_count() Actual: 1 Expected: 0UL Which is: 0 [ FAILED ] IndexedDBBrowserTestWithGCExposed.BlobDidAck, where TypeParam = and GetParam() = (268 ms)
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:320001) has been created in https://codereview.chromium.org/1207273003/ by dcheng@chromium.org. The reason for reverting is: May have broken IndexedDBBrowserTestWithGCExposed.BlobDidAck.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
