Prevent browser thumbnailer from trying to read from frontbuffer (surface texture) when none exists.

Prevent browser thumbnailer from trying to read from frontbuffer (surface texture) when none exists.

BUG=123817, 129410
TEST=Manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139089

[mmocny, 2012-05-03 19:15:35]

This patch adds another condition to "ShouldUpdateThumbnail()" to check if we
even have a frontbuffer surface texture to read from.

@mazda and I discussed possible races with discarding frontbuffer during the
thumbnailer scaling.  However, the way the drop fb patch is shaping up, a
thumbnail will not be started after the *request* to drop fb is recieved, and
the request to drop fb will not be turned into an actual drop until after
synchronizing with the ui context.

I will run some tests by changing the logic of the thumbnailer to always
thumbnail unless we don't have a fb, rapid tab switch, and make sure they are
never corrupt.

[mmocny, 2012-05-03 19:46:07]

https://chromiumcodereview.appspot.com/10349013/diff/6001/content/public/brow...
File content/public/browser/render_widget_host_view.h (right):

https://chromiumcodereview.appspot.com/10349013/diff/6001/content/public/brow...
content/public/browser/render_widget_host_view.h:86: virtual bool HasSurface()
const = 0;
This name is not nearly descriptive enough, but I needed to start with
something.  Suggestions welcome.

[mazda, 2012-05-03 19:57:30]

http://codereview.chromium.org/10349013/diff/1/chrome/browser/tab_contents/th...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/10349013/diff/1/chrome/browser/tab_contents/th...
chrome/browser/tab_contents/thumbnail_generator.cc:584:
Profile::FromBrowserContext(web_contents->GetBrowserContext());
NULL check for profile is needed.

http://codereview.chromium.org/10349013/diff/1/chrome/browser/tab_contents/th...
chrome/browser/tab_contents/thumbnail_generator.cc:585: history::TopSites*
top_sites = profile->GetTopSites();
Could you move this just before |top_sites| is used and add NULL check for it.

http://codereview.chromium.org/10349013/diff/1/chrome/browser/tab_contents/th...
chrome/browser/tab_contents/thumbnail_generator.cc:587: if (!rwhv->HasSurface())
In order to use this function here, it needs to handle the following cases.
1. Backing store is available.
2. Accelerated surface is available.

I added comments to HasSurface, but I have not tested them.

http://codereview.chromium.org/10349013/diff/1/chrome/browser/tab_contents/th...
chrome/browser/tab_contents/thumbnail_generator.cc:589: if (!profile ||
!top_sites)
Please delete this with the changes above.

http://codereview.chromium.org/10349013/diff/1/chrome/browser/tab_contents/th...
File chrome/browser/tab_contents/thumbnail_generator.h (right):

http://codereview.chromium.org/10349013/diff/1/chrome/browser/tab_contents/th...
chrome/browser/tab_contents/thumbnail_generator.h:144: // Returns true if we
should update the thumbnail of the given URL.
URL -> web contents

http://codereview.chromium.org/10349013/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/10349013/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_view_gtk.cc:738: return false;
return !!host_->GetBackingStore(false);

http://codereview.chromium.org/10349013/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/10349013/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_view_mac.mm:518: return false;
return compositing_iosurface_.get() && compositing_iosurface_->HasIOSurface();

http://codereview.chromium.org/10349013/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/10349013/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_view_win.cc:667: return false;
return !!accelerated_surface_.get();

[mmocny, 2012-05-03 20:05:41]

Some of the comments you made for the ShouldUpdateThumbnail function were
reverted back to the way they were originally in order to be testable.

Thank you very much for giving me implementations for the other platforms!  I
also have not tested but this goes a long way forward.

[mazda, 2012-05-03 20:19:33]

http://codereview.chromium.org/10349013/diff/6001/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

