New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 158747 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Dec 2012
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Chrome: Crash Report - Stack Signature: cc::LayerTreeHost::applyScrollAndScale(cc::...

Project Member Reported by dharani@google.com, Oct 31 2012

Issue description

ccameron: is this related to any of your changes? This is one of the top crashes in renderer.

Product: Chrome
Stack Signature: cc::LayerTreeHost::applyScrollAndScale(cc::ScrollAndScaleSet const &)-6AD8CD7
New Signature Label: cc::LayerTreeHost::applyScrollAndScale(cc::ScrollAndScaleSet const &)
New Signature Hash: 66f69e51_0007c570_69f5186f_d3f2d68b_c1b35c4c

Report link: http://go/crash/reportdetail?reportid=1bc8c17ecde94005

Meta information:
Product Name: Chrome
Product Version: 24.0.1312.1
Report ID: 1bc8c17ecde94005
Report Time: 2012/10/31 07:46:05, Wed
Uptime: 12 sec
Cumulative Uptime: 0 sec
OS Name: Windows NT
OS Version: 6.1.7601 Service Pack 1
CPU Architecture: x86
CPU Info: GenuineIntel family 6 model 37 stepping 2
ptype: renderer


Thread 0 *CRASHED* ( EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000004 )

0x033dffe3	 [chrome.dll]	 - layer_tree_host.cc:699 (cs|src|ann)]	cc::LayerTreeHost::applyScrollAndScale(cc::ScrollAndScaleSet const &)
0x033ee914	 [chrome.dll]	 - thread_proxy.cc:559 (cs|src|ann)]	cc::ThreadProxy::beginFrame()
0x033ecab9	 [chrome.dll]	 - scoped_thread_proxy.h:67 (cs|src|ann)]	cc::ScopedThreadProxy::runTaskIfNotShutdown(WTF::PassOwnPtr<cc::Thread::Task>)
0x033ec342	 [chrome.dll]	 - thread_task.h:61 (cs|src|ann)]	cc::ThreadTask1<cc::ScopedThreadProxy,WTF::PassOwnPtr<cc::Thread::Task>,WTF::PassOwnPtr<cc::Thread::Task> >::performTask()
0x019715e5	 [chrome.dll]	 - message_loop.cc:470 (cs|src|ann)]	MessageLoop::RunTask(base::PendingTask const &)
0x01971032	 [chrome.dll]	 - message_loop.cc:665 (cs|src|ann)]	MessageLoop::DoWork()
0x019717d1	 [chrome.dll]	 - message_pump_default.cc:28 (cs|src|ann)]	base::MessagePumpDefault::Run(base::MessagePump::Delegate *)
0x01970c82	 [chrome.dll]	 - message_loop.cc:427 (cs|src|ann)]	MessageLoop::RunInternal()
0x01970bda	 [chrome.dll]	 - run_loop.cc:45 (cs|src|ann)]	base::RunLoop::Run()
0x019a22dd	 [chrome.dll]	 - message_loop.cc:307 (cs|src|ann)]	MessageLoop::Run()
0x019be75e	 [chrome.dll]	 - renderer_main.cc:241 (cs|src|ann)]	content::RendererMain(content::MainFunctionParams const &)
0x01958a1b	 [chrome.dll]	 - content_main_runner.cc:448 (cs|src|ann)]	content::RunNamedProcessTypeMain(std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,content::MainFunctionParams const &,content::ContentMainDelegate *)
0x019589a2	 [chrome.dll]	 - content_main_runner.cc:741 (cs|src|ann)]	content::ContentMainRunnerImpl::Run()
0x0194a8fa	 [chrome.dll]	 - content_main.cc:35 (cs|src|ann)]	content::ContentMain(HINSTANCE__ *,sandbox::SandboxInterfaceInfo *,content::ContentMainDelegate *)
0x0194a5ec	 [chrome.dll]	 - chrome_main.cc:28 (cs|src|ann)]	ChromeMain
0x002e5333	 [chrome.exe]	 - client_util.cc:440 (cs|src|ann)]	MainDllLoader::Launch(HINSTANCE__ *,sandbox::SandboxInterfaceInfo *)
0x002e7ec3	 [chrome.exe]	 - chrome_exe_main_win.cc:76 (cs|src|ann)]	RunChrome(HINSTANCE__ *)
0x002e7f2e	 [chrome.exe]	 - chrome_exe_main_win.cc:92 (cs|src|ann)]	wWinMain
0x00340a3c	 [chrome.exe]	 - crt0.c:275]	__tmainCRTStartup
0x762c33a9	 [kernel32.dll]	 + 0x000133a9]	BaseThreadInitThunk
0x777e9ef1	 [ntdll.dll]	 + 0x00039ef1]	__RtlUserThreadStart
0x777e9ec4	 [ntdll.dll]	 + 0x00039ec4]	_RtlUserThreadStart
 
