Disallow heap objects containing unsafe on-heap iterators.

Disallow heap objects containing unsafe on-heap iterators.

Do not allow BlinkGC managed objects to include unsafe iterators of
other heap objects; that is, do not allow them to keep iterator
part objects as fields.

These iterators contain untraced references, which is in general
unsafe practice and breaks the general rule that all heap references
must be known to the GC infrastructure, and be marked and traced
through.

This applies to all heap collection iterators but HeapListHashSet<>'s,
which can be safely traced. It is also the only collection iterator
which is kept as a field of an on-heap object (CSSSegmentedFontFace.)

R=haraken
BUG=672030
Committed: https://crrev.com/1fb76ab1b86a2c6f1c2b8512b7bf39701a2a756f
Cr-Commit-Position: refs/heads/master@{#439784}

[sof, 2016-12-19 22:11:44]

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

[commit-bot: I haz the power, 2016-12-19 22:13:32]

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

[sof, 2016-12-19 22:24:12]

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

[sof, 2016-12-19 22:24:13]

please take a look.

[haraken, 2016-12-20 02:47:45]

LGTM

[haraken, 2016-12-20 02:55:16]

(We could also try to remove the iterator on CSSSegmentedFontFace.)

[sof, 2016-12-20 08:26:21]

On 2016/12/20 02:55:16, haraken wrote:
> (We could also try to remove the iterator on CSSSegmentedFontFace.)

It appears feasible, the iterator seems to be part of a scheme to implement two
insertion-ordered sets by keeping one ListHashSet<>. I haven't looked into
details why this representation was used, but decided to allow it. Safely
supporting it is simple, and won't require extending heap compaction.

We can extend the range of allowed on-heap iterators later, as needs arise.

[sof, 2016-12-20 11:46:35]

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

[commit-bot: I haz the power, 2016-12-20 11:46:45]

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

[sof, 2016-12-20 11:50:55]

Updated; the GC plugin will now also check that "safe" iterators are indeed
trace()d.

[commit-bot: I haz the power, 2016-12-20 11:54:32]

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

[commit-bot: I haz the power, 2016-12-20 11:54:33]

Dry run: This issue passed the CQ dry run.

[sof, 2016-12-20 12:52:16]

Description was changed from

==========
Disallow heap objects containing unsafe on-heap iterators.

Do not allow BlinkGC managed objects to include unsafe iterators of
other heap objects; that is, do not allow them to keep iterator
part objects as fields.

These iterators contain untraced references, which is in general
unsafe practice and breaks the general rule that all heap references
must be known to the GC infrastructure, and be marked and traced
through.

This applies to all heap collection iterators but HeapListHashSet<>'s,
which can be safely traced. It is also the only collection iterator
which is kept as a field of an on-heap object (CSSSegmentedFontFace.)

R=
BUG=
==========

to

==========
Disallow heap objects containing unsafe on-heap iterators.

Do not allow BlinkGC managed objects to include unsafe iterators of
other heap objects; that is, do not allow them to keep iterator
part objects as fields.

These iterators contain untraced references, which is in general
unsafe practice and breaks the general rule that all heap references
must be known to the GC infrastructure, and be marked and traced
through.

This applies to all heap collection iterators but HeapListHashSet<>'s,
which can be safely traced. It is also the only collection iterator
which is kept as a field of an on-heap object (CSSSegmentedFontFace.)

R=haraken
BUG=672030
==========

[sof, 2016-12-20 12:53:31]

Verified to build ToT (r439759) cleanly; landing.

[sof, 2016-12-20 12:53:38]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2016-12-20 12:53:38]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2588943002/#ps60001
(title: "formatting")

[commit-bot: I haz the power, 2016-12-20 12:53:52]

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, 2016-12-20 13:22:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482238418020700,
"parent_rev": "ee3b2e1e1de2c60fe230292373c27a85ac3330dc", "commit_rev":
"b81fe119fe9f43fbddded06be250056795ab4b77"}

[commit-bot: I haz the power, 2016-12-20 13:22:50]

Description was changed from

==========
Disallow heap objects containing unsafe on-heap iterators.

