DocumentThreadableLoader: Add guards for sync notifyFinished() in setResource()

third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp

 /*
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  * Copyright (C) 2013, Intel Corporation
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 824 matching lines @@
             resourceLoaderOptions.dataBufferingPolicy = BufferData;
 
         if (m_options.timeoutMilliseconds > 0)
             m_timeoutTimer.startOneShot(m_options.timeoutMilliseconds / 1000.0, BLINK_FROM_HERE);
 
         FetchRequest newRequest(request, m_options.initiator, resourceLoaderOptions);
         if (m_options.crossOriginRequestPolicy == AllowCrossOriginRequests)
             newRequest.setOriginRestriction(FetchRequest::NoOriginRestriction);
         ASSERT(!resource());
 
+        WeakPtr<DocumentThreadableLoader> self(m_weakFactory.createWeakPtr());
+
         if (request.requestContext() == WebURLRequest::RequestContextVideo || request.requestContext() == WebURLRequest::RequestContextAudio)
             setResource(RawResource::fetchMedia(newRequest, document().fetcher()));
         else if (request.requestContext() == WebURLRequest::RequestContextManifest)
             setResource(RawResource::fetchManifest(newRequest, document().fetcher()));
         else
             setResource(RawResource::fetch(newRequest, document().fetcher()));
 
+        // setResource() might call notifyFinished() synchronously, and thus
+        // clear() might be called and |this| may be dead here.
+        if (!self)
+            return;
+
         if (!resource()) {
             ThreadableLoaderClient* client = m_client;
             clear();
+            if (!client)
+                return;

[tyoshino (SeeGerritForStatus), 2016/04/19 06:27:44]

add comment to note that we need to call clear() for the case fetch.*() method
failed but in case clear() is called inside setResource() synchronously,
m_client is already cleared and therefore we need to return here.

[tyoshino (SeeGerritForStatus), 2016/04/19 06:29:00]

oh, basically the same as what you've put in the description.

[hiroshige, 2016/04/19 06:40:48]

Done.

[tyoshino (SeeGerritForStatus), 2016/04/19 08:31:32]

Thanks. LGTM

             client->didFail(ResourceError(errorDomainBlinkInternal, 0, requestURL.getString(), "Failed to start loading."));
             // |this| may be dead here.
             return;
         }
 
         if (resource()->loader()) {
             unsigned long identifier = resource()->identifier();
             InspectorInstrumentation::documentThreadableLoaderStartedLoadingForClient(m_document, identifier, m_client);
         }
         return;
@@ skipping 71 matching lines @@
     return m_securityOrigin ? m_securityOrigin.get() : document().getSecurityOrigin();
 }
 
 Document& DocumentThreadableLoader::document() const
 {
     ASSERT(m_document);
     return *m_document;
 }
 
 } // namespace blink