http://codereview.chromium.org/10349013/diff/6001/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:86: virtual bool HasSurface()
const = 0;
On 2012/05/03 19:46:08, mmocny wrote:
> This name is not nearly descriptive enough, but I needed to start with
> something.  Suggestions welcome.

This is also confusing because there is similar function,
RenderWidgetHostViewPort::HasAcceleratedSurface.

HasCopyableSurface or something?

http://codereview.chromium.org/10349013/diff/13003/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/10349013/diff/13003/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_mac.mm:518: return
compositing_iosurface_.get() && compositing_iosurface_->HasIOSurface();
Sorry, I omitted the check of the backing store in the previous comment.

return !!render_widget_host_->GetBackingStore(false) ||
(compositing_iosurface_.get() && compositing_iosurface_->HasIOSurface());

http://codereview.chromium.org/10349013/diff/13003/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/10349013/diff/13003/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:667: return
!!accelerated_surface_.get();
Same here.

return !!render_widget_host_->GetBackingStore(false) ||
!!accelerated_surface_.get();

[mmocny, 2012-05-03 20:51:28]

On 2012/05/03 20:19:33, mazda wrote:
>
http://codereview.chromium.org/10349013/diff/6001/content/public/browser/rend...
> File content/public/browser/render_widget_host_view.h (right):
> 
>
http://codereview.chromium.org/10349013/diff/6001/content/public/browser/rend...
> content/public/browser/render_widget_host_view.h:86: virtual bool HasSurface()
> const = 0;
> On 2012/05/03 19:46:08, mmocny wrote:
> > This name is not nearly descriptive enough, but I needed to start with
> > something.  Suggestions welcome.
> 
> This is also confusing because there is similar function,
> RenderWidgetHostViewPort::HasAcceleratedSurface.
> 
> HasCopyableSurface or something?
I think this name would suffer the same problem: "Has" is ambiguous on
"currently exists" and "supports the feature of".
Is something like "IsSurfaceCurrentlyCopyable" too wordy?

> 
>
http://codereview.chromium.org/10349013/diff/13003/content/browser/renderer_h...
> File content/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
http://codereview.chromium.org/10349013/diff/13003/content/browser/renderer_h...
> content/browser/renderer_host/render_widget_host_view_mac.mm:518: return
> compositing_iosurface_.get() && compositing_iosurface_->HasIOSurface();
> Sorry, I omitted the check of the backing store in the previous comment.
> 
> return !!render_widget_host_->GetBackingStore(false) ||
> (compositing_iosurface_.get() && compositing_iosurface_->HasIOSurface());
> 
>
http://codereview.chromium.org/10349013/diff/13003/content/browser/renderer_h...
> File content/browser/renderer_host/render_widget_host_view_win.cc (right):
> 
>
http://codereview.chromium.org/10349013/diff/13003/content/browser/renderer_h...
> content/browser/renderer_host/render_widget_host_view_win.cc:667: return
> !!accelerated_surface_.get();
> Same here.
> 
> return !!render_widget_host_->GetBackingStore(false) ||
> !!accelerated_surface_.get();

[mazda, 2012-05-04 22:20:31]

http://codereview.chromium.org/10349013/diff/6001/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

http://codereview.chromium.org/10349013/diff/6001/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:86: virtual bool HasSurface()
const = 0;
On 2012/05/03 20:19:33, mazda wrote:
> On 2012/05/03 19:46:08, mmocny wrote:
> > This name is not nearly descriptive enough, but I needed to start with
> > something.  Suggestions welcome.
> 
> This is also confusing because there is similar function,
> RenderWidgetHostViewPort::HasAcceleratedSurface.
> 
> HasCopyableSurface or something?

IsSurfaceAvailableForCopy in your latest patch looks good.

http://codereview.chromium.org/10349013/diff/16001/chrome/browser/tab_content...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/10349013/diff/16001/chrome/browser/tab_content...
chrome/browser/tab_contents/thumbnail_generator.cc:508: if
(!ShouldUpdateThumbnail(profile, top_sites, url, surface_available))
How about just doing as follows?

