Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1411603007:
[Oilpan] Add use-after-free detector in Member<>
|
Created:
5 years, 1 month ago by peria Modified:
5 years ago CC:
Mads Ager (chromium), darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, kouhei+heap_chromium.org, oilpan-reviews, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd use-after-free detector in Member<> and its subclasses.
After this CL, HeapObjectHeader and Member<>s will have a
new parameter "gcGeneration".
In HeapObjectHeader, gcGeneration means when the object has
been allocated, and in Member<>, it means when the pointee
object should have been allocated.
On ASAN builds, Member<> checks if its gcGeneration is equal
to the pointee's gcGeneration on every reference.
If they have different values, the reference is a use-after-free,
even if the pointee is a live object.
Note: currently, this verification is skipped if the assignment
happens in a constructor of GCMixin interface class.
BUG=550325
Patch Set 1 : #
Total comments: 14
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Diff from cl/1433843002 #Patch Set 5 : ConstructGCMixin state #Patch Set 6 : Move GCMixinTrait with a TODO #Patch Set 7 : Remove "magic" field used for use-after-free #Patch Set 8 : Rebase + remove a checkPointer #Patch Set 9 : Fix test failures #
Total comments: 16
Patch Set 10 : #
Total comments: 4
Patch Set 11 : #Patch Set 12 : Add NO_SANITIZE_ADDRESS to HeapObjectHeader::gcGeneration #Patch Set 13 : #Patch Set 14 : Rebase #
Total comments: 24
Patch Set 15 : #Patch Set 16 : Replace 0 with named constant #Patch Set 17 : Avoid using HeapObjectHeader while constructing GCMixin #Patch Set 18 : Drop HeapObjectHeaderTrait::gcGeneration #
Total comments: 16
Patch Set 19 : Introduce gcGenerationUnchecked #
Total comments: 3
Patch Set 20 : Require full definition of T #
Total comments: 14
Patch Set 21 : Work for comments #Patch Set 22 : Seek in page #Patch Set 23 : #Patch Set 24 : Rebase #
Total comments: 1
Messages
Total messages: 76 (26 generated)
Description was changed from ========== [Oilpan][NOCOMMIT] Add gcGeneration in Handles. BUG=550325 ========== to ========== *Not ready for commit now* Add verifier in Member<> to detect use-after-free of on-heap objects. This CL is a forked one from 1430983002. This CL does not run the verification during constructing interface classes for GarbageCollectedMixin. TODOs before commiting: - Need to work for non fully defined types BUG=550325 ==========
Description was changed from ========== *Not ready for commit now* Add verifier in Member<> to detect use-after-free of on-heap objects. This CL is a forked one from 1430983002. This CL does not run the verification during constructing interface classes for GarbageCollectedMixin. TODOs before commiting: - Need to work for non fully defined types BUG=550325 ========== to ========== *Not ready for commit now* Add use-after-free detector in Member<>. This detector checks a handle Member<>'s gcGeneration is same with the pointee object's gcGeneration on every referencing. If they have different values, the reference is a use-after-free, even if the pointee is a live object. Currently, this verification is skipped if the assignment happens in a constructor of GCMixin interface class. BUG=550325 ==========
Description was changed from ========== *Not ready for commit now* Add use-after-free detector in Member<>. This detector checks a handle Member<>'s gcGeneration is same with the pointee object's gcGeneration on every referencing. If they have different values, the reference is a use-after-free, even if the pointee is a live object. Currently, this verification is skipped if the assignment happens in a constructor of GCMixin interface class. BUG=550325 ========== to ========== *Not ready for commit now* Add use-after-free detector in Member<>. This detector checks a handle Member<>'s gcGeneration is same with the pointee object's gcGeneration on every referencing. If they have different values, the reference is a use-after-free, even if the pointee is a live object. This detector works also for WeakMember<> and UntracedMember<>. Currently, this verification is skipped if the assignment happens in a constructor of GCMixin interface class. BUG=550325 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== *Not ready for commit now* Add use-after-free detector in Member<>. This detector checks a handle Member<>'s gcGeneration is same with the pointee object's gcGeneration on every referencing. If they have different values, the reference is a use-after-free, even if the pointee is a live object. This detector works also for WeakMember<> and UntracedMember<>. Currently, this verification is skipped if the assignment happens in a constructor of GCMixin interface class. BUG=550325 ========== to ========== Add use-after-free detector in Member<>. This detector checks a handle Member<>'s gcGeneration is same with the pointee object's gcGeneration on every referencing. If they have different values, the reference is a use-after-free, even if the pointee is a live object. This detector works also for WeakMember<> and UntracedMember<>. Currently, this verification is skipped if the assignment happens in a constructor of GCMixin interface class. BUG=550325 ==========
Description was changed from ========== Add use-after-free detector in Member<>. This detector checks a handle Member<>'s gcGeneration is same with the pointee object's gcGeneration on every referencing. If they have different values, the reference is a use-after-free, even if the pointee is a live object. This detector works also for WeakMember<> and UntracedMember<>. Currently, this verification is skipped if the assignment happens in a constructor of GCMixin interface class. BUG=550325 ========== to ========== Add use-after-free detector in Member<>. This detector checks a handle Member<>'s gcGeneration is same with the pointee object's gcGeneration on every referencing. If they have different values, the reference is a use-after-free, even if the pointee is a live object. This detector works also for WeakMember<> and UntracedMember<>. Currently, this verification is skipped if the assignment happens in a constructor of GCMixin interface class. NOTE: https://codereview.chromium.org/1414253004/ must be landed before this CL. BUG=550325 ==========
peria@chromium.org changed reviewers: + haraken@chromium.org, oilpan-reviews@chromium.org
The approach looks good. Would you concatenate this CL with https://codereview.chromium.org/1414253004/? I think putGCGeneration() won't be needed. https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/StylePropertySet.cpp (right): https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/StylePropertySet.cpp:45: return sizeof(ImmutableStylePropertySet) - sizeof(void*) + sizeof(RawPtrWillBeMember<CSSValue>) * count + sizeof(StylePropertyMetadata) * count; Let's land this change separately. https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:670: // for gcGeneration, because the object may not be fully allocated. Sorry, I'm getting confused. Why can't we call heapObjectHeader(obj)->gcGeneration() against a GarbageCollectedMixin object that is being constructed? heapObjectHeader(obj)->gcGeneration() shouldn't depend on the vtable of the mixin object, so I think we can just call it... (I may be missing something.) https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:671: if (ThreadState::current()->isGCForbidden()) Let's rename isGCForbidden() to isConstructingGCMixin(). https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:810: // Verify that this handle points a live on-heap object, if a pointer points to https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:838: m_gcGeneration = 0; You won't need this. Given that you're checking if(m_raw && m_gcGeneration) at line 812, you don't need to reset m_gcGeneration when m_raw becomes 0. This also means that you don't need to call checkPointer() when assigning nullptr.
Description was changed from ========== Add use-after-free detector in Member<>. This detector checks a handle Member<>'s gcGeneration is same with the pointee object's gcGeneration on every referencing. If they have different values, the reference is a use-after-free, even if the pointee is a live object. This detector works also for WeakMember<> and UntracedMember<>. Currently, this verification is skipped if the assignment happens in a constructor of GCMixin interface class. NOTE: https://codereview.chromium.org/1414253004/ must be landed before this CL. BUG=550325 ========== to ========== Add use-after-free detector in Oilpan handles (e.g. Member<>). After this CL, HeapObjectHeader and handles will have a parameter "gcGeneration" on ASSERT build. gcGeneration means when the object has been allocated in HeapObjectHeader, and it means when the pointee object should have been allocated in handles. On referring a Member<>, it checks if its gcGeneration is equal to the pointee's gcGeneration. If they have different values, the reference is a use-after-free, even if the pointee is a live object. Currently, this verification is skipped if the assignment happens in a constructor of GCMixin interface class. BUG=550325 ==========
https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/StylePropertySet.cpp (right): https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/StylePropertySet.cpp:45: return sizeof(ImmutableStylePropertySet) - sizeof(void*) + sizeof(RawPtrWillBeMember<CSSValue>) * count + sizeof(StylePropertyMetadata) * count; On 2015/11/10 16:37:05, haraken wrote: > > Let's land this change separately. Done. https://codereview.chromium.org/1433843002/ https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:670: // for gcGeneration, because the object may not be fully allocated. On 2015/11/10 16:37:05, haraken wrote: > > Sorry, I'm getting confused. Why can't we call > heapObjectHeader(obj)->gcGeneration() against a GarbageCollectedMixin object > that is being constructed? heapObjectHeader(obj)->gcGeneration() shouldn't > depend on the vtable of the mixin object, so I think we can just call it... (I > may be missing something.) > obj->heapObjectHeader() (on line#665) depends on the vtable of GCMixin. What we want to do here is to get the head address of the object, that |obj| is pointing to. Because 'T*' is a pointer for GCMixin class, |obj| does not point to the head of the actual object. I think using a virtual method 'heapObjectHeader()' is the only solution, but if we can get the head address without virtual methods, I'm happy to update this. https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:810: // Verify that this handle points a live on-heap object, if a pointer On 2015/11/10 16:37:05, haraken wrote: > > points to Done. https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:838: m_gcGeneration = 0; On 2015/11/10 16:37:05, haraken wrote: > > You won't need this. Given that you're checking if(m_raw && m_gcGeneration) at > line 812, you don't need to reset m_gcGeneration when m_raw becomes 0. This also > means that you don't need to call checkPointer() when assigning nullptr. Agree. Done.
https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:670: // for gcGeneration, because the object may not be fully allocated. On 2015/11/11 02:46:34, peria wrote: > On 2015/11/10 16:37:05, haraken wrote: > > > > Sorry, I'm getting confused. Why can't we call > > heapObjectHeader(obj)->gcGeneration() against a GarbageCollectedMixin object > > that is being constructed? heapObjectHeader(obj)->gcGeneration() shouldn't > > depend on the vtable of the mixin object, so I think we can just call it... (I > > may be missing something.) > > > obj->heapObjectHeader() (on line#665) depends on the vtable of GCMixin. > > What we want to do here is to get the head address of the object, that |obj| is > pointing to. Because 'T*' is a pointer for GCMixin class, |obj| does not point > to the head of the actual object. > I think using a virtual method 'heapObjectHeader()' is the only solution, but if > we can get the head address without virtual methods, I'm happy to update this. Thanks for the clarification; now I understand the issue. Maybe can we introduce HeapObjectHeaderTrait like this? template<typename T> class HeapObjectHeaderTrait<T, true> { public: static HeapObjectHeader* heapObjectHeader(T* obj) { ASSERT(!ThreadState::current()->inMixinConstructor()); return obj->heapObjectHeader(); } static HeapObjectHeader* heapObjectHeaderIfExists(T* obj) { if (ThreadState::current()->inMixinConstructor()) return nullptr; return obj->heapObjectHeader(); } }; If we have the HeapObjectHeaderTrait, we can remove ObjectAliveTrait (in a follow-up CL). https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:858: m_gcGeneration = GarbageCollectedMixinTrait<T>::gcGeneration(m_raw); So you can update the code to: HeapObjectHeader* header = HeapObjectHeaderTrait<T>::heapObjectHeaderIfExists(m_raw); if (!header) return; ASSERT(header->checkHeader()); m_gcGeneration = header->gcGeneration(); ASSERT(!m_gcGeneration); My question is if the code compiles without including T.h... (The above comment implies that the code won't compile without including T.h.)
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:670: // for gcGeneration, because the object may not be fully allocated. On 2015/11/11 04:19:29, haraken wrote: > On 2015/11/11 02:46:34, peria wrote: > > On 2015/11/10 16:37:05, haraken wrote: > > > > > > Sorry, I'm getting confused. Why can't we call > > > heapObjectHeader(obj)->gcGeneration() against a GarbageCollectedMixin object > > > that is being constructed? heapObjectHeader(obj)->gcGeneration() shouldn't > > > depend on the vtable of the mixin object, so I think we can just call it... > (I > > > may be missing something.) > > > > > obj->heapObjectHeader() (on line#665) depends on the vtable of GCMixin. > > > > What we want to do here is to get the head address of the object, that |obj| > is > > pointing to. Because 'T*' is a pointer for GCMixin class, |obj| does not > point > > to the head of the actual object. > > I think using a virtual method 'heapObjectHeader()' is the only solution, but > if > > we can get the head address without virtual methods, I'm happy to update this. > > Thanks for the clarification; now I understand the issue. > > Maybe can we introduce HeapObjectHeaderTrait like this? > > template<typename T> > class HeapObjectHeaderTrait<T, true> { > public: > static HeapObjectHeader* heapObjectHeader(T* obj) { > ASSERT(!ThreadState::current()->inMixinConstructor()); > return obj->heapObjectHeader(); > } > static HeapObjectHeader* heapObjectHeaderIfExists(T* obj) { > if (ThreadState::current()->inMixinConstructor()) > return nullptr; > return obj->heapObjectHeader(); > } > }; > > If we have the HeapObjectHeaderTrait, we can remove ObjectAliveTrait (in a > follow-up CL). SGTM I moved this Trait to Heap.h in PS6. https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:671: if (ThreadState::current()->isGCForbidden()) On 2015/11/10 16:37:05, haraken wrote: > > Let's rename isGCForbidden() to isConstructingGCMixin(). Renaming it looks confusing in other places, so I made it as a new method in PS5. https://codereview.chromium.org/1411603007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Handle.h:858: m_gcGeneration = GarbageCollectedMixinTrait<T>::gcGeneration(m_raw); On 2015/11/11 04:19:29, haraken wrote: > So you can update the code to: > > HeapObjectHeader* header = > HeapObjectHeaderTrait<T>::heapObjectHeaderIfExists(m_raw); > if (!header) > return; > ASSERT(header->checkHeader()); > m_gcGeneration = header->gcGeneration(); > ASSERT(!m_gcGeneration); Acknowledged. > My question is if the code compiles without including T.h... (The above comment > implies that the code won't compile without including T.h.) IIUC, in all files using Member<T>, we need to include T.h to judge if T is a GCed class. Otherwise, GC plugin should stop compiling.
On 2015/11/10 16:37:06, haraken wrote: > I think putGCGeneration() won't be needed. It is needed to be a method to hide a dependency on Heap class, because HeapObjectHeader's constructor is described in HeapPage.h. HeapPage.h is included in Heap.h and thus it does not include Heap.h. So we can't use methods of Heap in HeapPage.h.
Getting closer. https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/GarbageCollected.h (right): https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/GarbageCollected.h:130: virtual bool isHeapObjectAlive() const = 0; In a follow-up, I hope we can remove this. If we have heapObjectHeader(), isHeapObjectAlive() won't be needed. https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:77: class GarbageCollectedMixinTrait<T, true> { I'd rename this to HeapObjectHeaderTrait since this is a Trait for getting a HeapObjectHeader, regardless of whether T is GarbageCollected or GarbageCollectedMixin. https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:80: { Add ASSERT(!ThreadState::current()->isConstructingGCMixin()). https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:97: { Add ASSERT(!ThreadState::current()->isConstructingGCMixin()). https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:174: putGcGeneration(); Actually this is not quite right. Remember that HeapObjectHeader's constructor is called for creating a FreeListEntry (which is a header for the free list entry). The FreeListEntry should not have a valid m_gcGeneration. I think we should add one more parameter 'gcGeneration' to the HeapObjectHeader's constructor and explicitly pass the gcGeneration recorded to the HeapObjectHeader. https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:216: void putGcGeneration(); putGcGeneration => recordGCGeneration https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:227: uint32_t m_gcGeneration; Add a comment. // m_gcGeneration keeps track of the number of GC cycle where the object gets allocated. m_gcGeneration == 0 indicates that the object has already been freed.
Patchset #10 (id:220001) has been deleted
https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/GarbageCollected.h (right): https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/GarbageCollected.h:130: virtual bool isHeapObjectAlive() const = 0; On 2015/11/11 10:03:16, haraken wrote: > > In a follow-up, I hope we can remove this. If we have heapObjectHeader(), > isHeapObjectAlive() won't be needed. Acknowledged. https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:77: class GarbageCollectedMixinTrait<T, true> { On 2015/11/11 10:03:16, haraken wrote: > > I'd rename this to HeapObjectHeaderTrait since this is a Trait for getting a > HeapObjectHeader, regardless of whether T is GarbageCollected or > GarbageCollectedMixin. Done. https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:80: { On 2015/11/11 10:03:16, haraken wrote: > > Add ASSERT(!ThreadState::current()->isConstructingGCMixin()). This method may be called in Member<>::get(), and it can happen in a constructor of another GCMixin class. https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:97: { On 2015/11/11 10:03:16, haraken wrote: > > Add ASSERT(!ThreadState::current()->isConstructingGCMixin()). ditto. https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:174: putGcGeneration(); On 2015/11/11 10:03:16, haraken wrote: > > Actually this is not quite right. Remember that HeapObjectHeader's constructor > is called for creating a FreeListEntry (which is a header for the free list > entry). The FreeListEntry should not have a valid m_gcGeneration. > > I think we should add one more parameter 'gcGeneration' to the > HeapObjectHeader's constructor and explicitly pass the gcGeneration recorded to > the HeapObjectHeader. Done. https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:216: void putGcGeneration(); On 2015/11/11 10:03:16, haraken wrote: > > putGcGeneration => recordGCGeneration I removed this method with making gcGeneration a parameter of this constructor https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:227: uint32_t m_gcGeneration; On 2015/11/11 10:03:16, haraken wrote: > > Add a comment. > > // m_gcGeneration keeps track of the number of GC cycle where the object gets > allocated. m_gcGeneration == 0 indicates that the object has already been freed. Done.
https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:617: HeapObjectHeader* freedHeader = new (NotNull, shrinkAddress) HeapObjectHeader(shrinkSize, header->gcInfoIndex(), Heap::gcGeneration()); This must be 0. https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:206: bool checkHeader() const; Can you insert the following ASSERT to the checkHeader() and make sure that the ASSERT doesn't fire? m_gcGeneration ==0 iff. the object is alive Concretely speaking, something like: if (isFree()) ASSERT(!m_gcGeneration); else ASSERT(m_gcGeneration);
I want your advice again. On ASAN bot, many unit_tests fail with use-after-poison on calling HeapObjectHeader::gcGeneration() for live objects. I think it can be fixed with a simple change, but it needs what I don't know. Do you have any ideas around it? https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:617: HeapObjectHeader* freedHeader = new (NotNull, shrinkAddress) HeapObjectHeader(shrinkSize, header->gcInfoIndex(), Heap::gcGeneration()); On 2015/11/12 15:59:08, haraken wrote: > > This must be 0. Done. https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:206: bool checkHeader() const; On 2015/11/12 15:59:08, haraken wrote: > > Can you insert the following ASSERT to the checkHeader() and make sure that the > ASSERT doesn't fire? > > m_gcGeneration ==0 iff. the object is alive > > Concretely speaking, something like: > > if (isFree()) > ASSERT(!m_gcGeneration); > else > ASSERT(m_gcGeneration); > Done. checkHeader() is used as ASSERT(checkHeader()); in most cases, so I did not put additional ASSERT() in checkHeader().
On 2015/11/13 05:44:38, peria wrote: > I want your advice again. > On ASAN bot, many unit_tests fail with use-after-poison on calling > HeapObjectHeader::gcGeneration() for live objects. > I think it can be fixed with a simple change, but it needs what I don't know. > Do you have any ideas around it? This was fixed with adding NO_SANITIZE_ADDRESS.
Patchset #12 (id:280001) has been deleted
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411603007/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411603007/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
On 2015/11/13 05:44:38, peria wrote: > I want your advice again. > On ASAN bot, many unit_tests fail with use-after-poison on calling > HeapObjectHeader::gcGeneration() for live objects. > I think it can be fixed with a simple change, but it needs what I don't know. > Do you have any ideas around it? > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/HeapPage.cpp:617: HeapObjectHeader* > freedHeader = new (NotNull, shrinkAddress) HeapObjectHeader(shrinkSize, > header->gcInfoIndex(), Heap::gcGeneration()); > On 2015/11/12 15:59:08, haraken wrote: > > > > This must be 0. > > Done. > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/HeapPage.h (right): > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/HeapPage.h:206: bool checkHeader() > const; > On 2015/11/12 15:59:08, haraken wrote: > > > > Can you insert the following ASSERT to the checkHeader() and make sure that > the > > ASSERT doesn't fire? > > > > m_gcGeneration ==0 iff. the object is alive > > > > Concretely speaking, something like: > > > > if (isFree()) > > ASSERT(!m_gcGeneration); > > else > > ASSERT(m_gcGeneration); > > > Done. > checkHeader() is used as > ASSERT(checkHeader()); > in most cases, so I did not put additional ASSERT() in checkHeader(). Then how about: bool checkHeader() { if ((!isFree() || m_gcGeneration) && (isFree() || !m_gcGeneration)) return false; ...; } ? I want to verify the invariant about the m_gcGeneration somehow.
On 2015/11/13 12:13:17, haraken wrote: > On 2015/11/13 05:44:38, peria wrote: > > I want your advice again. > > On ASAN bot, many unit_tests fail with use-after-poison on calling > > HeapObjectHeader::gcGeneration() for live objects. > > I think it can be fixed with a simple change, but it needs what I don't know. > > Do you have any ideas around it? > > > > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): > > > > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > > third_party/WebKit/Source/platform/heap/HeapPage.cpp:617: HeapObjectHeader* > > freedHeader = new (NotNull, shrinkAddress) HeapObjectHeader(shrinkSize, > > header->gcInfoIndex(), Heap::gcGeneration()); > > On 2015/11/12 15:59:08, haraken wrote: > > > > > > This must be 0. > > > > Done. > > > > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/platform/heap/HeapPage.h (right): > > > > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > > third_party/WebKit/Source/platform/heap/HeapPage.h:206: bool checkHeader() > > const; > > On 2015/11/12 15:59:08, haraken wrote: > > > > > > Can you insert the following ASSERT to the checkHeader() and make sure that > > the > > > ASSERT doesn't fire? > > > > > > m_gcGeneration ==0 iff. the object is alive > > > > > > Concretely speaking, something like: > > > > > > if (isFree()) > > > ASSERT(!m_gcGeneration); > > > else > > > ASSERT(m_gcGeneration); > > > > > Done. > > checkHeader() is used as > > ASSERT(checkHeader()); > > in most cases, so I did not put additional ASSERT() in checkHeader(). > > Then how about: > > bool checkHeader() { > if ((!isFree() || m_gcGeneration) && (isFree() || !m_gcGeneration)) > return false; > ...; > } > > ? > > I want to verify the invariant about the m_gcGeneration somehow. Also it seems you need to address some compile failures on the bots. The CL mostly looks good to me.
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411603007/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411603007/320001
On 2015/11/13 12:13:17, haraken wrote: > On 2015/11/13 05:44:38, peria wrote: > > I want your advice again. > > On ASAN bot, many unit_tests fail with use-after-poison on calling > > HeapObjectHeader::gcGeneration() for live objects. > > I think it can be fixed with a simple change, but it needs what I don't know. > > Do you have any ideas around it? > > > > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): > > > > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > > third_party/WebKit/Source/platform/heap/HeapPage.cpp:617: HeapObjectHeader* > > freedHeader = new (NotNull, shrinkAddress) HeapObjectHeader(shrinkSize, > > header->gcInfoIndex(), Heap::gcGeneration()); > > On 2015/11/12 15:59:08, haraken wrote: > > > > > > This must be 0. > > > > Done. > > > > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/platform/heap/HeapPage.h (right): > > > > > https://codereview.chromium.org/1411603007/diff/240001/third_party/WebKit/Sou... > > third_party/WebKit/Source/platform/heap/HeapPage.h:206: bool checkHeader() > > const; > > On 2015/11/12 15:59:08, haraken wrote: > > > > > > Can you insert the following ASSERT to the checkHeader() and make sure that > > the > > > ASSERT doesn't fire? > > > > > > m_gcGeneration ==0 iff. the object is alive > > > > > > Concretely speaking, something like: > > > > > > if (isFree()) > > > ASSERT(!m_gcGeneration); > > > else > > > ASSERT(m_gcGeneration); > > > > > Done. > > checkHeader() is used as > > ASSERT(checkHeader()); > > in most cases, so I did not put additional ASSERT() in checkHeader(). > > Then how about: > > bool checkHeader() { > if ((!isFree() || m_gcGeneration) && (isFree() || !m_gcGeneration)) > return false; > ...; > } > > ? > > I want to verify the invariant about the m_gcGeneration somehow. It makes sense. I don't have a strong opinion to not have ASSERT in checkHeader.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTL
https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:779: if (m_raw && m_gcGeneration) { This needs to be changed to: if (m_raw && m_gcGeneration && !ThreadState::current()->isConstructingGCMixin()) https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:821: // not insist that T's definition is in scope. Update this comment. https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:822: m_gcGeneration = HeapObjectHeaderTrait<T>::gcGeneration(m_raw); Can we change this to: if (ThreadState::current()->isConstructingGCMixin()) m_gcGeneration = 0; else m_gcGeneration = HeapObjectHeaderTrait<T>::heapObjectHeader()->gcGeneration(); ? Then we can completely remove HeapObjectHeaderTrait<T>::gcGeneration. https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:899: // WeakMember may point to a dead object, so we skip the verification. I wonder why this happens. It seems this CL is using too many unsafeGet()s than needed. I'm fine with landing these unsafeGet()s in this CL but most of them should be replaced with get()s in follow-up CLs. https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:80: { Can you add: static_assert(sizeof(T), "T must be fully defined"); ? https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:97: { Can you add: ASSERT(!ThreadState::current()->isConstructingGCMixin()); ? https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:98: return HeapObjectHeader::fromPayload(obj); Can you add: ASSERT(header->checkHeader()); for the returned header? https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:918: new (NotNull, address) HeapObjectHeader(size, gcInfoIndexForFreeListHeader, 0); Can you define: unsigned gcGenerationForFreeListEntry = 0; and use it instead of hard-coding 0 in many places? https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:216: // m_gcGeneration keeps track of the number of GC cycle where the object gets GC cycles gets allocated https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:755: ASSERT(isFree() == (m_gcGeneration == 0)); Looks much nicer than the predicate I suggested! https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:828: inline Address NormalPageHeap::allocateObject(size_t allocationSize, size_t gcInfoIndex, uint32_t generation) Why do we need to pass the generation parameter to allocateObject()? Can't we just call Heap::gcGeneration in allocateObject()?
Patchset #15 (id:360001) has been deleted
https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:779: if (m_raw && m_gcGeneration) { On 2015/11/16 02:47:49, haraken wrote: > > This needs to be changed to: > > if (m_raw && m_gcGeneration && > !ThreadState::current()->isConstructingGCMixin()) > It will make this part less safety. We can check Member<GCedClass>::get() while we're constructing a GCMixin class object. https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:821: // not insist that T's definition is in scope. On 2015/11/16 02:47:49, haraken wrote: > > Update this comment. Done. https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:822: m_gcGeneration = HeapObjectHeaderTrait<T>::gcGeneration(m_raw); On 2015/11/16 02:47:49, haraken wrote: > > Can we change this to: > > if (ThreadState::current()->isConstructingGCMixin()) > m_gcGeneration = 0; > else > m_gcGeneration = > HeapObjectHeaderTrait<T>::heapObjectHeader()->gcGeneration(); > > ? > > Then we can completely remove HeapObjectHeaderTrait<T>::gcGeneration. We can call HOHTrait<GCedClass>::gcGeneration() correctly while we're constructing a GCMixin class object. If we want to remove Trait<>::gcGeneration(), we have to change this line to if (ThreadState::current()->isConstructingGCMixin() && IsGarabgeCollectedMixin<T>::value) m_gcGeneration = 0; else m_gcGeneration = HeapObjectHeaderTrait<T>::heapObjectHeader()->gcGeneration(); and need similar work in get(). https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:899: // WeakMember may point to a dead object, so we skip the verification. On 2015/11/16 02:47:49, haraken wrote: > > I wonder why this happens. It seems this CL is using too many unsafeGet()s than > needed. It happens in weak processing of HeapHashSet<WeakMember<>>, and I agree this CL uses unsafeGet() more generally than needed. > I'm fine with landing these unsafeGet()s in this CL but most of them should be > replaced with get()s in follow-up CLs. I'll do it. https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:80: { On 2015/11/16 02:47:49, haraken wrote: > > Can you add: > > static_assert(sizeof(T), "T must be fully defined"); > > ? Done. https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:97: { On 2015/11/16 02:47:50, haraken wrote: > > Can you add: > > ASSERT(!ThreadState::current()->isConstructingGCMixin()); > > ? We did the same discussion in #16 https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:98: return HeapObjectHeader::fromPayload(obj); On 2015/11/16 02:47:49, haraken wrote: > > Can you add: > > ASSERT(header->checkHeader()); > > for the returned header? It is called in HeapObjectHeader::fromPayload(). https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:918: new (NotNull, address) HeapObjectHeader(size, gcInfoIndexForFreeListHeader, 0); On 2015/11/16 02:47:50, haraken wrote: > > Can you define: > > unsigned gcGenerationForFreeListEntry = 0; > > and use it instead of hard-coding 0 in many places? Done. https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:216: // m_gcGeneration keeps track of the number of GC cycle where the object gets On 2015/11/16 02:47:50, haraken wrote: > > GC cycles > > gets allocated Done. https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:755: ASSERT(isFree() == (m_gcGeneration == 0)); On 2015/11/16 02:47:50, haraken wrote: > > Looks much nicer than the predicate I suggested! :) https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:828: inline Address NormalPageHeap::allocateObject(size_t allocationSize, size_t gcInfoIndex, uint32_t generation) On 2015/11/16 02:47:50, haraken wrote: > > Why do we need to pass the generation parameter to allocateObject()? Can't we > just call Heap::gcGeneration in allocateObject()? This is just a dependency problem of .h files. We already have a strong dependency from Heap.h to HeapPage.h, so we can't make a dependency from HeapPage.h to Heap.h.
https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:80: { On 2015/11/12 14:38:42, peria wrote: > On 2015/11/11 10:03:16, haraken wrote: > > > > Add ASSERT(!ThreadState::current()->isConstructingGCMixin()). > > This method may be called in Member<>::get(), and > it can happen in a constructor of another GCMixin class. Isn't it unsafe? If obj->heapObjectHeader() is called while constructing a mixin class, the behavior would be undefined. My point is that HeapObjectHeader::heapObjectHeader() should not be called when ThreadState::current()->isConstructingGCMixin() is true. https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:780: ASSERT(m_gcGeneration == HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw)->gcGeneration()); Why is this safe? The behavior of HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw) should be undefined if this is called while constructing a mixin object.
denice1302@gmail.com changed reviewers: + denice1302@gmail.com
https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:80: { On 2015/11/16 06:20:30, haraken wrote: > On 2015/11/12 14:38:42, peria wrote: > > On 2015/11/11 10:03:16, haraken wrote: > > > > > > Add ASSERT(!ThreadState::current()->isConstructingGCMixin()). > > > > This method may be called in Member<>::get(), and > > it can happen in a constructor of another GCMixin class. > > Isn't it unsafe? If obj->heapObjectHeader() is called while constructing a mixin > class, the behavior would be undefined. > > My point is that HeapObjectHeader::heapObjectHeader() should not be called when > ThreadState::current()->isConstructingGCMixin() is true. If an object which is being constructed is an argument of this call, it is unsafe and must be avoided. But it doesn't mean to prevent to call this method while ThreadState::current()->isConstructingGCMixin() is true. In following code, we should be able to call HeapObjectHeaderTrait<X>::heapObjectHeader(x) in the constructor of A. struct X : public GarbageCollectedMixin { ... }; struct Y : public GarbageCollected<Y>, public X { ... }; struct A : public GarbageCollectedMixin { A() : x(new Y) { ASSERT(HeapObjectHeadertrait<X>::heapObjectHeader(x)); } Member x; }; struct B : public GarbageCollected<B>, public A { ... }; https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:780: ASSERT(m_gcGeneration == HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw)->gcGeneration()); On 2015/11/16 06:20:30, haraken wrote: > > Why is this safe? The behavior of > HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw) should be undefined if this is > called while constructing a mixin object. > Please consider the following code. We cannot refer HeapObjectHeader with m_a at (1), as you expect. It fails with a pure virtual function call. And we can refer m_x correctly at (2), because its construction (of class Y) has been completed. Why can we refer m_a safely at (3)? It is because its m_gcGeneration must be 0 and it does not call the ASSERT(). struct X : public GarbageCollectedMixin { ... }; struct Y : public GarbageCollected<Y>, public X { ... }; struct A : public GarbageCollectedMixin { A() : m_x(new Y), m_a(this) { // HeapObjectHeaderTrait<T>::heapObjectHeader(m_a); // ... (1) m_x.get(); // ... (2) m_a.get(); // ... (3) } Member<X> m_x; Member<A> m_a; }; struct B : public GarbageCollected<B>, public A { B() : A() { } }
On 2015/11/16 07:04:42, peria wrote: > https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/Heap.h (right): > > https://codereview.chromium.org/1411603007/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/Heap.h:80: { > On 2015/11/16 06:20:30, haraken wrote: > > On 2015/11/12 14:38:42, peria wrote: > > > On 2015/11/11 10:03:16, haraken wrote: > > > > > > > > Add ASSERT(!ThreadState::current()->isConstructingGCMixin()). > > > > > > This method may be called in Member<>::get(), and > > > it can happen in a constructor of another GCMixin class. > > > > Isn't it unsafe? If obj->heapObjectHeader() is called while constructing a > mixin > > class, the behavior would be undefined. > > > > My point is that HeapObjectHeader::heapObjectHeader() should not be called > when > > ThreadState::current()->isConstructingGCMixin() is true. > > If an object which is being constructed is an argument of this call, > it is unsafe and must be avoided. > But it doesn't mean to prevent to call this method while > ThreadState::current()->isConstructingGCMixin() is true. > In following code, we should be able to call > HeapObjectHeaderTrait<X>::heapObjectHeader(x) in the constructor of A. > > > struct X : public GarbageCollectedMixin { ... }; > struct Y : public GarbageCollected<Y>, public X { ... }; > struct A : public GarbageCollectedMixin { > A() : x(new Y) { > ASSERT(HeapObjectHeadertrait<X>::heapObjectHeader(x)); > } > Member x; > }; > struct B : public GarbageCollected<B>, public A { ... }; > > https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/Handle.h (right): > > https://codereview.chromium.org/1411603007/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/Handle.h:780: ASSERT(m_gcGeneration == > HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw)->gcGeneration()); > On 2015/11/16 06:20:30, haraken wrote: > > > > Why is this safe? The behavior of > > HeapObjectHeaderTrait<T>::heapObjectHeader(m_raw) should be undefined if this > is > > called while constructing a mixin object. > > > > Please consider the following code. > > We cannot refer HeapObjectHeader with m_a at (1), as you expect. It fails with > a pure virtual function call. > And we can refer m_x correctly at (2), because its construction (of class Y) has > been completed. > Why can we refer m_a safely at (3)? It is because its m_gcGeneration must be 0 > and it does not call the ASSERT(). > > > struct X : public GarbageCollectedMixin { ... }; > struct Y : public GarbageCollected<Y>, public X { ... }; > > struct A : public GarbageCollectedMixin { > A() : m_x(new Y), m_a(this) { > // HeapObjectHeaderTrait<T>::heapObjectHeader(m_a); // ... (1) > m_x.get(); // ... (2) > m_a.get(); // ... (3) > } > Member<X> m_x; > Member<A> m_a; > }; > struct B : public GarbageCollected<B>, public A { > B() : A() { } > } Discussed with haraken, and it turned out that his suggestion is correct. In my examples, X can have a Member<A>, and it can be registered 'this' in A's constructor. In this case, a pure virtual function call happens. Thank you, haraken.
PTL
Thanks for being persistent on this! I had to add a couple of additional comments (see Handle.h)... It's hard to get this verification right :-/ https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:779: if (m_raw && m_gcGeneration && !ThreadState::current()->isConstructingGCMixin()) { I begin to think that we should introduce gcGenerationUnchecked. In other words, we should distinguish the fact that the Member is pointing to an already-freed object (<== This is a use-after-free bug!) from the fact that the gcGeneration doesn't need to be checked (e.g., cannot be checked while constructing an object). Then this check should be: m_gcGeneration != gcGenerationUnchecked https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:789: // So do NOT use it, if you do not know what it means. I believe that unsafeGet() should not be needed. You don't need to address it in this CL but let's add a TODO. // TODO: We should remove this. https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:824: m_gcGeneration = header->gcGeneration(); Who assigns 0 to m_gcGeneration when we fail at getting a valid gcGeneration (e.g., m_raw == 0, ThreadState::current()->isConstructingGCMixin() is true etc)? I think the invariant should be: m_raw == 0 or we don't want to check the gcGeneration for some reason iff. m_gcGeneration == gcGenerationUnchecked use-after-free iff. m_gcGeneration == gcGenerationForFreeListEntry valid otherwise https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:50: // TODO(peria): Refactor following two sets of template. templates https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:82: // We can use this method for other GCMixin objects. Slightly better: // TODO(peria): This ASSERT is too restrictive. The ASSERT forbids to call heapObjectHeader() for an object while another (totally independent) mixin object is under construction. https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:92: { Add ASSERT(!ThreadState::current()->isConstructingGCMixin()), just in case. https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:93: if (!IsFullyDefined<T>::value) What happens if you use static_assert(sizeof(T)) ? https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:831: inline Address NormalPageHeap::allocateObject(size_t allocationSize, size_t gcInfoIndex, uint32_t generation) gcGeneration
https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:779: if (m_raw && m_gcGeneration && !ThreadState::current()->isConstructingGCMixin()) { On 2015/11/16 09:38:20, haraken wrote: > > I begin to think that we should introduce gcGenerationUnchecked. In other words, > we should distinguish the fact that the Member is pointing to an already-freed > object (<== This is a use-after-free bug!) from the fact that the gcGeneration > doesn't need to be checked (e.g., cannot be checked while constructing an > object). > > Then this check should be: > > m_gcGeneration != gcGenerationUnchecked Done. https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:789: // So do NOT use it, if you do not know what it means. On 2015/11/16 09:38:20, haraken wrote: > > I believe that unsafeGet() should not be needed. You don't need to address it in > this CL but let's add a TODO. > > // TODO: We should remove this. Yes, let's. https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:824: m_gcGeneration = header->gcGeneration(); On 2015/11/16 09:38:20, haraken wrote: > > Who assigns 0 to m_gcGeneration when we fail at getting a valid gcGeneration > (e.g., m_raw == 0, ThreadState::current()->isConstructingGCMixin() is true etc)? Oops. Set it by default. > I think the invariant should be: > > m_raw == 0 or we don't want to check the gcGeneration for some reason iff. > m_gcGeneration == gcGenerationUnchecked > > use-after-free iff. m_gcGeneration == gcGenerationForFreeListEntry > > valid otherwise It should be if m_raw == 0 or m_gcGeneration == gcGenerationUnchecked: skip verification if m_gcGeneration == gcGenerationForFreeListEntry: invalid if m_gcGeneration == HeapObjectHeader::gcGeneration: valid otherwise: invalid 'otherwise' includes that a new object of the same class is allocated on the same address. https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:50: // TODO(peria): Refactor following two sets of template. On 2015/11/16 09:38:20, haraken wrote: > > templates Done. https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:82: // We can use this method for other GCMixin objects. On 2015/11/16 09:38:20, haraken wrote: > > Slightly better: > > // TODO(peria): This ASSERT is too restrictive. The ASSERT forbids to call > heapObjectHeader() for an object while another (totally independent) mixin > object is under construction. Done. https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:92: { On 2015/11/16 09:38:20, haraken wrote: > > Add ASSERT(!ThreadState::current()->isConstructingGCMixin()), just in case. Done. https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:93: if (!IsFullyDefined<T>::value) On 2015/11/16 09:38:20, haraken wrote: > > What happens if you use static_assert(sizeof(T)) ? It fails in compiling some header files which calls Mebmer<>::get() for a forward declared class. e.g. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:831: inline Address NormalPageHeap::allocateObject(size_t allocationSize, size_t gcInfoIndex, uint32_t generation) On 2015/11/16 09:38:21, haraken wrote: > > gcGeneration Done.
On 2015/11/16 12:28:45, peria wrote: > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/Handle.h (right): > > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/Handle.h:779: if (m_raw && > m_gcGeneration && !ThreadState::current()->isConstructingGCMixin()) { > On 2015/11/16 09:38:20, haraken wrote: > > > > I begin to think that we should introduce gcGenerationUnchecked. In other > words, > > we should distinguish the fact that the Member is pointing to an already-freed > > object (<== This is a use-after-free bug!) from the fact that the gcGeneration > > doesn't need to be checked (e.g., cannot be checked while constructing an > > object). > > > > Then this check should be: > > > > m_gcGeneration != gcGenerationUnchecked > > Done. > > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/Handle.h:789: // So do NOT use it, if > you do not know what it means. > On 2015/11/16 09:38:20, haraken wrote: > > > > I believe that unsafeGet() should not be needed. You don't need to address it > in > > this CL but let's add a TODO. > > > > // TODO: We should remove this. > > Yes, let's. > > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/Handle.h:824: m_gcGeneration = > header->gcGeneration(); > On 2015/11/16 09:38:20, haraken wrote: > > > > Who assigns 0 to m_gcGeneration when we fail at getting a valid gcGeneration > > (e.g., m_raw == 0, ThreadState::current()->isConstructingGCMixin() is true > etc)? > > Oops. > Set it by default. > > > I think the invariant should be: > > > > m_raw == 0 or we don't want to check the gcGeneration for some reason iff. > > m_gcGeneration == gcGenerationUnchecked > > > > use-after-free iff. m_gcGeneration == gcGenerationForFreeListEntry > > > > valid otherwise > > It should be > > if m_raw == 0 or m_gcGeneration == gcGenerationUnchecked: > skip verification > if m_gcGeneration == gcGenerationForFreeListEntry: > invalid > if m_gcGeneration == HeapObjectHeader::gcGeneration: > valid > otherwise: > invalid > > 'otherwise' includes that a new object of the same class is allocated > on the same address. Thanks, your condition looks correct. > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/Heap.h (right): > > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/Heap.h:50: // TODO(peria): Refactor > following two sets of template. > On 2015/11/16 09:38:20, haraken wrote: > > > > templates > > Done. > > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/Heap.h:82: // We can use this method for > other GCMixin objects. > On 2015/11/16 09:38:20, haraken wrote: > > > > Slightly better: > > > > // TODO(peria): This ASSERT is too restrictive. The ASSERT forbids to call > > heapObjectHeader() for an object while another (totally independent) mixin > > object is under construction. > > Done. > > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/Heap.h:92: { > On 2015/11/16 09:38:20, haraken wrote: > > > > Add ASSERT(!ThreadState::current()->isConstructingGCMixin()), just in case. > > Done. > > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/Heap.h:93: if > (!IsFullyDefined<T>::value) > On 2015/11/16 09:38:20, haraken wrote: > > > > What happens if you use static_assert(sizeof(T)) ? > > It fails in compiling some header files which calls Mebmer<>::get() > for a forward declared class. > > e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Sorry for digging, but how much? If the number of files is limited, I want to add the #includes. If T is not fully defined when the HeapObjectHeaderTrait is instantiated, I'm afraid that it has a risk of instantiating a wrong HeapObjectHeaderTrait. > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/HeapPage.h (right): > > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/HeapPage.h:831: inline Address > NormalPageHeap::allocateObject(size_t allocationSize, size_t gcInfoIndex, > uint32_t generation) > On 2015/11/16 09:38:21, haraken wrote: > > > > gcGeneration > > Done.
On 2015/11/16 12:32:34, haraken wrote: > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > > third_party/WebKit/Source/platform/heap/Heap.h:93: if > > (!IsFullyDefined<T>::value) > > On 2015/11/16 09:38:20, haraken wrote: > > > > > > What happens if you use static_assert(sizeof(T)) ? > > > > It fails in compiling some header files which calls Mebmer<>::get() > > for a forward declared class. > > > > e.g. > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Sorry for digging, but how much? If the number of files is limited, I want to > add the #includes. > > If T is not fully defined when the HeapObjectHeaderTrait is instantiated, I'm > afraid that it has a risk of instantiating a wrong HeapObjectHeaderTrait. > At least 24 classes are used with forward declarations in 86 files.
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411603007/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411603007/450001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/11/16 12:53:29, peria wrote: > On 2015/11/16 12:32:34, haraken wrote: > > > https://codereview.chromium.org/1411603007/diff/430001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/platform/heap/Heap.h:93: if > > > (!IsFullyDefined<T>::value) > > > On 2015/11/16 09:38:20, haraken wrote: > > > > > > > > What happens if you use static_assert(sizeof(T)) ? > > > > > > It fails in compiling some header files which calls Mebmer<>::get() > > > for a forward declared class. > > > > > > e.g. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > Sorry for digging, but how much? If the number of files is limited, I want to > > add the #includes. > > > > If T is not fully defined when the HeapObjectHeaderTrait is instantiated, I'm > > afraid that it has a risk of instantiating a wrong HeapObjectHeaderTrait. > > > > At least 24 classes are used with forward declarations in 86 files. Hmm. Let's ask yutak@ for reviewing this part tomorrow. I'm not quite sure if it's safe to instantiate HeapObjectHeader without having a fully-defined T. If it is safe, I think you can just the if(!IsFullyDefined<T>::value) check because the subsequent HeapObjectHeader::fromPayload(obj) does not depend on T. Other parts looks good.
peria@chromium.org changed reviewers: + yutak@chromium.org
Yuta-san, This part needs your help as a template expert. https://codereview.chromium.org/1411603007/diff/450001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/450001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:90: class HeapObjectHeaderTrait<T, false> { Yuta-san, Is it safe to instantiate HeapObjectHeaderTrait without having a fully-defined T?
https://codereview.chromium.org/1411603007/diff/450001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/450001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:90: class HeapObjectHeaderTrait<T, false> { On 2015/11/17 at 01:36:49, peria wrote: > Yuta-san, > Is it safe to instantiate HeapObjectHeaderTrait without having a fully-defined T? If the lines 95-96 are NOT present, I think it's safe, because there's nothing in this specific specialization depends on whether T is fully defined or not; in other words, this specialization only depends on the existance of T* and T* -> void* conversion, both of which can be instantiated without T's full declaration. With lines 95-96 things get more complex, because this piece of code changes the behavior depending on whether T is fully defined *at the point of first instantiation*. Consider the following example: class A; bool a1 = IsFullyDefined<A>::value; class A { }; bool a2 = IsFullyDefined<A>::value; The values of a1 and a2 are *both* false, because IsFullyDefined<A>::value is instantiated at the place of its first use, and not again on the later uses. Therefore, your code with lines 95-96 would effectively cause the heapObjectHeader function to return nullptr for some random "T"s and return a valid header for the other "T"s. You probably don't want that behavior, nor lines 95-96, I guess.
On 2015/11/17 06:25:13, Yuta Kitamura wrote: > https://codereview.chromium.org/1411603007/diff/450001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/Heap.h (right): > > https://codereview.chromium.org/1411603007/diff/450001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/Heap.h:90: class > HeapObjectHeaderTrait<T, false> { > On 2015/11/17 at 01:36:49, peria wrote: > > Yuta-san, > > Is it safe to instantiate HeapObjectHeaderTrait without having a fully-defined > T? > > If the lines 95-96 are NOT present, I think it's safe, because there's nothing > in > this specific specialization depends on whether T is fully defined or not; in > other > words, this specialization only depends on the existance of T* and > T* -> void* conversion, both of which can be instantiated without T's full > declaration. > > With lines 95-96 things get more complex, because this piece of code changes the > behavior > depending on whether T is fully defined *at the point of first instantiation*. > > Consider the following example: > > class A; > bool a1 = IsFullyDefined<A>::value; > class A { }; > bool a2 = IsFullyDefined<A>::value; > > The values of a1 and a2 are *both* false, because IsFullyDefined<A>::value is > instantiated > at the place of its first use, and not again on the later uses. > > Therefore, your code with lines 95-96 would effectively cause the > heapObjectHeader > function to return nullptr for some random "T"s and return a valid header for > the other > "T"s. You probably don't want that behavior, nor lines 95-96, I guess. Discussed offline. We've agreed that we should require including T.h when using Member<T>::checkPointer() or Member<T>::get(). (This wouldn't be a strange requirement given that RefPtr<T> requires including T.h.)
https://codereview.chromium.org/1411603007/diff/450001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/450001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:90: class HeapObjectHeaderTrait<T, false> { On 2015/11/17 06:25:12, Yuta Kitamura wrote: > On 2015/11/17 at 01:36:49, peria wrote: > > Yuta-san, > > Is it safe to instantiate HeapObjectHeaderTrait without having a fully-defined > T? > > If the lines 95-96 are NOT present, I think it's safe, because there's nothing > in > this specific specialization depends on whether T is fully defined or not; in > other > words, this specialization only depends on the existance of T* and > T* -> void* conversion, both of which can be instantiated without T's full > declaration. > > With lines 95-96 things get more complex, because this piece of code changes the > behavior > depending on whether T is fully defined *at the point of first instantiation*. > > Consider the following example: > > class A; > bool a1 = IsFullyDefined<A>::value; > class A { }; > bool a2 = IsFullyDefined<A>::value; > > The values of a1 and a2 are *both* false, because IsFullyDefined<A>::value is > instantiated > at the place of its first use, and not again on the later uses. > > Therefore, your code with lines 95-96 would effectively cause the > heapObjectHeader > function to return nullptr for some random "T"s and return a valid header for > the other > "T"s. You probably don't want that behavior, nor lines 95-96, I guess. Thank you for the clarification and additional explanation in off-line communication. As you said, using IsFullyDefined<A>::value has another problem in link phase. (Linker will choose which instantiation to link.) As a result, I will add #include to define T and put a static assert to check it here.
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411603007/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411603007/470001
Description was changed from ========== Add use-after-free detector in Oilpan handles (e.g. Member<>). After this CL, HeapObjectHeader and handles will have a parameter "gcGeneration" on ASSERT build. gcGeneration means when the object has been allocated in HeapObjectHeader, and it means when the pointee object should have been allocated in handles. On referring a Member<>, it checks if its gcGeneration is equal to the pointee's gcGeneration. If they have different values, the reference is a use-after-free, even if the pointee is a live object. Currently, this verification is skipped if the assignment happens in a constructor of GCMixin interface class. BUG=550325 ========== to ========== Add use-after-free detector in Member<> and its subclasses. After this CL, HeapObjectHeader and Member<>s will have a new parameter "gcGeneration". In HeapObjectHeader, gcGeneration means when the object has been allocated, and in Member<>, it means when the pointee object should have been allocated. On ASAN builds, Member<> checks if its gcGeneration is equal to the pointee's gcGeneration on every reference. If they have different values, the reference is a use-after-free, even if the pointee is a live object. Note: currently, this verification is skipped if the assignment happens in a constructor of GCMixin interface class. BUG=550325 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/11/17 07:24:57, peria wrote: > https://codereview.chromium.org/1411603007/diff/450001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/Heap.h (right): > > https://codereview.chromium.org/1411603007/diff/450001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/Heap.h:90: class > HeapObjectHeaderTrait<T, false> { > On 2015/11/17 06:25:12, Yuta Kitamura wrote: > > On 2015/11/17 at 01:36:49, peria wrote: > > > Yuta-san, > > > Is it safe to instantiate HeapObjectHeaderTrait without having a > fully-defined > > T? > > > > If the lines 95-96 are NOT present, I think it's safe, because there's nothing > > in > > this specific specialization depends on whether T is fully defined or not; in > > other > > words, this specialization only depends on the existance of T* and > > T* -> void* conversion, both of which can be instantiated without T's full > > declaration. > > > > With lines 95-96 things get more complex, because this piece of code changes > the > > behavior > > depending on whether T is fully defined *at the point of first instantiation*. > > > > Consider the following example: > > > > class A; > > bool a1 = IsFullyDefined<A>::value; > > class A { }; > > bool a2 = IsFullyDefined<A>::value; > > > > The values of a1 and a2 are *both* false, because IsFullyDefined<A>::value is > > instantiated > > at the place of its first use, and not again on the later uses. > > > > Therefore, your code with lines 95-96 would effectively cause the > > heapObjectHeader > > function to return nullptr for some random "T"s and return a valid header for > > the other > > "T"s. You probably don't want that behavior, nor lines 95-96, I guess. > > Thank you for the clarification and additional explanation in off-line > communication. > As you said, using IsFullyDefined<A>::value has another problem in link phase. > (Linker will choose which instantiation to link.) > > As a result, I will add #include to define T and put a static assert to check it > here. Done. Now HeapObjectHeaderTrait requires full definition of T. haraken@ could you take a look?
Thanks for the long journey. LGTM. https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:803: void checkPointer() In a follow-up CL, can you rename this to setPointer(void* raw)? I think it's better to redirect all pointer assignments to the setPointer, instead of calling checkPointer() at every assignment. https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:819: // nullptr, the verification is skipped. gcGenerationForFreeListEntry => gcGenerationUnchecked https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:822: // of a GCMixin object. We set gcGenerationUnchecked while constructing a GC mixin object (and skip the verification unfortunately) because HeapObjectHeaderTrait<T>::heapObjectHeader doesn't work before the vtable of the object is complete. https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:826: if (IsFullyDefined<T>::value && !ThreadState::current()->isConstructingGCMixin()) { Remove the IsFullyDefined<T>::value check. As we discussed, it is wrong to use IsFullyDefined<T>::value dynamically. https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:460: // 0 and 1 are used to figure specific states in gcGeneration. to represent special states https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:290: static uint32_t gcGeneration() { return s_gcGeneration; } Can we add: ASSERT(s_gcGeneration != gcGenerationUnchecked); ASSERT(s_gcGeneration != gcGenerationForFreeListEntry); ? https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:761: ASSERT(isFree() == (m_gcGeneration == gcGenerationForFreeListEntry)); Can we add ASSERT(m_gcGeneration != gcGenerationUnchecked)?
https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:803: void checkPointer() On 2015/11/20 01:59:06, haraken wrote: > > In a follow-up CL, can you rename this to setPointer(void* raw)? I think it's > better to redirect all pointer assignments to the setPointer, instead of calling > checkPointer() at every assignment. > Acknowledged. https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:819: // nullptr, the verification is skipped. On 2015/11/20 01:59:06, haraken wrote: > > gcGenerationForFreeListEntry => gcGenerationUnchecked Done. https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:826: if (IsFullyDefined<T>::value && !ThreadState::current()->isConstructingGCMixin()) { On 2015/11/20 01:59:06, haraken wrote: > > Remove the IsFullyDefined<T>::value check. As we discussed, it is wrong to use > IsFullyDefined<T>::value dynamically. Done. And it is checked statically at the beginning of this method. https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:460: // 0 and 1 are used to figure specific states in gcGeneration. On 2015/11/20 01:59:06, haraken wrote: > > to represent special states Done. https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:290: static uint32_t gcGeneration() { return s_gcGeneration; } On 2015/11/20 01:59:06, haraken wrote: > > Can we add: > > ASSERT(s_gcGeneration != gcGenerationUnchecked); > ASSERT(s_gcGeneration != gcGenerationForFreeListEntry); > > ? Done. https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:761: ASSERT(isFree() == (m_gcGeneration == gcGenerationForFreeListEntry)); On 2015/11/20 01:59:06, haraken wrote: > > Can we add ASSERT(m_gcGeneration != gcGenerationUnchecked)? Done.
forgot to reply to a comment. https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Handle.h (right): https://codereview.chromium.org/1411603007/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Handle.h:822: // of a GCMixin object. On 2015/11/20 01:59:06, haraken wrote: > > We set gcGenerationUnchecked while constructing a GC mixin object (and skip the > verification unfortunately) because HeapObjectHeaderTrait<T>::heapObjectHeader > doesn't work before the vtable of the object is complete. Done.
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411603007/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411603007/490001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #23 (id:530001) has been deleted
Update the patch. PS22 was a prototype to try to find HeapObjectHeader without HeapObjectHeaderTrait. A few browser tests fail with ThreadState::isConstructingGCMixin(). PS23 is after cleaning up. PTAL, haraken@.
On 2015/11/25 02:01:58, peria wrote:
> Update the patch.
> PS22 was a prototype to try to find HeapObjectHeader without
> HeapObjectHeaderTrait. A few browser tests fail with
> ThreadState::isConstructingGCMixin().
> PS23 is after cleaning up.
>
>
> PTAL, haraken@.
I think this is a bug of the WebAudio code.
#0 0x8f5b0d9 in isConstructingGCMixin
third_party/WebKit/Source/platform/heap/ThreadState.h:311:49
#1 0x8f5b0d9 in blink::Member<blink::AudioDestinationNode>::get() const
third_party/WebKit/Source/platform/heap/Handle.h:779
#2 0x9621287 in destination
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:114:56
#3 0x9621287 in isDestinationInitialized
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:106
#4 0x9621287 in blink::AudioDestinationHandler::render(blink::AudioBus*,
blink::AudioBus*, unsigned long)
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp:59
#5 0x11aff896 in fillBuffer
third_party/WebKit/Source/platform/audio/AudioPullFIFO.cpp:65:9
#6 0x11aff896 in blink::AudioPullFIFO::consume(blink::AudioBus*, unsigned
long) third_party/WebKit/Source/platform/audio/AudioPullFIFO.cpp:52
#7 0x11afd7c7 in blink::AudioDestination::render(blink::WebVector<float*>
const&, blink::WebVector<float*> const&, unsigned long)
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:164:5
#8 0xcf8f433 in
content::RendererWebAudioDeviceImpl::Render(media::AudioBus*, int)
content/renderer/media/renderer_webaudiodevice_impl.cc:115:3
#9 0x1092908b in
media::AudioOutputDevice::AudioThreadCallback::Process(unsigned int)
media/audio/audio_output_device.cc:423:3
#10 0x10aa0609 in media::AudioDeviceThread::Thread::Run()
media/audio/audio_device_thread.cc:183:9
#11 0x10aa02d7 in media::AudioDeviceThread::Thread::ThreadMain()
media/audio/audio_device_thread.cc:158:3
#12 0x328449a in base::(anonymous namespace)::ThreadFunc(void*)
base/threading/platform_thread_posix.cc:64:3
[18319:18339:1124/081401:WARNING:audio_sync_reader.cc(97)] AudioSyncReader::Read
timed out, audio glitch count=13
[18319:18319:1124/081401:INFO:CONSOLE(0)] "The provided value 'undefined' is not
a valid enum value of type OscillatorType.", source:
chrome-extension://ddchlicdkolnonkihahngkmmmjnjlkkf/end_to_end_sender.html?port=50813&aesKey=30313233343536373839616263646566&aesIvMask=66656463626139383736353433323130
(0)
#13 0x7f49e60dbe99 in start_thread
/build/buildd/eglibc-2.15/nptl/pthread_create.c:308
ThreadState::isConstructingGCMixin is crashing because Member::get is called by
the audio thread, which is not attached to Oilpan. The audio thread must not use
Members.
With or without oilpan, this seems problematic because the access to
AbstractAudioContext::m_destinationNode is causing a threading race. The audio
thread is accessing the m_destinationNode without acquiring a graph lock.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
I'll file a bug to the audio folks.
https://codereview.chromium.org/1411603007/diff/570001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1411603007/diff/570001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.cpp:1310: HeapObjectHeader* NormalPage::findHeaderFromObject(const void* obj) I'm afraid that this method would be super heavy. How much does the cycle time of layout tests increase when you enable the use-after-free detection?
On 2015/11/25 02:37:04, haraken wrote: > On 2015/11/25 02:01:58, peria wrote: > > Update the patch. > > PS22 was a prototype to try to find HeapObjectHeader without > > HeapObjectHeaderTrait. A few browser tests fail with > > ThreadState::isConstructingGCMixin(). > > PS23 is after cleaning up. > > > > > > PTAL, haraken@. > > I think this is a bug of the WebAudio code. > > #0 0x8f5b0d9 in isConstructingGCMixin > third_party/WebKit/Source/platform/heap/ThreadState.h:311:49 > #1 0x8f5b0d9 in blink::Member<blink::AudioDestinationNode>::get() const > third_party/WebKit/Source/platform/heap/Handle.h:779 > #2 0x9621287 in destination > third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:114:56 > #3 0x9621287 in isDestinationInitialized > third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:106 > #4 0x9621287 in blink::AudioDestinationHandler::render(blink::AudioBus*, > blink::AudioBus*, unsigned long) > third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp:59 > #5 0x11aff896 in fillBuffer > third_party/WebKit/Source/platform/audio/AudioPullFIFO.cpp:65:9 > #6 0x11aff896 in blink::AudioPullFIFO::consume(blink::AudioBus*, unsigned > long) third_party/WebKit/Source/platform/audio/AudioPullFIFO.cpp:52 > #7 0x11afd7c7 in blink::AudioDestination::render(blink::WebVector<float*> > const&, blink::WebVector<float*> const&, unsigned long) > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:164:5 > #8 0xcf8f433 in > content::RendererWebAudioDeviceImpl::Render(media::AudioBus*, int) > content/renderer/media/renderer_webaudiodevice_impl.cc:115:3 > #9 0x1092908b in > media::AudioOutputDevice::AudioThreadCallback::Process(unsigned int) > media/audio/audio_output_device.cc:423:3 > #10 0x10aa0609 in media::AudioDeviceThread::Thread::Run() > media/audio/audio_device_thread.cc:183:9 > #11 0x10aa02d7 in media::AudioDeviceThread::Thread::ThreadMain() > media/audio/audio_device_thread.cc:158:3 > #12 0x328449a in base::(anonymous namespace)::ThreadFunc(void*) > base/threading/platform_thread_posix.cc:64:3 > [18319:18339:1124/081401:WARNING:audio_sync_reader.cc(97)] AudioSyncReader::Read > timed out, audio glitch count=13 > [18319:18319:1124/081401:INFO:CONSOLE(0)] "The provided value 'undefined' is not > a valid enum value of type OscillatorType.", source: > chrome-extension://ddchlicdkolnonkihahngkmmmjnjlkkf/end_to_end_sender.html?port=50813&aesKey=30313233343536373839616263646566&aesIvMask=66656463626139383736353433323130 > (0) > #13 0x7f49e60dbe99 in start_thread > /build/buildd/eglibc-2.15/nptl/pthread_create.c:308 > > ThreadState::isConstructingGCMixin is crashing because Member::get is called by > the audio thread, which is not attached to Oilpan. The audio thread must not use > Members. > > With or without oilpan, this seems problematic because the access to > AbstractAudioContext::m_destinationNode is causing a threading race. The audio > thread is accessing the m_destinationNode without acquiring a graph lock. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > I'll file a bug to the audio folks. Thank you for this investigation!
On 2015/11/25 02:38:54, haraken wrote: > https://codereview.chromium.org/1411603007/diff/570001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): > > https://codereview.chromium.org/1411603007/diff/570001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/HeapPage.cpp:1310: HeapObjectHeader* > NormalPage::findHeaderFromObject(const void* obj) > > I'm afraid that this method would be super heavy. > > How much does the cycle time of layout tests increase when you enable the > use-after-free detection? Yes, it is so heavy that layout tests becomes 4x slower comparing with tot. Let me consider how to work for it. |
