Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2146403004:
ThreadableLoader::cancel should be called before loader destruction (Closed)
|
Created:
4 years, 5 months ago by yhirano Modified:
4 years, 5 months ago CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews_chromium.org, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@onheap-threadable-loader-client-wrapper Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThreadableLoader::cancel should be called before loader destruction
Currently ThreadableLoader's destructor implicitly cancels loading.
This CL requires a user to cancel the loader explicitly before
destructing a ThreadableLoader, as a preparation for moving it to
Oilpan heap.
BUG=587663
Committed: https://crrev.com/56b1490f30437ec92df7897ee8e7a12a9ed98691
Cr-Commit-Position: refs/heads/master@{#407074}
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : fix #
Total comments: 12
Patch Set 4 : fix #
Depends on Patchset: Messages
Total messages: 46 (35 generated)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:100001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:80001) has been deleted
Description was changed from ========== ThreadableLoader::cancel should be called before loader destruction BUG=587663 ========== to ========== ThreadableLoader::cancel should be called before loader destruction Currently ThreadableLoader's destructor implicitly cancels loading. This CL requires a user to cancel the loader explicitly before destructing a ThreadableLoader, as a preparation for moving it to Oilpan heap. BUG=587663 ==========
yhirano@chromium.org changed reviewers: + hiroshige@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:348: DCHECK(!m_client); optional: how about making this CHECK() for a while on canary to catch unexpected violations to this assumption? https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:940: ThreadableLoaderClient* client = m_client; IIUC the changes of lines 940--961 are made in order to make |m_client| null when loading is completed, i.e. didFail.*() is called, right? https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ThreadableLoader.h (right): https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ThreadableLoader.h:156: // client gets invalid. Also, a user must guarantee that the loading nits: s/loading complete /loading completes / s/threadable loader/ThreadableLoader/ https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ThreadableLoader.h:172: // completed. Is this equivalent to the comment in Lines 156--157? https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ThreadableLoaderClientWrapper.h (right): https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ThreadableLoaderClientWrapper.h:94: ThreadableLoaderClient* client = m_client; Lines 93--127: same as the comment for DocumentThreadableLoader.cpp. https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (left): https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:458: TEST_P(ThreadableLoaderTest, ClearAfterStart) How about keeping these Clear.* tests by adding cancel() calls to DocumentThreadableLoaderTestHelper/WorkerThreadableLoaderTestHelper::clearLoader(), in order to check that we can safely destruct ThreadableLoader synchronously in the ThreadableLoaderClient callbacks?
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:348: DCHECK(!m_client); On 2016/07/19 06:56:13, hiroshige wrote: > optional: how about making this CHECK() for a while on canary to catch > unexpected violations to this assumption? Done. https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:940: ThreadableLoaderClient* client = m_client; On 2016/07/19 06:56:14, hiroshige wrote: > IIUC the changes of lines 940--961 are made in order to make |m_client| null > when loading is completed, i.e. didFail.*() is called, right? Yes, as m_client will never be used after a completion method is called, there should be no problem. https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ThreadableLoader.h (right): https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ThreadableLoader.h:156: // client gets invalid. Also, a user must guarantee that the loading On 2016/07/19 06:56:14, hiroshige wrote: > nits: > s/loading complete /loading completes / > s/threadable loader/ThreadableLoader/ Done. https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ThreadableLoader.h:172: // completed. On 2016/07/19 06:56:14, hiroshige wrote: > Is this equivalent to the comment in Lines 156--157? Thanks, fixed. https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ThreadableLoaderClientWrapper.h (right): https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ThreadableLoaderClientWrapper.h:94: ThreadableLoaderClient* client = m_client; On 2016/07/19 06:56:14, hiroshige wrote: > Lines 93--127: same as the comment for DocumentThreadableLoader.cpp. Yes https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (left): https://codereview.chromium.org/2146403004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:458: TEST_P(ThreadableLoaderTest, ClearAfterStart) On 2016/07/19 06:56:14, hiroshige wrote: > How about keeping these Clear.* tests by adding cancel() calls to > DocumentThreadableLoaderTestHelper/WorkerThreadableLoaderTestHelper::clearLoader(), > in order to check that we can safely destruct ThreadableLoader synchronously in > the ThreadableLoaderClient callbacks? Done.
lgtm if bots are happy.
yhirano@chromium.org changed reviewers: + mkwst@chromium.org
+mkwst@
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM.
The CQ bit was checked by yhirano@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2146333002 Patch 120001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== ThreadableLoader::cancel should be called before loader destruction Currently ThreadableLoader's destructor implicitly cancels loading. This CL requires a user to cancel the loader explicitly before destructing a ThreadableLoader, as a preparation for moving it to Oilpan heap. BUG=587663 ========== to ========== ThreadableLoader::cancel should be called before loader destruction Currently ThreadableLoader's destructor implicitly cancels loading. This CL requires a user to cancel the loader explicitly before destructing a ThreadableLoader, as a preparation for moving it to Oilpan heap. BUG=587663 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== ThreadableLoader::cancel should be called before loader destruction Currently ThreadableLoader's destructor implicitly cancels loading. This CL requires a user to cancel the loader explicitly before destructing a ThreadableLoader, as a preparation for moving it to Oilpan heap. BUG=587663 ========== to ========== ThreadableLoader::cancel should be called before loader destruction Currently ThreadableLoader's destructor implicitly cancels loading. This CL requires a user to cancel the loader explicitly before destructing a ThreadableLoader, as a preparation for moving it to Oilpan heap. BUG=587663 Committed: https://crrev.com/56b1490f30437ec92df7897ee8e7a12a9ed98691 Cr-Commit-Position: refs/heads/master@{#407074} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/56b1490f30437ec92df7897ee8e7a12a9ed98691 Cr-Commit-Position: refs/heads/master@{#407074} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