if (!surface_available || !ShouldUpdateThumbnail(profile, top_sites, url))

[mmocny, 2012-05-07 13:28:22]

http://codereview.chromium.org/10349013/diff/16001/chrome/browser/tab_content...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/10349013/diff/16001/chrome/browser/tab_content...
chrome/browser/tab_contents/thumbnail_generator.cc:508: if
(!ShouldUpdateThumbnail(profile, top_sites, url, surface_available))
Yes, it is certainly a bit silly to move the bool around just to test.

I wasn't sure if the ShouldUpdateThumbnail could ever be used in other
locations, and so moving it into there would make the condition explicit.

But, I can move it out.

On 2012/05/04 22:20:32, mazda wrote:
> How about just doing as follows?
> 
> if (!surface_available || !ShouldUpdateThumbnail(profile, top_sites, url))
>

[mazda, 2012-05-07 16:37:19]

http://codereview.chromium.org/10349013/diff/16001/chrome/browser/tab_content...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/10349013/diff/16001/chrome/browser/tab_content...
chrome/browser/tab_contents/thumbnail_generator.cc:508: if
(!ShouldUpdateThumbnail(profile, top_sites, url, surface_available))
On 2012/05/07 13:28:23, mmocny wrote:
> Yes, it is certainly a bit silly to move the bool around just to test.
> 
> I wasn't sure if the ShouldUpdateThumbnail could ever be used in other
> locations, and so moving it into there would make the condition explicit.
> 
> But, I can move it out.
> 
> On 2012/05/04 22:20:32, mazda wrote:
> > How about just doing as follows?
> > 
> > if (!surface_available || !ShouldUpdateThumbnail(profile, top_sites, url))
> > 
> 

We do not plan to use ShouldUpdateThumbnail in other location for now, so I
think it's OK to move it out.

[mmocny, 2012-05-23 14:07:26]

mazda: Can I confirm that the win/mac accelerated CopyFromCompositingSurface
path is not asynchronous?  It appears not, but I want to make sure that
thumbnailing is certain to be safe (in the face of fb discarding) as long as the
browser had a handle to the surface at the start.  I think we are safe as it
stands.

For aura, the read is really async and having a handle at the beginning is not
sufficient once the fb may be discarded. I plan to add an Observer for
ThumbnailGenerator::AsyncUpdateThumbnailFinish and use it to delay fb discard
whenever we are in the middle of thumbnailing.  I will add that in a subsequent
patch, since we don't discard fb on aura yet.

[mmocny, 2012-05-23 15:08:52]

Reviewers may you please take a look?
brettw: You seem to have been reviewing the thumbnailer code already.
sky: for browser code.

Thanks.

[sky, 2012-05-23 15:28:43]

chrome LGTM

[mazda, 2012-05-23 16:04:06]

lgtm

> mazda: Can I confirm that the win/mac accelerated CopyFromCompositingSurface
path is not asynchronous? 
Right. CopyFromCompositingSurface path is synchronous on Win and Mac for now,
and it should be safe.

[piman, 2012-05-23 17:38:54]

On Wed, May 23, 2012 at 7:07 AM, <mmocny@chromium.org> wrote:

> mazda: Can I confirm that the win/mac accelerated
> CopyFromCompositingSurface
> path is not asynchronous?  It appears not, but I want to make sure that
> thumbnailing is certain to be safe (in the face of fb discarding) as long
> as the
> browser had a handle to the surface at the start.  I think we are safe as
> it
> stands.
>
> For aura, the read is really async and having a handle at the beginning is
> not
> sufficient once the fb may be discarded. I plan to add an Observer for
> ThumbnailGenerator::**AsyncUpdateThumbnailFinish and use it to delay fb
> discard
> whenever we are in the middle of thumbnailing.  I will add that in a
> subsequent
> patch, since we don't discard fb on aura yet.
>