Cc: ccameron@chromium.org
Owner: ----
No, this is not related to any areas that I've changed.  Moving myself from owner -> cc.

Comment 3 by jam...@chromium.org, Oct 31 2012

Cc: mazda@chromium.org vangelis@chromium.org piman@chromium.org
Owner: jam...@chromium.org
The crash in #1 does seem related.  Looks like we have a race on cc::ThreadProxy::m_pendingBeginFrameRequest causing it to get switched out from under one of the threads, otherwise I can't think of how we would crash at this point.  The most complicated logic here would be compositeAndReadback().

@mazda - the renderer-side thumbnailer is complete dead these days on win7, right?

@vangelis - the field trials are not fully there on this report, what's going on?

Comment 4 by jam...@chromium.org, Oct 31 2012

Cc: nduca@chromium.org
Looks like there's an extension API that goes down the compositeAndReadback() path so we have to support that anyway.  Really not sure why this code is using a member instead of just using message passing.

Comment 5 by piman@chromium.org, Oct 31 2012

Cc: backer@chromium.org
+backer who touched this code a bit, though I'm not sure how his changes could cause this.

Comment 6 by dharani@google.com, Oct 31 2012

enne@ pointed out that it regressed from 1305.3 build. Here are the changes within that regressed range:

http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&range=163227:163501&mode=html

Comment 7 by enne@chromium.org, Oct 31 2012

dharani: Sadly I renamed all the classes during that range, so I think it's been happening for longer, e.g. m22: https://crash.corp.google.com/reportdetail?reportid=c5b6a6bbcefe1981

See: https://crash.corp.google.com/search?query=%22CCLayerTreeHost%3A%3AapplyScrollAndScale%28cc%3A%3ACCScrollAndScaleSet+const+%26%29

Comment 8 by jam...@chromium.org, Oct 31 2012

I don't think it was backer@'s change, given that this is on windows, but he has been playing with this code a lot lately so he might have some insight.

Here's an old crash that I think is the same issue: http://code.google.com/p/chromium/issues/detail?id=124847

Comment 9 by dharani@google.com, Oct 31 2012

Labels: -ReleaseBlock-Beta
since its a long tail, removing the blocker label.
@jamesr : it looks like the field trials string is getting truncated.  At this point we should be running threaded compositing on windows in canary and dev. 

Cc: skyos...@chromium.org apatrick@chromium.org
I think this is a bad interaction with compositeAndReadback().  Using Sami's excellent suggestion of putting a sleep(1) in a strategic location (from http://code.google.com/p/chromium/issues/detail?id=124847#c15) I can repro if I force lots of compositeAndReadback() calls while scrolling a page from the impl thread.

I suspect the reason this crash is somewhat rare is we don't actually use compositeAndReadback() all that much - I think we never use it on chromeos and we only use it on windows when using the XP presentation path.  The crashes cited so far match the texture_sharing blacklist entry as far as I can tell (optimus info isn't in the crash report), so we'll only see crashes from those users.

The fix might be a little tricky.
There are two possible entry points to compositeAndReadback:
1.) Thumbnailer
2.) extensions calling the chrome.tabs.captureVisibleTab() API
@#11: We use the compositeAndReadback in the browser UI for snapshotting windows (Alt-Shift-I) on CrOS.
Patch up here: https://codereview.chromium.org/11362054/

For m24, this is probably only impacting windows vista+ users who have texture_sharing blacklisted since with the XP presentation path readbacks go to the renderer instead of just reading back the browser surface. As an alternative to merging the code fix we could disable threaded compositing for those users.
would you like to disable it directly in the branch?
We could disable on trunk and let that sit in canary - I think it'll be a smaller patch, although I'm not as familiar with that part of the code.
 Issue 158200  has been merged into this issue.
https://codereview.chromium.org/11361077/ will disable the thread on machines that are hitting this path - I think that'll suffice for m24 and we can land + bake the real fix on trunk for m25+
Project Member

Comment 19 by bugdroid1@chromium.org, Nov 4 2012

