Remove a bunch of pointless null checks in FrameLoaderClientImpl.

third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * 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.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 75 matching lines @@
       m_mainFrame(nullptr),
       m_askedToTerminate(false),
       m_workerInspectorProxy(WorkerInspectorProxy::create()),
       m_client(client),
       m_pauseWorkerContextOnStart(false),
       m_isPausedOnStart(false),
       m_creationAddressSpace(WebAddressSpacePublic) {}
 
 WebSharedWorkerImpl::~WebSharedWorkerImpl() {
   DCHECK(m_webView);
-  // Detach the client before closing the view to avoid getting called back.
-  m_mainFrame->setClient(0);
 
   m_webView->close();
-  m_mainFrame->close();
+
   if (m_loaderProxy)
     m_loaderProxy->detachProvider(this);
 }
 
 void WebSharedWorkerImpl::terminateWorkerThread() {
   if (m_askedToTerminate)
     return;
   m_askedToTerminate = true;
   if (m_mainScriptLoader) {
     m_mainScriptLoader->cancel();
@@ skipping 29 matching lines @@
   m_client->workerReadyForInspection();
   if (m_pauseWorkerContextOnStart) {
     m_isPausedOnStart = true;
     return;
   }
   loadShadowPage();
 }
 
 WebApplicationCacheHost* WebSharedWorkerImpl::createApplicationCacheHost(
     WebApplicationCacheHostClient* appcacheHostClient) {
+  if (m_askedToTerminate)
+    return nullptr;
   return m_client->createApplicationCacheHost(appcacheHostClient);
 }
 
 void WebSharedWorkerImpl::loadShadowPage() {
   // Construct substitute data source for the 'shadow page'. We only need it
   // to have same origin as the worker so the loading checks work correctly.
   CString content("");
   RefPtr<SharedBuffer> buffer(
       SharedBuffer::create(content.data(), content.length()));
   m_mainFrame->frame()->loader().load(
       FrameLoadRequest(0, ResourceRequest(m_url),
                        SubstituteData(buffer, "text/html", "UTF-8", KURL())));
 }
 
 void WebSharedWorkerImpl::willSendRequest(WebLocalFrame* frame,
                                           WebURLRequest& request) {
+  if (m_askedToTerminate)
+    return;
+
   if (m_networkProvider)
     m_networkProvider->willSendRequest(frame->dataSource(), request);
 }
 
 void WebSharedWorkerImpl::didFinishDocumentLoad(WebLocalFrame* frame) {
+  if (m_askedToTerminate)

[dcheng, 2016/10/17 22:33:29]

Similarly, I've added early returns. It feels a bit weird to need these: I'm
hoping that not invoking (most of) the successful loading callbacks if a
Document hasn't otherwise completed loading during Frame::detach might improve
this...

+    return;
+
   DCHECK(!m_loadingDocument);
   DCHECK(!m_mainScriptLoader);
   m_networkProvider = wrapUnique(
       m_client->createServiceWorkerNetworkProvider(frame->dataSource()));
   m_mainScriptLoader = WorkerScriptLoader::create();
   m_mainScriptLoader->setRequestContext(
       WebURLRequest::RequestContextSharedWorker);
   m_loadingDocument = toWebLocalFrameImpl(frame)->frame()->document();
   m_mainScriptLoader->loadAsynchronously(
       *m_loadingDocument.get(), m_url, DenyCrossOriginRequests,
       m_creationAddressSpace,
       bind(&WebSharedWorkerImpl::didReceiveScriptLoaderResponse,
            WTF::unretained(this)),
       bind(&WebSharedWorkerImpl::onScriptLoaderFinished,
            WTF::unretained(this)));

[dcheng, 2016/10/17 22:33:29]

Same question here: why does onScriptLoaderFinished() also need to check
m_askedToTerminate?

[nhiroki, 2016/10/20 05:57:12]

ditto(https://codereview.chromium.org/451603002)

   // Do nothing here since onScriptLoaderFinished() might have been already
   // invoked and |this| might have been deleted at this point.
 }
 
 bool WebSharedWorkerImpl::isControlledByServiceWorker(
     WebDataSource& dataSource) {
+  if (m_askedToTerminate)

[esprehn, 2016/10/24 21:27:07]

should m_networkProvider be nulled out once this is true?

+    return false;
+
   return m_networkProvider &&
          m_networkProvider->isControlledByServiceWorker(dataSource);
 }
 
 int64_t WebSharedWorkerImpl::serviceWorkerID(WebDataSource& dataSource) {
-  if (!m_networkProvider)
+  if (m_askedToTerminate || !m_networkProvider)
     return -1;
   return m_networkProvider->serviceWorkerID(dataSource);

[esprehn, 2016/10/24 21:27:07]

ditto?

 }
 
 InterfaceProvider* WebSharedWorkerImpl::interfaceProvider() {
+  if (m_askedToTerminate)
+    return nullptr;

[esprehn, 2016/10/24 21:27:07]

Can we return getEmptyInterfaceProvider() instead? That avoids null checks on
interfaceProvider() which should ideally never return null

   return Platform::current()->interfaceProvider();
 }
 
 void WebSharedWorkerImpl::sendProtocolMessage(int sessionId,
                                               int callId,
                                               const WebString& message,
                                               const WebString& state) {
   m_client->sendDevToolsMessage(sessionId, callId, message, state);
 }
 
@@ skipping 223 matching lines @@
   if (devtoolsAgent)
     devtoolsAgent->dispatchOnInspectorBackend(sessionId, callId, method,
                                               message);
 }
 
 WebSharedWorker* WebSharedWorker::create(WebSharedWorkerClient* client) {
   return new WebSharedWorkerImpl(client);
 }
 
 }  // namespace blink