Suggestion: handle this in RWHVA directly without the need for an observer.
It gets passed the callback that currently it passes through, but it can
install its own to track when the front buffer is "busy" by the thumbnailer.


>
http://codereview.chromium.**org/10349013/<http://codereview.chromium.org/103...
>

[mmocny, 2012-05-23 18:05:12]

On 2012/05/23 17:38:54, piman wrote:
> On Wed, May 23, 2012 at 7:07 AM, <mailto:mmocny@chromium.org> wrote:
> 
> > mazda: Can I confirm that the win/mac accelerated
> > CopyFromCompositingSurface
> > path is not asynchronous?  It appears not, but I want to make sure that
> > thumbnailing is certain to be safe (in the face of fb discarding) as long
> > as the
> > browser had a handle to the surface at the start.  I think we are safe as
> > it
> > stands.
> >
> > For aura, the read is really async and having a handle at the beginning is
> > not
> > sufficient once the fb may be discarded. I plan to add an Observer for
> > ThumbnailGenerator::**AsyncUpdateThumbnailFinish and use it to delay fb
> > discard
> > whenever we are in the middle of thumbnailing.  I will add that in a
> > subsequent
> > patch, since we don't discard fb on aura yet.
> >
> 
> Suggestion: handle this in RWHVA directly without the need for an observer.
> It gets passed the callback that currently it passes through, but it can
> install its own to track when the front buffer is "busy" by the thumbnailer.
> 
Excellent suggestion!  This will still be added in another patch
(http://codereview.chromium.org/10052018/ likely).

> 
> >
>
http://codereview.chromium.**org/10349013/%3Chttp://codereview.chromium.org/1...>
> >

[mmocny, 2012-05-23 19:00:34]

Just found a segfault in tip that is fixed by this patch
(https://code.google.com/p/chromium/issues/detail?id=129410).

We may not have a surface to copy from not just after we discard it, but also
before the first frame is generated.  If you switch away from a tab opened in
the background, we may segfault assuming the surface exists.

I added a DCHECK to help guard against the issue, but with this patch, we
instead bailout early.

[mazda, 2012-05-23 20:06:23]

http://codereview.chromium.org/10349013/diff/34001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/10349013/diff/34001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:456:
image_transport_clients_.end());
When this condition holds, |container| is initialized with NULL, so this
function returns as failure before calling GLHelper::CopyTextureTo, right?
I think just returning as failure is appropriate instead of causing CHECK error.

[mmocny, 2012-05-23 20:28:30]

http://codereview.chromium.org/10349013/diff/34001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/10349013/diff/34001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:456:
image_transport_clients_.end());
You are right.  The issue was that we expect the existence of a client in the
map to imply !NULL, and so the use of map op[] was causing issues elsewhere. 
Changed this to exit early as you suggest instead.

On 2012/05/23 20:06:23, mazda wrote:
> When this condition holds, |container| is initialized with NULL, so this
> function returns as failure before calling GLHelper::CopyTextureTo, right?
> I think just returning as failure is appropriate instead of causing CHECK
error.

[brettw, 2012-05-25 17:34:00]

thumbnail_generator LGTM

[commit-bot: I haz the power, 2012-05-25 17:34:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10349013/38001

[commit-bot: I haz the power, 2012-05-25 17:35:17]

Failed to apply patch for
content/browser/renderer_host/render_widget_host_view_aura.cc:
While running patch -p1 --forward --force;
patching file content/browser/renderer_host/render_widget_host_view_aura.cc
Hunk #1 succeeded at 333 (offset 1 line).
Hunk #2 FAILED at 452.
1 out of 2 hunks FAILED -- saving rejects to file
content/browser/renderer_host/render_widget_host_view_aura.cc.rej

[commit-bot: I haz the power, 2012-05-25 17:52:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10349013/42002

[commit-bot: I haz the power, 2012-05-25 19:09:37]

Change committed as 139089