Summary: Chrome: Crash Report - Stack Signature: cc::LayerTreeHost::applyScrollAndScale(cc::...
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=165866

------------------------------------------------------------------------
r165866 | jamesr@chromium.org | 2012-11-04T00:03:56.633891Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/gpu/compositor_util.cc?r1=165866&r2=165865&pathrev=165866
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/compositor_bindings/web_compositor_support_impl.cc?r1=165866&r2=165865&pathrev=165866
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/gpu/gpu_feature_browsertest.cc?r1=165866&r2=165865&pathrev=165866

Disable threaded compositing when texture sharing is blacklisted

When texture_sharing is blacklisted, we can't do readbacks from the
browser process and thus hit the render process to do readbacks for
the thumbnailer or the extensions getVisibleTab() API. This path is
a bit buggy in threaded compositing mode, so disable the thread.

BUG= 158747 


Review URL: https://chromiumcodereview.appspot.com/11361077
------------------------------------------------------------------------
Labels: Merge-Requested
@dharani - r165866 missed the 25.0.1317.0 canary by 10 revs, but once it goes out in canary I'd like to merge it to m24 since I believe it'll take care of the crashes there.  Setting the Merge-Requested bit now, but I'm happy to wait for canary if you'd like.
Labels: -Merge-Requested Merge-Approved
let's merge it in 1312 branch. I'll look at tomorrow's canary for any side effects.
Labels: -Merge-Approved Merge-Merged
OK - http://src.chromium.org/viewvc/chrome?view=rev&revision=165955
Project Member

Comment 23 by bugdroid1@chromium.org, Nov 5 2012

Labels: merge-merged-1312
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=165955

------------------------------------------------------------------------
r165955 | jamesr@chromium.org | 2012-11-05T18:08:58.147971Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/chrome/test/gpu/gpu_feature_browsertest.cc?r1=165955&r2=165954&pathrev=165955
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/content/browser/gpu/compositor_util.cc?r1=165955&r2=165954&pathrev=165955
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/webkit/compositor_bindings/web_compositor_support_impl.cc?r1=165955&r2=165954&pathrev=165955

Merge 165866 - Disable threaded compositing when texture sharing is blacklisted

When texture_sharing is blacklisted, we can't do readbacks from the
browser process and thus hit the render process to do readbacks for
the thumbnailer or the extensions getVisibleTab() API. This path is
a bit buggy in threaded compositing mode, so disable the thread.

BUG= 158747 


Review URL: https://chromiumcodereview.appspot.com/11361077

TBR=jamesr@chromium.org
Review URL: https://codereview.chromium.org/11368084
------------------------------------------------------------------------
James, even after we got this fix merged, 1312.5 still has these crashes :(
James, is request->scrollInfo in:

http://code.google.com/searchframe#OAMlx_jo-ck/src/cc/thread_proxy.cc&exact_package=chromium&q=thread_proxy.cc&type=cs&l=559

guaranteed to be pointing to valid, initialized memory ? 
#24 - yeah I see that.  I don't have any great ideas for now other than http://codereview.chromium.org/11362054/ which should fix the underlying race condition.

#25 - if we hit this race is isn't, otherwise it should be.
Project Member

Comment 27 by bugdroid1@chromium.org, Nov 14 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=167537

------------------------------------------------------------------------
r167537 | jamesr@chromium.org | 2012-11-14T00:46:18.307124Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/scheduler.cc?r1=167537&r2=167536&pathrev=167537
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/scheduler_state_machine_unittest.cc?r1=167537&r2=167536&pathrev=167537
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/scheduler_state_machine.cc?r1=167537&r2=167536&pathrev=167537
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/scheduler_state_machine.h?r1=167537&r2=167536&pathrev=167537
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/layer_tree_host_unittest.cc?r1=167537&r2=167536&pathrev=167537
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/layer_tree_host.cc?r1=167537&r2=167536&pathrev=167537
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/thread_proxy.cc?r1=167537&r2=167536&pathrev=167537
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/thread_proxy.h?r1=167537&r2=167536&pathrev=167537

Use message passing for BeginFrameAndCommitState and clean up forced commit logic

This passes the BeginFrameAndCommitState struct along with messages instead of
relying on (racy) shared memory and slightly simplifies the forced commit logic.
This logic is for compositeAndReadback() and is trying to go through the normal
commit flow while the main thread is blocked. The normal commit flow is this:

1.) Main thread posts task to compositor thread requesting commit
2.) Compositor thread receives notification, after some delay posts task to main
to run the beginFrame logic
3.) Main thread runs the beginFrameTask, posts a blocking task to compositor
thread
  4.) With the main thread is blocked, compositor thread runs the beginFrameComplete
  task and signals the main thread to get on with life
5.) Compositor thread waits for an opportunity to draw (vsync) then goes back to
the initial state.


compositeAndReadback() requires that we run through the whole flow up through 5
without yielding the main thread at all. However, while it's doing this there
may very well be another beginFrame task pending on the main thread's message
queue. When this task does eventually run it needs to have a valid BFAC state
and the reply has to be expected on the compositor side.

This patch always passes the BFAC state on the beginFrame task and uses a null
BFAC state for compositeAndReadback-forced beginFrame()s. This means when we do
a compositeAndReadback we won't "see" state that has updated only on the
compositor thread, but that's fine since we'll apply that state once we process
the beginFrame task later on and BFAC state addition is additive. The tricky
bit from the compositor side is keeping track of whether there's a pending
beginFrame message after going through the forced commit flow. To keep track
of this I've added SchedulerStateMachine::m_expectImmediateBeginFrame that
keeps the scheduler in a state that expects a beginFrameComplete after
finishing the readback.

Typical flow:

