blink_gc_plugin: detect singletons with embedded ScriptWrappables.

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

[sof, 2017-02-15 14:47:50]

Description was changed from

==========
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.

R=
BUG=688569
==========

to

==========
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
==========

[sof, 2017-02-15 14:57:25]

sigbjornf@opera.com changed reviewers:
+ dcheng@chromium.org, oilpan-reviews@chromium.org

[sof, 2017-02-15 14:57:26]

This turned out to be a bit of a rabbit hole that I want to get out of.. :) 

The GC plugin extension should now be ready if we really want to do something
about ScriptWrappables and singletons. The check/warning is off-by-default.

[sof, 2017-02-15 18:34:21]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 18:35:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 20:45:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 20:45:55]

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_...)

[dcheng, 2017-02-16 04:35:51]

Seems reasonable overall. I only have two small comments.

https://codereview.chromium.org/2696713003/diff/40001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp (right):

https://codereview.chromium.org/2696713003/diff/40001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp:593: for (auto error :
visitor.illegal_types()) {
const auto& to suppress a copy

https://codereview.chromium.org/2696713003/diff/40001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/RecordInfo.h (right):

https://codereview.chromium.org/2696713003/diff/40001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/RecordInfo.h:122: // its type. |is_persistent| is
set to |true| if the base type name is
Make this match the actual param name? =)

[sof, 2017-02-16 06:45:34]

Data point on performance -- compiling WebKit/Source/ is about 40 secs slower
with this extra verification enabled.

This is with overall build time > 90 mins on a now-puny i5/16G box, so I'd say
this is acceptable.

(The extra traversal of method body declarations is something we've wanted for a
while for detecting bad Oilpan type practices, so this something to build on for
those at least.)

https://codereview.chromium.org/2696713003/diff/40001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp (right):

https://codereview.chromium.org/2696713003/diff/40001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp:593: for (auto error :
visitor.illegal_types()) {
On 2017/02/16 04:35:51, dcheng wrote:
> const auto& to suppress a copy

done, thanks for catching that one.

https://codereview.chromium.org/2696713003/diff/40001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/RecordInfo.h (right):

https://codereview.chromium.org/2696713003/diff/40001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/RecordInfo.h:122: // its type. |is_persistent| is
set to |true| if the base type name is
On 2017/02/16 04:35:51, dcheng wrote:
> Make this match the actual param name? =)

Updated to use version N naming, not (N-1). :)

[sof, 2017-02-19 09:13:40]

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.