Fix race condition with windowless plugin buffers. The problem, which is already fixed for Mac, is …

Fix race condition with windowless plugin buffers. The problem, which is already fixed for Mac, is that the buffers can be deleted during a paint because of a resize during an NPN_Evaluate call. So keep a local reference.

BUG=139462
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151734

[jam, 2012-08-14 17:09:56]

I spent half a day trying to repro this race condition, but I couldn't (it's
tricky because I tried to NPN_Evaluate javascrip that would reize the plugin,
but even with forcing layout WebKit ignores it till the NPN_Evaluate returns.
it's racy to repro this because i think it happens when there just happesns to
be a pending resize in the ipc queue anwyays..). Abhishek confirms that this
fixes the issue on the bots.

[reed1, 2012-08-14 17:19:04]

lgtm

http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
File content/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
content/plugin/webplugin_proxy.cc:388: skia::PlatformCanvas* saved_canvas =
windowless_canvas();
Do we want to keep this commented line? // SkAutoRef ...

http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
content/plugin/webplugin_proxy.cc:413: black_fill_paint.setARGB(0xFF, 0x00,
0x00, 0x00);
FYI -- canvas->drawColor(0xFF000000); works too

[jam, 2012-08-14 17:29:30]

http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
File content/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
content/plugin/webplugin_proxy.cc:388: skia::PlatformCanvas* saved_canvas =
windowless_canvas();
On 2012/08/14 17:19:04, reed1 wrote:
> Do we want to keep this commented line? // SkAutoRef ...

doh, this is actually the fix. if it didn't repro for Abhishek without it, I'm
really confused! I don't see how the lines below could help anything, since
what's reentrant is the delegate_->Paint() call

[Tom Hudson, 2012-08-14 17:34:55]

http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
File content/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
content/plugin/webplugin_proxy.cc:388: skia::PlatformCanvas* saved_canvas =
windowless_canvas();
On 2012/08/14 17:29:30, John Abd-El-Malek wrote:
> On 2012/08/14 17:19:04, reed1 wrote:
> > Do we want to keep this commented line? // SkAutoRef ...
> 
> doh, this is actually the fix. if it didn't repro for Abhishek without it, I'm
> really confused! I don't see how the lines below could help anything, since
> what's reentrant is the delegate_->Paint() call

It may be the ref isn't necessary.
It used to be we called windowless_canvas() all the time, which would pick up
any changes made by Paint().
Now that you're caching saved_canvas, we've got that data on the stack, instead
of pulling it off of the heap every time we use it. That should be much more
tolerant of reentry.

I suspect this is a standard (mis-)pattern throughout Chrome?

[jam, 2012-08-14 17:40:28]

http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
File content/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
content/plugin/webplugin_proxy.cc:388: skia::PlatformCanvas* saved_canvas =
windowless_canvas();
On 2012/08/14 17:34:55, Tom Hudson wrote:
> On 2012/08/14 17:29:30, John Abd-El-Malek wrote:
> > On 2012/08/14 17:19:04, reed1 wrote:
> > > Do we want to keep this commented line? // SkAutoRef ...
> > 
> > doh, this is actually the fix. if it didn't repro for Abhishek without it,
I'm
> > really confused! I don't see how the lines below could help anything, since
> > what's reentrant is the delegate_->Paint() call
> 
> It may be the ref isn't necessary.
> It used to be we called windowless_canvas() all the time, which would pick up
> any changes made by Paint().
> Now that you're caching saved_canvas, we've got that data on the stack,
instead
> of pulling it off of the heap every time we use it. That should be much more
> tolerant of reentry.

Paint() is the only function in this scope that can change what
windowless_canvas() returns, and the use-after-free happens inside Paint. So I
don't think the caching of the raw pointer helps here.

> 
> I suspect this is a standard (mis-)pattern throughout Chrome?

[aarya, 2012-08-14 18:12:18]

On 2012/08/14 17:34:55, Tom Hudson wrote:
>
http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
> File content/plugin/webplugin_proxy.cc (right):
> 
>
http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
> content/plugin/webplugin_proxy.cc:388: skia::PlatformCanvas* saved_canvas =
> windowless_canvas();
> On 2012/08/14 17:29:30, John Abd-El-Malek wrote:
> > On 2012/08/14 17:19:04, reed1 wrote:
> > > Do we want to keep this commented line? // SkAutoRef ...
> > 
> > doh, this is actually the fix. if it didn't repro for Abhishek without it,
I'm
> > really confused! I don't see how the lines below could help anything, since
> > what's reentrant is the delegate_->Paint() call
> 
> It may be the ref isn't necessary.
> It used to be we called windowless_canvas() all the time, which would pick up
> any changes made by Paint().
> Now that you're caching saved_canvas, we've got that data on the stack,
instead
> of pulling it off of the heap every time we use it. That should be much more
> tolerant of reentry.
> 
> I suspect this is a standard (mis-)pattern throughout Chrome?

