Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2539433002:
Properly trace OffscreenCanvas (Closed)
|
Created:
4 years ago by Michael Lippautz Modified:
4 years ago CC:
jochen (gone - plz use gerrit), blink-reviews, chromium-reviews, Marcel Hlopko Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionProperly trace OffscreenCanvas
BUG=chromium:668848
Committed: https://crrev.com/eb7ae5b79faeb3b4c79866b419ca8e3e7879d29d
Cr-Commit-Position: refs/heads/master@{#434674}
Patch Set 1 #Patch Set 2 : Properly trace OffscreenCanvas #
Messages
Total messages: 19 (12 generated)
Description was changed from ========== Properly trace OffscreenCanvas BUG= ========== to ========== Properly trace OffscreenCanvas BUG=chromum:668848 ==========
mlippautz@chromium.org changed reviewers: + haraken@chromium.org
haraken: ptal jochen: thanks for your help!
Description was changed from ========== Properly trace OffscreenCanvas BUG=chromum:668848 ========== to ========== Properly trace OffscreenCanvas BUG=chromium:668848 ==========
jochen@chromium.org changed reviewers: + jochen@chromium.org
how can we make sure there aren't more of these bugs in the codebase?
haraken@chromium.org changed reviewers: + keishi@chromium.org
LGTM +keishi: Why is this error not detected by the GC plugin? I guess it's because EventTarget's hierarchy is somewhat special but it would be nicer to tell the fact to the plugin.
The CQ bit was checked by mlippautz@chromium.org 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 mlippautz@chromium.org
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": 1480353719716110,
"parent_rev": "cf3d13294386a0bd29c0c50f215454d82ef1fd1f", "commit_rev":
"f4152958b67fa838b8b1006886e1d387344a4f5f"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Properly trace OffscreenCanvas BUG=chromium:668848 ========== to ========== Properly trace OffscreenCanvas BUG=chromium:668848 Committed: https://crrev.com/eb7ae5b79faeb3b4c79866b419ca8e3e7879d29d Cr-Commit-Position: refs/heads/master@{#434674} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/eb7ae5b79faeb3b4c79866b419ca8e3e7879d29d Cr-Commit-Position: refs/heads/master@{#434674}
Message was sent while issue was closed.
On 2016/11/28 15:55:06, haraken wrote:
> LGTM
>
> +keishi: Why is this error not detected by the GC plugin? I guess it's because
> EventTarget's hierarchy is somewhat special but it would be nicer to tell the
> fact to the plugin.
Looks like the plugin marks EventTargetWithInlineData as not requiring a trace
method because we GC_PLUGIN_IGNORE the single member that requires a trace.
According to the comment below, we want to skip over super classes that don't
require a trace method.
CheckTraceVisitor.cpp
// We want to deal with omitted trace() function in an intermediary
// class in the class hierarchy, e.g.:
// class A : public GarbageCollected<A> { trace() { ... } };
// class B : public A { /* No trace(); have nothing to trace. */ };
// class C : public B { trace() { B::trace(visitor); } }
// where, B::trace() is actually A::trace(), and in some cases we get
// A as |callee_record| instead of B. We somehow need to mark B as
// traced if we find A::trace() call.
I think the plugin should require a trace if a class has a trace method defined.
Created CL at https://codereview.chromium.org/2536403002
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
