Fix NTP thumbnail generation

Fix NTP thumbnail generation

This change delays browser compositor view suspention until pending
thumbnail generation has completed.

This solution uses polling to check if it's OK to suspend the
compositor, which is not great. Also, there's a potential race condition
between the user foregrounding the tab and the pending suspension.
Ideally the thumnail generation operation would signal the compositor
that it's OK to proceed with its suspension, and the compositor would only
suspend itself if it has not been reactivated. I'm just not sure of the
best way to approach that.

BUG=530707
Committed: https://crrev.com/d77a3639f04f83a5c5cf47d88dbac2f0d4345ea5
Cr-Commit-Position: refs/heads/master@{#359361}

[shrike, 2015-09-16 22:36:14]

shrike@chromium.org changed reviewers:
+ ccameron@chromium.org

[shrike, 2015-09-16 22:38:41]

Hello ccameron,

This cl takes a stab at fixing the thumbnail generation problem by delaying the
compositor's suspension. It's not a great solution for a couple reasons (please
see the cl description). I'm posting this to at least get the conversation
started on this bug/change. Please let me know your thoughts on strategies for
improve this.

[ccameron, 2015-09-16 22:57:30]

I just realized that we already have 2 mechanisms that can solve this.

1. We have basically the same issue when we're doing tab capture (e.g,
Chromcast). The way that we
- call WebContents::IncrementCapturerCount when we start the capture
- call WebContents::DecrementCapturerCount when we start the capture
Between these two calls, the RWHView never "hide" itself (like -- if you
background the tab or occlude the window, the RWHView don't get the message).

So, if we have access to the WebContents (or can get access in some way) in the
thumbnailer code, then we can just do "increment" when we request the thumbnail
and "decrement" when it completes (or is cancelled or given up on).

2. (well, I didn't realize this, Dana told me about it) -- the Android
thumbnailer. I don't know where this code lives, but one way to fix this would
be to go that route (in principle, we could move all platforms to that
mechanism, but that may be a bit of work).

[shrike, 2015-09-17 00:24:20]

Patchset #2 (id:20001) has been deleted

[shrike, 2015-09-17 00:32:02]

On 2015/09/16 22:57:30, ccameron wrote:
> I just realized that we already have 2 mechanisms that can solve this.
> 
> 1. We have basically the same issue when we're doing tab capture (e.g,
> Chromcast). The way that we
> - call WebContents::IncrementCapturerCount when we start the capture
> - call WebContents::DecrementCapturerCount when we start the capture
> Between these two calls, the RWHView never "hide" itself (like -- if you
> background the tab or occlude the window, the RWHView don't get the message).

Great! That did it!

In order to make it work I have to keep the web_contents around in the
ThumbnailingContext. Right now the ThumbnailingContext saves the GURL as an
instance variable. In theory it could avoid that and just return it straight
from the web_contents. I didn't make that change because I wasn't sure about the
GURL's lifetime within the web_contents (I guess since we're relying on the
web_contents to be what it was, we will know that the GURL won't change). Let me
know if you think I should make that change or leave it as is.

PTAL.

[danakj, 2015-09-17 17:54:14]

https://codereview.chromium.org/1348833003/diff/40001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1348833003/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:78:
context->web_contents->DecrementCapturerCount();
Do you not want to do this if the result wasn't SUCCESS?

https://codereview.chromium.org/1348833003/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:201: gfx::Size empty_size;
you could just pass the gfx::Size() directly if the context being added by the
name is just that it's empty.

Normally for a temp var like this, I'd want it to refer to the name of the
parameter the function is taking (ie showing the purpose of the variable rather
than what value is inside it).

[shrike, 2015-09-28 21:24:11]

I'm working on a browser test to make sure this fix does not get undone in the
future. I have the test navigating the browser to a URL and then away from it.
Once I navigate away I check to see if there's a pending CopyRequest on the root
layer. This works, except I think there is a race condition (I think there's no
guarantee the CopyRequest will execute before I query the layer). Is there a way
to get the compositor to perform a cycle of work and block until that cycle has
completed. Or is the best bet to insert a short delay in the test?

https://codereview.chromium.org/1348833003/diff/40001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1348833003/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:78:
context->web_contents->DecrementCapturerCount();
On 2015/09/17 17:54:14, danakj wrote:
> Do you not want to do this if the result wasn't SUCCESS?

Thank you for catching that. Done.

https://codereview.chromium.org/1348833003/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:201: gfx::Size empty_size;
On 2015/09/17 17:54:14, danakj wrote:
> you could just pass the gfx::Size() directly if the context being added by the
> name is just that it's empty.
> 
> Normally for a temp var like this, I'd want it to refer to the name of the
> parameter the function is taking (ie showing the purpose of the variable
rather
> than what value is inside it).

Yeah, you're right about passing it directly. And thank you for the naming tip -
will try to keep that in mind for the future. Done.

[shrike, 2015-10-06 21:30:13]

Hello ccameron,

I have been working on a test for this change in order to guard against a
regression, however I discovered that unit tests are a no-go because the render
widget host code lives in content but the thumbnail code does not. In my browser
test I tried a RunUntilIdle on the message loop but that sometimes causes the
test to fail. Without doing that my test succeeds and fails when appropriate,
but I'm not sure if there might be a race condition that could cause it to fail
on the test bots.

Bottom line: I don't know that coming up with a test is going to work out, but I
would like to get this change at least landed (and not have it reverted because
the test is failing). I guess others have encountered this thumbnail bug and are
waiting for this fix, and we may even want to cherry-pick it to a previous
release.

PTAL.

[ccameron, 2015-10-06 21:36:05]

On 2015/10/06 21:30:13, shrike wrote:
> Hello ccameron,
> 
> I have been working on a test for this change in order to guard against a
> regression, however I discovered that unit tests are a no-go because the
render
> widget host code lives in content but the thumbnail code does not. In my
browser
> test I tried a RunUntilIdle on the message loop but that sometimes causes the
> test to fail. Without doing that my test succeeds and fails when appropriate,
> but I'm not sure if there might be a race condition that could cause it to
fail
> on the test bots.
> 
> Bottom line: I don't know that coming up with a test is going to work out, but
I
> would like to get this change at least landed (and not have it reverted
because
> the test is failing). I guess others have encountered this thumbnail bug and
are
> waiting for this fix, and we may even want to cherry-pick it to a previous
> release.
> 
> PTAL.

Sound good -- could you upload the patchset with danakj's comments address?

[shrike, 2015-10-06 21:50:30]

On 2015/10/06 21:36:05, ccameron wrote:
> On 2015/10/06 21:30:13, shrike wrote:
> > Hello ccameron,
> > 
> > I have been working on a test for this change in order to guard against a
> > regression, however I discovered that unit tests are a no-go because the
> render
> > widget host code lives in content but the thumbnail code does not. In my
> browser
> > test I tried a RunUntilIdle on the message loop but that sometimes causes
the
> > test to fail. Without doing that my test succeeds and fails when
appropriate,
> > but I'm not sure if there might be a race condition that could cause it to
> fail
> > on the test bots.
> > 
> > Bottom line: I don't know that coming up with a test is going to work out,
but
> I
> > would like to get this change at least landed (and not have it reverted
> because
> > the test is failing). I guess others have encountered this thumbnail bug and
> are
> > waiting for this fix, and we may even want to cherry-pick it to a previous
> > release.
> > 
> > PTAL.
> 
> Sound good -- could you upload the patchset with danakj's comments address?

Whoops - I didn't realize I hadn't done that. Unfortunately I'm away from the
machine that has those changes for about an hour - I will upload them when I get
back there.

[shrike, 2015-10-06 23:18:16]

PTAL

[ccameron, 2015-10-07 00:49:10]

lgtm!

[shrike, 2015-10-07 16:22:37]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2015-10-07 16:24:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1348833003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1348833003/60001

[commit-bot: I haz the power, 2015-10-07 16:39:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-07 16:39:33]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[shrike, 2015-10-07 16:53:00]

shrike@chromium.org changed reviewers:
+ thestig@chromium.org

[shrike, 2015-10-07 16:55:43]

thestig for changes in chrome/browser/thumbnails (seems for these files I have
to fall all the way back to chrome/OWNERS?)

PTAL

[Lei Zhang, 2015-10-07 17:48:47]

thestig@chromium.org changed reviewers:
+ motek@chromium.org

[Lei Zhang, 2015-10-07 17:55:20]

https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:209:
AsyncProcessThumbnail(web_contents, context, algorithm);
Assuming having |web_contents| in |context| is safe, then we no longer need to
pass |web_contents| here.

https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnailing_context.h (right):

https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnailing_context.h:48: content::WebContents*
web_contents;
I added motek because git blame says he wrote this entire file. I'm not sure
what the lifetime of ThumbnailingContext is, so I don't know if it's safe to
store a raw WebContents* here.

[shrike, 2015-10-07 18:06:35]

thestig - changes made, PTAL, but I know we still need to get the OK from motek
on web_contents.

https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:209:
AsyncProcessThumbnail(web_contents, context, algorithm);
On 2015/10/07 17:55:20, Lei Zhang wrote:
> Assuming having |web_contents| in |context| is safe, then we no longer need to
> pass |web_contents| here.

Done.

https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnailing_context.h (right):

https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnailing_context.h:48: content::WebContents*
web_contents;
On 2015/10/07 17:55:20, Lei Zhang wrote:
> I added motek because git blame says he wrote this entire file. I'm not sure
> what the lifetime of ThumbnailingContext is, so I don't know if it's safe to
> store a raw WebContents* here.

Sounds good - I don't know the answer either.

[motek., 2015-10-07 20:59:04]

On 2015/10/07 18:06:35, shrike wrote:
> thestig - changes made, PTAL, but I know we still need to get the OK from
motek
> on web_contents.
> 
>
https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
> File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
> chrome/browser/thumbnails/thumbnail_tab_helper.cc:209:
> AsyncProcessThumbnail(web_contents, context, algorithm);
> On 2015/10/07 17:55:20, Lei Zhang wrote:
> > Assuming having |web_contents| in |context| is safe, then we no longer need
to
> > pass |web_contents| here.
> 
> Done.
> 
>
https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
> File chrome/browser/thumbnails/thumbnailing_context.h (right):
> 
>
https://codereview.chromium.org/1348833003/diff/60001/chrome/browser/thumbnai...
> chrome/browser/thumbnails/thumbnailing_context.h:48: content::WebContents*
> web_contents;
> On 2015/10/07 17:55:20, Lei Zhang wrote:
> > I added motek because git blame says he wrote this entire file. I'm not sure
> > what the lifetime of ThumbnailingContext is, so I don't know if it's safe to
> > store a raw WebContents* here.
> 
> Sounds good - I don't know the answer either.

That has been a while, hard to recall details. The intended lifetime is the
duration of ProcessCapturedBitmap, which itself is called on
CopyFromBackingStore and can take an arbitrary time (although in practice it
doesn't take much). It is hard to say if what you try is safe. I believed it
wasn't. This whole thing was created in a refactoring and I think that
ThumbnailingContext was created partly to avoid passing web_contents around when
I couldn't guarantee much about when the whole thing would finish. So, instead
whatever I needed was extracted up-front.

[Lei Zhang, 2015-10-07 21:28:45]

On 2015/10/07 20:59:04, motek. wrote:
> That has been a while, hard to recall details. The intended lifetime is the
> duration of ProcessCapturedBitmap, which itself is called on
> CopyFromBackingStore and can take an arbitrary time (although in practice it
> doesn't take much). It is hard to say if what you try is safe. I believed it
> wasn't. This whole thing was created in a refactoring and I think that
> ThumbnailingContext was created partly to avoid passing web_contents around
when
> I couldn't guarantee much about when the whole thing would finish. So, instead
> whatever I needed was extracted up-front.

Right, except we have to deal with a WebContents here, and this is my first
reading of the code. I'll need some reassurances that the WebContents will be
alive in ProcessCapturedBitmap(), or we need to have some other way of checking
that the WebContents is alive / checking for WebContents destruction.

[ccameron, 2015-10-07 22:06:25]

On 2015/10/07 21:28:45, Lei Zhang wrote:
> On 2015/10/07 20:59:04, motek. wrote:
> > That has been a while, hard to recall details. The intended lifetime is the
> > duration of ProcessCapturedBitmap, which itself is called on
> > CopyFromBackingStore and can take an arbitrary time (although in practice it
> > doesn't take much). It is hard to say if what you try is safe. I believed it
> > wasn't. This whole thing was created in a refactoring and I think that
> > ThumbnailingContext was created partly to avoid passing web_contents around
> when
> > I couldn't guarantee much about when the whole thing would finish. So,
instead
> > whatever I needed was extracted up-front.
> 
> Right, except we have to deal with a WebContents here, and this is my first
> reading of the code. I'll need some reassurances that the WebContents will be
> alive in ProcessCapturedBitmap(), or we need to have some other way of
checking
> that the WebContents is alive / checking for WebContents destruction.

We could register for NOTIFICATION_WEB_CONTENTS_DESTROYED on the WebContents in
ThumbnailingContext -- when it goes away, we can reset it. (or a weak pointer,
but for some reason WebContents seems resistant to those).

[shrike, 2015-10-26 23:20:46]

I have changed the thumbnailing context to derive from WebContentsObserver, so
that its web_contents reference gets zeroed out when the contents is destroyed.
That means the thumbnail still won't be generated if the web contents goes away
before the generator runs but it seems like that will happen rarely (the
thumbnail gets generated pretty soon after the request gets posted to the
layer).

thestig@ - PTAL

[Lei Zhang, 2015-10-26 23:31:35]

SGTM, can we make ThumbnailingContext a class with accessors? I hope there's not
too many call sites to update.

https://codereview.chromium.org/1348833003/diff/100001/chrome/browser/thumbna...
File chrome/browser/thumbnails/thumbnailing_context.h (right):

https://codereview.chromium.org/1348833003/diff/100001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnailing_context.h:38: struct ThumbnailingContext
This is looking a bit more complicated now. When in doubt, make it a class.

https://codereview.chromium.org/1348833003/diff/100001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnailing_context.h:57: ThumbnailingContext();
Maybe just DISALLOW_IMPLICIT_CONSTRUCTORS?

[shrike, 2015-11-02 21:48:27]

Hello thestig@,

PTAL

[Lei Zhang, 2015-11-02 22:41:31]

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:75:
context->web_contents()->DecrementCapturerCount();
FYI, this is fine, but ideally we'd have some scoped object so we:

a) don't forget
b) don't accidentally double decrement

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
File chrome/browser/thumbnails/thumbnailing_context.h (right):

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnailing_context.h:59: void
set_requested_copy_size(gfx::Size requested_size);
|requested_size| should be passed by const-ref. |result| above can be pass by
value because ClipResult is an enum which is really an int.

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
File chrome/browser/thumbnails/thumbnailing_context_unittest.cc (right):

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnailing_context_unittest.cc:49:
EXPECT_TRUE(context->score().boring_score == boring_score);
EXPECT_EQ(expected, actual);

Ditto all around.

https://codereview.chromium.org/1348833003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1348833003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:955: // occur in
this specific order.
This sentence sounds funny.

[shrike, 2015-11-03 18:36:23]

thestig@ - PTAL

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:75:
context->web_contents()->DecrementCapturerCount();
On 2015/11/02 22:41:30, Lei Zhang wrote:
> FYI, this is fine, but ideally we'd have some scoped object so we:
> 
> a) don't forget
> b) don't accidentally double decrement

I agree. But the decrement occurs in a separate callback, so it seems like we
would want to add the decrement to an object that gets destroyed when the
callback executes. Unfortunately it doesn't seem like there's a natural way to
construct that (i.e. the problem changes from remembering to decrement to
remembering to destroy the object that runs the decrement).

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
File chrome/browser/thumbnails/thumbnailing_context.h (right):

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnailing_context.h:59: void
set_requested_copy_size(gfx::Size requested_size);
On 2015/11/02 22:41:30, Lei Zhang wrote:
> |requested_size| should be passed by const-ref. |result| above can be pass by
> value because ClipResult is an enum which is really an int.

Got it. I'm not used to working with consts (coming from ObjC).

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
File chrome/browser/thumbnails/thumbnailing_context_unittest.cc (right):

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnailing_context_unittest.cc:49:
EXPECT_TRUE(context->score().boring_score == boring_score);
On 2015/11/02 22:41:30, Lei Zhang wrote:
> EXPECT_EQ(expected, actual);
> 
> Ditto all around.

Acknowledged.

https://codereview.chromium.org/1348833003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1348833003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:955: // occur in
this specific order.
On 2015/11/02 22:41:30, Lei Zhang wrote:
> This sentence sounds funny.

Sorry about that (big typo).

[Lei Zhang, 2015-11-03 18:59:25]

lgtm with another round of nits.

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
File chrome/browser/thumbnails/thumbnailing_context_unittest.cc (right):

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnailing_context_unittest.cc:49:
EXPECT_TRUE(context->score().boring_score == boring_score);
On 2015/11/03 18:36:23, shrike wrote:
> On 2015/11/02 22:41:30, Lei Zhang wrote:
> > EXPECT_EQ(expected, actual);
> > 
> > Ditto all around.
> 
> Acknowledged.

You've written EXPECT_EQ(actual, expected). In this case, the test mostly
behaves the same way, but gtest will print funny looking error messages on
failure. e.g.

#define PI 3.14
EXPECT_EQ(pi_var, 3.14);

will print something like the following if |pi_var| is not 3.14:

Test PiIsDelicious failed on line 5:
Expected: 42
Actual: 3.14

https://codereview.chromium.org/1348833003/diff/140001/chrome/browser/thumbna...
File chrome/browser/thumbnails/thumbnailing_context.cc (right):

https://codereview.chromium.org/1348833003/diff/140001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnailing_context.cc:51: requested_size) {
nit: not a good spot for a line break.

void ThumbnailingContext::set_requested_copy_size(
    const gfx::Size& requested_size) {

[shrike, 2015-11-03 19:10:49]

Hello thestig@ - PTAL

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
File chrome/browser/thumbnails/thumbnailing_context_unittest.cc (right):

https://codereview.chromium.org/1348833003/diff/120001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnailing_context_unittest.cc:49:
EXPECT_TRUE(context->score().boring_score == boring_score);
On 2015/11/03 18:59:24, Lei Zhang wrote:
> On 2015/11/03 18:36:23, shrike wrote:
> > On 2015/11/02 22:41:30, Lei Zhang wrote:
> > > EXPECT_EQ(expected, actual);
> > > 
> > > Ditto all around.
> > 
> > Acknowledged.
> 
> You've written EXPECT_EQ(actual, expected). In this case, the test mostly
> behaves the same way, but gtest will print funny looking error messages on
> failure. e.g.
> 
> #define PI 3.14
> EXPECT_EQ(pi_var, 3.14);
> 
> will print something like the following if |pi_var| is not 3.14:
> 
> Test PiIsDelicious failed on line 5:
> Expected: 42
> Actual: 3.14

Thank you for catching that.

https://codereview.chromium.org/1348833003/diff/140001/chrome/browser/thumbna...
File chrome/browser/thumbnails/thumbnailing_context.cc (right):

https://codereview.chromium.org/1348833003/diff/140001/chrome/browser/thumbna...
chrome/browser/thumbnails/thumbnailing_context.cc:51: requested_size) {
On 2015/11/03 18:59:25, Lei Zhang wrote:
> nit: not a good spot for a line break.
> 
> void ThumbnailingContext::set_requested_copy_size(
>     const gfx::Size& requested_size) {

Acknowledged.

[shrike, 2015-11-04 22:38:08]

The CQ bit was checked by shrike@chromium.org

[shrike, 2015-11-04 22:38:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1348833003/#ps160001
(title: "Fix more nits.")

[commit-bot: I haz the power, 2015-11-04 22:39:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1348833003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1348833003/160001

[commit-bot: I haz the power, 2015-11-04 22:55:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-04 22:55:01]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[shrike, 2015-11-10 23:13:27]

ccameron - I have browser tests failing because
DelegatedFrameHost::SwapDelegatedFrame() calls
AddOnCommitCallbackAndDisableLocks() but |compositor_| is nullptr. Near as I can
tell, my change to the thumbnail generator so that it watches for the
web_contents() going away, plsu the call to DecrementCapturerCount(), can cause
the compositor to disappear but the code/tests still expect it to be around.

Please take a look at this failing test. I could work around it by making sure
at some point that |compositor_| is not null but I don't know if that would be
masking a condition that never should happen. Please advise.

http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_10.6...

[ccameron, 2015-11-10 23:47:07]

On 2015/11/10 23:13:27, shrike wrote:
> ccameron - I have browser tests failing because
> DelegatedFrameHost::SwapDelegatedFrame() calls
> AddOnCommitCallbackAndDisableLocks() but |compositor_| is nullptr. Near as I
can
> tell, my change to the thumbnail generator so that it watches for the
> web_contents() going away, plsu the call to DecrementCapturerCount(), can
cause
> the compositor to disappear but the code/tests still expect it to be around.
> 
> Please take a look at this failing test. I could work around it by making sure
> at some point that |compositor_| is not null but I don't know if that would be
> masking a condition that never should happen. Please advise.
> 
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_10.6...

(trying to reproduce this locally)

[ccameron, 2015-11-11 00:04:39]

On 2015/11/10 23:47:07, ccameron wrote:
> On 2015/11/10 23:13:27, shrike wrote:
> > ccameron - I have browser tests failing because
> > DelegatedFrameHost::SwapDelegatedFrame() calls
> > AddOnCommitCallbackAndDisableLocks() but |compositor_| is nullptr. Near as I
> can
> > tell, my change to the thumbnail generator so that it watches for the
> > web_contents() going away, plsu the call to DecrementCapturerCount(), can
> cause
> > the compositor to disappear but the code/tests still expect it to be around.
> > 
> > Please take a look at this failing test. I could work around it by making
sure
> > at some point that |compositor_| is not null but I don't know if that would
be
> > masking a condition that never should happen. Please advise.
> > 
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_10.6...
> 
> (trying to reproduce this locally)

I can get this to happen with a release build (I need to change that DCHECK to a
CHECK), but only on a retina screen (same machine, external monitor, no crash).
Looking a bit more.

[ccameron, 2015-11-11 01:57:58]

On 2015/11/11 00:04:39, ccameron wrote:
> On 2015/11/10 23:47:07, ccameron wrote:
> > On 2015/11/10 23:13:27, shrike wrote:
> > > ccameron - I have browser tests failing because
> > > DelegatedFrameHost::SwapDelegatedFrame() calls
> > > AddOnCommitCallbackAndDisableLocks() but |compositor_| is nullptr. Near as
I
> > can
> > > tell, my change to the thumbnail generator so that it watches for the
> > > web_contents() going away, plsu the call to DecrementCapturerCount(), can
> > cause
> > > the compositor to disappear but the code/tests still expect it to be
around.
> > > 
> > > Please take a look at this failing test. I could work around it by making
> sure
> > > at some point that |compositor_| is not null but I don't know if that
would
> be
> > > masking a condition that never should happen. Please advise.
> > > 
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_10.6...
> > 
> > (trying to reproduce this locally)
> 
> I can get this to happen with a release build (I need to change that DCHECK to
a
> CHECK), but only on a retina screen (same machine, external monitor, no
crash).
> Looking a bit more.

Sorry, you hit a landmine hanging out in DelegatedFrameHost -- the following
patch will clean it up -- merge it in and you'll be in good shape.

You can see that we were already trying to avoid stuff when compositor_ was null
by the "if (compositor_ && !immediate_ack)" below. I've rename it to
skip_frame_size_mismatch, so we don't try to roll !compositor_ checks into it.

diff --git a/content/browser/compositor/delegated_frame_host.cc
b/content/browser/compositor/delegated_frame_host.cc
index 293f96f..cde0c96 100644
--- a/content/browser/compositor/delegated_frame_host.cc
+++ b/content/browser/compositor/delegated_frame_host.cc
@@ -375,7 +375,7 @@ void DelegatedFrameHost::SwapDelegatedFrame(
     }
     last_output_surface_id_ = output_surface_id;
   }
-  bool immediate_ack = !compositor_;
+  bool skip_frame_size_mismatch = false;
   pending_delegated_ack_count_++;
 
   if (frame_size.IsEmpty()) {
@@ -417,10 +417,10 @@ void DelegatedFrameHost::SwapDelegatedFrame(
 
       gfx::Size desired_size = client_->DelegatedFrameHostDesiredSizeInDIP();
       if (desired_size != frame_size_in_dip && !desired_size.IsEmpty())
-        immediate_ack = true;
+        skip_frame_size_mismatch = true;
 
       cc::SurfaceFactory::DrawCallback ack_callback;
-      if (compositor_ && !immediate_ack) {
+      if (compositor_ && !skip_frame_size_mismatch) {
         ack_callback = base::Bind(&DelegatedFrameHost::SurfaceDrawn,
                                   AsWeakPtr(), output_surface_id);
       }
@@ -456,7 +456,9 @@ void DelegatedFrameHost::SwapDelegatedFrame(
     client_->DelegatedFrameHostGetLayer()->OnDelegatedFrameDamage(
         damage_rect_in_dip);
 
-  if (immediate_ack) {
+  // Note that |compositor_| may be reset by SetShowSurface or
+  // SetShowDelegatedContent above.
+  if (compositor_ && skip_frame_size_mismatch) {
     SendDelegatedFrameAck(output_surface_id);
   } else if (!use_surfaces_) {
     std::vector<ui::LatencyInfo>::const_iterator it;

[ccameron, 2015-11-11 02:00:18]

On 2015/11/11 01:57:58, ccameron wrote:
> On 2015/11/11 00:04:39, ccameron wrote:
> > On 2015/11/10 23:47:07, ccameron wrote:
> > > On 2015/11/10 23:13:27, shrike wrote:
> > > > ccameron - I have browser tests failing because
> > > > DelegatedFrameHost::SwapDelegatedFrame() calls
> > > > AddOnCommitCallbackAndDisableLocks() but |compositor_| is nullptr. Near
as
> I
> > > can
> > > > tell, my change to the thumbnail generator so that it watches for the
> > > > web_contents() going away, plsu the call to DecrementCapturerCount(),
can
> > > cause
> > > > the compositor to disappear but the code/tests still expect it to be
> around.
> > > > 
> > > > Please take a look at this failing test. I could work around it by
making
> > sure
> > > > at some point that |compositor_| is not null but I don't know if that
> would
> > be
> > > > masking a condition that never should happen. Please advise.
> > > > 
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_10.6...
> > > 
> > > (trying to reproduce this locally)
> > 
> > I can get this to happen with a release build (I need to change that DCHECK
to
> a
> > CHECK), but only on a retina screen (same machine, external monitor, no
> crash).
> > Looking a bit more.
> 
> Sorry, you hit a landmine hanging out in DelegatedFrameHost -- the following
> patch will clean it up -- merge it in and you'll be in good shape.
> 
> You can see that we were already trying to avoid stuff when compositor_ was
null
> by the "if (compositor_ && !immediate_ack)" below. I've rename it to
> skip_frame_size_mismatch, so we don't try to roll !compositor_ checks into it.
> 
> diff --git a/content/browser/compositor/delegated_frame_host.cc
> b/content/browser/compositor/delegated_frame_host.cc
> index 293f96f..cde0c96 100644
> --- a/content/browser/compositor/delegated_frame_host.cc
> +++ b/content/browser/compositor/delegated_frame_host.cc
> @@ -375,7 +375,7 @@ void DelegatedFrameHost::SwapDelegatedFrame(
>      }
>      last_output_surface_id_ = output_surface_id;
>    }
> -  bool immediate_ack = !compositor_;
> +  bool skip_frame_size_mismatch = false;
>    pending_delegated_ack_count_++;
>  
>    if (frame_size.IsEmpty()) {
> @@ -417,10 +417,10 @@ void DelegatedFrameHost::SwapDelegatedFrame(
>  
>        gfx::Size desired_size = client_->DelegatedFrameHostDesiredSizeInDIP();
>        if (desired_size != frame_size_in_dip && !desired_size.IsEmpty())
> -        immediate_ack = true;
> +        skip_frame_size_mismatch = true;
>  
>        cc::SurfaceFactory::DrawCallback ack_callback;
> -      if (compositor_ && !immediate_ack) {
> +      if (compositor_ && !skip_frame_size_mismatch) {
>          ack_callback = base::Bind(&DelegatedFrameHost::SurfaceDrawn,
>                                    AsWeakPtr(), output_surface_id);
>        }
> @@ -456,7 +456,9 @@ void DelegatedFrameHost::SwapDelegatedFrame(
>      client_->DelegatedFrameHostGetLayer()->OnDelegatedFrameDamage(
>          damage_rect_in_dip);
>  
> -  if (immediate_ack) {
> +  // Note that |compositor_| may be reset by SetShowSurface or
> +  // SetShowDelegatedContent above.
> +  if (compositor_ && skip_frame_size_mismatch) {
>      SendDelegatedFrameAck(output_surface_id);
>    } else if (!use_surfaces_) {
>      std::vector<ui::LatencyInfo>::const_iterator it;

And... I lied. It should read

-  if (immediate_ack) {
+  // Note that |compositor_| may be reset by SetShowSurface or
+  // SetShowDelegatedContent above.
+  if (!compositor_ || skip_frame_size_mismatch) {
     SendDelegatedFrameAck(output_surface_id);
   } else if (!use_surfaces_) {
     std::vector<ui::LatencyInfo>::const_iterator it;

[shrike, 2015-11-11 21:58:36]

On 2015/11/11 02:00:18, ccameron wrote:
> And... I lied. It should read
> 
> -  if (immediate_ack) {
> +  // Note that |compositor_| may be reset by SetShowSurface or
> +  // SetShowDelegatedContent above.
> +  if (!compositor_ || skip_frame_size_mismatch) {
>      SendDelegatedFrameAck(output_surface_id);
>    } else if (!use_surfaces_) {
>      std::vector<ui::LatencyInfo>::const_iterator it;

Great! Thank you for the patch (I will give it a shot).

[shrike, 2015-11-12 02:32:12]

The CQ bit was checked by shrike@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-12 02:37:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1348833003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1348833003/240001

[commit-bot: I haz the power, 2015-11-12 03:21:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-12 03:21:54]

Dry run: This issue passed the CQ dry run.

[shrike, 2015-11-12 05:27:30]

thestig - would you please take a quick look at thumbnailing_context.cc and
content_based_thumbnailing_algorithm.cc? I made a few changes there since your
approval to get the tests to succeed.

PTAL

[Lei Zhang, 2015-11-12 07:13:17]

lgtm still

https://codereview.chromium.org/1348833003/diff/240001/chrome/browser/thumbna...
File chrome/browser/thumbnails/content_based_thumbnailing_algorithm.cc (right):

https://codereview.chromium.org/1348833003/diff/240001/chrome/browser/thumbna...
chrome/browser/thumbnails/content_based_thumbnailing_algorithm.cc:179: if
(context->web_contents() != nullptr) {
nit: no need for "!= nullptr"

[shrike, 2015-11-12 17:59:23]

https://codereview.chromium.org/1348833003/diff/240001/chrome/browser/thumbna...
File chrome/browser/thumbnails/content_based_thumbnailing_algorithm.cc (right):

https://codereview.chromium.org/1348833003/diff/240001/chrome/browser/thumbna...
chrome/browser/thumbnails/content_based_thumbnailing_algorithm.cc:179: if
(context->web_contents() != nullptr) {
On 2015/11/12 07:13:16, Lei Zhang wrote:
> nit: no need for "!= nullptr"

Acknowledged.

[shrike, 2015-11-12 17:59:29]

The CQ bit was checked by shrike@chromium.org

[shrike, 2015-11-12 17:59:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1348833003/#ps260001
(title: "Fix nit.")

[commit-bot: I haz the power, 2015-11-12 18:00:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1348833003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1348833003/260001

[commit-bot: I haz the power, 2015-11-12 19:41:16]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2015-11-12 20:09:10]

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

[zhaoqin, 2015-11-13 15:55:32]

A revert of this CL (patchset #13 id:260001) has been created in
https://codereview.chromium.org/1443723002/ by zhaoqin@google.com.

The reason for reverting is: possible culprit of an uninit error

BUG=555567
TBR=shrike@chromium.org.