Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1171463006:
Make Timer and CancellableTaskFactory embeddeable in lazily swept objects. (Closed)
|
Created:
5 years, 6 months ago by sof Modified:
5 years, 6 months ago CC:
blink-reviews, scheduler-bugs_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionMake Timer and CancellableTaskFactory embeddeable in lazily swept objects.
As CancellableTaskFactory is a part object, it might be part of an
object that's on the Oilpan heap. TimerBase's task factory being the
main example.
With ASan and Oilpan, heap objects will be poisoned prior to the Oilpan
GC sweep stage. Hence, should a cancellable task be attempted run while
the owning object is poisoned, poisoned memory will be accessed. Allow
such accesses by explicitly unpoisoning the object first. This is
considered acceptable as the memory region is within a larger object
that still is poisoned, hence ASan will continue to be effective.
For Timer, reposition its is-alive check over the object so as to make
it compatible with the timer tasks that now are run.
R=haraken
BUG=482388
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196674
Patch Set 1 #
Total comments: 8
Patch Set 2 : mark ScriptRunner's CancellableTaskFactory as needing unpoisoning #
Messages
Total messages: 24 (4 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look. https://codereview.chromium.org/1134523002 relanded last night, address (expected) ASan problems, http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... File Source/platform/scheduler/CancellableTaskFactory.cpp (right): https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... Source/platform/scheduler/CancellableTaskFactory.cpp:30: ASAN_UNPOISON_MEMORY_REGION(reinterpret_cast<unsigned char*>(taskFactory), sizeof(CancellableTaskFactory)); Just help me understand: Where can the CancellableTaskFactory get accessed after it's poisoned? I'm curious why it's hard to circumvent the errors with NO_LAZY_SWEEP_SANITIZE_ADDRESS. It looks like that CancellableTaskFactory is used only by HTMLParserScheduler and ScriptRunner. Maybe can we just change them to use OwnPtr<CancellableTaskFactory>s?
https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... File Source/platform/scheduler/CancellableTaskFactory.cpp (right): https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... Source/platform/scheduler/CancellableTaskFactory.cpp:30: ASAN_UNPOISON_MEMORY_REGION(reinterpret_cast<unsigned char*>(taskFactory), sizeof(CancellableTaskFactory)); On 2015/06/06 15:25:20, haraken wrote: > > Just help me understand: Where can the CancellableTaskFactory get accessed after > it's poisoned? I'm curious why it's hard to circumvent the errors with > NO_LAZY_SWEEP_SANITIZE_ADDRESS. > > It looks like that CancellableTaskFactory is used only by HTMLParserScheduler > and ScriptRunner. Maybe can we just change them to use > OwnPtr<CancellableTaskFactory>s? > Hmm, it's accessed from right here? Timer's use of it is the one to be concerned with.
https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... File Source/platform/scheduler/CancellableTaskFactory.cpp (right): https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... Source/platform/scheduler/CancellableTaskFactory.cpp:30: ASAN_UNPOISON_MEMORY_REGION(reinterpret_cast<unsigned char*>(taskFactory), sizeof(CancellableTaskFactory)); On 2015/06/06 15:46:11, sof wrote: > On 2015/06/06 15:25:20, haraken wrote: > > > > Just help me understand: Where can the CancellableTaskFactory get accessed > after > > it's poisoned? I'm curious why it's hard to circumvent the errors with > > NO_LAZY_SWEEP_SANITIZE_ADDRESS. > > > > It looks like that CancellableTaskFactory is used only by HTMLParserScheduler > > and ScriptRunner. Maybe can we just change them to use > > OwnPtr<CancellableTaskFactory>s? > > > > Hmm, it's accessed from right here? This function has NO_LAZY_SWEEP_SANITIZE_ADDRESS. I'm wondering why it isn't enough to circumvent the poisoning error?
https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... File Source/platform/scheduler/CancellableTaskFactory.cpp (right): https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... Source/platform/scheduler/CancellableTaskFactory.cpp:30: ASAN_UNPOISON_MEMORY_REGION(reinterpret_cast<unsigned char*>(taskFactory), sizeof(CancellableTaskFactory)); On 2015/06/07 16:40:46, haraken wrote: > On 2015/06/06 15:46:11, sof wrote: > > On 2015/06/06 15:25:20, haraken wrote: > > > > > > Just help me understand: Where can the CancellableTaskFactory get accessed > > after > > > it's poisoned? I'm curious why it's hard to circumvent the errors with > > > NO_LAZY_SWEEP_SANITIZE_ADDRESS. > > > > > > It looks like that CancellableTaskFactory is used only by > HTMLParserScheduler > > > and ScriptRunner. Maybe can we just change them to use > > > OwnPtr<CancellableTaskFactory>s? > > > > > > > Hmm, it's accessed from right here? > > This function has NO_LAZY_SWEEP_SANITIZE_ADDRESS. I'm wondering why it isn't > enough to circumvent the poisoning error? > That's a leaf-level annotation. OwnPtr::get() is not covered, nor is revokeAll().
https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... File Source/platform/scheduler/CancellableTaskFactory.cpp (right): https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... Source/platform/scheduler/CancellableTaskFactory.cpp:30: ASAN_UNPOISON_MEMORY_REGION(reinterpret_cast<unsigned char*>(taskFactory), sizeof(CancellableTaskFactory)); On 2015/06/07 17:23:05, sof wrote: > On 2015/06/07 16:40:46, haraken wrote: > > On 2015/06/06 15:46:11, sof wrote: > > > On 2015/06/06 15:25:20, haraken wrote: > > > > > > > > Just help me understand: Where can the CancellableTaskFactory get accessed > > > after > > > > it's poisoned? I'm curious why it's hard to circumvent the errors with > > > > NO_LAZY_SWEEP_SANITIZE_ADDRESS. > > > > > > > > It looks like that CancellableTaskFactory is used only by > > HTMLParserScheduler > > > > and ScriptRunner. Maybe can we just change them to use > > > > OwnPtr<CancellableTaskFactory>s? > > > > > > > > > > Hmm, it's accessed from right here? > > > > This function has NO_LAZY_SWEEP_SANITIZE_ADDRESS. I'm wondering why it isn't > > enough to circumvent the poisoning error? > > > > That's a leaf-level annotation. OwnPtr::get() is not covered, nor is > revokeAll(). Thanks, understood. So the final question would be: isn't there any option to make HTMLParserScheduler and ScriptRunner hold an OwnPtr<CancellableTaskFactory>? Then the CancellableTaskFactory won't be poisoned and thus the problem will be gone.
On 2015/06/07 23:46:24, haraken wrote: > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > File Source/platform/scheduler/CancellableTaskFactory.cpp (right): > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > Source/platform/scheduler/CancellableTaskFactory.cpp:30: > ASAN_UNPOISON_MEMORY_REGION(reinterpret_cast<unsigned char*>(taskFactory), > sizeof(CancellableTaskFactory)); > On 2015/06/07 17:23:05, sof wrote: > > On 2015/06/07 16:40:46, haraken wrote: > > > On 2015/06/06 15:46:11, sof wrote: > > > > On 2015/06/06 15:25:20, haraken wrote: > > > > > > > > > > Just help me understand: Where can the CancellableTaskFactory get > accessed > > > > after > > > > > it's poisoned? I'm curious why it's hard to circumvent the errors with > > > > > NO_LAZY_SWEEP_SANITIZE_ADDRESS. > > > > > > > > > > It looks like that CancellableTaskFactory is used only by > > > HTMLParserScheduler > > > > > and ScriptRunner. Maybe can we just change them to use > > > > > OwnPtr<CancellableTaskFactory>s? > > > > > > > > > > > > > Hmm, it's accessed from right here? > > > > > > This function has NO_LAZY_SWEEP_SANITIZE_ADDRESS. I'm wondering why it isn't > > > enough to circumvent the poisoning error? > > > > > > > That's a leaf-level annotation. OwnPtr::get() is not covered, nor is > > revokeAll(). > > Thanks, understood. > > So the final question would be: isn't there any option to make > HTMLParserScheduler and ScriptRunner hold an OwnPtr<CancellableTaskFactory>? > Then the CancellableTaskFactory won't be poisoned and thus the problem will be > gone. Hmm, I don't understand - the problem here is all about Timer(Base) use of CancellableTaskFactory, no?
On 2015/06/08 05:12:23, sof wrote: > On 2015/06/07 23:46:24, haraken wrote: > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > File Source/platform/scheduler/CancellableTaskFactory.cpp (right): > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > Source/platform/scheduler/CancellableTaskFactory.cpp:30: > > ASAN_UNPOISON_MEMORY_REGION(reinterpret_cast<unsigned char*>(taskFactory), > > sizeof(CancellableTaskFactory)); > > On 2015/06/07 17:23:05, sof wrote: > > > On 2015/06/07 16:40:46, haraken wrote: > > > > On 2015/06/06 15:46:11, sof wrote: > > > > > On 2015/06/06 15:25:20, haraken wrote: > > > > > > > > > > > > Just help me understand: Where can the CancellableTaskFactory get > > accessed > > > > > after > > > > > > it's poisoned? I'm curious why it's hard to circumvent the errors with > > > > > > NO_LAZY_SWEEP_SANITIZE_ADDRESS. > > > > > > > > > > > > It looks like that CancellableTaskFactory is used only by > > > > HTMLParserScheduler > > > > > > and ScriptRunner. Maybe can we just change them to use > > > > > > OwnPtr<CancellableTaskFactory>s? > > > > > > > > > > > > > > > > Hmm, it's accessed from right here? > > > > > > > > This function has NO_LAZY_SWEEP_SANITIZE_ADDRESS. I'm wondering why it > isn't > > > > enough to circumvent the poisoning error? > > > > > > > > > > That's a leaf-level annotation. OwnPtr::get() is not covered, nor is > > > revokeAll(). > > > > Thanks, understood. > > > > So the final question would be: isn't there any option to make > > HTMLParserScheduler and ScriptRunner hold an OwnPtr<CancellableTaskFactory>? > > Then the CancellableTaskFactory won't be poisoned and thus the problem will be > > gone. > > Hmm, I don't understand - the problem here is all about Timer(Base) use of > CancellableTaskFactory, no? Sorry if I'm misunderstanding: If we put CancellableTaskFactory off the heap, the memory for the CancellableTaskFactory is not poisoned. Thus the TimerBase can access the CancellableTaskFactory without causing any issue, can't it?
On 2015/06/08 05:18:37, haraken wrote: > On 2015/06/08 05:12:23, sof wrote: > > On 2015/06/07 23:46:24, haraken wrote: > > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > > File Source/platform/scheduler/CancellableTaskFactory.cpp (right): > > > > > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > > Source/platform/scheduler/CancellableTaskFactory.cpp:30: > > > ASAN_UNPOISON_MEMORY_REGION(reinterpret_cast<unsigned char*>(taskFactory), > > > sizeof(CancellableTaskFactory)); > > > On 2015/06/07 17:23:05, sof wrote: > > > > On 2015/06/07 16:40:46, haraken wrote: > > > > > On 2015/06/06 15:46:11, sof wrote: > > > > > > On 2015/06/06 15:25:20, haraken wrote: > > > > > > > > > > > > > > Just help me understand: Where can the CancellableTaskFactory get > > > accessed > > > > > > after > > > > > > > it's poisoned? I'm curious why it's hard to circumvent the errors > with > > > > > > > NO_LAZY_SWEEP_SANITIZE_ADDRESS. > > > > > > > > > > > > > > It looks like that CancellableTaskFactory is used only by > > > > > HTMLParserScheduler > > > > > > > and ScriptRunner. Maybe can we just change them to use > > > > > > > OwnPtr<CancellableTaskFactory>s? > > > > > > > > > > > > > > > > > > > Hmm, it's accessed from right here? > > > > > > > > > > This function has NO_LAZY_SWEEP_SANITIZE_ADDRESS. I'm wondering why it > > isn't > > > > > enough to circumvent the poisoning error? > > > > > > > > > > > > > That's a leaf-level annotation. OwnPtr::get() is not covered, nor is > > > > revokeAll(). > > > > > > Thanks, understood. > > > > > > So the final question would be: isn't there any option to make > > > HTMLParserScheduler and ScriptRunner hold an OwnPtr<CancellableTaskFactory>? > > > Then the CancellableTaskFactory won't be poisoned and thus the problem will > be > > > gone. > > > > Hmm, I don't understand - the problem here is all about Timer(Base) use of > > CancellableTaskFactory, no? > > Sorry if I'm misunderstanding: > > If we put CancellableTaskFactory off the heap, the memory for the > CancellableTaskFactory is not poisoned. Thus the TimerBase can access the > CancellableTaskFactory without causing any issue, can't it? I don't think that is what you were saying, TimerBase's use of it seemed to be an unknown. And I don't think that's a good idea, making it(Timer) require another allocation&free rather than being a part object. So, please, review what is here.
On 2015/06/08 05:33:51, sof wrote: > On 2015/06/08 05:18:37, haraken wrote: > > On 2015/06/08 05:12:23, sof wrote: > > > On 2015/06/07 23:46:24, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > > > File Source/platform/scheduler/CancellableTaskFactory.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > > > Source/platform/scheduler/CancellableTaskFactory.cpp:30: > > > > ASAN_UNPOISON_MEMORY_REGION(reinterpret_cast<unsigned char*>(taskFactory), > > > > sizeof(CancellableTaskFactory)); > > > > On 2015/06/07 17:23:05, sof wrote: > > > > > On 2015/06/07 16:40:46, haraken wrote: > > > > > > On 2015/06/06 15:46:11, sof wrote: > > > > > > > On 2015/06/06 15:25:20, haraken wrote: > > > > > > > > > > > > > > > > Just help me understand: Where can the CancellableTaskFactory get > > > > accessed > > > > > > > after > > > > > > > > it's poisoned? I'm curious why it's hard to circumvent the errors > > with > > > > > > > > NO_LAZY_SWEEP_SANITIZE_ADDRESS. > > > > > > > > > > > > > > > > It looks like that CancellableTaskFactory is used only by > > > > > > HTMLParserScheduler > > > > > > > > and ScriptRunner. Maybe can we just change them to use > > > > > > > > OwnPtr<CancellableTaskFactory>s? > > > > > > > > > > > > > > > > > > > > > > Hmm, it's accessed from right here? > > > > > > > > > > > > This function has NO_LAZY_SWEEP_SANITIZE_ADDRESS. I'm wondering why it > > > isn't > > > > > > enough to circumvent the poisoning error? > > > > > > > > > > > > > > > > That's a leaf-level annotation. OwnPtr::get() is not covered, nor is > > > > > revokeAll(). > > > > > > > > Thanks, understood. > > > > > > > > So the final question would be: isn't there any option to make > > > > HTMLParserScheduler and ScriptRunner hold an > OwnPtr<CancellableTaskFactory>? > > > > Then the CancellableTaskFactory won't be poisoned and thus the problem > will > > be > > > > gone. > > > > > > Hmm, I don't understand - the problem here is all about Timer(Base) use of > > > CancellableTaskFactory, no? > > > > Sorry if I'm misunderstanding: > > > > If we put CancellableTaskFactory off the heap, the memory for the > > CancellableTaskFactory is not poisoned. Thus the TimerBase can access the > > CancellableTaskFactory without causing any issue, can't it? > > I don't think that is what you were saying, TimerBase's use of it seemed to be > an unknown. > > And I don't think that's a good idea, making it(Timer) require another > allocation&free rather than being a part object. So, please, review what is > here. But the CancellableTaskFactory is used only by HTMLParserScheduler and ScriptRunner, which are not performance-sensitive. What's the problem in making them hold OwnPtr<CancellableTaskFactory>? That is what I wanted to say...
On 2015/06/08 05:39:50, haraken wrote: > On 2015/06/08 05:33:51, sof wrote: > > On 2015/06/08 05:18:37, haraken wrote: > > > On 2015/06/08 05:12:23, sof wrote: > > > > On 2015/06/07 23:46:24, haraken wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > > > > File Source/platform/scheduler/CancellableTaskFactory.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > > > > Source/platform/scheduler/CancellableTaskFactory.cpp:30: > > > > > ASAN_UNPOISON_MEMORY_REGION(reinterpret_cast<unsigned > char*>(taskFactory), > > > > > sizeof(CancellableTaskFactory)); > > > > > On 2015/06/07 17:23:05, sof wrote: > > > > > > On 2015/06/07 16:40:46, haraken wrote: > > > > > > > On 2015/06/06 15:46:11, sof wrote: > > > > > > > > On 2015/06/06 15:25:20, haraken wrote: > > > > > > > > > > > > > > > > > > Just help me understand: Where can the CancellableTaskFactory > get > > > > > accessed > > > > > > > > after > > > > > > > > > it's poisoned? I'm curious why it's hard to circumvent the > errors > > > with > > > > > > > > > NO_LAZY_SWEEP_SANITIZE_ADDRESS. > > > > > > > > > > > > > > > > > > It looks like that CancellableTaskFactory is used only by > > > > > > > HTMLParserScheduler > > > > > > > > > and ScriptRunner. Maybe can we just change them to use > > > > > > > > > OwnPtr<CancellableTaskFactory>s? > > > > > > > > > > > > > > > > > > > > > > > > > Hmm, it's accessed from right here? > > > > > > > > > > > > > > This function has NO_LAZY_SWEEP_SANITIZE_ADDRESS. I'm wondering why > it > > > > isn't > > > > > > > enough to circumvent the poisoning error? > > > > > > > > > > > > > > > > > > > That's a leaf-level annotation. OwnPtr::get() is not covered, nor is > > > > > > revokeAll(). > > > > > > > > > > Thanks, understood. > > > > > > > > > > So the final question would be: isn't there any option to make > > > > > HTMLParserScheduler and ScriptRunner hold an > > OwnPtr<CancellableTaskFactory>? > > > > > Then the CancellableTaskFactory won't be poisoned and thus the problem > > will > > > be > > > > > gone. > > > > > > > > Hmm, I don't understand - the problem here is all about Timer(Base) use of > > > > CancellableTaskFactory, no? > > > > > > Sorry if I'm misunderstanding: > > > > > > If we put CancellableTaskFactory off the heap, the memory for the > > > CancellableTaskFactory is not poisoned. Thus the TimerBase can access the > > > CancellableTaskFactory without causing any issue, can't it? > > > > I don't think that is what you were saying, TimerBase's use of it seemed to be > > an unknown. > > > > And I don't think that's a good idea, making it(Timer) require another > > allocation&free rather than being a part object. So, please, review what is > > here. > > But the CancellableTaskFactory is used only by HTMLParserScheduler and > ScriptRunner, which are not performance-sensitive. What's the problem in making > them hold OwnPtr<CancellableTaskFactory>? That is what I wanted to say... I'm bewildered by that "only", doesn't https://codereview.chromium.org/1171463006/patch/1/10002 indicate that TimerBase has a CancellableTaskFactory? And is the reason why we've got this CL & Oilpan ASan meltdown?
Sorry! I now got your point, LGTM. (The CancellableTaskFactory in Timer was not grepped in the code search...) https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... File Source/platform/scheduler/CancellableTaskFactory.cpp (right): https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... Source/platform/scheduler/CancellableTaskFactory.cpp:29: if (taskFactory->m_unpoisonBeforeUpdate) Do we need this condition? I mean, wouldn't it be better to unpoison the memory unconditionally? For example, this CL takes care of CancellableTaskFactory in Timer but doesn't take care of CancellableTaskFactory in ScriptRunner. It seems that what we want here is: - Mark CancellableTaskFactory DISALLOW_ALLOCATION() - Unconditionally unpoison the memory here.
https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... File Source/platform/scheduler/CancellableTaskFactory.cpp (right): https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... Source/platform/scheduler/CancellableTaskFactory.cpp:29: if (taskFactory->m_unpoisonBeforeUpdate) On 2015/06/08 06:51:52, haraken wrote: > > Do we need this condition? I mean, wouldn't it be better to unpoison the memory > unconditionally? For example, this CL takes care of CancellableTaskFactory in > Timer but doesn't take care of CancellableTaskFactory in ScriptRunner. It seems > that what we want here is: > > - Mark CancellableTaskFactory DISALLOW_ALLOCATION() > - Unconditionally unpoison the memory here. If you do it that way, you will also unpoison non-heap uses of Timer(Base) et al, which I wanted to avoid. CancelleableTaskFactory cannot tidily(?) determine if it is appearing on the heap.
https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... File Source/platform/scheduler/CancellableTaskFactory.cpp (right): https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... Source/platform/scheduler/CancellableTaskFactory.cpp:29: if (taskFactory->m_unpoisonBeforeUpdate) On 2015/06/08 07:04:37, sof wrote: > On 2015/06/08 06:51:52, haraken wrote: > > > > Do we need this condition? I mean, wouldn't it be better to unpoison the > memory > > unconditionally? For example, this CL takes care of CancellableTaskFactory in > > Timer but doesn't take care of CancellableTaskFactory in ScriptRunner. It > seems > > that what we want here is: > > > > - Mark CancellableTaskFactory DISALLOW_ALLOCATION() > > - Unconditionally unpoison the memory here. > > If you do it that way, you will also unpoison non-heap uses of Timer(Base) et > al, which I wanted to avoid. CancelleableTaskFactory cannot tidily(?) determine > if it is appearing on the heap. ah, makes sense. The current CL looks good :)
On 2015/06/08 07:11:10, haraken wrote: > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > File Source/platform/scheduler/CancellableTaskFactory.cpp (right): > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > Source/platform/scheduler/CancellableTaskFactory.cpp:29: if > (taskFactory->m_unpoisonBeforeUpdate) > On 2015/06/08 07:04:37, sof wrote: > > On 2015/06/08 06:51:52, haraken wrote: > > > > > > Do we need this condition? I mean, wouldn't it be better to unpoison the > > memory > > > unconditionally? For example, this CL takes care of CancellableTaskFactory > in > > > Timer but doesn't take care of CancellableTaskFactory in ScriptRunner. It > > seems > > > that what we want here is: > > > > > > - Mark CancellableTaskFactory DISALLOW_ALLOCATION() > > > - Unconditionally unpoison the memory here. > > > > If you do it that way, you will also unpoison non-heap uses of Timer(Base) et > > al, which I wanted to avoid. CancelleableTaskFactory cannot tidily(?) > determine > > if it is appearing on the heap. > > ah, makes sense. The current CL looks good :) Just realized there is simpler way to arrange for this -- if CancellableTaskFactory knows it is on the heap, its run() can use willObjectBeLazilySwept(). That way we avoid this explicit unpoisoning + Timer can do without canFire() -- the timer will never be 'run'.
On 2015/06/08 09:27:03, sof wrote: > On 2015/06/08 07:11:10, haraken wrote: > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > File Source/platform/scheduler/CancellableTaskFactory.cpp (right): > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > Source/platform/scheduler/CancellableTaskFactory.cpp:29: if > > (taskFactory->m_unpoisonBeforeUpdate) > > On 2015/06/08 07:04:37, sof wrote: > > > On 2015/06/08 06:51:52, haraken wrote: > > > > > > > > Do we need this condition? I mean, wouldn't it be better to unpoison the > > > memory > > > > unconditionally? For example, this CL takes care of CancellableTaskFactory > > in > > > > Timer but doesn't take care of CancellableTaskFactory in ScriptRunner. It > > > seems > > > > that what we want here is: > > > > > > > > - Mark CancellableTaskFactory DISALLOW_ALLOCATION() > > > > - Unconditionally unpoison the memory here. > > > > > > If you do it that way, you will also unpoison non-heap uses of Timer(Base) > et > > > al, which I wanted to avoid. CancelleableTaskFactory cannot tidily(?) > > determine > > > if it is appearing on the heap. > > > > ah, makes sense. The current CL looks good :) > > Just realized there is simpler way to arrange for this -- if > CancellableTaskFactory knows it is on the heap, its run() can use > willObjectBeLazilySwept(). > > That way we avoid this explicit unpoisoning + Timer can do without canFire() -- > the timer will never be 'run'. Sounds nicer!
On 2015/06/08 10:57:49, haraken wrote: > On 2015/06/08 09:27:03, sof wrote: > > On 2015/06/08 07:11:10, haraken wrote: > > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > > File Source/platform/scheduler/CancellableTaskFactory.cpp (right): > > > > > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > > Source/platform/scheduler/CancellableTaskFactory.cpp:29: if > > > (taskFactory->m_unpoisonBeforeUpdate) > > > On 2015/06/08 07:04:37, sof wrote: > > > > On 2015/06/08 06:51:52, haraken wrote: > > > > > > > > > > Do we need this condition? I mean, wouldn't it be better to unpoison the > > > > memory > > > > > unconditionally? For example, this CL takes care of > CancellableTaskFactory > > > in > > > > > Timer but doesn't take care of CancellableTaskFactory in ScriptRunner. > It > > > > seems > > > > > that what we want here is: > > > > > > > > > > - Mark CancellableTaskFactory DISALLOW_ALLOCATION() > > > > > - Unconditionally unpoison the memory here. > > > > > > > > If you do it that way, you will also unpoison non-heap uses of Timer(Base) > > et > > > > al, which I wanted to avoid. CancelleableTaskFactory cannot tidily(?) > > > determine > > > > if it is appearing on the heap. > > > > > > ah, makes sense. The current CL looks good :) > > > > Just realized there is simpler way to arrange for this -- if > > CancellableTaskFactory knows it is on the heap, its run() can use > > willObjectBeLazilySwept(). > > > > That way we avoid this explicit unpoisoning + Timer can do without canFire() > -- > > the timer will never be 'run'. > > Sounds nicer! Too good to be true...this depends on finding the heap object header from a part object, which we do not really support. Unless there are objections, I will proceed to land what's here now (and we can potentially iterate afterwards.)
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1171463006/#ps20001 (title: "mark ScriptRunner's CancellableTaskFactory as needing unpoisoning")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1171463006/20001
On 2015/06/08 11:21:24, sof wrote: > On 2015/06/08 10:57:49, haraken wrote: > > On 2015/06/08 09:27:03, sof wrote: > > > On 2015/06/08 07:11:10, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > > > File Source/platform/scheduler/CancellableTaskFactory.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1171463006/diff/1/Source/platform/scheduler/C... > > > > Source/platform/scheduler/CancellableTaskFactory.cpp:29: if > > > > (taskFactory->m_unpoisonBeforeUpdate) > > > > On 2015/06/08 07:04:37, sof wrote: > > > > > On 2015/06/08 06:51:52, haraken wrote: > > > > > > > > > > > > Do we need this condition? I mean, wouldn't it be better to unpoison > the > > > > > memory > > > > > > unconditionally? For example, this CL takes care of > > CancellableTaskFactory > > > > in > > > > > > Timer but doesn't take care of CancellableTaskFactory in ScriptRunner. > > It > > > > > seems > > > > > > that what we want here is: > > > > > > > > > > > > - Mark CancellableTaskFactory DISALLOW_ALLOCATION() > > > > > > - Unconditionally unpoison the memory here. > > > > > > > > > > If you do it that way, you will also unpoison non-heap uses of > Timer(Base) > > > et > > > > > al, which I wanted to avoid. CancelleableTaskFactory cannot tidily(?) > > > > determine > > > > > if it is appearing on the heap. > > > > > > > > ah, makes sense. The current CL looks good :) > > > > > > Just realized there is simpler way to arrange for this -- if > > > CancellableTaskFactory knows it is on the heap, its run() can use > > > willObjectBeLazilySwept(). > > > > > > That way we avoid this explicit unpoisoning + Timer can do without canFire() > > -- > > > the timer will never be 'run'. > > > > Sounds nicer! > > Too good to be true...this depends on finding the heap object header from a part > object, which we do not really support. There would be no easy way to do that -- I hit the same problem a couple of times and gave up. We need to create a object-start-bitmap, which is heavy.
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196674 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
