Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2776473003:
Clear out prefinalizer-allocated vector for conservative GC safety. (Closed)
DescriptionClear out prefinalizer-allocated vector for conservative GC safety.
It is unsafe to revive dead objects by creating references to them
while in a finalizer, prefinalizer or not. Avoid doing so in
MediaStreamSource::setReadyState(), which MediaStreamComponent's
prefinalizer may end up running.
See associated comment for further details.
R=haraken
BUG=704234
Review-Url: https://codereview.chromium.org/2776473003
Cr-Commit-Position: refs/heads/master@{#459390}
Committed: https://chromium.googlesource.com/chromium/src/+/419fb6f42e0ead5bfc5983fddb6f404f835f05be
Patch Set 1 #
Total comments: 3
Patch Set 2 : improve comment #Messages
Total messages: 25 (13 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...
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
haraken@chromium.org changed reviewers: + haraken@chromium.org
Nice detective work!! Thanks for looking into this. A couple of thoughts: - Would it be a bit tidier to use Vector<Observer*> with a good comment? - Would it be possible to forbid an object allocation during pre-finalizer? I guess the allocation itself is safe because the new object is allocated on a page that is not a target for sweeping though. - What we really want to detect is an object mutation during pre-finalizer. Would it be possible to insert a check to Member/Persistent's assignment to verify that we're adding a reference to a dead object?
On 2017/03/24 01:24:29, haraken wrote: > Nice detective work!! Thanks for looking into this. > > A couple of thoughts: > > - Would it be a bit tidier to use Vector<Observer*> with a good comment? > Not safe, given that these are weak references that aren't directly stack-reachable. Unless we block out GCs. > - Would it be possible to forbid an object allocation during pre-finalizer? I > guess the allocation itself is safe because the new object is allocated on a > page that is not a target for sweeping though. > It isn't a problem to allow it per se, but any reference that revives a near-dead object makes it break.. as we now know. I haven't gone through https://codereview.chromium.org/2775573003/ results in detail, but the allocation above looks like the main user of prefinalizer allocation. > - What we really want to detect is an object mutation during pre-finalizer. > Would it be possible to insert a check to Member/Persistent's assignment to > verify that we're adding a reference to a dead object? That could probably be done, some cost to barrier every constructor & update of Members though.
This CL LGTM I want to think about a way to detect an object graph mutation during pre-finalizers and weak processing though. https://codereview.chromium.org/2776473003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.cpp (right): https://codereview.chromium.org/2776473003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/mediastream/MediaStreamSource.cpp:67: // live until the next/ GC (but be unreferenced by other heap objects), next https://codereview.chromium.org/2776473003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/mediastream/MediaStreamSource.cpp:76: for (size_t i = 0; i < observers.size(); ++i) Can we just call observers.clear()?
https://codereview.chromium.org/2776473003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.cpp (right): https://codereview.chromium.org/2776473003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/mediastream/MediaStreamSource.cpp:76: for (size_t i = 0; i < observers.size(); ++i) On 2017/03/24 04:58:43, haraken wrote: > > Can we just call observers.clear()? We need to clear the backing store contents, not what points to it.
On 2017/03/24 05:03:25, sof wrote: > https://codereview.chromium.org/2776473003/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.cpp > (right): > > https://codereview.chromium.org/2776473003/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/mediastream/MediaStreamSource.cpp:76: for > (size_t i = 0; i < observers.size(); ++i) > On 2017/03/24 04:58:43, haraken wrote: > > > > Can we just call observers.clear()? > > We need to clear the backing store contents, not what points to it. ah, you're right.
On 2017/03/24 04:58:43, haraken wrote: > This CL LGTM > > I want to think about a way to detect an object graph mutation during > pre-finalizers and weak processing though. > Yes, must be avoided & caught. We could cut cost of a Member/Persistent check by making it gated on first being in an (eager-)sweep/prefinalizing state, clearly. However, if MediaStreamSource::setReadyState()'s heap allocation cannot be avoided, such a check wouldn't be compatible with it. > https://codereview.chromium.org/2776473003/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.cpp > (right): > > https://codereview.chromium.org/2776473003/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/mediastream/MediaStreamSource.cpp:67: // live > until the next/ GC (but be unreferenced by other heap objects), > > next > Done.
Description was changed from ========== Clear out prefinalizer-allocated vector for conservative GC safety. It is unsafe to revive dead objects by creating references to them while in a finalizer, prefinalizer or not. Avoid doing so in MediaStreamSource::setReadyState(), which MediaStreamComponent's prefinalizer may end up running. See associated comment for further details. R= BUG=704234 ========== to ========== Clear out prefinalizer-allocated vector for conservative GC safety. It is unsafe to revive dead objects by creating references to them while in a finalizer, prefinalizer or not. Avoid doing so in MediaStreamSource::setReadyState(), which MediaStreamComponent's prefinalizer may end up running. See associated comment for further details. R=haraken BUG=704234 ==========
The CQ bit was checked by sigbjornf@opera.com
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/2776473003/#ps20001 (title: "improve comment")
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
Try jobs failed on following builders: linux_chromium_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
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": 20001, "attempt_start_ts": 1490350043356830,
"parent_rev": "8f7af87790319208abf7f4554e32a868b91b478d", "commit_rev":
"419fb6f42e0ead5bfc5983fddb6f404f835f05be"}
Message was sent while issue was closed.
Description was changed from ========== Clear out prefinalizer-allocated vector for conservative GC safety. It is unsafe to revive dead objects by creating references to them while in a finalizer, prefinalizer or not. Avoid doing so in MediaStreamSource::setReadyState(), which MediaStreamComponent's prefinalizer may end up running. See associated comment for further details. R=haraken BUG=704234 ========== to ========== Clear out prefinalizer-allocated vector for conservative GC safety. It is unsafe to revive dead objects by creating references to them while in a finalizer, prefinalizer or not. Avoid doing so in MediaStreamSource::setReadyState(), which MediaStreamComponent's prefinalizer may end up running. See associated comment for further details. R=haraken BUG=704234 Review-Url: https://codereview.chromium.org/2776473003 Cr-Commit-Position: refs/heads/master@{#459390} Committed: https://chromium.googlesource.com/chromium/src/+/419fb6f42e0ead5bfc5983fddb6f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/419fb6f42e0ead5bfc5983fddb6f...
Message was sent while issue was closed.
On 2017/03/24 05:04:49, haraken wrote: > On 2017/03/24 05:03:25, sof wrote: > > > https://codereview.chromium.org/2776473003/diff/1/third_party/WebKit/Source/p... > > File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.cpp > > (right): > > > > > https://codereview.chromium.org/2776473003/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/mediastream/MediaStreamSource.cpp:76: for > > (size_t i = 0; i < observers.size(); ++i) > > On 2017/03/24 04:58:43, haraken wrote: > > > > > > Can we just call observers.clear()? > > > > We need to clear the backing store contents, not what points to it. > > ah, you're right. clear()ing would have made the bug much less likely though (no longer directly reachable should the stack frame memory region be reused), but not impossible. |