i reverified everything again like blow away patch, recompiled, verified
multiple times, then applied your fix, then checked multiple times again. This
bug wasn't flaky at all on my local, it was 100% reliably reproducing.

[jam, 2012-08-14 20:43:08]

http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
File content/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/10855141/diff/5002/content/plugin/webplugin_pr...
content/plugin/webplugin_proxy.cc:388: skia::PlatformCanvas* saved_canvas =
windowless_canvas();
On 2012/08/14 17:40:28, John Abd-El-Malek wrote:
> On 2012/08/14 17:34:55, Tom Hudson wrote:
> > On 2012/08/14 17:29:30, John Abd-El-Malek wrote:
> > > On 2012/08/14 17:19:04, reed1 wrote:
> > > > Do we want to keep this commented line? // SkAutoRef ...
> > > 
> > > doh, this is actually the fix. if it didn't repro for Abhishek without it,
> I'm
> > > really confused! I don't see how the lines below could help anything,
since
> > > what's reentrant is the delegate_->Paint() call
> > 
> > It may be the ref isn't necessary.
> > It used to be we called windowless_canvas() all the time, which would pick
up
> > any changes made by Paint().
> > Now that you're caching saved_canvas, we've got that data on the stack,
> instead
> > of pulling it off of the heap every time we use it. That should be much more
> > tolerant of reentry.
> 
> Paint() is the only function in this scope that can change what
> windowless_canvas() returns, and the use-after-free happens inside Paint. So I
> don't think the caching of the raw pointer helps here.
> 
> > 
> > I suspect this is a standard (mis-)pattern throughout Chrome?
> 

ok, turns out this section was a red herring! If I revert the changes to this
function, it still works without a use-after-free (which makes sense, since the
changes were effectively doing nothing). It turns out the real fix is switching
from scoped_ptr<skia::PlatformCanvas> to SkRefPtr<skia::PlatformCanvas>. I'm not
familiar with skia, so I don't know what other parts are using this canvas, but
it seems like the generic chrome bug of putting a ref counted class in a
scoped_ptr.

[Tom Hudson, 2012-08-14 21:01:19]

Feeding "scoped_ptr<skia::PlatformCanvas> package:^chrome$ file:^src/" to code
search gives 13 hits, 4 of which are other header files for classes which are
holding our canvases. Can we get a review of all of those? [*] Do we even have a
sense of what other refcounted Skia objects are being exposed to Chrome that we
should be auditing?

