Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1111693003:
Remove the concept of a cleanup task (Closed)
|
Created:
5 years, 7 months ago by Sami Modified:
5 years, 6 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, falken, horo+watch_chromium.org, kinuko+worker_chromium.org, rwlbuis, sadrul Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionRemove the concept of a cleanup task
The only cleanup task in Blink which is not internal to a worker thread is
the task to close a WebSQL database. Because 1) only worker threads
support the concept of a cleanup task, and 2) WebSQL isn't supported on
worker threads anymore after https://codereview.chromium.org/561093003/,
we can safely remove the logic to handle cleanup tasks on worker
threads.
BUG=463143
TBR=jochen@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195191
Patch Set 1 #
Total comments: 2
Patch Set 2 : Potential shutdown sequence with some debug logging. #
Total comments: 2
Patch Set 3 : Cleanup. #
Total comments: 6
Patch Set 4 : Sadrul's comments. #Patch Set 5 : ASSERT => ASSERT_UNUSED #
Total comments: 13
Patch Set 6 : Rebased. #Patch Set 7 : Review comments. #
Total comments: 2
Patch Set 8 : Simplify assertion. #
Total comments: 2
Patch Set 9 : Rename mutex. #Patch Set 10 : ASSERT_UNUSED. #Patch Set 11 : Rebased. #Patch Set 12 : Shut down scheduler before clearing the heap. #Patch Set 13 : Don't run tasks if thread was never initialized. #
Messages
Total messages: 45 (9 generated)
skyostil@chromium.org changed reviewers: + haraken@chromium.org, kinuko@chromium.org
Hi. This refactoring isn't fully working yet, but I wanted to get some early feedback about it anyway -- in particular whether you think we can actually do this. Getting rid of cleanup tasks would greatly simplify the removal of the timer heap. Thanks!
On 2015/04/29 18:51:29, Sami wrote: > Hi. This refactoring isn't fully working yet, but I wanted to get some early > feedback about it anyway -- in particular whether you think we can actually do > this. > > Getting rid of cleanup tasks would greatly simplify the removal of the timer > heap. Thanks! I think this is a way to go. It would be safe to make this change now since we've removed WebSQL support from workers.
Yes this concept has been a bit awkward, we should clean up this. https://codereview.chromium.org/1111693003/diff/1/Source/core/workers/WorkerT... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/1/Source/core/workers/WorkerT... Source/core/workers/WorkerThread.cpp:448: cleanup(); stopInternal() is called on the main thread while most of these need to run on worker thread, we'll at least need to run one more task to do worker-thread side cleanup.
alexclarke@chromium.org changed reviewers: + alexclarke@chromium.org
https://codereview.chromium.org/1111693003/diff/20001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (left): https://codereview.chromium.org/1111693003/diff/20001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:427: virtual bool isCleanupTask() const { return true; } I wonder if you can now remove this from ExecutionContextTask too?
This seems to work based on local testing, so please have another look. https://codereview.chromium.org/1111693003/diff/1/Source/core/workers/WorkerT... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/1/Source/core/workers/WorkerT... Source/core/workers/WorkerThread.cpp:448: cleanup(); On 2015/04/30 04:03:44, kinuko (ooo til May 6) wrote: > stopInternal() is called on the main thread while most of these need to run on > worker thread, we'll at least need to run one more task to do worker-thread side > cleanup. Thanks for the hint! It took a while for me to work out the thread hop sequence during shutdown. Could you check that the latest version looks consistent? https://codereview.chromium.org/1111693003/diff/20001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (left): https://codereview.chromium.org/1111693003/diff/20001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:427: virtual bool isCleanupTask() const { return true; } On 2015/05/05 17:45:05, alexclarke1 wrote: > I wonder if you can now remove this from ExecutionContextTask too? Did you mean isCleanupTask()? Yes, there's no need for it so I already took it out.
Some drive-by comments: https://codereview.chromium.org/1111693003/diff/40001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/40001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:75: ASSERT(!m_wasClosed); Would it make sense to check m_workerThread->workerGlobalScope()->isClosing() here? https://codereview.chromium.org/1111693003/diff/40001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:375: workerGlobalScope()->dispose(); Can stopActiveDOMObjects() and removeAllEventListeners() be moved into WorkerGlobalScope::dispose()? https://codereview.chromium.org/1111693003/diff/40001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:398: backingThread().platformThread().scheduler()->shutdown(); Can this move into WebThreadSuppportingGC? For example, introduce WebThreadSupportingGC::shutdown(), which takes care of both detachGC() and scheduler()->shutdown()? Functionality regarding thread maintenance (initialization, destruction etc.) is slowly moving out of WorkerThread (soon to be renamed WorkerScript: https://codereview.chromium.org/1115923002/). Having a single point of shutting down the thread from here (instead of two: detachGC(), scheduler().shutdown()) would be cleaner.
Drive-bys most definitely appreciated! Thanks, PTAL. https://codereview.chromium.org/1111693003/diff/40001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/40001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:75: ASSERT(!m_wasClosed); On 2015/05/06 18:04:50, sadrul wrote: > Would it make sense to check m_workerThread->workerGlobalScope()->isClosing() > here? I was worried that workerGlobalScope is null for the first task, but I suppose we can check it conditionally like we do in didProcessTask. Done. https://codereview.chromium.org/1111693003/diff/40001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:375: workerGlobalScope()->dispose(); On 2015/05/06 18:04:50, sadrul wrote: > Can stopActiveDOMObjects() and removeAllEventListeners() be moved into > WorkerGlobalScope::dispose()? Good idea, done. https://codereview.chromium.org/1111693003/diff/40001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:398: backingThread().platformThread().scheduler()->shutdown(); On 2015/05/06 18:04:50, sadrul wrote: > Can this move into WebThreadSuppportingGC? For example, introduce > WebThreadSupportingGC::shutdown(), which takes care of both detachGC() and > scheduler()->shutdown()? > > Functionality regarding thread maintenance (initialization, destruction etc.) is > slowly moving out of WorkerThread (soon to be renamed WorkerScript: > https://codereview.chromium.org/1115923002/). Having a single point of shutting > down the thread from here (instead of two: detachGC(), scheduler().shutdown()) > would be cleaner. Yes, that definitely sounds more maintainable. I've now moved this there and renamed attachGC/detachGC to initialize/shutdown.
Friendly ping?
Sorry for slow review, looking good overall. https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:363: MutexLocker lock(m_threadCreationMutex); I don't think we need this? This lock's mainly used to protect things against initialize() which runs on the worker thread, while cleanup also runs on the worker thread.
https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... File Source/core/workers/WorkerGlobalScope.cpp (right): https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... Source/core/workers/WorkerGlobalScope.cpp:160: void WorkerGlobalScope::close() I'm just curious if we can add ASSERT(!m_closing) here. Can the close() be called more than once? https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... Source/core/workers/WorkerGlobalScope.cpp:162: // Let current script run to completion, but tell the worker thread to shut down after the script exits. This comment looks out-dated. We no longer tell the worker thread to shut down here. https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:361: void WorkerThread::cleanup() Nit: We're mixing cleanup, destroy, dispose and shutdown. We might want to unify the names. We can do this in a follow-up CL. https://codereview.chromium.org/1111693003/diff/80001/Source/platform/WebThre... File Source/platform/WebThreadSupportingGC.cpp (right): https://codereview.chromium.org/1111693003/diff/80001/Source/platform/WebThre... Source/platform/WebThreadSupportingGC.cpp:54: platformThread().scheduler()->shutdown(); Just to confirm: Not all threads in Blink are using WebThreadSupportingGC. Threads that don't need a GC are using WebThread (not WebThreadSupportingGC). For those threads, are we calling platformThread().scheduler()->shutdown() as well?
Thanks for the review! Comments addressed. https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... File Source/core/workers/WorkerGlobalScope.cpp (right): https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... Source/core/workers/WorkerGlobalScope.cpp:160: void WorkerGlobalScope::close() On 2015/05/09 15:54:36, haraken wrote: > > I'm just curious if we can add ASSERT(!m_closing) here. Can the close() be > called more than once? Correct me if I'm wrong but I think a worker script can still call close twice in the same task without yielding in the middle: self.close(); self.close(); For that reason I don't think we can assert here. https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... Source/core/workers/WorkerGlobalScope.cpp:162: // Let current script run to completion, but tell the worker thread to shut down after the script exits. On 2015/05/09 15:54:36, haraken wrote: > > This comment looks out-dated. We no longer tell the worker thread to shut down > here. Right, I've now tried to make it a little more accurate. Setting m_closing = true will make trigger the worker micro task runner to cleanup the thread after the current task. https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:361: void WorkerThread::cleanup() On 2015/05/09 15:54:36, haraken wrote: > > Nit: We're mixing cleanup, destroy, dispose and shutdown. We might want to unify > the names. We can do this in a follow-up CL. Agreed, I'm open to suggestions. I think renaming cleanup to shutdown might be a good first step -- which I've now taken. https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:363: MutexLocker lock(m_threadCreationMutex); On 2015/05/09 10:20:10, kinuko wrote: > I don't think we need this? This lock's mainly used to protect things against > initialize() which runs on the worker thread, while cleanup also runs on the > worker thread. I added it because Kinuko pointed out that WorkerThread::stopInternal() can run on the main thread and that function accesses members which are also written here (e.g., m_workerGlobalScope). https://codereview.chromium.org/1111693003/diff/80001/Source/platform/WebThre... File Source/platform/WebThreadSupportingGC.cpp (right): https://codereview.chromium.org/1111693003/diff/80001/Source/platform/WebThre... Source/platform/WebThreadSupportingGC.cpp:54: platformThread().scheduler()->shutdown(); On 2015/05/09 15:54:36, haraken wrote: > > Just to confirm: Not all threads in Blink are using WebThreadSupportingGC. > Threads that don't need a GC are using WebThread (not WebThreadSupportingGC). Right, that was my understanding as well. > For those threads, are we calling platformThread().scheduler()->shutdown() as > well? > So far worker threads are the only ones that I know of needing special handling for task execution during shutdown (i.e., calling |close| must prevent any further tasks from running according to the html spec). The other utility threads don't have this constraint, so not calling shutdown there is fine.
https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:363: MutexLocker lock(m_threadCreationMutex); On 2015/05/11 10:35:32, Sami wrote: > On 2015/05/09 10:20:10, kinuko wrote: > > I don't think we need this? This lock's mainly used to protect things against > > initialize() which runs on the worker thread, while cleanup also runs on the > > worker thread. > > I added it because Kinuko pointed out that WorkerThread::stopInternal() can run > on the main thread and that function accesses members which are also written > here (e.g., m_workerGlobalScope). Oops, somehow I read this comment being written by Kentaro and not you :) The point still stands I think :)
LGTM on my side. https://codereview.chromium.org/1111693003/diff/120001/Source/core/workers/Wo... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/120001/Source/core/workers/Wo... Source/core/workers/WorkerThread.cpp:74: ASSERT_UNUSED(globalScope, !globalScope->isClosing()); Slightly more common: WorkerGlobalScope* globalScope = m_workerThread->workerGlobalScope(); ASSERT(!globalScope || globalScope->isClosing()); BTW, when do we not have WorkerGlobalScope?
Thanks! https://codereview.chromium.org/1111693003/diff/120001/Source/core/workers/Wo... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/120001/Source/core/workers/Wo... Source/core/workers/WorkerThread.cpp:74: ASSERT_UNUSED(globalScope, !globalScope->isClosing()); On 2015/05/11 13:06:46, haraken wrote: > > Slightly more common: > > WorkerGlobalScope* globalScope = m_workerThread->workerGlobalScope(); > ASSERT(!globalScope || globalScope->isClosing()); Thanks, done! > BTW, when do we not have WorkerGlobalScope? It'll be null when willProcessTask() is called for the WorkerThread::initialize() task, i.e., we haven't created the global scope yet.
https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:363: MutexLocker lock(m_threadCreationMutex); On 2015/05/11 10:37:34, Sami wrote: > On 2015/05/11 10:35:32, Sami wrote: > > On 2015/05/09 10:20:10, kinuko wrote: > > > I don't think we need this? This lock's mainly used to protect things > against > > > initialize() which runs on the worker thread, while cleanup also runs on the > > > worker thread. > > > > I added it because Kinuko pointed out that WorkerThread::stopInternal() can > run > > on the main thread and that function accesses members which are also written > > here (e.g., m_workerGlobalScope). > > Oops, somehow I read this comment being written by Kentaro and not you :) The > point still stands I think :) ;) If I'm reading correctly stopInternal() is the only one that posts a task to call this method on the worker thread, is that right? If so I think there's a happens-before relationship, therefore nullifying m_workerGlobalScope here should be safe.
https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:363: MutexLocker lock(m_threadCreationMutex); On 2015/05/11 13:26:40, kinuko wrote: > On 2015/05/11 10:37:34, Sami wrote: > > On 2015/05/11 10:35:32, Sami wrote: > > > On 2015/05/09 10:20:10, kinuko wrote: > > > > I don't think we need this? This lock's mainly used to protect things > > against > > > > initialize() which runs on the worker thread, while cleanup also runs on > the > > > > worker thread. > > > > > > I added it because Kinuko pointed out that WorkerThread::stopInternal() can > > run > > > on the main thread and that function accesses members which are also written > > > here (e.g., m_workerGlobalScope). > > > > Oops, somehow I read this comment being written by Kentaro and not you :) The > > point still stands I think :) > > ;) > > If I'm reading correctly stopInternal() is the only one that posts a task to > call this method on the worker thread, is that right? If so I think there's a > happens-before relationship, therefore nullifying m_workerGlobalScope here > should be safe. I think this sequence of events could result in racy access to m_workerGlobalScope: 1. On the worker thread: self.close() (in Javascript) => WorkerMicroTaskRunner::didProcessTask => WorkerThread::shutdown() => writes to m_workerGlobalScope 2. On the main thread: WorkerThread::terminateAndWait() => WorkerThread::stop() => WorkerThread::stopInternal() => reads from m_workerGlobalScope
On 2015/05/11 14:02:37, Sami wrote: > https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... > File Source/core/workers/WorkerThread.cpp (right): > > https://codereview.chromium.org/1111693003/diff/80001/Source/core/workers/Wor... > Source/core/workers/WorkerThread.cpp:363: MutexLocker > lock(m_threadCreationMutex); > On 2015/05/11 13:26:40, kinuko wrote: > > On 2015/05/11 10:37:34, Sami wrote: > > > On 2015/05/11 10:35:32, Sami wrote: > > > > On 2015/05/09 10:20:10, kinuko wrote: > > > > > I don't think we need this? This lock's mainly used to protect things > > > against > > > > > initialize() which runs on the worker thread, while cleanup also runs on > > the > > > > > worker thread. > > > > > > > > I added it because Kinuko pointed out that WorkerThread::stopInternal() > can > > > run > > > > on the main thread and that function accesses members which are also > written > > > > here (e.g., m_workerGlobalScope). > > > > > > Oops, somehow I read this comment being written by Kentaro and not you :) > The > > > point still stands I think :) > > > > ;) > > > > If I'm reading correctly stopInternal() is the only one that posts a task to > > call this method on the worker thread, is that right? If so I think there's a > > happens-before relationship, therefore nullifying m_workerGlobalScope here > > should be safe. > > I think this sequence of events could result in racy access to > m_workerGlobalScope: > > 1. On the worker thread: > > self.close() (in Javascript) > => WorkerMicroTaskRunner::didProcessTask > => WorkerThread::shutdown() > => writes to m_workerGlobalScope > > 2. On the main thread: > > WorkerThread::terminateAndWait() > => WorkerThread::stop() > => WorkerThread::stopInternal() > => reads from m_workerGlobalScope Oh I see, you're right, I was overlooking the case 1. If we use m_threadCreationMutex for protecting m_workerGlobalScope in shutdown case too, can we rename the mutex to avoid confusion like I had?
https://codereview.chromium.org/1111693003/diff/140001/Source/core/workers/Wo... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/140001/Source/core/workers/Wo... Source/core/workers/WorkerThread.cpp:85: m_workerThread->shutdown(); Hm, sorry I have one more question. Why we call shutdown() here while workerGlobalScopeClosed() will eventually call WorkerThread::stop()?
On 2015/05/11 14:22:46, kinuko wrote: > Oh I see, you're right, I was overlooking the case 1. If we use > m_threadCreationMutex for protecting m_workerGlobalScope in shutdown case too, > can we rename the mutex to avoid confusion like I had? Yes, definitely. I've renamed it and added some clarifying comments.
Thanks. On 2015/05/11 14:32:35, kinuko wrote: > https://codereview.chromium.org/1111693003/diff/140001/Source/core/workers/Wo... > File Source/core/workers/WorkerThread.cpp (right): > > https://codereview.chromium.org/1111693003/diff/140001/Source/core/workers/Wo... > Source/core/workers/WorkerThread.cpp:85: m_workerThread->shutdown(); > Hm, sorry I have one more question. Why we call shutdown() here while > workerGlobalScopeClosed() will eventually call WorkerThread::stop()? Could you answer this question? I might be still missing something?
https://codereview.chromium.org/1111693003/diff/140001/Source/core/workers/Wo... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1111693003/diff/140001/Source/core/workers/Wo... Source/core/workers/WorkerThread.cpp:85: m_workerThread->shutdown(); On 2015/05/11 14:32:35, kinuko wrote: > Hm, sorry I have one more question. Why we call shutdown() here while > workerGlobalScopeClosed() will eventually call WorkerThread::stop()? The problem is that workerGlobalScopeClosed won't call WorkerThread::stop() synchronously. The html spec requires that the worker stops processing any new tasks right after WorkerGlobalScope.close() is called, and shutting down the thread here in didProcessTask is a way to enforce that. Note that we can't easily do this shutdown anywhere else because it can't rely on posted tasks (because then the shutdown wouldn't be atomic and we'd risk running other work) and it can't have any JS on the call stack (since we are tearing down the isolate).
On 2015/05/11 14:46:13, Sami wrote: > https://codereview.chromium.org/1111693003/diff/140001/Source/core/workers/Wo... > File Source/core/workers/WorkerThread.cpp (right): > > https://codereview.chromium.org/1111693003/diff/140001/Source/core/workers/Wo... > Source/core/workers/WorkerThread.cpp:85: m_workerThread->shutdown(); > On 2015/05/11 14:32:35, kinuko wrote: > > Hm, sorry I have one more question. Why we call shutdown() here while > > workerGlobalScopeClosed() will eventually call WorkerThread::stop()? > > The problem is that workerGlobalScopeClosed won't call WorkerThread::stop() > synchronously. The html spec requires that the worker stops processing any new > tasks right after WorkerGlobalScope.close() is called, and shutting down the > thread here in didProcessTask is a way to enforce that. > > Note that we can't easily do this shutdown anywhere else because it can't rely > on posted tasks (because then the shutdown wouldn't be atomic and we'd risk > running other work) and it can't have any JS on the call stack (since we are > tearing down the isolate). I see thanks for the explanation. So either shutdown() or stop() could run first, depending on how the worker is closed/terminated. I think this lgtm... thanks for cleaning this up!
On 2015/05/11 15:01:58, kinuko wrote: > I see thanks for the explanation. So either shutdown() or stop() could run > first, depending on how the worker is closed/terminated. I think this lgtm... > thanks for cleaning this up! That's right. I find it a little strange there even is a way to forcefully terminate the thread from the outside, but I guess we have to live with it :) Thanks for the detailed review!
skyostil@chromium.org changed reviewers: + jochen@chromium.org
TBR'ing public/platform/WebScheduler.h for jochen@ since it's just adding back a virtual we had there before.
The CQ bit was checked by skyostil@chromium.org
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/1111693003/#ps160001 (title: "Rename mutex.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1111693003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1111693003/#ps180001 (title: "ASSERT_UNUSED.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1111693003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195191
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
I can't reproduce locally with an asan-debug build, but this seems like a plausible candidate for some flaky asan crashers https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil... looking over the result history (where we unfortunately had a period last night with compile failures.) -- http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
Message was sent while issue was closed.
On 2015/05/12 13:22:21, sof wrote: > I can't reproduce locally with an asan-debug build, but this seems like a > plausible candidate for some flaky asan crashers > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil... > > looking over the result history (where we unfortunately had a period last night > with compile failures.) -- > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%... Yep, those call stacks look like they might be related. Let me have a look...
Message was sent while issue was closed.
On 2015/05/12 13:26:30, Sami wrote: > On 2015/05/12 13:22:21, sof wrote: > > I can't reproduce locally with an asan-debug build, but this seems like a > > plausible candidate for some flaky asan crashers > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil... > > > > looking over the result history (where we unfortunately had a period last > night > > with compile failures.) -- > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%... > > Yep, those call stacks look like they might be related. Let me have a look... Thanks, if anything suggests itself.. Oilpan instability is for the Oilpan team/project/effort to take care of though, still.
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/1134933003/ by dpranke@chromium.org. The reason for reverting is: This is also causing flaky failures on the regular Linux ASAN bot: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/b... is the first of multiple failures. I am able to reproduce the failure 100% of the time locally on my linux desktop with this patch; reverting the patch makes the test pass. Sorry!.
The two problems found by AddressSanitizer were: 1. The scheduler was being shut down *after* the heap was deleted. If there were any pending tasks, they destructors could access freed memory. 2. If the thread was terminated before it had a chance to initialize, we never ended up creating a WorkerGlobalScope. This meant that any tasks that were posted to the thread would try to execute with a null WorkerGlobalScope. Kinuko, could you please have a look at the fixes for these two issues (diff against patch set #11)? FWIW I tried to add a test for the second issue, but since the sequence was so racy I couldn't get the test to actually fail.
On 2015/05/13 16:43:09, Sami wrote: > The two problems found by AddressSanitizer were: > > 1. The scheduler was being shut down *after* the heap was deleted. If there were > any pending tasks, they destructors could access freed memory. > > 2. If the thread was terminated before it had a chance to initialize, we never > ended up creating a WorkerGlobalScope. This meant that any tasks that were > posted to the thread would try to execute with a null WorkerGlobalScope. > > Kinuko, could you please have a look at the fixes for these two issues (diff > against patch set #11)? FWIW I tried to add a test for the second issue, but > since the sequence was so racy I couldn't get the test to actually fail. This is looking good, but I recommend that uploading this change as a new patch (e.g. upload the original patch that has landed as a new issue, then and upload fixed one) so that each review is associated to only one commit log.
On 2015/05/14 06:06:52, kinuko wrote: > This is looking good, but I recommend that uploading this change as a new patch > (e.g. upload the original patch that has landed as a new issue, then and upload > fixed one) so that each review is associated to only one commit log. Thanks, done: https://codereview.chromium.org/1127123010/ (Although I prefer keeping everything in a single review because it is easier to track code review history.)
On 2015/05/14 10:16:10, Sami wrote: > On 2015/05/14 06:06:52, kinuko wrote: > > This is looking good, but I recommend that uploading this change as a new > patch > > (e.g. upload the original patch that has landed as a new issue, then and > upload > > fixed one) so that each review is associated to only one commit log. > > Thanks, done: https://codereview.chromium.org/1127123010/ (Although I prefer > keeping everything in a single review because it is easier to track code review > history.) Thanks! I thought it was discouraged as I was told not to do so several times, but looking it back now it seems it's just subjective. Thanks for doing this anyways. (Just found this thread: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/IcBDPaERTiI/1gOC7...) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