1.) Main thread enters compositeAndReadback(), posts a blocking forceBeginFrame
task to compositor thread
2.) Compositor thread posts beginFrame task to main thread
3.) Compositor thread processes forceBeginFrame task, sets scheduler bits,
signals main thread to continue
4.) Main thread runs beginFrame(NULL), posts beginFrameComplete to compositor thread
5.) Compositor thread processes beginFrameComplete, runs through state machine up
to draw and then goes back to waiting for a reply to the beginFrame it posted in
step (2)
6.) Main thread issues sync readback, finishes compositeAndReadback()
7.) Main thread processes beginFrame message posted in (2), posts reply
8.) Profit!

Note that steps (2) and (3) can happen in any order.


BUG= 158747 


Review URL: https://chromiumcodereview.appspot.com/11362054
------------------------------------------------------------------------
I believe r167537 is the general fix for this crash.  The issue is quite subtle so I think we will want to follow canary reports for a few days to be sure.  If this does fix the crash we can consider either merging this fix back to m24 or turning the feature off.  I don't know of any other way to address this.
Labels: -Merge-Merged -merge-merged-1312 Merge-Requested
I see a smallish number of crashes related to this bug on 25.0.1324.0 windows canary but zero on 25.0.1325.0 so far.  Shall we merge, Dharani?
Another day of crash data supports the theory that r167537 fixed this for realz
Labels: -Merge-Requested Merge-Approved
Thanks James for looking out! I was checking as well.
Labels: -Merge-Approved Merge-Merged
http://src.chromium.org/viewvc/chrome?view=rev&revision=168095
Project Member

Comment 33 by bugdroid1@chromium.org, Nov 16 2012

Labels: merge-merged-1312
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=168095

------------------------------------------------------------------------
r168095 | jamesr@chromium.org | 2012-11-16T00:25:31.078959Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/layer_tree_host_unittest.cc?r1=168095&r2=168094&pathrev=168095
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/layer_tree_host.cc?r1=168095&r2=168094&pathrev=168095
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.cc?r1=168095&r2=168094&pathrev=168095
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.h?r1=168095&r2=168094&pathrev=168095
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler.cc?r1=168095&r2=168094&pathrev=168095
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine_unittest.cc?r1=168095&r2=168094&pathrev=168095
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine.cc?r1=168095&r2=168094&pathrev=168095
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine.h?r1=168095&r2=168094&pathrev=168095

Merge 167537 - Use message passing for BeginFrameAndCommitState and clean up forced commit logic

This passes the BeginFrameAndCommitState struct along with messages instead of
relying on (racy) shared memory and slightly simplifies the forced commit logic.
This logic is for compositeAndReadback() and is trying to go through the normal
commit flow while the main thread is blocked. The normal commit flow is this:

1.) Main thread posts task to compositor thread requesting commit
2.) Compositor thread receives notification, after some delay posts task to main
to run the beginFrame logic
3.) Main thread runs the beginFrameTask, posts a blocking task to compositor
thread
  4.) With the main thread is blocked, compositor thread runs the beginFrameComplete
  task and signals the main thread to get on with life
5.) Compositor thread waits for an opportunity to draw (vsync) then goes back to
the initial state.


compositeAndReadback() requires that we run through the whole flow up through 5
without yielding the main thread at all. However, while it's doing this there
may very well be another beginFrame task pending on the main thread's message
queue. When this task does eventually run it needs to have a valid BFAC state
and the reply has to be expected on the compositor side.

This patch always passes the BFAC state on the beginFrame task and uses a null
BFAC state for compositeAndReadback-forced beginFrame()s. This means when we do
a compositeAndReadback we won't "see" state that has updated only on the
compositor thread, but that's fine since we'll apply that state once we process
the beginFrame task later on and BFAC state addition is additive. The tricky
bit from the compositor side is keeping track of whether there's a pending
beginFrame message after going through the forced commit flow. To keep track
of this I've added SchedulerStateMachine::m_expectImmediateBeginFrame that
keeps the scheduler in a state that expects a beginFrameComplete after
finishing the readback.

Typical flow:

1.) Main thread enters compositeAndReadback(), posts a blocking forceBeginFrame
task to compositor thread
2.) Compositor thread posts beginFrame task to main thread
3.) Compositor thread processes forceBeginFrame task, sets scheduler bits,
signals main thread to continue
4.) Main thread runs beginFrame(NULL), posts beginFrameComplete to compositor thread
5.) Compositor thread processes beginFrameComplete, runs through state machine up
to draw and then goes back to waiting for a reply to the beginFrame it posted in
step (2)
6.) Main thread issues sync readback, finishes compositeAndReadback()
7.) Main thread processes beginFrame message posted in (2), posts reply
8.) Profit!

Note that steps (2) and (3) can happen in any order.


BUG= 158747 


Review URL: https://chromiumcodereview.appspot.com/11362054

TBR=jamesr@chromium.org
Review URL: https://codereview.chromium.org/11419027
------------------------------------------------------------------------
Project Member

