blink_gc_plugin should warn if USING_GARBAGE_COLLECTED_MIXIN is forgotten |
|||
Issue descriptionIt is apparently possible to derive from a GarbageCollectedMixin (in this case, ActiveScriptWrappable) in a GC class (in this case, a derived class of ScriptWrappable) but neglect to use USING_GARBAGE_COLLECTED_MIXIN (which sets up GetHeapObjectHeader and such). This leads to confusing crashes. blink_gc_plugin should detect this an emit an error.
,
Nov 16
Okay, this has now come up again since (and who knows how many times that I was not the person asked to help!), in blink::DocumentPortals. Also there's another instance of this bug checked in. I'm actually going to fix this.
,
Nov 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/83789b82fc93d276cd44b542faa3711d56d6aa4d commit 83789b82fc93d276cd44b542faa3711d56d6aa4d Author: Jeremy Roman <jbroman@chromium.org> Date: Fri Nov 16 20:21:13 2018 Add missing USING_GARBAGE_COLLECTED_MIXIN to RTCQuicStream. This is required for correct heap tracing, since RTCQuicStream derives ContextClient, which is a GC mixin. Bug: 869498 Change-Id: I124c3a23e4a6e670fc3af195fa43498c3f9a857e Reviewed-on: https://chromium-review.googlesource.com/c/1340790 Reviewed-by: Steve Anton <steveanton@chromium.org> Commit-Queue: Jeremy Roman <jbroman@chromium.org> Cr-Commit-Position: refs/heads/master@{#608920} [modify] https://crrev.com/83789b82fc93d276cd44b542faa3711d56d6aa4d/third_party/blink/renderer/modules/peerconnection/rtc_quic_stream.h
,
Nov 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/30bc14da62522bcbbf900e2d44d2314cd8e11696 commit 30bc14da62522bcbbf900e2d44d2314cd8e11696 Author: Jeremy Roman <jbroman@chromium.org> Date: Mon Nov 19 14:27:38 2018 Augment the blink gc plugin to detect missing USING_GARBAGE_COLLECTED_MIXIN. It's a common and difficult to debug pitfall when you derive a GC mixin but fail to use this macro. It leads to confusing crashes during tracing. With this CL, the compiler will emit a warning like this: In file included from ../../third_party/blink/renderer/core/html/portal/document_portals.cc:5: ../../third_party/blink/renderer/core/html/portal/document_portals.h:17:25: error: [blink-gc] Garbage-collected class 'DocumentPortals' derives mixin class 'Supplement<blink::Document>'. You must add USING_GARBAGE_COLLECTED_MIXIN(DocumentPortals). public Supplement<Document> { ~~~~~~~^~~~~~~~~~~~~~~~~~~~ ../../third_party/blink/renderer/platform/supplementable.h:119:27: note: [blink-gc] Mixin base class derived here: class Supplement : public GarbageCollectedMixin { ~~~~~~~^~~~~~~~~~~~~~~~~~~~~ 1 error generated. Bug: 869498 Change-Id: I93454c418d53ebe0fcd411e0a829597dd9aaf925 Reviewed-on: https://chromium-review.googlesource.com/c/1340784 Commit-Queue: Jeremy Roman <jbroman@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#609280} [modify] https://crrev.com/30bc14da62522bcbbf900e2d44d2314cd8e11696/tools/clang/blink_gc_plugin/BadPatternFinder.cpp [modify] https://crrev.com/30bc14da62522bcbbf900e2d44d2314cd8e11696/tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp [modify] https://crrev.com/30bc14da62522bcbbf900e2d44d2314cd8e11696/tools/clang/blink_gc_plugin/DiagnosticsReporter.h [modify] https://crrev.com/30bc14da62522bcbbf900e2d44d2314cd8e11696/tools/clang/blink_gc_plugin/tests/heap/stubs.h [add] https://crrev.com/30bc14da62522bcbbf900e2d44d2314cd8e11696/tools/clang/blink_gc_plugin/tests/missing_mixin_marker.cpp [add] https://crrev.com/30bc14da62522bcbbf900e2d44d2314cd8e11696/tools/clang/blink_gc_plugin/tests/missing_mixin_marker.txt
,
Dec 12
This has rolled and this warning is now enabled for everyone. |
|||
►
Sign in to add a comment |
|||
Comment 1 by mlippautz@chromium.org
, Aug 13