Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1749633002:
Remove StopDetector (Closed)
DescriptionRemove StopDetector
By making FetchManager a ContextLifecycleObserver, we no longer need the StopDetector.
This CL also makes the following changes:
- FetchManager::Loader doesn't need to be a ContextLifecycleObserver.
FetchManager::Loader should just hold RawPtrWillBeMember<ExecutionContext>.
- FetchManager::Loader should have a pre-finalizer so that it calls m_loader->cancel()
before starting lazy sweeping.
- Mark SRIVerifier as eagerly-finalized to promptly clear m_handle and m_reader.
BUG=589507
Committed: https://crrev.com/f885b41b4d4d00374cc29e62962634681b18c218
Cr-Commit-Position: refs/heads/master@{#378969}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 14
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 1
Patch Set 8 : #
Total comments: 5
Patch Set 9 : #Patch Set 10 : #
Messages
Total messages: 40 (12 generated)
haraken@chromium.org changed reviewers: + yhirano@chromium.org
PTAL
haraken@chromium.org changed reviewers: + sigbjornf@opera.com
(cc Sigbjorn for sanity checking)
The CQ bit was checked by haraken@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749633002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749633002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:687: if (executionContext()) In the !OILPAN case, FetchManager will now be eagerly finalized (as all lifecycle observers are.) Loaders already are ContextLifecycleObservers & eagerly finalized, so combining the two isn't needed. i.e., this is a no-op (and potentially harmful.) https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.h (left): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.h:23: ~FetchManager(); I think you need to use "override" in the !OILPAN case. https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.h (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.h:22: explicit FetchManager(ExecutionContext*); nit: can remain "private"?
https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:694: if (!executionContext()) ASSERT(executionContext()) looks better. https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.h (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.h:22: explicit FetchManager(ExecutionContext*); On 2016/02/29 13:14:12, sof wrote: > nit: can remain "private"? [optional] Perhaps you can delete create function.
https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:687: if (executionContext()) On 2016/02/29 13:14:12, sof wrote: > In the !OILPAN case, FetchManager will now be eagerly finalized (as all > lifecycle observers are.) Loaders already are ContextLifecycleObservers & > eagerly finalized, so combining the two isn't needed. i.e., this is a no-op (and > potentially harmful.) > Hmm. I'm getting confused about this. - If we remove this code, who calls loader->dispose() when the FetchManager gets destructed? - I begin to wonder why we don't need to call loader->dispose() in ENABLE(OILPAN). Maybe do we need to add a pre-finalizer to Loader and call Loader::dispose()? - Why does the Loader need to be ContextLifecycleObserver? Can we make the Loader hold RefPtrWillBeMember<ExecutionContext>? I don't think using RefPtr causes a leak. https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:694: if (!executionContext()) On 2016/02/29 18:56:51, yhirano wrote: > ASSERT(executionContext()) looks better. I can, but how is it guaranteed that executionContext is non-null here?
https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:694: if (!executionContext()) On 2016/03/01 00:18:04, haraken wrote: > On 2016/02/29 18:56:51, yhirano wrote: > > ASSERT(executionContext()) looks better. > > I can, but how is it guaranteed that executionContext is non-null here? The only caller is GlobalFetch::fetch and it's checking that the manager have an execution context.
https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:687: if (executionContext()) On 2016/03/01 00:18:04, haraken wrote: > On 2016/02/29 13:14:12, sof wrote: > > In the !OILPAN case, FetchManager will now be eagerly finalized (as all > > lifecycle observers are.) Loaders already are ContextLifecycleObservers & > > eagerly finalized, so combining the two isn't needed. i.e., this is a no-op > (and > > potentially harmful.) > > > > Hmm. I'm getting confused about this. > > - If we remove this code, who calls loader->dispose() when the FetchManager gets > destructed? > Loader dies at the same time as FetchManager. Loader is a ContextLifeCycleObserver. LifecycleObserver is annotated with EAGERLY_FINALIZE_WILL_BE_REMOVED(). => the loader will be finalized (=> disposed) as soon as possible.
https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:687: if (executionContext()) On 2016/03/01 06:13:01, sof wrote: > On 2016/03/01 00:18:04, haraken wrote: > > On 2016/02/29 13:14:12, sof wrote: > > > In the !OILPAN case, FetchManager will now be eagerly finalized (as all > > > lifecycle observers are.) Loaders already are ContextLifecycleObservers & > > > eagerly finalized, so combining the two isn't needed. i.e., this is a no-op > > (and > > > potentially harmful.) > > > > > > > Hmm. I'm getting confused about this. > > > > - If we remove this code, who calls loader->dispose() when the FetchManager > gets > > destructed? > > > > Loader dies at the same time as FetchManager. Loader is a > ContextLifeCycleObserver. LifecycleObserver is annotated with > EAGERLY_FINALIZE_WILL_BE_REMOVED(). > > => the loader will be finalized (=> disposed) as soon as possible. In oilpan, who calls m_loader->cancel() when the Loader and the FetchManager dies? Not calling m_loader->cancel() looks like a problem in oilpan since the Loader object isn't eagerly collected.
https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:687: if (executionContext()) On 2016/03/01 08:27:10, haraken wrote: > On 2016/03/01 06:13:01, sof wrote: > > On 2016/03/01 00:18:04, haraken wrote: > > > On 2016/02/29 13:14:12, sof wrote: > > > > In the !OILPAN case, FetchManager will now be eagerly finalized (as all > > > > lifecycle observers are.) Loaders already are ContextLifecycleObservers & > > > > eagerly finalized, so combining the two isn't needed. i.e., this is a > no-op > > > (and > > > > potentially harmful.) > > > > > > > > > > Hmm. I'm getting confused about this. > > > > > > - If we remove this code, who calls loader->dispose() when the FetchManager > > gets > > > destructed? > > > > > > > Loader dies at the same time as FetchManager. Loader is a > > ContextLifeCycleObserver. LifecycleObserver is annotated with > > EAGERLY_FINALIZE_WILL_BE_REMOVED(). > > > > => the loader will be finalized (=> disposed) as soon as possible. > > In oilpan, who calls m_loader->cancel() when the Loader and the FetchManager > dies? Not calling m_loader->cancel() looks like a problem in oilpan since the > Loader object isn't eagerly collected. Agreed, promptly de-ref'ing and cancelling m_loader is needed to stay safe w/Oilpan. Additionally, Loader::SRIVerifier either needs to be explicitly disposed of during Loader finalization, or be marked as eagerly finalized.
https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:7: #include "core/dom/ContextLifecycleObserver.h" I this line needed?
Description was changed from ========== Remove StopDetector By making FetchManager a ContextLifecycleObserver, we no longer need the StopDetector. BUG=589507 ========== to ========== Remove StopDetector By making FetchManager a ContextLifecycleObserver, we no longer need the StopDetector. This CL also makes the following changes: - FetchManager::Loader doesn't need to be a ContextLifecycleObserver. FetchManager::Loader should just hold RawPtrWillBeMember<ExecutionContext>. - FetchManager::Loader should have a pre-finalizer so that it calls m_loader->cancel() before starting lazy sweeping. BUG=589507 ==========
PTAL https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:687: if (executionContext()) On 2016/03/01 10:22:50, sof wrote: > On 2016/03/01 08:27:10, haraken wrote: > > On 2016/03/01 06:13:01, sof wrote: > > > On 2016/03/01 00:18:04, haraken wrote: > > > > On 2016/02/29 13:14:12, sof wrote: > > > > > In the !OILPAN case, FetchManager will now be eagerly finalized (as all > > > > > lifecycle observers are.) Loaders already are ContextLifecycleObservers > & > > > > > eagerly finalized, so combining the two isn't needed. i.e., this is a > > no-op > > > > (and > > > > > potentially harmful.) > > > > > > > > > > > > > Hmm. I'm getting confused about this. > > > > > > > > - If we remove this code, who calls loader->dispose() when the > FetchManager > > > gets > > > > destructed? > > > > > > > > > > Loader dies at the same time as FetchManager. Loader is a > > > ContextLifeCycleObserver. LifecycleObserver is annotated with > > > EAGERLY_FINALIZE_WILL_BE_REMOVED(). > > > > > > => the loader will be finalized (=> disposed) as soon as possible. > > > > In oilpan, who calls m_loader->cancel() when the Loader and the FetchManager > > dies? Not calling m_loader->cancel() looks like a problem in oilpan since the > > Loader object isn't eagerly collected. > > Agreed, promptly de-ref'ing and cancelling m_loader is needed to stay safe > w/Oilpan. Done. > Additionally, Loader::SRIVerifier either needs to be explicitly disposed of > during Loader finalization, or be marked as eagerly finalized. Would you elaborate on why it needs to be marked as eagerly finalized? https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp (right): https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:7: #include "core/dom/ContextLifecycleObserver.h" On 2016/03/01 17:45:50, yhirano wrote: > I this line needed? Done.
On 2016/03/02 08:54:14, haraken wrote: > PTAL > > https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): > > https://codereview.chromium.org/1749633002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/fetch/FetchManager.cpp:687: if > (executionContext()) > On 2016/03/01 10:22:50, sof wrote: > > On 2016/03/01 08:27:10, haraken wrote: > > > On 2016/03/01 06:13:01, sof wrote: > > > > On 2016/03/01 00:18:04, haraken wrote: > > > > > On 2016/02/29 13:14:12, sof wrote: > > > > > > In the !OILPAN case, FetchManager will now be eagerly finalized (as > all > > > > > > lifecycle observers are.) Loaders already are > ContextLifecycleObservers > > & > > > > > > eagerly finalized, so combining the two isn't needed. i.e., this is a > > > no-op > > > > > (and > > > > > > potentially harmful.) > > > > > > > > > > > > > > > > Hmm. I'm getting confused about this. > > > > > > > > > > - If we remove this code, who calls loader->dispose() when the > > FetchManager > > > > gets > > > > > destructed? > > > > > > > > > > > > > Loader dies at the same time as FetchManager. Loader is a > > > > ContextLifeCycleObserver. LifecycleObserver is annotated with > > > > EAGERLY_FINALIZE_WILL_BE_REMOVED(). > > > > > > > > => the loader will be finalized (=> disposed) as soon as possible. > > > > > > In oilpan, who calls m_loader->cancel() when the Loader and the FetchManager > > > dies? Not calling m_loader->cancel() looks like a problem in oilpan since > the > > > Loader object isn't eagerly collected. > > > > Agreed, promptly de-ref'ing and cancelling m_loader is needed to stay safe > > w/Oilpan. > > Done. > > > Additionally, Loader::SRIVerifier either needs to be explicitly disposed of > > during Loader finalization, or be marked as eagerly finalized. > > Would you elaborate on why it needs to be marked as eagerly finalized? > In a meeting atm, but it needs to be for the same reasons as https://codereview.chromium.org/1714613003 , I reckon.
Description was changed from ========== Remove StopDetector By making FetchManager a ContextLifecycleObserver, we no longer need the StopDetector. This CL also makes the following changes: - FetchManager::Loader doesn't need to be a ContextLifecycleObserver. FetchManager::Loader should just hold RawPtrWillBeMember<ExecutionContext>. - FetchManager::Loader should have a pre-finalizer so that it calls m_loader->cancel() before starting lazy sweeping. BUG=589507 ========== to ========== Remove StopDetector By making FetchManager a ContextLifecycleObserver, we no longer need the StopDetector. This CL also makes the following changes: - FetchManager::Loader doesn't need to be a ContextLifecycleObserver. FetchManager::Loader should just hold RawPtrWillBeMember<ExecutionContext>. - FetchManager::Loader should have a pre-finalizer so that it calls m_loader->cancel() before starting lazy sweeping. - Mark SRIVerifier as eagerly-finalized to promptly clear m_handle and m_reader. BUG=589507 ==========
https://codereview.chromium.org/1749633002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp (right): https://codereview.chromium.org/1749633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:40: if (!m_fetchManager->executionContext()) { This isn't equal to what was; is it safe to issue new fetch()es after contextDestroyed() has run (but the next GC hasn't)?
On 2016/03/02 12:52:10, sof wrote: > https://codereview.chromium.org/1749633002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp (right): > > https://codereview.chromium.org/1749633002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:40: if > (!m_fetchManager->executionContext()) { > This isn't equal to what was; is it safe to issue new fetch()es after > contextDestroyed() has run (but the next GC hasn't)? Yeah, you're right. Reverted the part that takes care of m_isStopped, unfortunately. It would be great if we could land https://codereview.chromium.org/1752693002 soon and remove the m_isStopped hack.
https://codereview.chromium.org/1749633002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1749633002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:59: EAGERLY_FINALIZE(); Add this to SRIVerifier below also? https://codereview.chromium.org/1749633002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp (right): https://codereview.chromium.org/1749633002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:40: if (!m_fetchManager->isStopped()) { Remove "!"
On 2016/03/02 15:29:22, haraken wrote: > On 2016/03/02 12:52:10, sof wrote: > > > https://codereview.chromium.org/1749633002/diff/120001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp (right): > > > > > https://codereview.chromium.org/1749633002/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:40: if > > (!m_fetchManager->executionContext()) { > > This isn't equal to what was; is it safe to issue new fetch()es after > > contextDestroyed() has run (but the next GC hasn't)? > > Yeah, you're right. Reverted the part that takes care of m_isStopped, > unfortunately. It would be great if we could land > https://codereview.chromium.org/1752693002 soon and remove the m_isStopped hack. Given what we now have become aware of, should https://codereview.chromium.org/1744133002/ be reverted?
https://codereview.chromium.org/1749633002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1749633002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:57: USING_PRE_FINALIZER(FetchManager::Loader, dispose); Missed this, but eagerly finalized and prefinalized?
https://codereview.chromium.org/1749633002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1749633002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchManager.cpp:59: EAGERLY_FINALIZE(); On 2016/03/02 15:37:15, sof wrote: > Add this to SRIVerifier below also? Oh, I thought I've added this to SRIVerifier. Fixed. https://codereview.chromium.org/1749633002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp (right): https://codereview.chromium.org/1749633002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:40: if (!m_fetchManager->isStopped()) { On 2016/03/02 15:37:15, sof wrote: > Remove "!" Done.
lgtm
yhirano@: PTAL
lgtm
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749633002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749633002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1749633002/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749633002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749633002/180001
Message was sent while issue was closed.
Description was changed from ========== Remove StopDetector By making FetchManager a ContextLifecycleObserver, we no longer need the StopDetector. This CL also makes the following changes: - FetchManager::Loader doesn't need to be a ContextLifecycleObserver. FetchManager::Loader should just hold RawPtrWillBeMember<ExecutionContext>. - FetchManager::Loader should have a pre-finalizer so that it calls m_loader->cancel() before starting lazy sweeping. - Mark SRIVerifier as eagerly-finalized to promptly clear m_handle and m_reader. BUG=589507 ========== to ========== Remove StopDetector By making FetchManager a ContextLifecycleObserver, we no longer need the StopDetector. This CL also makes the following changes: - FetchManager::Loader doesn't need to be a ContextLifecycleObserver. FetchManager::Loader should just hold RawPtrWillBeMember<ExecutionContext>. - FetchManager::Loader should have a pre-finalizer so that it calls m_loader->cancel() before starting lazy sweeping. - Mark SRIVerifier as eagerly-finalized to promptly clear m_handle and m_reader. BUG=589507 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Remove StopDetector By making FetchManager a ContextLifecycleObserver, we no longer need the StopDetector. This CL also makes the following changes: - FetchManager::Loader doesn't need to be a ContextLifecycleObserver. FetchManager::Loader should just hold RawPtrWillBeMember<ExecutionContext>. - FetchManager::Loader should have a pre-finalizer so that it calls m_loader->cancel() before starting lazy sweeping. - Mark SRIVerifier as eagerly-finalized to promptly clear m_handle and m_reader. BUG=589507 ========== to ========== Remove StopDetector By making FetchManager a ContextLifecycleObserver, we no longer need the StopDetector. This CL also makes the following changes: - FetchManager::Loader doesn't need to be a ContextLifecycleObserver. FetchManager::Loader should just hold RawPtrWillBeMember<ExecutionContext>. - FetchManager::Loader should have a pre-finalizer so that it calls m_loader->cancel() before starting lazy sweeping. - Mark SRIVerifier as eagerly-finalized to promptly clear m_handle and m_reader. BUG=589507 Committed: https://crrev.com/f885b41b4d4d00374cc29e62962634681b18c218 Cr-Commit-Position: refs/heads/master@{#378969} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f885b41b4d4d00374cc29e62962634681b18c218 Cr-Commit-Position: refs/heads/master@{#378969} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