Comment 34 by bugdroid1@chromium.org, Nov 16 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=168101

------------------------------------------------------------------------
r168101 | jamesr@chromium.org | 2012-11-16T01:35:39.188910Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/layer_tree_host_unittest.cc?r1=168101&r2=168100&pathrev=168101
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/layer_tree_host.cc?r1=168101&r2=168100&pathrev=168101
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.cc?r1=168101&r2=168100&pathrev=168101
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.h?r1=168101&r2=168100&pathrev=168101
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler.cc?r1=168101&r2=168100&pathrev=168101
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine_unittest.cc?r1=168101&r2=168100&pathrev=168101
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine.cc?r1=168101&r2=168100&pathrev=168101
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine.h?r1=168101&r2=168100&pathrev=168101

Revert 168095 - Merge 167537 - Use message passing for BeginFrameAndCommitState and clean up forced commit logic

This passes the BeginFrameAndCommitState struct along with messages instead of
relying on (racy) shared memory and slightly simplifies the forced commit logic.
This logic is for compositeAndReadback() and is trying to go through the normal
commit flow while the main thread is blocked. The normal commit flow is this:

1.) Main thread posts task to compositor thread requesting commit
2.) Compositor thread receives notification, after some delay posts task to main
to run the beginFrame logic
3.) Main thread runs the beginFrameTask, posts a blocking task to compositor
thread
  4.) With the main thread is blocked, compositor thread runs the beginFrameComplete
  task and signals the main thread to get on with life
5.) Compositor thread waits for an opportunity to draw (vsync) then goes back to
the initial state.


compositeAndReadback() requires that we run through the whole flow up through 5
without yielding the main thread at all. However, while it's doing this there
may very well be another beginFrame task pending on the main thread's message
queue. When this task does eventually run it needs to have a valid BFAC state
and the reply has to be expected on the compositor side.

This patch always passes the BFAC state on the beginFrame task and uses a null
BFAC state for compositeAndReadback-forced beginFrame()s. This means when we do
a compositeAndReadback we won't "see" state that has updated only on the
compositor thread, but that's fine since we'll apply that state once we process
the beginFrame task later on and BFAC state addition is additive. The tricky
bit from the compositor side is keeping track of whether there's a pending
beginFrame message after going through the forced commit flow. To keep track
of this I've added SchedulerStateMachine::m_expectImmediateBeginFrame that
keeps the scheduler in a state that expects a beginFrameComplete after
finishing the readback.

Typical flow:

1.) Main thread enters compositeAndReadback(), posts a blocking forceBeginFrame
task to compositor thread
2.) Compositor thread posts beginFrame task to main thread
3.) Compositor thread processes forceBeginFrame task, sets scheduler bits,
signals main thread to continue
4.) Main thread runs beginFrame(NULL), posts beginFrameComplete to compositor thread
5.) Compositor thread processes beginFrameComplete, runs through state machine up
to draw and then goes back to waiting for a reply to the beginFrame it posted in
step (2)
6.) Main thread issues sync readback, finishes compositeAndReadback()
7.) Main thread processes beginFrame message posted in (2), posts reply
8.) Profit!

Note that steps (2) and (3) can happen in any order.


BUG= 158747 


Review URL: https://chromiumcodereview.appspot.com/11362054

TBR=jamesr@chromium.org
Review URL: https://codereview.chromium.org/11419027

TBR=jamesr@chromium.org
Review URL: https://codereview.chromium.org/11348083
------------------------------------------------------------------------
Project Member

Comment 35 by bugdroid1@chromium.org, Nov 16 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=168254

------------------------------------------------------------------------
r168254 | jamesr@chromium.org | 2012-11-16T19:09:09.220109Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/layer_tree_host_unittest.cc?r1=168254&r2=168253&pathrev=168254
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/layer_tree_host.cc?r1=168254&r2=168253&pathrev=168254
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.cc?r1=168254&r2=168253&pathrev=168254
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.h?r1=168254&r2=168253&pathrev=168254
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler.cc?r1=168254&r2=168253&pathrev=168254
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine_unittest.cc?r1=168254&r2=168253&pathrev=168254
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine.cc?r1=168254&r2=168253&pathrev=168254
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine.h?r1=168254&r2=168253&pathrev=168254

Merge 167537 - Use message passing for BeginFrameAndCommitState and clean up forced commit logic

This passes the BeginFrameAndCommitState struct along with messages instead of
relying on (racy) shared memory and slightly simplifies the forced commit logic.
This logic is for compositeAndReadback() and is trying to go through the normal
commit flow while the main thread is blocked. The normal commit flow is this:

1.) Main thread posts task to compositor thread requesting commit
2.) Compositor thread receives notification, after some delay posts task to main
to run the beginFrame logic
3.) Main thread runs the beginFrameTask, posts a blocking task to compositor
thread
  4.) With the main thread is blocked, compositor thread runs the beginFrameComplete
  task and signals the main thread to get on with life
