Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2680843006:
Tidy DEFINE_(THREAD_SAFE_)STATIC_LOCAL() implementations. (Closed)
|
Created:
3 years, 10 months ago by sof Modified:
3 years, 10 months ago CC:
chromium-reviews, oilpan-reviews, Mads Ager (chromium), blink-reviews-bindings_chromium.org, blink-reviews, kinuko+watch, kouhei+heap_chromium.org, blink-reviews-wtf_chromium.org, Mikhail Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTidy DEFINE_(THREAD_SAFE_)STATIC_LOCAL() implementations.
Move the handling of static local singletons into the
WTF::StaticSingleton<T> wrapper class, including the "same thread"
debug verification.
Use it to also implement the "thread safe" variant also, which can
now be done in a straightforward manner after issue 686866 enabled
C++ thread safe statics.
R=kinuko
BUG=686866
Review-Url: https://codereview.chromium.org/2680843006
Cr-Commit-Position: refs/heads/master@{#451305}
Committed: https://chromium.googlesource.com/chromium/src/+/03165229c01b328598a7ba00bfd2d4ab6fa79f0a
Patch Set 1 #Patch Set 2 : remove loose ends, tidying #Patch Set 3 : static assert for toplevel Persistent<T>s #Patch Set 4 : comment update #Patch Set 5 : rebased and made dependent #Patch Set 6 : explain safety of HTMLTableSectionElement singletons #
Total comments: 10
Patch Set 7 : drop no-ScriptWrappable dcheck support #
Total comments: 2
Patch Set 8 : simplify DEFINE_THREAD_SAFE_STATIC_LOCAL() #
Total comments: 1
Depends on Patchset: Messages
Total messages: 102 (69 generated)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Disallow ScriptWrappables inside of static singletons. R= BUG= ========== to ========== Disallow ScriptWrappables embedded in static singletons. R= BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Disallow ScriptWrappables embedded in static singletons. R= BUG= ========== to ========== Disallow ScriptWrappables embedded in static singletons. Add a debug verification check that static singletons declared using DEFINE_STATIC_LOCAL()s do not contain any embedded ScriptWrappable-derived references. Given that a singleton is accessible across window contexts in a renderer process, having such ScriptWrappables risks sharing their underlying v8 wrapper objects across them contexts, which is not allowed and must not happen. Should the singleton itself be ScriptWrappable-derived, a compile time static_assert() will trigger, otherwise a DCHECK() will trigger when the singleton is instantiated. The codebase does have a small set of singletons which do have embedded ScriptWrappables, but those have been inspected as being safe, as the ScriptWrappable is either not instantiated via the singleton or it is only used in non-production (unit test) settings. DEFINE_STATIC_LOCAL() does provide an opt-out mechanism for the ScriptWrappable verification check. Along with this verification check, clean up the implementation underpinning DEFINE_STATIC_LOCAL(). R= BUG=688569 ==========
Description was changed from ========== Disallow ScriptWrappables embedded in static singletons. Add a debug verification check that static singletons declared using DEFINE_STATIC_LOCAL()s do not contain any embedded ScriptWrappable-derived references. Given that a singleton is accessible across window contexts in a renderer process, having such ScriptWrappables risks sharing their underlying v8 wrapper objects across them contexts, which is not allowed and must not happen. Should the singleton itself be ScriptWrappable-derived, a compile time static_assert() will trigger, otherwise a DCHECK() will trigger when the singleton is instantiated. The codebase does have a small set of singletons which do have embedded ScriptWrappables, but those have been inspected as being safe, as the ScriptWrappable is either not instantiated via the singleton or it is only used in non-production (unit test) settings. DEFINE_STATIC_LOCAL() does provide an opt-out mechanism for the ScriptWrappable verification check. Along with this verification check, clean up the implementation underpinning DEFINE_STATIC_LOCAL(). R= BUG=688569 ========== to ========== Disallow ScriptWrappables embedded in static singletons. Add a debug verification check that static singletons declared using DEFINE_STATIC_LOCAL()s do not contain any embedded ScriptWrappable-derived references. Given that a singleton can in general be accessible across window contexts in a renderer process, having such ScriptWrappables risks sharing their underlying v8 wrapper objects across them contexts, which is not allowed and must not happen. Should the singleton itself be ScriptWrappable-derived, a compile time static_assert() will trigger, otherwise a DCHECK() will trigger when the singleton is instantiated. The codebase does have a small set of singletons which do have embedded ScriptWrappables, but those have been inspected as being safe, as the ScriptWrappable is either not instantiated via the singleton or it is only used in non-production (unit test) settings. DEFINE_STATIC_LOCAL() does provide an opt-out mechanism for the ScriptWrappable verification check. Along with this verification check, clean up the implementation underpinning DEFINE_STATIC_LOCAL(). R= BUG=688569 ==========
sigbjornf@opera.com changed reviewers: + dcheng@chromium.org, esprehn@chromium.org, haraken@chromium.org
please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This verification looks complicated but yeah I don't have any better idea... https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:153: DEFINE_STATIC_LOCAL(MutableStylePropertySet, leftToRightDecl, Ditto. If there's a way to avoid using DEFINE_STATIC_LOCAL, that should be better. https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not accidentally leak between contexts. Consequently, 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. https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLTableElement.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLTableElement.cpp:466: // the no-ScriptWrappables-in-singletons verification check. Your analysis looks correct, but it looks better to avoid using DEFINE_STATIC_LOCAL... by e.g., associating the StylePropertySet with Document (or StyleEngine) etc. https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Member.h (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:182: DCHECK(m_creationThreadState->canAllocateScriptWrappable()); Would you help me understand why we need to have the check here? i.e., why is the check in ScriptWrappable's constructor not enough?
https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not accidentally leak between contexts. Consequently, On 2017/02/11 10:25:20, haraken wrote: > > 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. > Hmm, doesn't the test from https://codereview.chromium.org/2675793005/ demonstrate the information leak? https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLTableElement.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLTableElement.cpp:466: // the no-ScriptWrappables-in-singletons verification check. On 2017/02/11 10:25:20, haraken wrote: > > Your analysis looks correct, but it looks better to avoid using > DEFINE_STATIC_LOCAL... by e.g., associating the StylePropertySet with Document > (or StyleEngine) etc. That could probably be done without it being too ad-hoc. https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Member.h (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:182: DCHECK(m_creationThreadState->canAllocateScriptWrappable()); On 2017/02/11 10:25:20, haraken wrote: > > Would you help me understand why we need to have the check here? i.e., why is > the check in ScriptWrappable's constructor not enough? Good question to raise -- the "no ScriptWrappable" scope is something we set up while the singleton is constructed. So, in order to detect the fields referring to ScriptWrappables that aren't eagerly initialized, we fall back to the MemberBase<> check above for the *Member<> fields that are default initialized. Which is of course only an approximation, as ScriptWrappables may reside deeper within the object type. I should document that more clearly, if we end up taking this on.. The other original suggestion of using the type information that GC plugin has to do this detection, would be accurate. But that might generate false negatives while computing ScriptWrappable reachability (e.g., should we trace through a LocalFrame* reference or not.)
https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not accidentally leak between contexts. Consequently, On 2017/02/11 12:09:11, sof wrote: > On 2017/02/11 10:25:20, haraken wrote: > > > > 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. > > > > Hmm, doesn't the test from https://codereview.chromium.org/2675793005/ > demonstrate the information leak? 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?
https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not accidentally leak between contexts. Consequently, On 2017/02/12 09:33:45, dcheng wrote: > On 2017/02/11 12:09:11, sof wrote: > > On 2017/02/11 10:25:20, haraken wrote: > > > > > > 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. > > > > > > > Hmm, doesn't the test from https://codereview.chromium.org/2675793005/ > > demonstrate the information leak? > > 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? 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()).
https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not accidentally leak between contexts. Consequently, On 2017/02/12 09:33:45, dcheng wrote: > On 2017/02/11 12:09:11, sof wrote: > > On 2017/02/11 10:25:20, haraken wrote: > > > > > > 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. > > > > > > > Hmm, doesn't the test from https://codereview.chromium.org/2675793005/ > > demonstrate the information leak? > > 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. 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.) > > 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?
On 2017/02/12 23:56:47, haraken wrote: > https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/Frame.cpp (right): > > https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not > accidentally leak between contexts. Consequently, > On 2017/02/12 09:33:45, dcheng wrote: > > On 2017/02/11 12:09:11, sof wrote: > > > On 2017/02/11 10:25:20, haraken wrote: > > > > > > > > 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. > > > > > > > > > > Hmm, doesn't the test from https://codereview.chromium.org/2675793005/ > > > demonstrate the information leak? > > > > 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. > > 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.) For different worlds, it's OK because DOMWrapperWorld lets us store one wrapper per-world. However, for different contexts, it's a different problem. esprehn@ posted a brief example on https://bugs.chromium.org/p/chromium/issues/detail?id=687844 which demonstrate that one context is able to set properties on a non-cross origin object, and another context will see those properties. This seems like a clear info leak. > > > > > 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?
On 2017/02/13 01:27:37, dcheng wrote: > On 2017/02/12 23:56:47, haraken wrote: > > > https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/frame/Frame.cpp (right): > > > > > https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not > > accidentally leak between contexts. Consequently, > > On 2017/02/12 09:33:45, dcheng wrote: > > > On 2017/02/11 12:09:11, sof wrote: > > > > On 2017/02/11 10:25:20, haraken wrote: > > > > > > > > > > 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. > > > > > > > > > > > > > Hmm, doesn't the test from https://codereview.chromium.org/2675793005/ > > > > demonstrate the information leak? > > > > > > 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. > > > > 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.) > > For different worlds, it's OK because DOMWrapperWorld lets us store one wrapper > per-world. > > However, for different contexts, it's a different problem. esprehn@ posted a > brief example on https://bugs.chromium.org/p/chromium/issues/detail?id=687844 > which demonstrate that one context is able to set properties on a non-cross > origin object, and another context will see those properties. This seems like a > clear info leak. I understand it's a bug but it is not a new security hole, right? If the context is a non-cross-origin context, there is already a way to see the property. (My point is just that "info leak between contexts (worlds)" normally means a security-sensitive info leak between contexts (worlds) :-)
On 2017/02/13 01:38:04, haraken wrote: > On 2017/02/13 01:27:37, dcheng wrote: > > On 2017/02/12 23:56:47, haraken wrote: > > > > > > https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/frame/Frame.cpp (right): > > > > > > > > > https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not > > > accidentally leak between contexts. Consequently, > > > On 2017/02/12 09:33:45, dcheng wrote: > > > > On 2017/02/11 12:09:11, sof wrote: > > > > > On 2017/02/11 10:25:20, haraken wrote: > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > Hmm, doesn't the test from https://codereview.chromium.org/2675793005/ > > > > > demonstrate the information leak? > > > > > > > > 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. > > > > > > 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.) > > > > For different worlds, it's OK because DOMWrapperWorld lets us store one > wrapper > > per-world. > > > > However, for different contexts, it's a different problem. esprehn@ posted a > > brief example on https://bugs.chromium.org/p/chromium/issues/detail?id=687844 > > which demonstrate that one context is able to set properties on a non-cross > > origin object, and another context will see those properties. This seems like > a > > clear info leak. > > I understand it's a bug but it is not a new security hole, right? If the context > is a non-cross-origin context, there is already a way to see the property. > > (My point is just that "info leak between contexts (worlds)" normally means a > security-sensitive info leak between contexts (worlds) :-) Well, it is if you can use it to get to a cross-origin context's getOwnPropertyDescriptor function and return other properties cross-origin, right?
On 2017/02/13 01:42:27, dcheng wrote: > On 2017/02/13 01:38:04, haraken wrote: > > On 2017/02/13 01:27:37, dcheng wrote: > > > On 2017/02/12 23:56:47, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/core/frame/Frame.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will > not > > > > accidentally leak between contexts. Consequently, > > > > On 2017/02/12 09:33:45, dcheng wrote: > > > > > On 2017/02/11 12:09:11, sof wrote: > > > > > > On 2017/02/11 10:25:20, haraken wrote: > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > Hmm, doesn't the test from https://codereview.chromium.org/2675793005/ > > > > > > demonstrate the information leak? > > > > > > > > > > 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. > > > > > > > > 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.) > > > > > > For different worlds, it's OK because DOMWrapperWorld lets us store one > > wrapper > > > per-world. > > > > > > However, for different contexts, it's a different problem. esprehn@ posted a > > > brief example on > https://bugs.chromium.org/p/chromium/issues/detail?id=687844 > > > which demonstrate that one context is able to set properties on a non-cross > > > origin object, and another context will see those properties. This seems > like > > a > > > clear info leak. > > > > I understand it's a bug but it is not a new security hole, right? If the > context > > is a non-cross-origin context, there is already a way to see the property. > > > > (My point is just that "info leak between contexts (worlds)" normally means a > > security-sensitive info leak between contexts (worlds) :-) > > Well, it is if you can use it to get to a cross-origin context's > getOwnPropertyDescriptor function and return other properties cross-origin, > right? Ah, thanks! You're right.
Worlds are just a special case of contexts. Leaks between worlds can be a security issue if the world contexts that are being exposed are cross origin.
https://codereview.chromium.org/2696713003/ is the WIP for GC plugin verification; it is complete, but needs more polish before in a reviewable state. However, there's an unsettling amount of singletons where the verification currently fails -- I'll put up a CL having the list later. For this CL, I'd like to use it to tidy up the definition of singletons only. Uploaded a patchset which does (just) that.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/14 16:01:17, sof wrote: > https://codereview.chromium.org/2696713003/ is the WIP for GC plugin > verification; it is complete, but needs more polish before in a reviewable > state. > > However, there's an unsettling amount of singletons where the verification > currently fails -- I'll put up a CL having the list later. > https://codereview.chromium.org/2694283003/ has a subset.
Description was changed from ========== Disallow ScriptWrappables embedded in static singletons. Add a debug verification check that static singletons declared using DEFINE_STATIC_LOCAL()s do not contain any embedded ScriptWrappable-derived references. Given that a singleton can in general be accessible across window contexts in a renderer process, having such ScriptWrappables risks sharing their underlying v8 wrapper objects across them contexts, which is not allowed and must not happen. Should the singleton itself be ScriptWrappable-derived, a compile time static_assert() will trigger, otherwise a DCHECK() will trigger when the singleton is instantiated. The codebase does have a small set of singletons which do have embedded ScriptWrappables, but those have been inspected as being safe, as the ScriptWrappable is either not instantiated via the singleton or it is only used in non-production (unit test) settings. DEFINE_STATIC_LOCAL() does provide an opt-out mechanism for the ScriptWrappable verification check. Along with this verification check, clean up the implementation underpinning DEFINE_STATIC_LOCAL(). R= BUG=688569 ========== to ========== Tidy DEFINE_STATIC_LOCAL() implementation. Move the handling of static local singletons into the WTF::StaticSingleton<T> wrapper class, including the "same thread" debug verification. R= BUG=688569 ==========
ptal - narrowed focus and purpose of this one.
https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/StdLibExtras.h (right): https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/StdLibExtras.h:126: WTF::isAtomicallyInitializedStaticMutexLockHeld(); Since we have threadsafe function-local statics by default now... do we actually still need these checks? I don't quite understand the history behind these or what they're trying to verify.
https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/StdLibExtras.h (right): https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/StdLibExtras.h:126: WTF::isAtomicallyInitializedStaticMutexLockHeld(); On 2017/02/15 08:29:56, dcheng wrote: > Since we have threadsafe function-local statics by default now... do we actually > still need these checks? I don't quite understand the history behind these or > what they're trying to verify. https://codereview.chromium.org/874203002#msg11 covers that.
On 2017/02/15 08:44:57, sof wrote: > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > File third_party/WebKit/Source/wtf/StdLibExtras.h (right): > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > third_party/WebKit/Source/wtf/StdLibExtras.h:126: > WTF::isAtomicallyInitializedStaticMutexLockHeld(); > On 2017/02/15 08:29:56, dcheng wrote: > > Since we have threadsafe function-local statics by default now... do we > actually > > still need these checks? I don't quite understand the history behind these or > > what they're trying to verify. > > https://codereview.chromium.org/874203002#msg11 covers that. Hmm... but it looks like that should all be unnecessary with thread-safe statics? What am I missing?
On 2017/02/15 08:49:22, dcheng wrote: > On 2017/02/15 08:44:57, sof wrote: > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/wtf/StdLibExtras.h (right): > > > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > third_party/WebKit/Source/wtf/StdLibExtras.h:126: > > WTF::isAtomicallyInitializedStaticMutexLockHeld(); > > On 2017/02/15 08:29:56, dcheng wrote: > > > Since we have threadsafe function-local statics by default now... do we > > actually > > > still need these checks? I don't quite understand the history behind these > or > > > what they're trying to verify. > > > > https://codereview.chromium.org/874203002#msg11 covers that. > > Hmm... but it looks like that should all be unnecessary with thread-safe > statics? What am I missing? That seems like a comment for that review?
dcheng@chromium.org changed reviewers: + kinuko@chromium.org
On 2017/02/15 08:53:42, sof wrote: > On 2017/02/15 08:49:22, dcheng wrote: > > On 2017/02/15 08:44:57, sof wrote: > > > > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/wtf/StdLibExtras.h (right): > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/wtf/StdLibExtras.h:126: > > > WTF::isAtomicallyInitializedStaticMutexLockHeld(); > > > On 2017/02/15 08:29:56, dcheng wrote: > > > > Since we have threadsafe function-local statics by default now... do we > > > actually > > > > still need these checks? I don't quite understand the history behind these > > or > > > > what they're trying to verify. > > > > > > https://codereview.chromium.org/874203002#msg11 covers that. > > > > Hmm... but it looks like that should all be unnecessary with thread-safe > > statics? What am I missing? > > That seems like a comment for that review? We didn't have thread-safe static initialization two years ago =) +kinuko to see if there are other reasons we need this... if not, it would be nice to just simplify this.
On 2017/02/15 at 08:55:58, dcheng wrote: > On 2017/02/15 08:53:42, sof wrote: > > On 2017/02/15 08:49:22, dcheng wrote: > > > On 2017/02/15 08:44:57, sof wrote: > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/wtf/StdLibExtras.h (right): > > > > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/wtf/StdLibExtras.h:126: > > > > WTF::isAtomicallyInitializedStaticMutexLockHeld(); > > > > On 2017/02/15 08:29:56, dcheng wrote: > > > > > Since we have threadsafe function-local statics by default now... do we > > > > actually > > > > > still need these checks? I don't quite understand the history behind these > > > or > > > > > what they're trying to verify. > > > > > > > > https://codereview.chromium.org/874203002#msg11 covers that. > > > > > > Hmm... but it looks like that should all be unnecessary with thread-safe > > > statics? What am I missing? > > > > That seems like a comment for that review? > > We didn't have thread-safe static initialization two years ago =) > > +kinuko to see if there are other reasons we need this... if not, it would be nice to just simplify this. Where did we turn on thread safe statics? I can't seem to find an announcement about it.
On Wed, Feb 15, 2017 at 1:39 PM, <esprehn@chromium.org> wrote: > On 2017/02/15 at 08:55:58, dcheng wrote: > > On 2017/02/15 08:53:42, sof wrote: > > > On 2017/02/15 08:49:22, dcheng wrote: > > > > On 2017/02/15 08:44:57, sof wrote: > > > > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/ > third_party/WebKit/Source/wtf/StdLibExtras.h > > > > > File third_party/WebKit/Source/wtf/StdLibExtras.h (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/ > third_party/WebKit/Source/wtf/StdLibExtras.h#newcode126 > > > > > third_party/WebKit/Source/wtf/StdLibExtras.h:126: > > > > > WTF::isAtomicallyInitializedStaticMutexLockHeld(); > > > > > On 2017/02/15 08:29:56, dcheng wrote: > > > > > > Since we have threadsafe function-local statics by default > now... do > we > > > > > actually > > > > > > still need these checks? I don't quite understand the history > behind > these > > > > or > > > > > > what they're trying to verify. > > > > > > > > > > https://codereview.chromium.org/874203002#msg11 covers that. > > > > > > > > Hmm... but it looks like that should all be unnecessary with > thread-safe > > > > statics? What am I missing? > > > > > > That seems like a comment for that review? > > > > We didn't have thread-safe static initialization two years ago =) > > > > +kinuko to see if there are other reasons we need this... if not, it > would be > nice to just simplify this. > > Where did we turn on thread safe statics? I can't seem to find an > announcement > about it. > https://groups.google.com/a/chromium.org/d/msg/cxx/j5rFewBzSBQ/SLMznef7CgAJ https://bugs.chromium.org/p/chromium/issues/detail?id=686866 I'm not sure that I saw an announcement to chromium-dev, though I'd expected one. > https://codereview.chromium.org/2680843006/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Wed, Feb 15, 2017 at 1:39 PM, <esprehn@chromium.org> wrote: > On 2017/02/15 at 08:55:58, dcheng wrote: > > On 2017/02/15 08:53:42, sof wrote: > > > On 2017/02/15 08:49:22, dcheng wrote: > > > > On 2017/02/15 08:44:57, sof wrote: > > > > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/ > third_party/WebKit/Source/wtf/StdLibExtras.h > > > > > File third_party/WebKit/Source/wtf/StdLibExtras.h (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/ > third_party/WebKit/Source/wtf/StdLibExtras.h#newcode126 > > > > > third_party/WebKit/Source/wtf/StdLibExtras.h:126: > > > > > WTF::isAtomicallyInitializedStaticMutexLockHeld(); > > > > > On 2017/02/15 08:29:56, dcheng wrote: > > > > > > Since we have threadsafe function-local statics by default > now... do > we > > > > > actually > > > > > > still need these checks? I don't quite understand the history > behind > these > > > > or > > > > > > what they're trying to verify. > > > > > > > > > > https://codereview.chromium.org/874203002#msg11 covers that. > > > > > > > > Hmm... but it looks like that should all be unnecessary with > thread-safe > > > > statics? What am I missing? > > > > > > That seems like a comment for that review? > > > > We didn't have thread-safe static initialization two years ago =) > > > > +kinuko to see if there are other reasons we need this... if not, it > would be > nice to just simplify this. > > Where did we turn on thread safe statics? I can't seem to find an > announcement > about it. > https://groups.google.com/a/chromium.org/d/msg/cxx/j5rFewBzSBQ/SLMznef7CgAJ https://bugs.chromium.org/p/chromium/issues/detail?id=686866 I'm not sure that I saw an announcement to chromium-dev, though I'd expected one. > https://codereview.chromium.org/2680843006/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/15 08:55:58, dcheng wrote: > On 2017/02/15 08:53:42, sof wrote: > > On 2017/02/15 08:49:22, dcheng wrote: > > > On 2017/02/15 08:44:57, sof wrote: > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/wtf/StdLibExtras.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/wtf/StdLibExtras.h:126: > > > > WTF::isAtomicallyInitializedStaticMutexLockHeld(); > > > > On 2017/02/15 08:29:56, dcheng wrote: > > > > > Since we have threadsafe function-local statics by default now... do we > > > > actually > > > > > still need these checks? I don't quite understand the history behind > these > > > or > > > > > what they're trying to verify. > > > > > > > > https://codereview.chromium.org/874203002#msg11 covers that. > > > > > > Hmm... but it looks like that should all be unnecessary with thread-safe > > > statics? What am I missing? > > > > That seems like a comment for that review? > > We didn't have thread-safe static initialization two years ago =) > > +kinuko to see if there are other reasons we need this... if not, it would be > nice to just simplify this. The right way would be to phase out DEFINE_THREAD_SAFE_STATIC_LOCAL() followed by simplifying the above condition. ?
On 2017/02/15 19:11:19, sof wrote: > On 2017/02/15 08:55:58, dcheng wrote: > > On 2017/02/15 08:53:42, sof wrote: > > > On 2017/02/15 08:49:22, dcheng wrote: > > > > On 2017/02/15 08:44:57, sof wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > > > > File third_party/WebKit/Source/wtf/StdLibExtras.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > > > > third_party/WebKit/Source/wtf/StdLibExtras.h:126: > > > > > WTF::isAtomicallyInitializedStaticMutexLockHeld(); > > > > > On 2017/02/15 08:29:56, dcheng wrote: > > > > > > Since we have threadsafe function-local statics by default now... do > we > > > > > actually > > > > > > still need these checks? I don't quite understand the history behind > > these > > > > or > > > > > > what they're trying to verify. > > > > > > > > > > https://codereview.chromium.org/874203002#msg11 covers that. > > > > > > > > Hmm... but it looks like that should all be unnecessary with thread-safe > > > > statics? What am I missing? > > > > > > That seems like a comment for that review? > > > > We didn't have thread-safe static initialization two years ago =) > > > > +kinuko to see if there are other reasons we need this... if not, it would be > > nice to just simplify this. > > The right way would be to phase out DEFINE_THREAD_SAFE_STATIC_LOCAL() followed > by simplifying the above condition. ? (created issue 692669)
On 2017/02/15 19:22:19, sof wrote: > On 2017/02/15 19:11:19, sof wrote: > > On 2017/02/15 08:55:58, dcheng wrote: > > > On 2017/02/15 08:53:42, sof wrote: > > > > On 2017/02/15 08:49:22, dcheng wrote: > > > > > On 2017/02/15 08:44:57, sof wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > > > > > File third_party/WebKit/Source/wtf/StdLibExtras.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Sou... > > > > > > third_party/WebKit/Source/wtf/StdLibExtras.h:126: > > > > > > WTF::isAtomicallyInitializedStaticMutexLockHeld(); > > > > > > On 2017/02/15 08:29:56, dcheng wrote: > > > > > > > Since we have threadsafe function-local statics by default now... do > > we > > > > > > actually > > > > > > > still need these checks? I don't quite understand the history behind > > > these > > > > > or > > > > > > > what they're trying to verify. > > > > > > > > > > > > https://codereview.chromium.org/874203002#msg11 covers that. > > > > > > > > > > Hmm... but it looks like that should all be unnecessary with thread-safe > > > > > statics? What am I missing? > > > > > > > > That seems like a comment for that review? > > > > > > We didn't have thread-safe static initialization two years ago =) > > > > > > +kinuko to see if there are other reasons we need this... if not, it would > be > > > nice to just simplify this. After I saw the thread for thread-safe static initialization I have been thinking that we could probably just get rid of all those checks, so I'm all for it! > > The right way would be to phase out DEFINE_THREAD_SAFE_STATIC_LOCAL() followed > > by simplifying the above condition. ? > > (created issue 692669)
On 2017/02/16 03:44:13, kinuko wrote: > On 2017/02/15 19:22:19, sof wrote: > > On 2017/02/15 19:11:19, sof wrote: > > > On 2017/02/15 08:55:58, dcheng wrote: .... > > > > > > > > We didn't have thread-safe static initialization two years ago =) > > > > > > > > +kinuko to see if there are other reasons we need this... if not, it would > > be > > > > nice to just simplify this. > > After I saw the thread for thread-safe static initialization I have been > thinking that we could probably just get rid of all those checks, so I'm all for > it! > The same-thread verification is worth keeping, I think.
On 2017/02/16 06:29:36, sof wrote: > On 2017/02/16 03:44:13, kinuko wrote: > > On 2017/02/15 19:22:19, sof wrote: > > > On 2017/02/15 19:11:19, sof wrote: > > > > On 2017/02/15 08:55:58, dcheng wrote: > .... > > > > > > > > > > We didn't have thread-safe static initialization two years ago =) > > > > > > > > > > +kinuko to see if there are other reasons we need this... if not, it > would > > > be > > > > > nice to just simplify this. > > > > After I saw the thread for thread-safe static initialization I have been > > thinking that we could probably just get rid of all those checks, so I'm all > for > > it! > > > > The same-thread verification is worth keeping, I think. Yes I agree, sorry I wanted to say I'm all for simplifying DEFINE_THREAD_SAFE_STATIC_LOCAL usages and some of the checks around.
On 2017/02/16 07:12:57, kinuko wrote: > On 2017/02/16 06:29:36, sof wrote: > > On 2017/02/16 03:44:13, kinuko wrote: > > > On 2017/02/15 19:22:19, sof wrote: > > > > On 2017/02/15 19:11:19, sof wrote: > > > > > On 2017/02/15 08:55:58, dcheng wrote: > > .... > > > > > > > > > > > > We didn't have thread-safe static initialization two years ago =) > > > > > > > > > > > > +kinuko to see if there are other reasons we need this... if not, it > > would > > > > be > > > > > > nice to just simplify this. > > > > > > After I saw the thread for thread-safe static initialization I have been > > > thinking that we could probably just get rid of all those checks, so I'm all > > for > > > it! > > > > > > > The same-thread verification is worth keeping, I think. > > Yes I agree, sorry I wanted to say I'm all for simplifying > DEFINE_THREAD_SAFE_STATIC_LOCAL usages and some of the checks around. For the Persistent<>-wrapped statics, it is a helpful dcheck to prevent unwanted cross-thread access. But we could consider pushing that dcheck into a Persistent<>::get() variant instead, as we already track thread owner there. So maybe it'll possible to do without it here in the end.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Tidy DEFINE_STATIC_LOCAL() implementation. Move the handling of static local singletons into the WTF::StaticSingleton<T> wrapper class, including the "same thread" debug verification. R= BUG=688569 ========== to ========== Tidy DEFINE_(THREAD_SAFE_)STATIC_LOCAL() implementations. Move the handling of static local singletons into the WTF::StaticSingleton<T> wrapper class, including the "same thread" debug verification. Use it to also implement the "thread safe" variant also, which can now be done in a straightforward manner after issue 686866 enabled C++ thread safe statics. R= BUG=686866 ==========
Updated to handle the "threadsafe" case; ptal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2680843006/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/StdLibExtras.h (right): https://codereview.chromium.org/2680843006/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/StdLibExtras.h:137: return allowCrossThreadUse || m_safelyInitialized || fyi, https://codereview.chromium.org/2698953002/ has some crash stacks to help explain why the m_safelyInitialized check is still reqd.
lgtm
Description was changed from ========== Tidy DEFINE_(THREAD_SAFE_)STATIC_LOCAL() implementations. Move the handling of static local singletons into the WTF::StaticSingleton<T> wrapper class, including the "same thread" debug verification. Use it to also implement the "thread safe" variant also, which can now be done in a straightforward manner after issue 686866 enabled C++ thread safe statics. R= BUG=686866 ========== to ========== Tidy DEFINE_(THREAD_SAFE_)STATIC_LOCAL() implementations. Move the handling of static local singletons into the WTF::StaticSingleton<T> wrapper class, including the "same thread" debug verification. Use it to also implement the "thread safe" variant also, which can now be done in a straightforward manner after issue 686866 enabled C++ thread safe statics. R=kinuko BUG=686866 ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1487344120008930,
"parent_rev": "848f7e4273c840fb95028b513e4902e6f6157a58", "commit_rev":
"03165229c01b328598a7ba00bfd2d4ab6fa79f0a"}
Message was sent while issue was closed.
Description was changed from ========== Tidy DEFINE_(THREAD_SAFE_)STATIC_LOCAL() implementations. Move the handling of static local singletons into the WTF::StaticSingleton<T> wrapper class, including the "same thread" debug verification. Use it to also implement the "thread safe" variant also, which can now be done in a straightforward manner after issue 686866 enabled C++ thread safe statics. R=kinuko BUG=686866 ========== to ========== Tidy DEFINE_(THREAD_SAFE_)STATIC_LOCAL() implementations. Move the handling of static local singletons into the WTF::StaticSingleton<T> wrapper class, including the "same thread" debug verification. Use it to also implement the "thread safe" variant also, which can now be done in a straightforward manner after issue 686866 enabled C++ thread safe statics. R=kinuko BUG=686866 Review-Url: https://codereview.chromium.org/2680843006 Cr-Commit-Position: refs/heads/master@{#451305} Committed: https://chromium.googlesource.com/chromium/src/+/03165229c01b328598a7ba00bfd2... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:300001) as https://chromium.googlesource.com/chromium/src/+/03165229c01b328598a7ba00bfd2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
