Stop ResourceCallback from being an on-heap class (issue 2287483003 by yhirano@chromium.org)

yhi...@chromium.org

unread,

Aug 26, 2016, 11:51:48 AM8/26/16

to har...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org

Reviewers: haraken
CL: https://codereview.chromium.org/2287483003/

Description:
Stop ResourceCallback from being an on-heap class

The first ResourceCallback::callbackHandler() call instantiates the
ResourceCallback singleton. As we call it in the pre-finalization
step, it should not be an on-heap class.

BUG=587663

Affected files (+6, -9 lines):
M third_party/WebKit/Source/core/fetch/Resource.cpp

Index: third_party/WebKit/Source/core/fetch/Resource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/Resource.cpp b/third_party/WebKit/Source/core/fetch/Resource.cpp
index 31065b424de1b0d4ae6081c43870045d0a58e1d1..ca35056b36931d066b15d2287ead0f511db42717 100644
--- a/third_party/WebKit/Source/core/fetch/Resource.cpp
+++ b/third_party/WebKit/Source/core/fetch/Resource.cpp
@@ -226,10 +226,12 @@ void Resource::ServiceWorkerResponseCachedMetadataHandler::sendToPlatform()
}
}

-class Resource::ResourceCallback final : public GarbageCollectedFinalized<ResourceCallback> {
+// This class cannot be on-heap because the first callbackHandler() call
+// instantiates the singleton object while we can call it in the
+// pre-finalization step.
+class Resource::ResourceCallback final {
public:
static ResourceCallback& callbackHandler();
- DECLARE_TRACE();
void schedule(Resource*);
void cancel(Resource*);
bool isScheduled(Resource*) const;
@@ -238,7 +240,7 @@ private:

void runTask();
std::unique_ptr<CancellableTaskFactory> m_callbackTaskFactory;
- HeapHashSet<Member<Resource>> m_resourcesWithPendingClients;
+ HashSet<Persistent<Resource>> m_resourcesWithPendingClients;
};

Resource::ResourceCallback& Resource::ResourceCallback::callbackHandler()
@@ -249,15 +251,10 @@ Resource::ResourceCallback& Resource::ResourceCallback::callbackHandler()
//
// Keep it out of LSan's reach instead.
LEAK_SANITIZER_DISABLED_SCOPE;
- DEFINE_STATIC_LOCAL(ResourceCallback, callbackHandler, (new ResourceCallback));
+ DEFINE_STATIC_LOCAL(ResourceCallback, callbackHandler, ());
return callbackHandler;
}

-DEFINE_TRACE(Resource::ResourceCallback)
-{
- visitor->trace(m_resourcesWithPendingClients);
-}
-
Resource::ResourceCallback::ResourceCallback()
: m_callbackTaskFactory(CancellableTaskFactory::create(this, &ResourceCallback::runTask))
{

har...@chromium.org

unread,

Aug 26, 2016, 8:36:13 PM8/26/16

to yhi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org

LGTM

https://codereview.chromium.org/2287483003/

yhi...@chromium.org

unread,

Aug 29, 2016, 12:34:11 AM8/29/16

to har...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org

https://codereview.chromium.org/2287483003/diff/60001/third_party/WebKit/Source/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2287483003/diff/60001/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode248
third_party/WebKit/Source/core/fetch/Resource.cpp:248: // Oilpan + LSan:
as the callbackHandler() singleton is used by Resource
haraken@: Sorry I forgot to ask, I don't know if we should keep this
comment and LEAK_SANITIZER_DISABLED_SCOPE. Can you tell me?

https://codereview.chromium.org/2287483003/

har...@chromium.org

unread,

Aug 29, 2016, 12:37:03 AM8/29/16

to yhi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org

https://codereview.chromium.org/2287483003/diff/60001/third_party/WebKit/Source/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2287483003/diff/60001/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode248
third_party/WebKit/Source/core/fetch/Resource.cpp:248: // Oilpan + LSan:
as the callbackHandler() singleton is used by Resource

On 2016/08/29 04:34:11, yhirano wrote:
> haraken@: Sorry I forgot to ask, I don't know if we should keep this
comment and
> LEAK_SANITIZER_DISABLED_SCOPE. Can you tell me?

This is a oilpan & DISABLE_STATIC_LOCAL specific thing. So you can just
remove the comment and the scope.

https://codereview.chromium.org/2287483003/

yhi...@chromium.org

unread,

Aug 29, 2016, 12:40:46 AM8/29/16

to har...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org

https://codereview.chromium.org/2287483003/diff/60001/third_party/WebKit/Source/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2287483003/diff/60001/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode248
third_party/WebKit/Source/core/fetch/Resource.cpp:248: // Oilpan + LSan:
as the callbackHandler() singleton is used by Resource

On 2016/08/29 04:37:02, haraken wrote:
> On 2016/08/29 04:34:11, yhirano wrote:
> > haraken@: Sorry I forgot to ask, I don't know if we should keep this
comment
> and
> > LEAK_SANITIZER_DISABLED_SCOPE. Can you tell me?
>
> This is a oilpan & DISABLE_STATIC_LOCAL specific thing. So you can
just remove
> the comment and the scope.

Thanks!

https://codereview.chromium.org/2287483003/