5.) Compositor thread waits for an opportunity to draw (vsync) then goes back to
the initial state.


compositeAndReadback() requires that we run through the whole flow up through 5
without yielding the main thread at all. However, while it's doing this there
may very well be another beginFrame task pending on the main thread's message
queue. When this task does eventually run it needs to have a valid BFAC state
and the reply has to be expected on the compositor side.

This patch always passes the BFAC state on the beginFrame task and uses a null
BFAC state for compositeAndReadback-forced beginFrame()s. This means when we do
a compositeAndReadback we won't "see" state that has updated only on the
compositor thread, but that's fine since we'll apply that state once we process
the beginFrame task later on and BFAC state addition is additive. The tricky
bit from the compositor side is keeping track of whether there's a pending
beginFrame message after going through the forced commit flow. To keep track
of this I've added SchedulerStateMachine::m_expectImmediateBeginFrame that
keeps the scheduler in a state that expects a beginFrameComplete after
finishing the readback.

Typical flow:

1.) Main thread enters compositeAndReadback(), posts a blocking forceBeginFrame
task to compositor thread
2.) Compositor thread posts beginFrame task to main thread
3.) Compositor thread processes forceBeginFrame task, sets scheduler bits,
signals main thread to continue
4.) Main thread runs beginFrame(NULL), posts beginFrameComplete to compositor thread
5.) Compositor thread processes beginFrameComplete, runs through state machine up
to draw and then goes back to waiting for a reply to the beginFrame it posted in
step (2)
6.) Main thread issues sync readback, finishes compositeAndReadback()
7.) Main thread processes beginFrame message posted in (2), posts reply
8.) Profit!

Note that steps (2) and (3) can happen in any order.


BUG= 158747 


Review URL: https://chromiumcodereview.appspot.com/11362054

TBR=jamesr@chromium.org
Review URL: https://codereview.chromium.org/11416043
------------------------------------------------------------------------
Project Member

Comment 36 by bugdroid1@chromium.org, Nov 28 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=170057

------------------------------------------------------------------------
r170057 | dharani@google.com | 2012-11-28T21:28:04.051965Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/layer_tree_host_unittest.cc?r1=170057&r2=170056&pathrev=170057
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/layer_tree_host.cc?r1=170057&r2=170056&pathrev=170057
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.cc?r1=170057&r2=170056&pathrev=170057
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.h?r1=170057&r2=170056&pathrev=170057
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler.cc?r1=170057&r2=170056&pathrev=170057
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine_unittest.cc?r1=170057&r2=170056&pathrev=170057
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine.cc?r1=170057&r2=170056&pathrev=170057
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine.h?r1=170057&r2=170056&pathrev=170057

Revert 168254 - Merge 167537 - Use message passing for BeginFrameAndCommitState and clean up forced commit logic

This passes the BeginFrameAndCommitState struct along with messages instead of
relying on (racy) shared memory and slightly simplifies the forced commit logic.
This logic is for compositeAndReadback() and is trying to go through the normal
commit flow while the main thread is blocked. The normal commit flow is this:

1.) Main thread posts task to compositor thread requesting commit
2.) Compositor thread receives notification, after some delay posts task to main
to run the beginFrame logic
3.) Main thread runs the beginFrameTask, posts a blocking task to compositor
thread
  4.) With the main thread is blocked, compositor thread runs the beginFrameComplete
  task and signals the main thread to get on with life
5.) Compositor thread waits for an opportunity to draw (vsync) then goes back to
the initial state.


compositeAndReadback() requires that we run through the whole flow up through 5
without yielding the main thread at all. However, while it's doing this there
may very well be another beginFrame task pending on the main thread's message
queue. When this task does eventually run it needs to have a valid BFAC state
and the reply has to be expected on the compositor side.

This patch always passes the BFAC state on the beginFrame task and uses a null
BFAC state for compositeAndReadback-forced beginFrame()s. This means when we do
a compositeAndReadback we won't "see" state that has updated only on the
compositor thread, but that's fine since we'll apply that state once we process
the beginFrame task later on and BFAC state addition is additive. The tricky
bit from the compositor side is keeping track of whether there's a pending
beginFrame message after going through the forced commit flow. To keep track
of this I've added SchedulerStateMachine::m_expectImmediateBeginFrame that
keeps the scheduler in a state that expects a beginFrameComplete after
finishing the readback.

Typical flow:

1.) Main thread enters compositeAndReadback(), posts a blocking forceBeginFrame
task to compositor thread
2.) Compositor thread posts beginFrame task to main thread
3.) Compositor thread processes forceBeginFrame task, sets scheduler bits,
signals main thread to continue
4.) Main thread runs beginFrame(NULL), posts beginFrameComplete to compositor thread
5.) Compositor thread processes beginFrameComplete, runs through state machine up
to draw and then goes back to waiting for a reply to the beginFrame it posted in
step (2)
6.) Main thread issues sync readback, finishes compositeAndReadback()
7.) Main thread processes beginFrame message posted in (2), posts reply
8.) Profit!