Interestingly enough, SkRefPtr was experimental code that is not well supported
by the Skia team and may eventually be deprecated (qv
http://code.google.com/p/skia/issues/detail?id=361). Is there a Chrome
reference-holding-pointer class we could be using?

[*] I'll help; however, I'm moving to London in a week, so I'm trying to wrap up
projects rather than start new ones.

[Tom Hudson, 2012-08-14 21:04:23]

Mike points out that we do support SkAutoTUnref<>. We're not likely to accept a
change that spreads SkRefPtr<>.

[jam, 2012-08-14 21:08:44]

btw it looks like with this change, the SkCanvas leaks and is never destructed,
so that's why the use-after-free appeared fix. Investigating more.

On 2012/08/14 21:01:19, Tom Hudson wrote:
> Feeding "scoped_ptr<skia::PlatformCanvas> package:^chrome$ file:^src/" to code
> search gives 13 hits, 4 of which are other header files for classes which are
> holding our canvases. Can we get a review of all of those? [*] Do we even have
a
> sense of what other refcounted Skia objects are being exposed to Chrome that
we
> should be auditing?

These are good questions. I think it's best to file bugs against people who
wrote each of the code if you think there's an issue.

> 
> Interestingly enough, SkRefPtr was experimental code that is not well
supported
> by the Skia team and may eventually be deprecated (qv
> http://code.google.com/p/skia/issues/detail?id=361). Is there a Chrome
> reference-holding-pointer class we could be using?
> 
> [*] I'll help; however, I'm moving to London in a week, so I'm trying to wrap
up
> projects rather than start new ones.

[jam, 2012-08-14 21:10:31]

On 2012/08/14 21:04:23, Tom Hudson wrote:
> Mike points out that we do support SkAutoTUnref<>. We're not likely to accept
a
> change that spreads SkRefPtr<>.

one thing that's not clear to me is how is SkAutoRef different from SkRefPtr?

[reed1, 2012-08-14 21:17:23]

On 2012/08/14 21:10:31, John Abd-El-Malek wrote:
> On 2012/08/14 21:04:23, Tom Hudson wrote:
> > Mike points out that we do support SkAutoTUnref<>. We're not likely to
accept
> a
> > change that spreads SkRefPtr<>.
> 
> one thing that's not clear to me is how is SkAutoRef different from SkRefPtr?

SkRefPtr has (tricky) copy/assignment semantics. SkAutoUnref has none. SkRefPtr
does not have an 'adopt' pattern; SkAutoUnref *only* has an adopt pattern (hence
the name Un-ref).

[jam, 2012-08-14 21:29:58]

please see the latest patchset (4). you can diff it with patchset 2. 2 had a
leak as I mentioned earlier, and that's why the "SkAutoRef local_ref" wasn't
(very confusingly) needed. Fixing the leak, I can confirm that adding the local
reference fixes the crash.

[Tom Hudson, 2012-08-14 22:18:00]

From the fact that you named patchset 5 the "real fix", I assume that you found
the cached pointer & the SkAutoRef actually were necessary?

Thanks for the suggestion in #11; I'll do that in the morning.

[jam, 2012-08-14 22:23:21]

On 2012/08/14 22:18:00, Tom Hudson wrote:
> From the fact that you named patchset 5 the "real fix", I assume that you
found
> the cached pointer & the SkAutoRef actually were necessary?

right, I had verified that without the SkAutoRef a use-after-free occurred.

> 
> Thanks for the suggestion in #11; I'll do that in the morning.


in good news, I found out why my regression test was hitting a dead-end, and I'm
working on it now.

[jam, 2012-08-14 22:44:04]

added regression test

[Tom Hudson, 2012-08-15 17:04:00]

LGTM

http://codereview.chromium.org/10855141/diff/8/webkit/plugins/npapi/test/plug...
File webkit/plugins/npapi/test/plugin_windowless_test.cc (right):

http://codereview.chromium.org/10855141/diff/8/webkit/plugins/npapi/test/plug...
webkit/plugins/npapi/test/plugin_windowless_test.cc:128: // _it's_ painting (as
opposed to the plugin).
Nit: I'd like to understand this, but I don't follow any of the comments in the
new code block here - perhaps I just don't have enough chromium test framework
background.

[jam, 2012-08-15 17:28:10]

Ironically enough, even though the new test is passing on Linux trybots (where
it uses xvfb), it fails when running locally. I've finally found the problem
(need to increment the transport dib as well), but I'm still trying to find a
solution. It's strange that the test case url does work without this, perhaps
the flash plugin does something that makes this possible but not our test
plugin.

http://codereview.chromium.org/10855141/diff/8/webkit/plugins/npapi/test/plug...
File webkit/plugins/npapi/test/plugin_windowless_test.cc (right):

http://codereview.chromium.org/10855141/diff/8/webkit/plugins/npapi/test/plug...
webkit/plugins/npapi/test/plugin_windowless_test.cc:128: // _it's_ painting (as
opposed to the plugin).
On 2012/08/15 17:04:00, Tom Hudson wrote:
> Nit: I'd like to understand this, but I don't follow any of the comments in
the
> new code block here - perhaps I just don't have enough chromium test framework
> background.

my bad, I was lazy and wrote a comment that I myself would forget in a few weeks
:)

I've beefed this up, let me know if this is more understandable.

[Tom Hudson, 2012-08-15 17:59:04]

http://codereview.chromium.org/10855141/diff/8/webkit/plugins/npapi/test/plug...
File webkit/plugins/npapi/test/plugin_windowless_test.cc (right):

http://codereview.chromium.org/10855141/diff/8/webkit/plugins/npapi/test/plug...
webkit/plugins/npapi/test/plugin_windowless_test.cc:128: // _it's_ painting (as
opposed to the plugin).
So there are two paints, and both are asynchronous?
The first is managed by WebKit, the second by the plugin?

The first sentence implies to me that the first paint isn't asynchronous, but
the second sentence immediately says it is.

[jam, 2012-08-15 18:22:22]

http://codereview.chromium.org/10855141/diff/8/webkit/plugins/npapi/test/plug...
File webkit/plugins/npapi/test/plugin_windowless_test.cc (right):

http://codereview.chromium.org/10855141/diff/8/webkit/plugins/npapi/test/plug...
webkit/plugins/npapi/test/plugin_windowless_test.cc:128: // _it's_ painting (as
opposed to the plugin).
On 2012/08/15 17:59:04, Tom Hudson wrote:
> So there are two paints, and both are asynchronous?
> The first is managed by WebKit, the second by the plugin?
> 
> The first sentence implies to me that the first paint isn't asynchronous, but
> the second sentence immediately says it is.

that was a typo, check out the update

[Tom Hudson, 2012-08-15 18:33:34]

Yep, I was getting the old version. New is much better. Thanks.

[jam, 2012-08-15 19:09:07]

+piman for the Linux bits (specifically, diff of patchset 8 and 9)

[piman, 2012-08-15 19:15:50]

lgtm