Remove a bunch of pointless null checks in FrameLoaderClientImpl.

third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp

 /*
  * Copyright (C) 2013 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 97 matching lines @@
   // Prevent onScriptLoaderFinished from deleting 'this'.
   m_askedToTerminate = true;
 
   if (m_workerThread)
     m_workerThread->terminateAndWait();
 
   DCHECK(runningWorkerInstances().contains(this));
   runningWorkerInstances().remove(this);
   DCHECK(m_webView);
 
-  // Detach the client before closing the view to avoid getting called back.
-  m_mainFrame->setClient(0);

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

This breaks the invariant. For now, I've added early returns to the various
WebFrameClient methods implemented by the workers.

-
   if (m_workerGlobalScopeProxy) {
     m_workerGlobalScopeProxy->detach();
     m_workerGlobalScopeProxy.clear();
   }
 
   m_webView->close();
-  m_mainFrame->close();

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

The default WebFrameClient implementation already does this.

   if (m_loaderProxy)
     m_loaderProxy->detachProvider(this);
 }
 
 void WebEmbeddedWorkerImpl::startWorkerContext(
     const WebEmbeddedWorkerStartData& data) {
   DCHECK(!m_askedToTerminate);
   DCHECK(!m_mainScriptLoader);
   DCHECK_EQ(m_pauseAfterDownloadState, DontPauseAfterDownload);
   m_workerStartData = data;
@@ skipping 181 matching lines @@
   RefPtr<SharedBuffer> buffer(
       SharedBuffer::create(content.data(), content.length()));
   m_loadingShadowPage = true;
   m_mainFrame->frame()->loader().load(
       FrameLoadRequest(0, ResourceRequest(m_workerStartData.scriptURL),
                        SubstituteData(buffer, "text/html", "UTF-8", KURL())));
 }
 
 void WebEmbeddedWorkerImpl::willSendRequest(WebLocalFrame* frame,
                                             WebURLRequest& request) {
+  // TODO(dcheng): Investigate not plumbing through load notifications when a
+  // frame is detached.
+  if (m_askedToTerminate)
+    return;
+
   if (m_networkProvider)
     m_networkProvider->willSendRequest(frame->dataSource(), request);
 }
 
 void WebEmbeddedWorkerImpl::didFinishDocumentLoad(WebLocalFrame* frame) {
+  // TODO(dcheng): Investigate not plumbing through load notifications when a
+  // frame is detached.
+  if (m_askedToTerminate)
+    return;
+
   DCHECK(!m_mainScriptLoader);
   DCHECK(!m_networkProvider);
   DCHECK(m_mainFrame);
   DCHECK(m_workerContextClient);
   DCHECK(m_loadingShadowPage);
-  DCHECK(!m_askedToTerminate);
   m_loadingShadowPage = false;
   m_networkProvider =
       wrapUnique(m_workerContextClient->createServiceWorkerNetworkProvider(
           frame->dataSource()));
   m_mainScriptLoader = WorkerScriptLoader::create();
   m_mainScriptLoader->setRequestContext(
       WebURLRequest::RequestContextServiceWorker);
   m_mainScriptLoader->loadAsynchronously(
       *m_mainFrame->frame()->document(), m_workerStartData.scriptURL,
       DenyCrossOriginRequests, m_workerStartData.addressSpace, nullptr,
       bind(&WebEmbeddedWorkerImpl::onScriptLoaderFinished,
            WTF::unretained(this)));

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

It's unclear to me why onScriptLoaderFinished also needs to guard against
m_askedToTerminate: this seems unsafe by default (if it's true, the dtor has
run, and running the callback is unsafe)

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

Good point. Yeah, that guard looks strange. According to the git history, the
guard could be valid until this change:
https://codereview.chromium.org/449533003

I think we can replace it with DCHECK(!m_askedToTerminate) or just remove it.

[kinuko, 2016/10/20 13:01:54]

WebViewImpl::close in this class's dtor could synchronously call
onScriptLoaderFinished callback, where it might try to delete this again without
the guard.  I guess probably a better fix could be using a weak ptr and
invalidate it at the beginning of the dtor.

   // Do nothing here since onScriptLoaderFinished() might have been already
   // invoked and |this| might have been deleted at this point.
 }
 
 void WebEmbeddedWorkerImpl::sendProtocolMessage(int sessionId,
                                                 int callId,
                                                 const WebString& message,
                                                 const WebString& state) {
   m_workerContextClient->sendDevToolsMessage(sessionId, callId, message, state);
 }
@@ skipping 103 matching lines @@
       *this, *document, *m_workerContextClient);
   m_loaderProxy = WorkerLoaderProxy::create(this);
   m_workerThread =
       ServiceWorkerThread::create(m_loaderProxy, *m_workerGlobalScopeProxy);
   m_workerThread->start(std::move(startupData));
   m_workerInspectorProxy->workerThreadCreated(document, m_workerThread.get(),
                                               scriptURL);
 }
 
 }  // namespace blink