Note that steps (2) and (3) can happen in any order.


BUG= 158747 


Review URL: https://chromiumcodereview.appspot.com/11362054

TBR=jamesr@chromium.org
Review URL: https://codereview.chromium.org/11416043

TBR=jamesr@chromium.org
Review URL: https://codereview.chromium.org/11411240
------------------------------------------------------------------------
Project Member

Comment 37 by bugdroid1@chromium.org, Nov 30 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=170403

------------------------------------------------------------------------
r170403 | ccameron@chromium.org | 2012-11-30T05:21:14.356904Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/prioritized_resource.h?r1=170403&r2=170402&pathrev=170403
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/prioritized_resource_manager.cc?r1=170403&r2=170402&pathrev=170403
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/prioritized_resource_manager.h?r1=170403&r2=170402&pathrev=170403
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/thread_proxy.cc?r1=170403&r2=170402&pathrev=170403
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/single_thread_proxy.cc?r1=170403&r2=170402&pathrev=170403
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/prioritized_resource_unittest.cc?r1=170403&r2=170402&pathrev=170403
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/thread_proxy.h?r1=170403&r2=170402&pathrev=170403
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/prioritized_resource.cc?r1=170403&r2=170402&pathrev=170403
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/tiled_layer_unittest.cc?r1=170403&r2=170402&pathrev=170403

Use a mutex to deal with concurrent access to the m_evictedBackings
list, instead of passing the list of pointers around.

In ThreadProxy::beginFrame, we unlink all evicted textures, so that
the call to updateLayers will know to re-paint the evicted layers.

In ThreadProxy::scheduleActionCommit, we determine if we can draw now
by checking to see if there exist any linked evicted textures. If there
are none, we can draw what we are in the process of committing.

This assumes that once we get to m_layerTreeHost->willBeginFrame()
in ThreadProxy::beginFrame, the next frame to be commited by
ThreadProxy::scheduleActionCommit be at or after the frame that is
being produced by that call to ThreadProxy::beginFrame.

BUG= 158747 


Review URL: https://chromiumcodereview.appspot.com/11411251
------------------------------------------------------------------------
We went from holding the #1 and #2 chromecrash slots in 25.0.1338.0:
    cc::PrioritizedResourceManager::unlinkEvictedBackings 38.40%
    cc::PrioritizedResource::Backing::~Backing 9.25%
To not appearing on the list in 25.0.1342.0.

I need to do a full checkout of the M24 tree to merge this (the change is big enough that I don't trust it to work without a local build). I should have it in by the end of the day.
Issue 161676 has been merged into this issue.
Project Member

Comment 40 by bugdroid1@chromium.org, Dec 1 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=170635

------------------------------------------------------------------------
r170635 | ccameron@chromium.org | 2012-12-01T02:31:35.281595Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.cc?r1=170635&r2=170634&pathrev=170635
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/single_thread_proxy.cc?r1=170635&r2=170634&pathrev=170635
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.h?r1=170635&r2=170634&pathrev=170635
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/tiled_layer_unittest.cc?r1=170635&r2=170634&pathrev=170635
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/prioritized_texture_manager.cc?r1=170635&r2=170634&pathrev=170635
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/prioritized_texture_manager.h?r1=170635&r2=170634&pathrev=170635
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/prioritized_texture_unittest.cc?r1=170635&r2=170634&pathrev=170635

Merge 170403 - Use a lock to deal with concurrent access to the m_evictedBackings

Use a mutex to deal with concurrent access to the m_evictedBackings
list, instead of passing the list of pointers around.

In ThreadProxy::beginFrame, we unlink all evicted textures, so that
the call to updateLayers will know to re-paint the evicted layers.

In ThreadProxy::scheduleActionCommit, we determine if we can draw now
by checking to see if there exist any linked evicted textures. If there
are none, we can draw what we are in the process of committing.

This assumes that once we get to m_layerTreeHost->willBeginFrame()
in ThreadProxy::beginFrame, the next frame to be commited by
ThreadProxy::scheduleActionCommit be at or after the frame that is
being produced by that call to ThreadProxy::beginFrame.

BUG= 158747 
TBR=ccameron@chromium.org

Review URL: https://codereview.chromium.org/11280268
------------------------------------------------------------------------
@dharani can you revert r170057 now on the 1312 branch now?  Otherwise we'll still get crashes from the race fixed earlier.
I'm doing the drover for 170057 now (there's a conflict, nothing serious though).
Project Member

Comment 43 by bugdroid1@chromium.org, Dec 1 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=170638

