Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2181243002:
Move ThreadableLoader to Oilpan heap (2/3) (Closed)
|
Created:
4 years, 5 months ago by yhirano Modified:
4 years, 4 months ago CC:
chromium-reviews, kinuko+worker_chromium.org, tyoshino+watch_chromium.org, dtapuska+blinkwatch_chromium.org, tzik, Peter Beverloo, eae+blinkwatch, nhiroki, yhirano+watch_chromium.org, falken, loading-reviews_chromium.org, dglazkov+blink, blink-reviews-events_chromium.org, gavinp+loader_chromium.org, blink-reviews, horo+watch_chromium.org, kinuko+watch, Nate Chapin, kinuko+fileapi, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@onheap-bridge-peer-in-worker-threadable-loader Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove ThreadableLoader to Oilpan heap (2/3)
This change makes ThreadableLoader GarbageCollectedFinalized. Apart from
trivial changes, this CL merges WorkerThreadableLoader,
ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge.
BUG=587663
Committed: https://crrev.com/82e485d87b1bd0f704e2e706db02203ea60801e5
Committed: https://crrev.com/1aa2c65ed37380e35a7aacd199d9767ca734b472
Cr-Original-Commit-Position: refs/heads/master@{#409736}
Cr-Commit-Position: refs/heads/master@{#411561}
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : fix #Patch Set 4 : fix #Patch Set 5 : fix #Patch Set 6 : fix #
Total comments: 14
Patch Set 7 : wip #Patch Set 8 : rebase #
Total comments: 4
Patch Set 9 : fix #Patch Set 10 : rebase #Patch Set 11 : rebase #Patch Set 12 : build fix #Patch Set 13 : fix #Dependent Patchsets: Messages
Total messages: 139 (120 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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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_asan_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
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
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...
Description was changed from ========== Move ThreadableLoader to Oilpan heap BUG=587663 ========== to ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL - Makes DocumentThreadableLoader inherit from ResourceOwner<RawResourceClient> , instead of inherit directly from RawResourceClient, - Merges WorkerThreadableLoader and WorkerThreadableLoader::Bridge, - Merges WorkerThreadableLoader and ThreadableLoaderClientWrapper. BUG=587663 ==========
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 #3 (id:180001) has been deleted
Patchset #2 (id:160001) has been deleted
Description was changed from ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL - Makes DocumentThreadableLoader inherit from ResourceOwner<RawResourceClient> , instead of inherit directly from RawResourceClient, - Merges WorkerThreadableLoader and WorkerThreadableLoader::Bridge, - Merges WorkerThreadableLoader and ThreadableLoaderClientWrapper. BUG=587663 ========== to ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL - Makes DocumentThreadableLoader inherit from ResourceOwner<RawResourceClient>, instead of inherit directly from RawResourceClient, - Merges WorkerThreadableLoader and WorkerThreadableLoader::Bridge, - Merges WorkerThreadableLoader and ThreadableLoaderClientWrapper. BUG=587663 ==========
Description was changed from ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL - Makes DocumentThreadableLoader inherit from ResourceOwner<RawResourceClient>, instead of inherit directly from RawResourceClient, - Merges WorkerThreadableLoader and WorkerThreadableLoader::Bridge, - Merges WorkerThreadableLoader and ThreadableLoaderClientWrapper. BUG=587663 ========== to ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL - Makes DocumentThreadableLoader inherit from ResourceOwner<RawResourceClient>, instead of inheriting directly from RawResourceClient, - Merges WorkerThreadableLoader and WorkerThreadableLoader::Bridge, - Merges WorkerThreadableLoader and ThreadableLoaderClientWrapper. BUG=587663 ==========
Description was changed from ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL - Makes DocumentThreadableLoader inherit from ResourceOwner<RawResourceClient>, instead of inheriting directly from RawResourceClient, - Merges WorkerThreadableLoader and WorkerThreadableLoader::Bridge, - Merges WorkerThreadableLoader and ThreadableLoaderClientWrapper. BUG=587663 ========== to ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL - Makes DocumentThreadableLoader inherit from ResourceOwner<RawResourceClient>, instead of inheriting directly from RawResourceClient, - Merges WorkerThreadableLoader and WorkerThreadableLoader::Bridge, - Merges WorkerThreadableLoader and ThreadableLoaderClientWrapper. BUG=587663 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
yhirano@chromium.org changed reviewers: + haraken@chromium.org, japhet@chromium.org
Patchset #2 (id:200001) has been deleted
Would it be possible to split the CL into pieces? If it's hard, I'll take a look at this one.
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...
Description was changed from ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL - Makes DocumentThreadableLoader inherit from ResourceOwner<RawResourceClient>, instead of inheriting directly from RawResourceClient, - Merges WorkerThreadableLoader and WorkerThreadableLoader::Bridge, - Merges WorkerThreadableLoader and ThreadableLoaderClientWrapper. BUG=587663 ========== to ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader and ThreadableLoaderClientWrapper. BUG=587663 ==========
Description was changed from ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader and ThreadableLoaderClientWrapper. BUG=587663 ========== to ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Description was changed from ========== Move ThreadableLoader to Oilpan heap This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 ========== to ========== Move ThreadableLoader to Oilpan heap (2/3) This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 ==========
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...
On 2016/07/28 11:33:08, haraken wrote: > Would it be possible to split the CL into pieces? If it's hard, I'll take a look > at this one. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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.
Mostly looks good, but I want to have someone else take a look at the cancellation logic in WorkerThreadableLoader.cpp. https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h (right): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h:235: WeakPtrFactory<DocumentThreadableLoader> m_weakFactory; Now that DocumentThreadableLoader is on the heap, can we remove the weak factory (in a follow-up CL)? https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ThreadableLoaderClientWrapper.h (left): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ThreadableLoaderClientWrapper.h:46: class ThreadableLoaderClientWrapper final : public GarbageCollected<ThreadableLoaderClientWrapper> { This removal is amazing! https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (left): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:380: DCHECK(!m_mainThreadLoader); Why do we need to remove the DCHECK? https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:432: DCHECK(isMainThread()); Can we add DCHECK(!m_workerLoader)? https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:161: ThreadableLoaderClient* m_client; I'm just curious but do you have a plan to move ThreadableLoaderClient to the heap? https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1027: ThreadableLoader* loader = m_loader; loader = m_loader.release() https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:30: { Can we add: ASSERT(!m_threadableLoader); ? https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/AssociatedURLLoader.cpp (right): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/web/AssociatedURLLoader.cpp:308: AssociatedURLLoader::~AssociatedURLLoader() Is it guaranteed that AssociatedURLLoader's destructor is not called during a GC? If it happens, it's problematic because the destructor is touching an on-heap object.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 #8 (id:340001) has been deleted
https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (left): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:380: DCHECK(!m_mainThreadLoader); On 2016/07/29 14:52:51, haraken wrote: > > Why do we need to remove the DCHECK? Generally I don't like to enforce clearing Member pointers manually. Before this CL m_mainThreadLoader is a unique pointer and clearing it in the destructor has a behavior impact. Now it's a Member pointer and I don't see a necessity to check the nullity. We need to check the loading in m_mainThreadLoader is completed, which is checked in DocumentThreadableLoader's destructor. https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:432: DCHECK(isMainThread()); On 2016/07/29 14:52:51, haraken wrote: > > Can we add DCHECK(!m_workerLoader)? Done. https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:161: ThreadableLoaderClient* m_client; On 2016/07/29 14:52:51, haraken wrote: > > I'm just curious but do you have a plan to move ThreadableLoaderClient to the > heap? I don't have a plan to do it immediately, but I think moving it is a good thing. https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1027: ThreadableLoader* loader = m_loader; On 2016/07/29 14:52:51, haraken wrote: > > loader = m_loader.release() Done. https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:30: { On 2016/07/29 14:52:51, haraken wrote: > > Can we add: > > ASSERT(!m_threadableLoader); > > ? It's possible, but as in WorkerThreadableLoader::Peer::~Peer(), I think requiring it is not necessary. https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/AssociatedURLLoader.cpp (right): https://codereview.chromium.org/2181243002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/web/AssociatedURLLoader.cpp:308: AssociatedURLLoader::~AssociatedURLLoader() On 2016/07/29 14:52:51, haraken wrote: > > Is it guaranteed that AssociatedURLLoader's destructor is not called during a > GC? If it happens, it's problematic because the destructor is touching an > on-heap object. As talked offline, this is not a problem because |m_loader| is retained as a Persistent.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM on my side. https://codereview.chromium.org/2181243002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (left): https://codereview.chromium.org/2181243002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:356: DCHECK(!m_resource); ToT still has clearResource() here. Maybe this patch is depending on some other patch? https://codereview.chromium.org/2181243002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right): https://codereview.chromium.org/2181243002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:72: // ThreableLoader functions ThreadableLoader
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2181243002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (left): https://codereview.chromium.org/2181243002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:356: DCHECK(!m_resource); On 2016/08/01 07:34:20, haraken wrote: > > ToT still has clearResource() here. Maybe this patch is depending on some other > patch? > Yes, this CL is depending on https://codereview.chromium.org/2179903006/ indirectly. https://codereview.chromium.org/2181243002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right): https://codereview.chromium.org/2181243002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:72: // ThreableLoader functions On 2016/08/01 07:34:20, haraken wrote: > > ThreadableLoader Thanks, done.
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.
lgtm
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
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2181243002/#ps400001 (title: "rebase")
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 ========== Move ThreadableLoader to Oilpan heap (2/3) This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 ========== to ========== Move ThreadableLoader to Oilpan heap (2/3) This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Move ThreadableLoader to Oilpan heap (2/3) This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 ========== to ========== Move ThreadableLoader to Oilpan heap (2/3) This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 Committed: https://crrev.com/82e485d87b1bd0f704e2e706db02203ea60801e5 Cr-Commit-Position: refs/heads/master@{#409736} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/82e485d87b1bd0f704e2e706db02203ea60801e5 Cr-Commit-Position: refs/heads/master@{#409736}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:400001) has been created in https://codereview.chromium.org/2228433002/ by yhirano@chromium.org. The reason for reverting is: To investigate crashes reported at https://crbug.com/634546. .
Message was sent while issue was closed.
Description was changed from ========== Move ThreadableLoader to Oilpan heap (2/3) This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 Committed: https://crrev.com/82e485d87b1bd0f704e2e706db02203ea60801e5 Cr-Commit-Position: refs/heads/master@{#409736} ========== to ========== Move ThreadableLoader to Oilpan heap (2/3) This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 Committed: https://crrev.com/82e485d87b1bd0f704e2e706db02203ea60801e5 Cr-Commit-Position: refs/heads/master@{#409736} ==========
reopen
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
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...
Let me reland this CL.
The CQ bit was unchecked by yhirano@chromium.org
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2181243002/#ps440001 (title: "build fix")
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
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 unchecked by yhirano@chromium.org
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2181243002/#ps460001 (title: "fix")
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 ========== Move ThreadableLoader to Oilpan heap (2/3) This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 Committed: https://crrev.com/82e485d87b1bd0f704e2e706db02203ea60801e5 Cr-Commit-Position: refs/heads/master@{#409736} ========== to ========== Move ThreadableLoader to Oilpan heap (2/3) This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 Committed: https://crrev.com/82e485d87b1bd0f704e2e706db02203ea60801e5 Cr-Commit-Position: refs/heads/master@{#409736} ==========
Message was sent while issue was closed.
Committed patchset #13 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== Move ThreadableLoader to Oilpan heap (2/3) This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 Committed: https://crrev.com/82e485d87b1bd0f704e2e706db02203ea60801e5 Cr-Commit-Position: refs/heads/master@{#409736} ========== to ========== Move ThreadableLoader to Oilpan heap (2/3) This change makes ThreadableLoader GarbageCollectedFinalized. Apart from trivial changes, this CL merges WorkerThreadableLoader, ThreadableLoaderClientWrapper and WorkerThreadableLoader::Bridge. BUG=587663 Committed: https://crrev.com/82e485d87b1bd0f704e2e706db02203ea60801e5 Committed: https://crrev.com/1aa2c65ed37380e35a7aacd199d9767ca734b472 Cr-Original-Commit-Position: refs/heads/master@{#409736} Cr-Commit-Position: refs/heads/master@{#411561} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/1aa2c65ed37380e35a7aacd199d9767ca734b472 Cr-Commit-Position: refs/heads/master@{#411561} |
