Tidy DEFINE_(THREAD_SAFE_)STATIC_LOCAL() implementations.

third_party/WebKit/Source/core/frame/Frame.cpp

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Simon Hausmann <hausmann@kde.org>
  *                     2000 Stefan Schimanski <1Stein@gmx.de>
  *                     2001 George Staikos <staikos@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All
  * rights reserved.
  * Copyright (C) 2005 Alexey Proskuryakov <ap@nypop.com>
@@ skipping 106 matching lines @@
 
   return tree().parent()->isRemoteFrame();
 }
 
 HTMLFrameOwnerElement* Frame::deprecatedLocalOwner() const {
   return m_owner && m_owner->isLocal() ? toHTMLFrameOwnerElement(m_owner)
                                        : nullptr;
 }
 
 static ChromeClient& emptyChromeClient() {
-  DEFINE_STATIC_LOCAL(EmptyChromeClient, client, (EmptyChromeClient::create()));
+  // |ChromeClient| contains a weak reference to a |Node| (which derives
+  // from |ScriptWrappable|). That reference is only used for unit testing
+  // purposes and will not accidentally leak between contexts. Consequently,

[haraken, 2017/02/11 10:25:20]

Nit: I don't think "leak between contexts" can happen because wrapper isolation
is realized by DOMWrapperWorld. I think the problem here is a memory leak, not a
leak between contexts.

[sof, 2017/02/11 12:09:11]

Hmm, doesn't the test from https://codereview.chromium.org/2675793005/
demonstrate the information leak?

[dcheng, 2017/02/12 09:33:45]

The cross-context leak happens if:
- we have a static ScriptWrappable object.
- the same object is returned to different contexts

Each ScriptWrappable can only hold wrapper per-world. If the same wrapper is
returned two different contexts, then it's possible that accessing
constructor.constructor and similar things will be problematic.

For this call site, another option is to just move the  logic that depends on
the Node reference out of ChromeClient into ChromeClientImpl and avoid this
problem altogether. WDYT?

[sof, 2017/02/12 21:52:28]

Yes, it could be made to work for the unit tests that need to access 
lastSetTooltipNodeForTesting(), by the looks of it.

Adding GC plugin verification for static locals seems like a worthy experiment.
It would be complete and we already have a mechanism for turning off plugin
checks where pragmatically needed (GC_PLUGIN_IGNORE()).

[haraken, 2017/02/12 23:56:47]

I think the problem here is *just* (=not security sensitive) that DOM may return
a wrong DOM object to JS. That's what the test from
https://codereview.chromium.org/2675793005/ is checking.

"Leaks between contexts" (more accurately, "leaks between worlds") means that a
JS object (including DOM wrappers) leak between two different worlds, which
enables one chrome extension to exploit the window object of the main page.

That should not happen because toV8() already guarantees that different DOM
wrappers are returned to different worlds for the same C++ DOM object
(ScriptWrappable). The fact that the same ScriptWrappable is returned to JS
doesn't mean that there is an information leak to JS. (Note that we're already
returning the same ScriptWrappable to all extension worlds.)

+  // disable the singleton verification check.
+  DEFINE_STATIC_LOCAL(EmptyChromeClient, client, (EmptyChromeClient::create()),
+                      CheckScriptWrappable::No);
   return client;
 }
 
 ChromeClient& Frame::chromeClient() const {
   if (Page* page = this->page())
     return page->chromeClient();
   return emptyChromeClient();
 }
 
 Frame* Frame::findFrameForNavigation(const AtomicString& name,
@@ skipping 277 matching lines @@
 
   ASSERT(page());
 
   if (m_owner)
     m_owner->setContentFrame(*this);
   else
     page()->setMainFrame(this);
 }
 
 }  // namespace blink