------------------------------------------------------------------------
r170638 | ccameron@chromium.org | 2012-12-01T02:56:20.155059Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/layer_tree_host_unittest.cc?r1=170638&r2=170637&pathrev=170638
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/layer_tree_host.cc?r1=170638&r2=170637&pathrev=170638
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.cc?r1=170638&r2=170637&pathrev=170638
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/thread_proxy.h?r1=170638&r2=170637&pathrev=170638
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler.cc?r1=170638&r2=170637&pathrev=170638
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine_unittest.cc?r1=170638&r2=170637&pathrev=170638
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine.cc?r1=170638&r2=170637&pathrev=170638
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/cc/scheduler_state_machine.h?r1=170638&r2=170637&pathrev=170638

Revert 170057
> Revert 168254 - Merge 167537 - Use message passing for BeginFrameAndCommitState and clean up forced commit logic
> 
> This passes the BeginFrameAndCommitState struct along with messages instead of
> relying on (racy) shared memory and slightly simplifies the forced commit logic.
> This logic is for compositeAndReadback() and is trying to go through the normal
> commit flow while the main thread is blocked. The normal commit flow is this:
> 
> 1.) Main thread posts task to compositor thread requesting commit
> 2.) Compositor thread receives notification, after some delay posts task to main
> to run the beginFrame logic
> 3.) Main thread runs the beginFrameTask, posts a blocking task to compositor
> thread
>   4.) With the main thread is blocked, compositor thread runs the beginFrameComplete
>   task and signals the main thread to get on with life
> 5.) Compositor thread waits for an opportunity to draw (vsync) then goes back to
> the initial state.
> 
> 
> compositeAndReadback() requires that we run through the whole flow up through 5
> without yielding the main thread at all. However, while it's doing this there
> may very well be another beginFrame task pending on the main thread's message
> queue. When this task does eventually run it needs to have a valid BFAC state
> and the reply has to be expected on the compositor side.
> 
> This patch always passes the BFAC state on the beginFrame task and uses a null
> BFAC state for compositeAndReadback-forced beginFrame()s. This means when we do
> a compositeAndReadback we won't "see" state that has updated only on the
> compositor thread, but that's fine since we'll apply that state once we process
> the beginFrame task later on and BFAC state addition is additive. The tricky
> bit from the compositor side is keeping track of whether there's a pending
> beginFrame message after going through the forced commit flow. To keep track
> of this I've added SchedulerStateMachine::m_expectImmediateBeginFrame that
> keeps the scheduler in a state that expects a beginFrameComplete after
> finishing the readback.
> 
> Typical flow:
> 
> 1.) Main thread enters compositeAndReadback(), posts a blocking forceBeginFrame
> task to compositor thread
> 2.) Compositor thread posts beginFrame task to main thread
> 3.) Compositor thread processes forceBeginFrame task, sets scheduler bits,
> signals main thread to continue
> 4.) Main thread runs beginFrame(NULL), posts beginFrameComplete to compositor thread
> 5.) Compositor thread processes beginFrameComplete, runs through state machine up
> to draw and then goes back to waiting for a reply to the beginFrame it posted in
> step (2)
> 6.) Main thread issues sync readback, finishes compositeAndReadback()
> 7.) Main thread processes beginFrame message posted in (2), posts reply
> 8.) Profit!
> 
> Note that steps (2) and (3) can happen in any order.
> 
> 
> BUG= 158747 
> 
> 
> Review URL: https://chromiumcodereview.appspot.com/11362054
> 
> TBR=jamesr@chromium.org
> Review URL: https://codereview.chromium.org/11416043
> 
> TBR=jamesr@chromium.org
> Review URL: https://codereview.chromium.org/11411240

TBR=dharani@google.com
Review URL: https://codereview.chromium.org/11419273
------------------------------------------------------------------------
Status: Fixed
My fix is in now, and James' fix has been un-reverted. This should be fixed now.
Project Member

Comment 45 by bugdroid1@chromium.org, Dec 3 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=170700

------------------------------------------------------------------------
r170700 | jamesr@chromium.org | 2012-12-03T04:08:14.416151Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/gpu/compositor_util.cc?r1=170700&r2=170699&pathrev=170700
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/compositor_bindings/web_compositor_support_impl.cc?r1=170700&r2=170699&pathrev=170700
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/gpu/gpu_feature_browsertest.cc?r1=170700&r2=170699&pathrev=170700

Revert 165866
> Disable threaded compositing when texture sharing is blacklisted
> 
> When texture_sharing is blacklisted, we can't do readbacks from the
> browser process and thus hit the render process to do readbacks for
> the thumbnailer or the extensions getVisibleTab() API. This path is
> a bit buggy in threaded compositing mode, so disable the thread.
> 
> BUG= 158747 
> 
> 
> Review URL: https://chromiumcodereview.appspot.com/11361077

TBR=jamesr@chromium.org
BUG= 163046 
Review URL: https://codereview.chromium.org/11280272
------------------------------------------------------------------------
Project Member

Comment 46 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Mstone-24 M-24

Sign in to add a comment