Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1761853002:
Shut down all threads before the main thread shuts down (Closed)
DescriptionShut down all threads before the main thread shuts down
Currently the contents in initialize/initializeWithoutV8 and shutdown/shutdownWithoutV8 are not balanced.
For example, HTMLParserThread is initialized in initializeWithoutV8 but shut down in shutdown.
Consequently, HTMLParserThread leaks in tests that call initializeWithoutV8 and shutdownWithoutV8.
This CL makes the contents balanced and thus make sure that the main thread joins all threads
before shutting down.
BUG=
Committed: https://crrev.com/5940f25b9d91258d698a5ce9f48e8a253c7b1652
Cr-Commit-Position: refs/heads/master@{#379543}
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 9
Patch Set 6 : #
Messages
Total messages: 38 (5 generated)
haraken@chromium.org changed reviewers: + kinuko@chromium.org, sigbjornf@opera.com, yhirano@chromium.org
PTAL This CL is necessary to land https://codereview.chromium.org/1754313002. https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webdatabase/DatabaseManager.cpp (right): https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webdatabase/DatabaseManager.cpp:61: ASSERT(isMainThread()); I moved this check because after this CL, shutdownWithoutV8 (which can be called by chromium-side tests) started calling terminateDatabaseThread(). https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebKit.cpp:99: V8Initializer::initializeMainThreadIfNeeded(); This should be renamed to V8Initializer::initializeMainThread. I'll do that in a follow up. https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebKit.cpp:213: V8PerIsolateData::destroy(isolate); Line 210 - 213 should be moved to V8Initializer::shutdownMainThread. I'll do that in a follow up.
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org
Looks like bots are not happy. I'll take a look.
https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebKit.cpp:218: void shutdownWithoutV8() I would like to guarantee that all non-main thread is dead before this function is called. Is it possible to decouple terminateThreads() from ModuleInitializer::shutdown() and call the former in blink::shutdown()?
https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebKit.cpp:218: void shutdownWithoutV8() On 2016/03/03 18:03:11, yhirano wrote: > I would like to guarantee that all non-main thread is dead before this function > is called. Is it possible to decouple terminateThreads() from > ModuleInitializer::shutdown() and call the former in blink::shutdown()? No, that is the key part of this CL. For example, modulesInitializer().init() in initializeWithoutV8 creates a parser thread. So shutdownWithoutV8 must join the parser thread. If we move it to shutdown, we end up with leaking the parser thread in chromium tests that use a pair of initializeWithoutV8/shutdownWithoutV8.
https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebKit.cpp:218: void shutdownWithoutV8() On 2016/03/03 23:47:37, haraken wrote: > On 2016/03/03 18:03:11, yhirano wrote: > > I would like to guarantee that all non-main thread is dead before this > function > > is called. Is it possible to decouple terminateThreads() from > > ModuleInitializer::shutdown() and call the former in blink::shutdown()? > > No, that is the key part of this CL. For example, modulesInitializer().init() in > initializeWithoutV8 creates a parser thread. So shutdownWithoutV8 must join the > parser thread. If we move it to shutdown, we end up with leaking the parser > thread in chromium tests that use a pair of > initializeWithoutV8/shutdownWithoutV8. modulesInitializer().init() creates a HTMLParserThread instance, but it will not create a platform thread until HTMLParserThread::postTask is called. Is the function called when initialize() is not called?
On 2016/03/03 23:58:18, yhirano wrote: > https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebKit.cpp (right): > > https://codereview.chromium.org/1761853002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebKit.cpp:218: void shutdownWithoutV8() > On 2016/03/03 23:47:37, haraken wrote: > > On 2016/03/03 18:03:11, yhirano wrote: > > > I would like to guarantee that all non-main thread is dead before this > > function > > > is called. Is it possible to decouple terminateThreads() from > > > ModuleInitializer::shutdown() and call the former in blink::shutdown()? > > > > No, that is the key part of this CL. For example, modulesInitializer().init() > in > > initializeWithoutV8 creates a parser thread. So shutdownWithoutV8 must join > the > > parser thread. If we move it to shutdown, we end up with leaking the parser > > thread in chromium tests that use a pair of > > initializeWithoutV8/shutdownWithoutV8. > > modulesInitializer().init() creates a HTMLParserThread instance, but it will not > create a platform thread until HTMLParserThread::postTask is called. Is the > function called when initialize() is not called? Yeah, the WebThreadSupportingGC for the parser thread is instantiated in many chromium tests that use initializeWithoutV8/shutdownWithoutV8. Another weird thing I found is that the chromium tests call initializeWithoutV8 and shutdownWithoutV8 from different threads, which is not supported in Blink. I think we need to fix the chromium tests somehow.
Now all tests pass. PTAL. yhirano@: Would you elaborate on why you want to move the shutdown code from shutdownWithoutV8() to shutdown()?
On 2016/03/04 07:14:06, haraken wrote: > Now all tests pass. PTAL. > > yhirano@: Would you elaborate on why you want to move the shutdown code from > shutdownWithoutV8() to shutdown()? Mainly because shutdownWithoutV8 can be called from a non-main thread. It makes things complicated - the change in DatabaseManager.cpp is one example.
On 2016/03/04 07:50:31, yhirano wrote: > On 2016/03/04 07:14:06, haraken wrote: > > Now all tests pass. PTAL. > > > > yhirano@: Would you elaborate on why you want to move the shutdown code from > > shutdownWithoutV8() to shutdown()? > > Mainly because shutdownWithoutV8 can be called from a non-main thread. It makes > things complicated - the change in DatabaseManager.cpp is one example. Which tests/code's calling shutdownWithoutV8 on non-main thread? (I can check that later but if you could just tell that'd be helpful...)
On 2016/03/04 09:03:30, kinuko wrote: > On 2016/03/04 07:50:31, yhirano wrote: > > On 2016/03/04 07:14:06, haraken wrote: > > > Now all tests pass. PTAL. > > > > > > yhirano@: Would you elaborate on why you want to move the shutdown code > from > > > shutdownWithoutV8() to shutdown()? > > > > Mainly because shutdownWithoutV8 can be called from a non-main thread. It > makes > > things complicated - the change in DatabaseManager.cpp is one example. > > Which tests/code's calling shutdownWithoutV8 on non-main thread? (I can check > that later but if you could just tell that'd be helpful...) All browser_tests, interactive_ui_tests and a bunch :) It happens here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
On 2016/03/04 07:50:31, yhirano wrote: > On 2016/03/04 07:14:06, haraken wrote: > > Now all tests pass. PTAL. > > > > yhirano@: Would you elaborate on why you want to move the shutdown code from > > shutdownWithoutV8() to shutdown()? > > Mainly because shutdownWithoutV8 can be called from a non-main thread. It makes > things complicated - the change in DatabaseManager.cpp is one example. Hmm. Your point makes sense but in fact it looks like that the HTML parser thread is created in interactive_ui_tests, browser_tests etc. interactive_ui_tests, browser_tests etc are using a SandboxIPCHandler's thread to control Blink (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...). This is not *the* main thread, but conceptually it is a main thread of the interactive_ui_tests, browser_tests etc...
On 2016/03/04 11:04:00, haraken wrote: > On 2016/03/04 07:50:31, yhirano wrote: > > On 2016/03/04 07:14:06, haraken wrote: > > > Now all tests pass. PTAL. > > > > > > yhirano@: Would you elaborate on why you want to move the shutdown code > from > > > shutdownWithoutV8() to shutdown()? > > > > Mainly because shutdownWithoutV8 can be called from a non-main thread. It > makes > > things complicated - the change in DatabaseManager.cpp is one example. > > Hmm. Your point makes sense but in fact it looks like that the HTML parser > thread is created in interactive_ui_tests, browser_tests etc. > > interactive_ui_tests, browser_tests etc are using a SandboxIPCHandler's thread > to control Blink > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...). > This is not *the* main thread, but conceptually it is a main thread of the > interactive_ui_tests, browser_tests etc... Can you tell me what happens when we post a task to Platform::current()->mainThread() in such an environment?
On 2016/03/05 00:59:14, yhirano wrote: > On 2016/03/04 11:04:00, haraken wrote: > > On 2016/03/04 07:50:31, yhirano wrote: > > > On 2016/03/04 07:14:06, haraken wrote: > > > > Now all tests pass. PTAL. > > > > > > > > yhirano@: Would you elaborate on why you want to move the shutdown code > > from > > > > shutdownWithoutV8() to shutdown()? > > > > > > Mainly because shutdownWithoutV8 can be called from a non-main thread. It > > makes > > > things complicated - the change in DatabaseManager.cpp is one example. > > > > Hmm. Your point makes sense but in fact it looks like that the HTML parser > > thread is created in interactive_ui_tests, browser_tests etc. > > > > interactive_ui_tests, browser_tests etc are using a SandboxIPCHandler's thread > > to control Blink > > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...). > > This is not *the* main thread, but conceptually it is a main thread of the > > interactive_ui_tests, browser_tests etc... > > Can you tell me what happens when we post a task to > Platform::current()->mainThread() in such an environment? I don't think it will work because initializeWithoutV8/shutdownWithoutV8 needs to be called on a thread that actually uses Blink. If you want to make sure that initializeWithoutV8/shutdownWithoutV8 is called on the main thread, I think we need to change the chromium-side tests so that it uses the main thread rather than the thread in SandboxIPCHandler (I'm not sure if this is doable).
Sorry, I still don't understand. Looking at [1], [2], [3] and [4], it seems A: isMainThread returns true if called on the thread on which initializeWithoutV8 is called ([1], [2], [3]). B: mainThread() returns the thread on which initializeWithoutV8 is called ([1], [4]). Are these correct? If the above statements are correct, assertion errors in [5] mean X: initializeWithoutV8 is not called while shutdownWithoutV8 is called, or Y: initializeWithoutV8 and shutdownWithoutV8 are called on different threads. Is this correct? 1: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... 2: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... 3: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... 4: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... 5: https://codereview.chromium.org/1768783002/
On 2016/03/04 09:59:02, haraken wrote: > On 2016/03/04 09:03:30, kinuko wrote: > > On 2016/03/04 07:50:31, yhirano wrote: > > > On 2016/03/04 07:14:06, haraken wrote: > > > > Now all tests pass. PTAL. > > > > > > > > yhirano@: Would you elaborate on why you want to move the shutdown code > > from > > > > shutdownWithoutV8() to shutdown()? > > > > > > Mainly because shutdownWithoutV8 can be called from a non-main thread. It > > makes > > > things complicated - the change in DatabaseManager.cpp is one example. > > > > Which tests/code's calling shutdownWithoutV8 on non-main thread? (I can check > > that later but if you could just tell that'd be helpful...) > > All browser_tests, interactive_ui_tests and a bunch :) > > It happens here: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... Hmm, they're called in the browser process and at this point what the 'main thread' would mean is getting a bit unclear... I suspect they would need to initialize smaller set of blink modules like WebFont than initializeWithoutV8 (where we definitely wouldn't need html parser threads), we might want to do a special minimal initialization/shutdown for this case?
On 2016/03/07 04:47:53, kinuko wrote: > On 2016/03/04 09:59:02, haraken wrote: > > On 2016/03/04 09:03:30, kinuko wrote: > > > On 2016/03/04 07:50:31, yhirano wrote: > > > > On 2016/03/04 07:14:06, haraken wrote: > > > > > Now all tests pass. PTAL. > > > > > > > > > > yhirano@: Would you elaborate on why you want to move the shutdown code > > > from > > > > > shutdownWithoutV8() to shutdown()? > > > > > > > > Mainly because shutdownWithoutV8 can be called from a non-main thread. It > > > makes > > > > things complicated - the change in DatabaseManager.cpp is one example. > > > > > > Which tests/code's calling shutdownWithoutV8 on non-main thread? (I can > check > > > that later but if you could just tell that'd be helpful...) > > > > All browser_tests, interactive_ui_tests and a bunch :) > > > > It happens here: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > Hmm, they're called in the browser process and at this point what the 'main > thread' would mean is getting a bit unclear... I suspect they would need to > initialize smaller set of blink modules like WebFont than initializeWithoutV8 > (where we definitely wouldn't need html parser threads), we might want to do a > special minimal initialization/shutdown for this case? Well looks like yhirano's questions should be answered first.
On 2016/03/07 04:52:48, kinuko wrote: > On 2016/03/07 04:47:53, kinuko wrote: > > On 2016/03/04 09:59:02, haraken wrote: > > > On 2016/03/04 09:03:30, kinuko wrote: > > > > On 2016/03/04 07:50:31, yhirano wrote: > > > > > On 2016/03/04 07:14:06, haraken wrote: > > > > > > Now all tests pass. PTAL. > > > > > > > > > > > > yhirano@: Would you elaborate on why you want to move the shutdown > code > > > > from > > > > > > shutdownWithoutV8() to shutdown()? > > > > > > > > > > Mainly because shutdownWithoutV8 can be called from a non-main thread. > It > > > > makes > > > > > things complicated - the change in DatabaseManager.cpp is one example. > > > > > > > > Which tests/code's calling shutdownWithoutV8 on non-main thread? (I can > > check > > > > that later but if you could just tell that'd be helpful...) > > > > > > All browser_tests, interactive_ui_tests and a bunch :) > > > > > > It happens here: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > Hmm, they're called in the browser process and at this point what the 'main > > thread' would mean is getting a bit unclear... I suspect they would need to > > initialize smaller set of blink modules like WebFont than initializeWithoutV8 > > (where we definitely wouldn't need html parser threads), we might want to do a > > special minimal initialization/shutdown for this case? > > Well looks like yhirano's questions should be answered first. ...and I guess Y. would be the case, initialize is called lazily on the linux IPC handler thread while it's shutdown in the dtor of linux IPC handler which will likely be called on the thread which created the handler thread.
On 2016/03/07 04:42:13, yhirano wrote: > Sorry, I still don't understand. > > Looking at [1], [2], [3] and [4], it seems > A: isMainThread returns true if called on the thread on which > initializeWithoutV8 is called ([1], [2], [3]). > B: mainThread() returns the thread on which initializeWithoutV8 is called ([1], > [4]). > > Are these correct? > > If the above statements are correct, assertion errors in [5] mean > X: initializeWithoutV8 is not called while shutdownWithoutV8 is called, or > Y: initializeWithoutV8 and shutdownWithoutV8 are called on different threads. > > Is this correct? > > 1: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > 2: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > 3: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > 4: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > 5: https://codereview.chromium.org/1768783002/ Good point. In PS5, I changed SandboxIPCHandler so that initializeWithoutV8 and shutdownWithoutV8 are called in the same thread. This guarantees that shutdownWithoutV8 is called by the main thread. I inserted ASSERT(isMainThread) to shutdownWithoutV8. Does this look reasonable?
If all tests go well this looks reasonable to me. https://codereview.chromium.org/1761853002/diff/80001/content/browser/rendere... File content/browser/renderer_host/sandbox_ipc_linux.cc (right): https://codereview.chromium.org/1761853002/diff/80001/content/browser/rendere... content/browser/renderer_host/sandbox_ipc_linux.cc:124: blink::shutdownWithoutV8(); Yeah this'd work.
Great! LGTM.
kinuko-san: Does it look good to you?
https://codereview.chromium.org/1761853002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/1761853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebKit.cpp:208: ThreadState::current()->unregisterTraceDOMWrappers(); Why is this needed now & here? (sorry, haven't followed the patchset updates here.)
https://codereview.chromium.org/1761853002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/1761853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebKit.cpp:208: ThreadState::current()->unregisterTraceDOMWrappers(); On 2016/03/07 07:29:28, sof wrote: > Why is this needed now & here? (sorry, haven't followed the patchset updates > here.) ThreadState::detachMainThread can trigger an Oilpan's GC. The Oilpan GC should not trace V8 wrappers because the isolate is already existed at that point. Also the Oilpan GC should assume that no V8 wrappers are reachable at that point. So I added unregisterTraceDOMWrapper here to let Oilpan know about it before the isolate gets destroyed.
https://codereview.chromium.org/1761853002/diff/80001/content/browser/rendere... File content/browser/renderer_host/sandbox_ipc_linux.cc (right): https://codereview.chromium.org/1761853002/diff/80001/content/browser/rendere... content/browser/renderer_host/sandbox_ipc_linux.cc:124: blink::shutdownWithoutV8(); naive question: would things break horribly if this called blink::shutdown() ? https://codereview.chromium.org/1761853002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/1761853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebKit.cpp:208: ThreadState::current()->unregisterTraceDOMWrappers(); On 2016/03/07 07:33:56, haraken wrote: > On 2016/03/07 07:29:28, sof wrote: > > Why is this needed now & here? (sorry, haven't followed the patchset updates > > here.) > > ThreadState::detachMainThread can trigger an Oilpan's GC. The Oilpan GC should > not trace V8 wrappers because the isolate is already existed at that point. Also > the Oilpan GC should assume that no V8 wrappers are reachable at that point. So > I added unregisterTraceDOMWrapper here to let Oilpan know about it before the > isolate gets destroyed. > I see, and this "detach" of v8 from Oilpan needs to be handled separately from detachMainThread(). Can detachMainThread() assert that the v8 detach has happened?
https://codereview.chromium.org/1761853002/diff/80001/content/browser/rendere... File content/browser/renderer_host/sandbox_ipc_linux.cc (right): https://codereview.chromium.org/1761853002/diff/80001/content/browser/rendere... content/browser/renderer_host/sandbox_ipc_linux.cc:124: blink::shutdownWithoutV8(); On 2016/03/07 07:54:03, sof wrote: > naive question: would things break horribly if this called blink::shutdown() ? Do you mean we should call blink::initialize() and blink::shutdown()? I think it will just initialize more things (i.e., V8) than needed. Actually, as kinuko-san pointed out before, initializeWithoutV8 would be already too much for the call sites. Initializing WebFonts etc would be enough. Calling blink::initializeWithoutV8() and blink::shutdown() will break things at the shutdown. https://codereview.chromium.org/1761853002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/1761853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebKit.cpp:208: ThreadState::current()->unregisterTraceDOMWrappers(); On 2016/03/07 07:54:03, sof wrote: > On 2016/03/07 07:33:56, haraken wrote: > > On 2016/03/07 07:29:28, sof wrote: > > > Why is this needed now & here? (sorry, haven't followed the patchset updates > > > here.) > > > > ThreadState::detachMainThread can trigger an Oilpan's GC. The Oilpan GC should > > not trace V8 wrappers because the isolate is already existed at that point. > Also > > the Oilpan GC should assume that no V8 wrappers are reachable at that point. > So > > I added unregisterTraceDOMWrapper here to let Oilpan know about it before the > > isolate gets destroyed. > > > > I see, and this "detach" of v8 from Oilpan needs to be handled separately from > detachMainThread(). Can detachMainThread() assert that the v8 detach has > happened? Done.
https://codereview.chromium.org/1761853002/diff/80001/content/browser/rendere... File content/browser/renderer_host/sandbox_ipc_linux.cc (right): https://codereview.chromium.org/1761853002/diff/80001/content/browser/rendere... content/browser/renderer_host/sandbox_ipc_linux.cc:124: blink::shutdownWithoutV8(); On 2016/03/07 08:02:34, haraken wrote: > On 2016/03/07 07:54:03, sof wrote: > > naive question: would things break horribly if this called blink::shutdown() ? > > Do you mean we should call blink::initialize() and blink::shutdown()? I think it > will just initialize more things (i.e., V8) than needed. Actually, as kinuko-san > pointed out before, initializeWithoutV8 would be already too much for the call > sites. Initializing WebFonts etc would be enough. > > Calling blink::initializeWithoutV8() and blink::shutdown() will break things at > the shutdown. ok, thanks. And allocating via Oilpan is an unavoidable Blink extra, hence it needs to be in the "withoutV8" subset. (Not that it was prior to this.)
https://codereview.chromium.org/1761853002/diff/80001/content/browser/rendere... File content/browser/renderer_host/sandbox_ipc_linux.cc (right): https://codereview.chromium.org/1761853002/diff/80001/content/browser/rendere... content/browser/renderer_host/sandbox_ipc_linux.cc:124: blink::shutdownWithoutV8(); On 2016/03/07 08:20:38, sof wrote: > On 2016/03/07 08:02:34, haraken wrote: > > On 2016/03/07 07:54:03, sof wrote: > > > naive question: would things break horribly if this called blink::shutdown() > ? > > > > Do you mean we should call blink::initialize() and blink::shutdown()? I think > it > > will just initialize more things (i.e., V8) than needed. Actually, as > kinuko-san > > pointed out before, initializeWithoutV8 would be already too much for the call > > sites. Initializing WebFonts etc would be enough. > > > > Calling blink::initializeWithoutV8() and blink::shutdown() will break things > at > > the shutdown. > > ok, thanks. And allocating via Oilpan is an unavoidable Blink extra, hence it > needs to be in the "withoutV8" subset. (Not that it was prior to this.) Yes. Heap::init was already in initializeWithoutV8 (before this CL).
lgtm.
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1761853002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1761853002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1761853002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Shut down all threads before the main thread shuts down Currently the contents in initialize/initializeWithoutV8 and shutdown/shutdownWithoutV8 are not balanced. For example, HTMLParserThread is initialized in initializeWithoutV8 but shut down in shutdown. Consequently, HTMLParserThread leaks in tests that call initializeWithoutV8 and shutdownWithoutV8. This CL makes the contents balanced and thus make sure that the main thread joins all threads before shutting down. BUG= ========== to ========== Shut down all threads before the main thread shuts down Currently the contents in initialize/initializeWithoutV8 and shutdown/shutdownWithoutV8 are not balanced. For example, HTMLParserThread is initialized in initializeWithoutV8 but shut down in shutdown. Consequently, HTMLParserThread leaks in tests that call initializeWithoutV8 and shutdownWithoutV8. This CL makes the contents balanced and thus make sure that the main thread joins all threads before shutting down. BUG= Committed: https://crrev.com/5940f25b9d91258d698a5ce9f48e8a253c7b1652 Cr-Commit-Position: refs/heads/master@{#379543} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5940f25b9d91258d698a5ce9f48e8a253c7b1652 Cr-Commit-Position: refs/heads/master@{#379543} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
