Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1217083002:
Only check for on-heap Member<T>s iff T is in scope. (Closed)
|
Created:
5 years, 5 months ago by sof Modified:
5 years, 5 months ago CC:
blink-reviews, haraken, oilpan-reviews, kouhei+heap_chromium.org, Mads Ager (chromium) Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOnly check for on-heap Member<T>s iff T is in scope.
As Member<T>::checkPointer() uses IsGarbageCollectedMixin<T>,
T's definition must be in scope for it to be precise.
Adjust condition accordingly.
R=haraken
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198050
Patch Set 1 #
Total comments: 1
Patch Set 2 : use sizeof() instead #Patch Set 3 : do better; introduce IsFullyDefined<> #Patch Set 4 : rebased #
Total comments: 2
Messages
Total messages: 23 (7 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org, tasak@google.com, thakis@chromium.org
please take a look. Not quite sure (how), but this might be fallout from r197973, Windows Oilpan bots starting to fail badly on unit tests -- http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20...
One thing I forgot to mention -- it would be appropriate for Frame.cpp to have an #include of "FrameOwner.h". Not having so is the direct cause of these failures. https://codereview.chromium.org/1167523007/ makes that change, but if it doesn't land soon enough, I can split that part out. An alternate approach here is to use sizeof(T) (or some such) alongside !IsGarbageCollectedMixin<T>::value in checkPointer(), to avoid fetching payloads out of false negatives.
haraken@chromium.org changed reviewers: + haraken@chromium.org
LGTM https://codereview.chromium.org/1217083002/diff/1/Source/platform/heap/Handle.h File Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1217083002/diff/1/Source/platform/heap/Handle... Source/platform/heap/Handle.h:775: #if ENABLE(ASSERT) && !OS(WIN) Shall we add a comment?
I'm not a fan of special cases like this :-/ Does this only happen in component builds? If so, I bet what instantiates the templates is that the destructors of the classes containing Member<T>s get instantiated by the dllimport, which forces instantiation of all virtual methods (so that they can go into the vtable). Could it work to give classes that contain Member<T>s with predeclared types out-of-line destructors?
MSVC just brings out the issue quicker/more clearly; let's use sizeof() instead for this approximate check? updated to do so.
https://codereview.chromium.org/1208223003/ touches on code nearby, so (trivial) conflict expected. Getting this landed would be good -- windows debug bot is not happy.
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://chromiumcodereview.appspot.com/1217083002/#ps20002 (title: "do better; introduce IsFullyDefined<>")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1217083002/20002
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
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://chromiumcodereview.appspot.com/1217083002/#ps50001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1217083002/50001
Message was sent while issue was closed.
Committed patchset #4 (id:50001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198050
Message was sent while issue was closed.
The bot probably was not-sad at some point. Rather that rush in CLs with pending concerns, it seems better to revert whatever made it sad? https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... File Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... Source/platform/heap/Handle.h:795: if (IsFullyDefined<T>::value && !IsGarbageCollectedMixin<T>::value) If this assert fires now depends on .h include order, right? If so, that seems kind of bad too. (If not: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... just inlines the static_assert – not sure this IsFullyDefined class is needed) Did you try my suggestion of out-of-lining a few destructors?
Message was sent while issue was closed.
https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... File Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... Source/platform/heap/Handle.h:795: if (IsFullyDefined<T>::value && !IsGarbageCollectedMixin<T>::value) On 2015/06/30 20:04:45, Nico wrote: > If this assert fires now depends on .h include order, right? If so, that seems > kind of bad too. > That's entirely to be expected. e.g., TrackBase doesn't #include HTMLMediaElement, because it cannot without stepping into a cyclic dependency. So, I don't know what your concern is wrt this particular check.
Message was sent while issue was closed.
On 2015/06/30 20:10:59, sof wrote: > https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... > File Source/platform/heap/Handle.h (right): > > https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... > Source/platform/heap/Handle.h:795: if (IsFullyDefined<T>::value && > !IsGarbageCollectedMixin<T>::value) > On 2015/06/30 20:04:45, Nico wrote: > > If this assert fires now depends on .h include order, right? If so, that seems > > kind of bad too. > > > > That's entirely to be expected. e.g., TrackBase doesn't #include > HTMLMediaElement, because it cannot without stepping into a cyclic dependency. > > So, I don't know what your concern is wrt this particular check. My concern is that this assert() may or may not fire depending on if you include a certain .h. Is that the case? Missing .h includes should only ever cause compile errors, not silently change runtime behavior.
Message was sent while issue was closed.
On 2015/06/30 20:13:54, Nico wrote: > On 2015/06/30 20:10:59, sof wrote: > > > https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... > > File Source/platform/heap/Handle.h (right): > > > > > https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... > > Source/platform/heap/Handle.h:795: if (IsFullyDefined<T>::value && > > !IsGarbageCollectedMixin<T>::value) > > On 2015/06/30 20:04:45, Nico wrote: > > > If this assert fires now depends on .h include order, right? If so, that > seems > > > kind of bad too. > > > > > > > That's entirely to be expected. e.g., TrackBase doesn't #include > > HTMLMediaElement, because it cannot without stepping into a cyclic dependency. > > > > > So, I don't know what your concern is wrt this particular check. > > My concern is that this assert() may or may not fire depending on if you include > a certain .h. Is that the case? Missing .h includes should only ever cause > compile errors, not silently change runtime behavior. This assert, an additional nicety besides what the clang plugin already tries to verify for Member<> usage, cannot ever be complete as types aren't guaranteed to be in scope. Unless we perform some whole program/type hierarchy analysis. It produced false negatives before for GarbageCollectedMixin<T> if T wasn't known; now it pessimises for that particular case,as not doing so is unsafe (calling fromPayload() on a reference to a real GC mixin.)
Message was sent while issue was closed.
On 2015/06/30 20:20:30, sof wrote: > On 2015/06/30 20:13:54, Nico wrote: > > On 2015/06/30 20:10:59, sof wrote: > > > > > > https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... > > > File Source/platform/heap/Handle.h (right): > > > > > > > > > https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... > > > Source/platform/heap/Handle.h:795: if (IsFullyDefined<T>::value && > > > !IsGarbageCollectedMixin<T>::value) > > > On 2015/06/30 20:04:45, Nico wrote: > > > > If this assert fires now depends on .h include order, right? If so, that > > seems > > > > kind of bad too. > > > > > > > > > > That's entirely to be expected. e.g., TrackBase doesn't #include > > > HTMLMediaElement, because it cannot without stepping into a cyclic > dependency. > > > > > > > > So, I don't know what your concern is wrt this particular check. > > > > My concern is that this assert() may or may not fire depending on if you > include > > a certain .h. Is that the case? Missing .h includes should only ever cause > > compile errors, not silently change runtime behavior. > > This assert, an additional nicety besides what the clang plugin already tries to > verify for Member<> usage, cannot ever be complete as types aren't guaranteed to > be in scope. You could guarantee this with a static_assert(sizeof(T) > 0). You say that this causes lots of errors. I asked what causes the early instantiation of check(), and wondered if it's due to dllimport forcing early instantation of this method. Looking at this more now, I see that checkPointer() is not virtual, so that can't be the case. Instead, it looks like Member calls this method from its constructors and assignment operators. So implicit constructors and assignment operators will require includes. Is it reasonable to require classes that use Member<> to have out-of-line constructors and assignment operators? (I don't know how heavily Member is used.) Having a check that can fail because header includes change looks super hard to debug to me. Say a bot turns from green to red and you need to figure out why – now you have to know the include graph of all of blink, and you need to look at _every_ CL in that build. And that's when you know about this check being include-dependent – imagine how hard this would be for someone to figure out who doesn't know that.
Message was sent while issue was closed.
On 2015/06/30 22:01:25, Nico wrote: > On 2015/06/30 20:20:30, sof wrote: > > On 2015/06/30 20:13:54, Nico wrote: > > > On 2015/06/30 20:10:59, sof wrote: > > > > > > > > > > https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... > > > > File Source/platform/heap/Handle.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1217083002/diff/50001/Source/platform/heap/Ha... > > > > Source/platform/heap/Handle.h:795: if (IsFullyDefined<T>::value && > > > > !IsGarbageCollectedMixin<T>::value) > > > > On 2015/06/30 20:04:45, Nico wrote: > > > > > If this assert fires now depends on .h include order, right? If so, that > > > seems > > > > > kind of bad too. > > > > > > > > > > > > > That's entirely to be expected. e.g., TrackBase doesn't #include > > > > HTMLMediaElement, because it cannot without stepping into a cyclic > > dependency. > > > > > > > > > > > So, I don't know what your concern is wrt this particular check. > > > > > > My concern is that this assert() may or may not fire depending on if you > > include > > > a certain .h. Is that the case? Missing .h includes should only ever cause > > > compile errors, not silently change runtime behavior. > > > > This assert, an additional nicety besides what the clang plugin already tries > to > > verify for Member<> usage, cannot ever be complete as types aren't guaranteed > to > > be in scope. > > You could guarantee this with a static_assert(sizeof(T) > 0). You say that this > causes lots of errors. I asked what causes the early instantiation of check(), > and wondered if it's due to dllimport forcing early instantation of this method. > If you look at TreeScope.h, it has a Member<Document> as a field. Clearly we cannot #include "core/dom/Document.h" in that header file, so the suggestion to use that static_assert() will cause a compilation error. Which isn't workable. The comment in checkPointer() touches on this. The template instantiation of Member<Document> will either happen because of the compiler's timing of such, but in the case of TreeScope, it's inline setDocument() will force it to happen earlier always. Hence, the instantiation of Member<Document> will happen. I don't think it has overall merit to out-of-line such accessors to reduce the likelihood of template instantiations happening without the definition of Document being in scope (i.e., in TreeScope.cpp). We use inline methods for a reason, in most cases. > Looking at this more now, I see that checkPointer() is not virtual, so that > can't be the case. Instead, it looks like Member calls this method from its > constructors and assignment operators. So implicit constructors and assignment > operators will require includes. Is it reasonable to require classes that use > Member<> to have out-of-line constructors and assignment operators? (I don't > know how heavily Member is used.) > It is used to wrap up all references to Oilpan heap objects, so it's everywhere. For the reasons above, I don't think it is reasonable to require that. > Having a check that can fail because header includes change looks super hard to > debug to me. Say a bot turns from green to red and you need to figure out why > – now you have to know the include graph of all of blink, and you need to look > at _every_ CL in that build. And that's when you know about this check being > include-dependent – imagine how hard this would be for someone to figure out who > doesn't know that. I don't think it is ideal either for the reasons you give, but having it not make the wrong inference when it just doesn't have the type information seems like an improvement? |
