Bug 96018 - [chromium] Do not delete CCPrioritizedTexture::Backing structures on the main thread
Summary: [chromium] Do not delete CCPrioritizedTexture::Backing structures on the main...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 96117
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-06 13:33 PDT by Christopher Cameron
Modified: 2012-09-09 16:59 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.55 KB, patch)
2012-09-06 13:36 PDT, Christopher Cameron
no flags Details | Formatted Diff | Diff
Patch (10.64 KB, patch)
2012-09-06 14:27 PDT, Christopher Cameron
no flags Details | Formatted Diff | Diff
Patch (11.05 KB, patch)
2012-09-07 15:54 PDT, Christopher Cameron
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Cameron 2012-09-06 13:33:32 PDT
[chromium] Do not delete CCPrioritizedTexture::Backing structures on the main thread
Comment 1 Christopher Cameron 2012-09-06 13:36:46 PDT
Created attachment 162571 [details]
Patch
Comment 2 Christopher Cameron 2012-09-06 13:37:57 PDT
Adding jamesr and epenner for review.

This is a step towards simplifying the change for bug 95057.
Comment 3 James Robinson 2012-09-06 14:23:31 PDT
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.
Comment 4 Christopher Cameron 2012-09-06 14:27:18 PDT
Created attachment 162586 [details]
Patch
Comment 5 Eric Penner 2012-09-06 14:27:26 PDT
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).
Comment 6 Christopher Cameron 2012-09-06 14:31:12 PDT
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 7 WebKit Review Bot 2012-09-06 21:39:27 PDT
Comment on attachment 162586 [details]
Patch

Clearing flags on attachment: 162586

Committed r127820: <http://trac.webkit.org/changeset/127820>
Comment 8 WebKit Review Bot 2012-09-06 21:39:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2012-09-07 09:37:34 PDT
Re-opened since this is blocked by 96117
Comment 10 James Robinson 2012-09-07 12:17:36 PDT
Fady - do you have any links or logs from failing runs?
Comment 11 Dana Jansens 2012-09-07 14:31:22 PDT
Try running all the webkit_unit_tests in debug. I was seeing an assertion on "isMainThread" there.
Comment 12 Dana Jansens 2012-09-07 14:33:11 PDT
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
Comment 13 Fady Samuel 2012-09-07 14:33:38 PDT
(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.
Comment 14 James Robinson 2012-09-07 14:41:18 PDT
Looks like this test didn't WebCompositorSupport::initialize() so our debug statics aren't set up
Comment 15 Christopher Cameron 2012-09-07 15:37:41 PDT
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.
Comment 16 Christopher Cameron 2012-09-07 15:54:12 PDT
Created attachment 162892 [details]
Patch
Comment 17 WebKit Review Bot 2012-09-09 16:59:18 PDT
Comment on attachment 162892 [details]
Patch

Clearing flags on attachment: 162892

Committed r128005: <http://trac.webkit.org/changeset/128005>
Comment 18 WebKit Review Bot 2012-09-09 16:59:23 PDT
All reviewed patches have been landed.  Closing bug.