Do not allow BlinkGC managed objects to include unsafe iterators of
other heap objects; that is, do not allow them to keep iterator
part objects as fields.

These iterators contain untraced references, which is in general
unsafe practice and breaks the general rule that all heap references
must be known to the GC infrastructure, and be marked and traced
through.

This applies to all heap collection iterators but HeapListHashSet<>'s,
which can be safely traced. It is also the only collection iterator
which is kept as a field of an on-heap object (CSSSegmentedFontFace.)

R=haraken
BUG=672030
==========

to

==========
Disallow heap objects containing unsafe on-heap iterators.

Do not allow BlinkGC managed objects to include unsafe iterators of
other heap objects; that is, do not allow them to keep iterator
part objects as fields.

These iterators contain untraced references, which is in general
unsafe practice and breaks the general rule that all heap references
must be known to the GC infrastructure, and be marked and traced
through.

This applies to all heap collection iterators but HeapListHashSet<>'s,
which can be safely traced. It is also the only collection iterator
which is kept as a field of an on-heap object (CSSSegmentedFontFace.)

R=haraken
BUG=672030

Review-Url: https://codereview.chromium.org/2588943002
==========

[commit-bot: I haz the power, 2016-12-20 13:22:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-20 13:24:24]

Description was changed from

==========
Disallow heap objects containing unsafe on-heap iterators.

Do not allow BlinkGC managed objects to include unsafe iterators of
other heap objects; that is, do not allow them to keep iterator
part objects as fields.

These iterators contain untraced references, which is in general
unsafe practice and breaks the general rule that all heap references
must be known to the GC infrastructure, and be marked and traced
through.

This applies to all heap collection iterators but HeapListHashSet<>'s,
which can be safely traced. It is also the only collection iterator
which is kept as a field of an on-heap object (CSSSegmentedFontFace.)

R=haraken
BUG=672030

Review-Url: https://codereview.chromium.org/2588943002
==========

to

==========
Disallow heap objects containing unsafe on-heap iterators.

Do not allow BlinkGC managed objects to include unsafe iterators of
other heap objects; that is, do not allow them to keep iterator
part objects as fields.

These iterators contain untraced references, which is in general
unsafe practice and breaks the general rule that all heap references
must be known to the GC infrastructure, and be marked and traced
through.

This applies to all heap collection iterators but HeapListHashSet<>'s,
which can be safely traced. It is also the only collection iterator
which is kept as a field of an on-heap object (CSSSegmentedFontFace.)

R=haraken
BUG=672030

Committed: https://crrev.com/1fb76ab1b86a2c6f1c2b8512b7bf39701a2a756f
Cr-Commit-Position: refs/heads/master@{#439784}
==========

[commit-bot: I haz the power, 2016-12-20 13:24:25]

Patchset 4 (id:??) landed as
https://crrev.com/1fb76ab1b86a2c6f1c2b8512b7bf39701a2a756f
Cr-Commit-Position: refs/heads/master@{#439784}

[dcheng, 2016-12-20 21:23:45]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-12-20 21:23:46]

https://codereview.chromium.org/2588943002/diff/60001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/tests/stack_allocated.txt (right):

https://codereview.chromium.org/2588943002/diff/60001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/stack_allocated.txt:23: ./heap/stubs.h:184:5:
note: expanded from macro 'STACK_ALLOCATED'
I think the test expectation is wrong. This is on line 178 of stubs.h, not line
184, which is causing me issues for rebasing my CL. I'm just going to update
this expectation.

[sof, 2016-12-20 21:30:28]

https://codereview.chromium.org/2588943002/diff/60001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/tests/stack_allocated.txt (right):

https://codereview.chromium.org/2588943002/diff/60001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/stack_allocated.txt:23: ./heap/stubs.h:184:5:
note: expanded from macro 'STACK_ALLOCATED'
On 2016/12/20 21:23:46, dcheng wrote:
> I think the test expectation is wrong. This is on line 178 of stubs.h, not
line
> 184, which is causing me issues for rebasing my CL. I'm just going to update
> this expectation.
> 

Thanks, please do. "git cl format" is a mixed blessing.