Temporarily remove DCHECK that triggers during hardware teardown.

Temporarily remove DCHECK that triggers during hardware teardown.

The logic that this DCHECK checks is correct, but during hardware
teardown the tracking information is also reset, even though the sync
RequestDrawGL has not set up the tracking info that it is resetting,
leaving the tracker in an unexpected state.

Ideally we should be differentiating the DrawGL calls required for
hardware teardown from the normal kModeProcess DrawGL calls, but that
seems to require touching framework code, making it difficult to fix.

BUG=451955
Committed: https://crrev.com/e020a9ef53a39b4010a68925933f4f1de6b83ca6
Cr-Commit-Position: refs/heads/master@{#315002}

[Tobias Sargeant, 2015-02-05 13:59:23]

tobiasjs@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2015-02-05 16:28:30]

Woh, this got more complicated since 2 hours and 38 minutes ago..

[boliu, 2015-02-05 17:10:22]

Seems like a lot of refactoring. Aside from checking IsVisible instead of
IsInsideHardwareRelease, what actually changed?

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:72: // DCHECK(pending_ui_ ==
state || pending_non_ui_ == state);
Just remove it.

We usually don't comment things out in hopes of uncommenting them in the future,
because it usually never happens and it's just cruft left in the source.

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:297: case
AwDrawGLInfo::kModeProcessNoContext: {
Indent here is wrong.

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi...

In general, don't really have to worry about style issues and just learn to use
clang-format: https://code.google.com/p/chromium/wiki/ClangFormat

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:304: // kModeProcessNoContext
should never happen because we tear down hardware
Move this comment up.

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:322: if
(browser_view_renderer_->IsVisible()) {
Not thread safe. BVR is a UI thread only object, and DrawGL is called on render
thread.

What's wrong with using !IsInsideHardwareRelease() here?

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:348: InsideHardwareReleaseReset
auto_inside_hardware_release_reset(this);
This used to wrap BVR::ReleaseHardware too.

[boliu, 2015-02-05 17:10:46]

cc hush

[Tobias Sargeant, 2015-02-05 17:29:39]

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:297: case
AwDrawGLInfo::kModeProcessNoContext: {
On 2015/02/05 17:10:22, boliu wrote:
> Indent here is wrong.
> 
> See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi...
> 
> In general, don't really have to worry about style issues and just learn to
use
> clang-format: https://code.google.com/p/chromium/wiki/ClangFormat

Done.

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:304: // kModeProcessNoContext
should never happen because we tear down hardware
On 2015/02/05 17:10:22, boliu wrote:
> Move this comment up.

Done.

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:322: if
(browser_view_renderer_->IsVisible()) {
The problem is that it seems possible to get draw calls that happen after the
teardown, when IsInsideHardwareRelease() is no longer true, but before the
shared_renderer_state destructor is called.

[boliu, 2015-02-05 17:36:17]

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/902813003/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:322: if
(browser_view_renderer_->IsVisible()) {
On 2015/02/05 17:29:39, Tobias Sargeant wrote:
> The problem is that it seems possible to get draw calls that happen after the
> teardown, when IsInsideHardwareRelease() is no longer true, but before the
> shared_renderer_state destructor is called.

And that call is kModeDraw (not kModeProcess)? The answer matters a lot to where
the actual bug is.

The goal here is DrawGL should not be called after
ReleaseHardwareDrawIfNeededOnUI (at least until the next time this BVR is
hardware initialized again)

[Tobias Sargeant, 2015-02-05 17:40:48]

> And that call is kModeDraw (not kModeProcess)? The answer matters a lot to
where
> the actual bug is.

yes, definitely a kModeDraw call.

[boliu, 2015-02-05 18:05:19]

lgtm

[Tobias Sargeant, 2015-02-06 09:08:44]

The CQ bit was checked by tobiasjs@chromium.org

[commit-bot: I haz the power, 2015-02-06 09:09:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/902813003/80001

[commit-bot: I haz the power, 2015-02-06 12:11:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-06 12:12:09]

Patchset 5 (id:??) landed as
https://crrev.com/e020a9ef53a39b4010a68925933f4f1de6b83ca6
Cr-Commit-Position: refs/heads/master@{#315002}