Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(403)

Issue 2696713003: blink_gc_plugin: detect singletons with embedded ScriptWrappables.

Created:
3 years, 10 months ago by sof
Modified:
3 years, 10 months ago
Reviewers:
oilpan-reviews, dcheng
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

blink_gc_plugin: detect singletons with embedded ScriptWrappables. It is in general unsafe for DEFINE_STATIC_LOCAL() singletons to keep references to ScriptWrappable instances, as that risks leaking information across contexts residing in the same renderer process. If enabled, have the GC plugin detect static singleton declarations and traverse their types, throwing warnings if any ScriptWrappable-derived references are encountered. Like other GC plugin checks, the warnings can be selectively disabled via GC_PLUGIN_IGNORE(). R= BUG=688569

Patch Set 1 #

Patch Set 2 : sync expected #

Patch Set 3 : better error reporting + polishing #

Total comments: 4

Patch Set 4 : non-copying iteration #

Unified diffs Side-by-side diffs Delta from patch set Stats (+646 lines, -45 lines) Patch
M tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.h View 1 chunk +2 lines, -0 lines 0 comments Download
M tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp View 1 2 3 4 chunks +36 lines, -1 line 0 comments Download
M tools/clang/blink_gc_plugin/BlinkGCPluginOptions.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M tools/clang/blink_gc_plugin/CMakeLists.txt View 1 chunk +1 line, -0 lines 0 comments Download
A tools/clang/blink_gc_plugin/CheckSingletonVisitor.h View 1 2 1 chunk +53 lines, -0 lines 0 comments Download
A tools/clang/blink_gc_plugin/CheckSingletonVisitor.cpp View 1 2 3 1 chunk +150 lines, -0 lines 0 comments Download
M tools/clang/blink_gc_plugin/CollectVisitor.h View 1 2 1 chunk +23 lines, -3 lines 0 comments Download
M tools/clang/blink_gc_plugin/CollectVisitor.cpp View 1 2 3 chunks +25 lines, -2 lines 0 comments Download
M tools/clang/blink_gc_plugin/Config.h View 2 chunks +9 lines, -4 lines 0 comments Download
M tools/clang/blink_gc_plugin/Config.cpp View 1 chunk +12 lines, -0 lines 0 comments Download
M tools/clang/blink_gc_plugin/DiagnosticsReporter.h View 1 2 4 chunks +12 lines, -6 lines 0 comments Download
M tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp View 1 2 7 chunks +37 lines, -6 lines 0 comments Download
M tools/clang/blink_gc_plugin/RecordInfo.h View 1 2 3 1 chunk +10 lines, -0 lines 0 comments Download
M tools/clang/blink_gc_plugin/RecordInfo.cpp View 1 2 3 chunks +36 lines, -16 lines 0 comments Download
M tools/clang/blink_gc_plugin/tests/heap/stubs.h View 3 chunks +10 lines, -4 lines 0 comments Download
M tools/clang/blink_gc_plugin/tests/legacy_naming/heap/stubs.h View 1 2 3 chunks +8 lines, -3 lines 0 comments Download
A tools/clang/blink_gc_plugin/tests/legacy_naming/static_singleton.h View 1 2 1 chunk +39 lines, -0 lines 0 comments Download
A tools/clang/blink_gc_plugin/tests/legacy_naming/static_singleton.cpp View 1 2 1 chunk +37 lines, -0 lines 0 comments Download
A tools/clang/blink_gc_plugin/tests/legacy_naming/static_singleton.flags View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A tools/clang/blink_gc_plugin/tests/legacy_naming/static_singleton.txt View 1 2 1 chunk +31 lines, -0 lines 0 comments Download
A tools/clang/blink_gc_plugin/tests/static_singleton.h View 1 2 1 chunk +39 lines, -0 lines 0 comments Download
A tools/clang/blink_gc_plugin/tests/static_singleton.cpp View 1 2 1 chunk +37 lines, -0 lines 0 comments Download
A tools/clang/blink_gc_plugin/tests/static_singleton.flags View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A tools/clang/blink_gc_plugin/tests/static_singleton.txt View 1 2 1 chunk +31 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (6 generated)
sof
This turned out to be a bit of a rabbit hole that I want to ...
3 years, 10 months ago (2017-02-15 14:57:26 UTC) #3
dcheng
Seems reasonable overall. I only have two small comments. https://codereview.chromium.org/2696713003/diff/40001/tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp File tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp (right): https://codereview.chromium.org/2696713003/diff/40001/tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp#newcode593 tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp:593: ...
3 years, 10 months ago (2017-02-16 04:35:51 UTC) #8
sof
Data point on performance -- compiling WebKit/Source/ is about 40 secs slower with this extra ...
3 years, 10 months ago (2017-02-16 06:45:34 UTC) #9
sof
3 years, 10 months ago (2017-02-19 09:13:40 UTC) #10
ftr, https://codereview.chromium.org/2694283003/ has the list of singletons that
this verification turns up.

The length of that list gives pause for multiple concerns. Including if we
should be concerned with this in the first place/in the end.

Powered by Google App Engine
This is Rietveld 408576698