[chromium] Do not delete CCPrioritizedTexture::Backing structures on the main thread
Created attachment 162571 [details] Patch
Adding jamesr and epenner for review. This is a step towards simplifying the change for bug 95057.
Comment on attachment 162571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162571&action=review > Source/WebCore/ChangeLog:17 > + No new tests (OOPS!). You'll have to do something about this line or an svn presubmit will reject your patch. Describing the existing tests that cover this would be sufficient IMO.
Created attachment 162586 [details] Patch
Comment on attachment 162571 [details] Patch Seems okay to me. I'd be fine creating a one-off list for the impl thread to use and keep this totally single threaded, but by splitting ownership of this class between threads we won't even need such a list? It looks like the only function that executes on the main thread and touches m_backings is PrioritizeTextures(), so it shouldn't be too hard to never touch m_backings on the main thread with a bit more work. Everything else occurs during the commit. I'm fine with this as long as at the end of the day this class can still be used on one thread as well (by just removing the thread asserts if that ever happens).
Thanks! ChangeLog fixed. (In reply to comment #5) > (From update of attachment 162571 [details]) > Seems okay to me. I'd be fine creating a one-off list for the impl thread to use and keep this totally single threaded, but by splitting ownership of this class between threads we won't even need such a list? Yes, we'll be able to avoid the extra list. > It looks like the only function that executes on the main thread and touches m_backings is PrioritizeTextures(), so it shouldn't be too hard to never touch m_backings on the main thread with a bit more work. Everything else occurs during the commit. Yes, looks like it won't need to do that much. > I'm fine with this as long as at the end of the day this class can still be used on one thread as well (by just removing the thread asserts if that ever happens). Yes, if/when we need it to be single-threaded (e.g, impl side painting), we'll need to kill off the asserts.
Comment on attachment 162586 [details] Patch Clearing flags on attachment: 162586 Committed r127820: <http://trac.webkit.org/changeset/127820>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 96117
Fady - do you have any links or logs from failing runs?
Try running all the webkit_unit_tests in debug. I was seeing an assertion on "isMainThread" there.
Aha I found it in my scrollback also: [ RUN ] WebLayerTreeViewSingleThreadTest.InstrumentationCallbacks ASSERTION FAILED: s_mainThread ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCProxy.cpp(84) : static bool WebCore::CCProxy::isMainThread() 1 0x7f52cce762cd 2 0x7f52cce46e37 3 0x7f52cce469d9 4 0x7f52cba09f27 5 0x7f52cba09df6 WebKit::WebLayerTreeView::create(WebKit::WebLayerTreeViewClient*, WebKit::WebLayer const&, WebKit::WebLayerTreeView::Settings const&) 6 0x7f52cb9a98a6 7 0x7f52cbab8173 8 0x7f52cbaaeb5e 9 0x7f52cbaa5fe3 10 0x7f52cbaa673b 11 0x7f52cbaa6ca7 12 0x7f52cbaaafc5 13 0x7f52cbab4c43 14 0x7f52cbab02ae 15 0x7f52cbaaacb4 16 0x7f52cbf4ff35 17 0x7f52cb54361a WebKit::RunAllUnitTests() 18 0x4345cc 19 0x7f52c587c76d __libc_start_main 20 0x4344d9 [18972:18972:0907/163532:237955112578:ERROR:process_util_posix.cc(143)] Received signal 11 base::debug::StackTrace::StackTrace() [0x7f52cfe277fe] base::(anonymous namespace)::StackDumpSignalHandler() [0x7f52cfe9d2e4] 0x7f52c58914c0 WebCore::CCProxy::isMainThread() [0x7f52cce762dc] WebCore::CCLayerTreeHost::CCLayerTreeHost() [0x7f52cce46e37] WebCore::CCLayerTreeHost::create() [0x7f52cce469d9] WebKit::WebLayerTreeViewImpl::initialize() [0x7f52cba09f27] WebKit::WebLayerTreeView::create() [0x7f52cba09df6] (anonymous namespace)::WebLayerTreeViewTestBase::SetUp() [0x7f52cb9a98a6] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x7f52cbab8173] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x7f52cbaaeb5e] testing::Test::Run() [0x7f52cbaa5fe3] testing::TestInfo::Run() [0x7f52cbaa673b] testing::TestCase::Run() [0x7f52cbaa6ca7] testing::internal::UnitTestImpl::RunAllTests() [0x7f52cbaaafc5] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x7f52cbab4c43] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x7f52cbab02ae] testing::UnitTest::Run() [0x7f52cbaaacb4] base::TestSuite::Run() [0x7f52cbf4ff35] WebKit::RunAllUnitTests() [0x7f52cb54361a] main [0x4345cc] 0x7f52c587c76d 0x4344d9
(In reply to comment #11) > Try running all the webkit_unit_tests in debug. I was seeing an assertion on "isMainThread" there. Thanks Dana! I saw this assert fail in garden-o-matic and rolled out the patch forgetting to update the bug report.
Looks like this test didn't WebCompositorSupport::initialize() so our debug statics aren't set up
This was a different issue -- stack trace is ASSERTION FAILED: CCProxy::isImplThread() && CCProxy::isMainThreadBlocked() WebCore::CCPrioritizedTextureManager::clearAllMemory(WebCore::CCResourceProvider *) WebCore::CCPrioritizedTextureManager::clearAllMemory(WebCore::CCResourceProvider*) WebCore::CCLayerTreeHost::deleteContentsTexturesOnImplThread(WebCore::CCResourceProvider*) WebCore::CCSingleThreadProxy::recreateContext() WebCore::CCLayerTreeHost::recreateContext() WebCore::CCLayerTreeHost::initializeRendererIfNeeded() WebCore::CCSingleThreadProxy::commitAndComposite() WebCore::CCSingleThreadProxy::compositeAndReadback(void*, WebCore::IntRect const&) WebCore::CCLayerTreeHost::compositeAndReadback(void*, WebCore::IntRect const&) ... The issue is that CCSingleThreadProxy::recreateContext doesn't correctly specify that the main thread is blocked.
Created attachment 162892 [details] Patch
Comment on attachment 162892 [details] Patch Clearing flags on attachment: 162892 Committed r128005: <http://trac.webkit.org/changeset/128005>