Bug 96018 – [chromium] Do not delete CCPrioritizedTexture::Backing structures on the main thread

• Component: New Bugs
• Status: RESOLVED FIXED

[Christopher Cameron]

[chromium] Do not delete CCPrioritizedTexture::Backing structures on the main thread

[Christopher Cameron]

Created attachment 162571 [details]
Patch

[Christopher Cameron]

Adding jamesr and epenner for review.

This is a step towards simplifying the change for bug 95057.

[James Robinson]

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.

[Christopher Cameron]

Created attachment 162586 [details]
Patch

[Eric Penner]

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).

[Christopher Cameron]

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.

[WebKit Review Bot]

Comment on attachment 162586 [details]
Patch

Clearing flags on attachment: 162586

Committed r127820: <http://trac.webkit.org/changeset/127820>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 96117

[James Robinson]

Fady - do you have any links or logs from failing runs?

[Dana Jansens]

Try running all the webkit_unit_tests in debug. I was seeing an assertion on "isMainThread" there.

[Dana Jansens]

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

[Fady Samuel]

(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.

[James Robinson]

Looks like this test didn't WebCompositorSupport::initialize() so our debug statics aren't set up

[Christopher Cameron]

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.

[Christopher Cameron]

Created attachment 162892 [details]
Patch

[WebKit Review Bot]

Comment on attachment 162892 [details]
Patch

Clearing flags on attachment: 162892

Committed r128005: <http://trac.webkit.org/changeset/128005>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.