Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
654623002:
aw: Check compositor pointer before use (Closed)
|
Created:
6 years, 2 months ago by boliu Modified:
6 years, 2 months ago Reviewers:
hush (inactive) CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptionaw: Check compositor pointer before use
BVR::DidDestroyCompositor can be called at any point, which resets
the compositor pointer. Which means each use of the pointer
should be checked or DCHECKed. In particular, DidDestroyCompositor
can happen before tearing down hardware mode.
This can happen when javascript calls window.close, which closes the
RenderView before WebContents.
BUG=424614
Committed: https://crrev.com/105144f946d65b9623509a6475d0cc69ce9240a0
Cr-Commit-Position: refs/heads/master@{#300297}
Patch Set 1 #
Total comments: 3
Patch Set 2 : realz fixz #
Total comments: 7
Patch Set 3 : review #Patch Set 4 : bring back DCHECK #Messages
Total messages: 18 (2 generated)
boliu@chromium.org changed reviewers: + hush@chromium.org
PTAL. I'm running into a weird crash, that in detach, compositor pointer is null. Not sure exactly about the cause yet as it happens ones every few days.
On 2014/10/13 16:09:32, boliu wrote: > PTAL. > > I'm running into a weird crash, that in detach, compositor pointer is null. Not > sure exactly about the cause yet as it happens ones every few days. please hold on a bit... maybe we should discover why DidDestroyCompositor is called before ReleaseHardware.
On 2014/10/13 19:30:50, hush wrote: > On 2014/10/13 16:09:32, boliu wrote: > > PTAL. > > > > I'm running into a weird crash, that in detach, compositor pointer is null. > Not > > sure exactly about the cause yet as it happens ones every few days. > > please hold on a bit... maybe we should discover why DidDestroyCompositor is > called before ReleaseHardware. So SynchronousCompositorOutputSurface is owned by the child compositor, which in theory can choose to destroy it at any point, like say on a context loss. But yeah, I can't imagine how that can happen. All these "theories"...
On 2014/10/13 19:38:29, boliu wrote: > On 2014/10/13 19:30:50, hush wrote: > > On 2014/10/13 16:09:32, boliu wrote: > > > PTAL. > > > > > > I'm running into a weird crash, that in detach, compositor pointer is null. > > Not > > > sure exactly about the cause yet as it happens ones every few days. > > > > please hold on a bit... maybe we should discover why DidDestroyCompositor is > > called before ReleaseHardware. > > So SynchronousCompositorOutputSurface is owned by the child compositor, which in > theory can choose to destroy it at any point, like say on a context loss. But > yeah, I can't imagine how that can happen. > > All these "theories"... let's hold on a bit. Seems like we haven't seen this for a while.
See crbug.com/424614
https://codereview.chromium.org/654623002/diff/1/android_webview/browser/brow... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/654623002/diff/1/android_webview/browser/brow... android_webview/browser/browser_view_renderer.cc:504: hardware_enabled_ = false; oh god that's so freaking wrong
https://codereview.chromium.org/654623002/diff/1/android_webview/browser/brow... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/654623002/diff/1/android_webview/browser/brow... android_webview/browser/browser_view_renderer.cc:504: hardware_enabled_ = false; On 2014/10/17 22:59:26, boliu wrote: > oh god that's so freaking wrong what do you mean? I thought what you did is replacing this line with "CHECK(!hardware_enabled)" and hit the crash
https://codereview.chromium.org/654623002/diff/1/android_webview/browser/brow... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/654623002/diff/1/android_webview/browser/brow... android_webview/browser/browser_view_renderer.cc:504: hardware_enabled_ = false; On 2014/10/17 23:04:15, hush wrote: > On 2014/10/17 22:59:26, boliu wrote: > > oh god that's so freaking wrong > > what do you mean? I thought what you did is replacing this line with > "CHECK(!hardware_enabled)" and hit the crash CHECK was to see it actually happens, not the fix. But this is totally the wrong fix too. I am ashamed..
ok, ps2 should look better
Fixed up the description
https://codereview.chromium.org/654623002/diff/80001/android_webview/browser/... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/654623002/diff/80001/android_webview/browser/... android_webview/browser/browser_view_renderer.cc:116: return; ideally, (memory_policy_ != zero_policy) means compositor != NULL && hardware_enabled_ != NULL. That means this early out is the same as the early out in line 131. So can you add a DCHECK after line 132 instead? https://codereview.chromium.org/654623002/diff/80001/android_webview/browser/... android_webview/browser/browser_view_renderer.cc:251: SynchronousCompositorMemoryPolicy new_policy = CalculateDesiredMemoryPolicy(); what is the policy to add DCHECK(compositor_)? if we need it every time we use it, then add one here... https://codereview.chromium.org/654623002/diff/80001/android_webview/browser/... android_webview/browser/browser_view_renderer.cc:474: compositor_ = NULL; since didDestroyCompositor could be called before ReleaseHardware, there is a period of time when compositor_ == NULL, but hardware_enabled_ is true. What does that mean? I think we should skip onDraw in this period at least.
https://codereview.chromium.org/654623002/diff/80001/android_webview/browser/... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/654623002/diff/80001/android_webview/browser/... android_webview/browser/browser_view_renderer.cc:116: return; On 2014/10/18 01:34:01, hush wrote: > ideally, (memory_policy_ != zero_policy) means compositor != NULL && > hardware_enabled_ != NULL. > That means this early out is the same as the early out in line 131. > > So can you add a DCHECK after line 132 instead? Done. https://codereview.chromium.org/654623002/diff/80001/android_webview/browser/... android_webview/browser/browser_view_renderer.cc:251: SynchronousCompositorMemoryPolicy new_policy = CalculateDesiredMemoryPolicy(); On 2014/10/18 01:34:01, hush wrote: > what is the policy to add DCHECK(compositor_)? > if we need it every time we use it, then add one here... Generally only DCHECK public methods that rely on compositor being present. Private methods we are supposed to check in this class ourselves. https://codereview.chromium.org/654623002/diff/80001/android_webview/browser/... android_webview/browser/browser_view_renderer.cc:440: GlobalTileManager::GetInstance()->Remove(tile_manager_key_); Woo this is wrong. Ned to request 0 policy *before* we remove the key. In the same vein, DidDestroy should only request memory if hardware is enabled. https://codereview.chromium.org/654623002/diff/80001/android_webview/browser/... android_webview/browser/browser_view_renderer.cc:474: compositor_ = NULL; On 2014/10/18 01:34:01, hush wrote: > since didDestroyCompositor could be called before ReleaseHardware, there is a > period of time when compositor_ == NULL, but hardware_enabled_ is true. > What does that mean? > I think we should skip onDraw in this period at least. We do already, both OnDrawHardware and will skip immediately if compositor_ is null.
lgtm
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/654623002/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/105144f946d65b9623509a6475d0cc69ce9240a0 Cr-Commit-Position: refs/heads/master@{#300297} |
