Add an UpdateContext call to WebPluginDelegateImpl on the Mac.

Add an UpdateContext call to WebPluginDelegateImpl on the Mac.

On the Mac, Flash appears to cache the CGContextRef provided in NPP_SetWindow until the next NPP_SetWindow call, so we need to call it sometime before the next plugin paint. This allows us to call NPP_SetWindow before telling the plugin to paint, but not from the Paint function itself (where it could have bad side-effects).

BUG=18894, 18980
TEST=Switch to HQ on a YouTube video; Flash should not crash.

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

[stuartmorgan, 2009-08-11 23:38:50]

I've only implemented this on the Mac side, since it would be painful for me to
test the other platforms to make sure I didn't regress anything, but I would
expect that Win and Linux would probably want to make similar changes for their
windowless plugins in follow-ups.

[jam, 2009-08-12 00:38:47]

I don't understand why there's any IPC here.  PluginMsg_UpdateContext is not
even handled anywhere.

[dglazkov, 2009-08-12 15:36:03]

Since we're not handling PluginMsg_UpdateContext, the next effect of this change
is removing of windowless_needs_set_window_ checks, which is pretty much where I
arrived with http://src.chromium.org/viewvc/chrome?view=rev&revision=22383. This
seemed resolve the crash (which I suspect is the same crash as yours), but
introduced a regression: http://crbug.com/18932 (I'll look at it this
afternoon).

Do you have a reduction for this problem (we should later use it to write
another test in plugin_tests.cc)? How is the flow different from the test in
http://src.chromium.org/viewvc/chrome?view=rev&revision=22383?

[dglazkov, 2009-08-12 17:14:02]

A quick update. My fix didn't introduce the regression. The problem, described
in http://crbug.com/18932 is reproducible on WebKit nightlies, as well.

[stuartmorgan, 2009-08-12 17:43:28]

On 2009/08/12 15:36:03, Dimitri Glazkov wrote:
> Since we're not handling PluginMsg_UpdateContext, the next effect of this
change
> is removing of windowless_needs_set_window_ checks

Not so; WebPluginProxy (which is in the plugin process) calls UpdateContext on
its delegate, which behind the interface is a WebPluginDelegateImpl. No IPC is
involved in the call, so it works fine--and fixes the crash. (The only reason
any of the IPC plumbing is here is because the inheritance forces me to
implement the new method in the proxy, even though it's not called from the
renderer side, so my choices were to add a stub that did nothing or to go ahead
and implement it even though it wasn't being used. The latter seemed less ugly,
but I'd be fine with ripping most of it out and adding a NOTREACHED instead if
people prefer that.)

windowless_needs_set_window_ wasn't doing anything in the Mac implementation; it
was just cruft, so removing it didn't change behavior.

> Do you have a reduction for this problem (we should later use it to write
> another test in plugin_tests.cc)?

Well, yes and no. It's trivial from the browser side (change the size of the
plugin), but the only way to make a self-contained test for it would be to write
a new NPAPI plugin just for testing that has the same
cache-the-context-between-NPP_SetWindow-calls behavior that the current version
of Flash for Mac has.

> How is the flow different from the test in
> http://src.chromium.org/viewvc/chrome?view=rev&revision=22383?

This crash has nothing to do with calling NPP_SetWindow during Paint; it's
caused by *not* calling NPP_SetWindow after a context change, and thus having
the Flash plugin continue to use its apparently internally cached, now-invalid
context from before the context changed. The purpose of the new method is to fix
that bug without adding a new case where NPP_SetWindow is called from Paint.

[stuartmorgan, 2009-08-12 20:22:36]

From discussion with Amanda, the options seem to be:
1) Have a bunch of unnecessary-but-functional IPC code (the current approach)
2) Have just a useless stub in the delegate proxy, with a NOTREACHED()
3) Change WebPluginProxy's delegate_ from WebPluginDelegate* to
WebPluginDelegateImpl*, and don't put the new method in WebPluginDelegate at all
(avoiding the IPC code entirely)

I'm okay with any of them (or a better one that I haven't thought of), I'd just
like to get a consensus and move forward since this bug accounts for at least
one, and almost certain two, of the top-crashers (by an order of magnitude) on
Mac

[Amanda Walker, 2009-08-12 20:25:41]

I vote for #3.

On Wed, Aug 12, 2009 at 4:22 PM, <stuartmorgan@chromium.org> wrote:
> From discussion with Amanda, the options seem to be:
> 1) Have a bunch of unnecessary-but-functional IPC code (the current
> approach)
> 2) Have just a useless stub in the delegate proxy, with a NOTREACHED()
> 3) Change WebPluginProxy's delegate_ from WebPluginDelegate* to
> WebPluginDelegateImpl*, and don't put the new method in
> WebPluginDelegate at all (avoiding the IPC code entirely)
>
> I'm okay with any of them (or a better one that I haven't thought of),
> I'd just like to get a consensus and move forward since this bug
> accounts for at least one, and almost certain two, of the top-crashers
> (by an order of magnitude) on Mac
>
> http://codereview.chromium.org/165344
>



-- 
"Portability is generally the result of advance planning rather than trench
warfare involving #ifdef" -- Henry Spencer (1992)

[jam, 2009-08-12 20:27:57]

looks like our emails crossed :)  yes I prefer that as well, however I'd
like to understand why this change is needed some more per my email.

On Wed, Aug 12, 2009 at 1:25 PM, Amanda Walker <amanda@chromium.org> wrote:

> I vote for #3.
>
> On Wed, Aug 12, 2009 at 4:22 PM, <stuartmorgan@chromium.org> wrote:
> > From discussion with Amanda, the options seem to be:
> > 1) Have a bunch of unnecessary-but-functional IPC code (the current
> > approach)
> > 2) Have just a useless stub in the delegate proxy, with a NOTREACHED()
> > 3) Change WebPluginProxy's delegate_ from WebPluginDelegate* to
> > WebPluginDelegateImpl*, and don't put the new method in
> > WebPluginDelegate at all (avoiding the IPC code entirely)
> >
> > I'm okay with any of them (or a better one that I haven't thought of),
> > I'd just like to get a consensus and move forward since this bug
> > accounts for at least one, and almost certain two, of the top-crashers
> > (by an order of magnitude) on Mac
> >
> > http://codereview.chromium.org/165344
> >
>
>
>
> --
> "Portability is generally the result of advance planning rather than trench
> warfare involving #ifdef" -- Henry Spencer (1992)
>

[stuartmorgan, 2009-08-12 21:12:01]

New version up, using a cast. Now 100% useless-IPC-code free.

[Amanda Walker, 2009-08-12 21:26:24]

Since we were not previously setting cg_context_.context to NULL, that may have
been the cause of some of the crashes.  It might be worth doing just that as its
own change in order to be sure we understand which crashes were for which
reason.

http://codereview.chromium.org/165344/diff/1031/1032
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/165344/diff/1031/1032#newcode514
Line 514: WindowlessContext());
You don't need a new accessor here, just use windowless_context_

http://codereview.chromium.org/165344/diff/1031/1032#newcode643
Line 643: 
Unnecessary (see comment above)

http://codereview.chromium.org/165344/diff/1031/1033
File chrome/plugin/webplugin_proxy.h (right):

http://codereview.chromium.org/165344/diff/1031/1033#newcode145
Line 145: gfx::NativeDrawingContext WindowlessContext();
Do we still need this addition?

[stuartmorgan, 2009-08-12 22:33:13]

> Since we were not previously setting cg_context_.context to NULL, that
> may have been the cause of some of the crashes.  It might be worth doing
> just that as its own change in order to be sure we understand which
> crashes were for which reason.

I tried a clean tree with just the initial NULL-ing, and it didn't help with the
YouTube crash at all.

http://codereview.chromium.org/165344/diff/1031/1032
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/165344/diff/1031/1032#newcode514
Line 514: WindowlessContext());
On 2009/08/12 21:26:24, Amanda Walker wrote:
> You don't need a new accessor here, just use windowless_context_

The accessor is to avoid stuffing a three-way #ifdef with duplicate method calls
into the middle of this function; it's pretty ugly, so I'd like to avoid it.

[jam, 2009-08-12 23:03:38]

http://codereview.chromium.org/165344/diff/1031/1032
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/165344/diff/1031/1032#newcode514
Line 514: WindowlessContext());
On 2009/08/12 22:33:14, stuartmorgan wrote:
> On 2009/08/12 21:26:24, Amanda Walker wrote:
> > You don't need a new accessor here, just use windowless_context_
> 
> The accessor is to avoid stuffing a three-way #ifdef with duplicate method
calls
> into the middle of this function; it's pretty ugly, so I'd like to avoid it.

Given that this is only needed on one platform, unless we see evidence
otherwise, this can be just moved to SetWindowlessBuffer which has a OS_MAC
version already.  Also that function isn't needed on other platforms, so it
should only be added to mac.

[jam, 2009-08-12 23:05:58]

On Wed, Aug 12, 2009 at 3:33 PM, <stuartmorgan@chromium.org> wrote:

> Since we were not previously setting cg_context_.context to NULL, that
>> may have been the cause of some of the crashes.  It might be worth
>>
> doing
>
>> just that as its own change in order to be sure we understand which
>> crashes were for which reason.
>>
>
> I tried a clean tree with just the initial NULL-ing, and it didn't help
> with the YouTube crash at all.


Can you upload that change to another rietveld issue?  I'd like to compare
the two side by side.  Thanks.


> http://codereview.chromium.org/165344
>

[stuartmorgan, 2009-08-12 23:48:20]

http://codereview.chromium.org/165344/diff/1031/1032
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/165344/diff/1031/1032#newcode514
Line 514: WindowlessContext());
On 2009/08/12 23:03:39, John Abd-El-Malek wrote:
> On 2009/08/12 22:33:14, stuartmorgan wrote:
> > On 2009/08/12 21:26:24, Amanda Walker wrote:
> > > You don't need a new accessor here, just use windowless_context_
> > 
> > The accessor is to avoid stuffing a three-way #ifdef with duplicate method
> calls
> > into the middle of this function; it's pretty ugly, so I'd like to avoid it.
> 
> Given that this is only needed on one platform, unless we see evidence
> otherwise, this can be just moved to SetWindowlessBuffer which has a OS_MAC
> version already.  Also that function isn't needed on other platforms, so it
> should only be added to mac.

Done.

[jam, 2009-08-13 02:35:38]

On Wed, Aug 12, 2009 at 4:24 PM, Stuart Morgan <stuartmorgan@chromium.org>wrote:

> On Wed, Aug 12, 2009 at 4:12 PM, John Abd-El-Malek<jam@chromium.org>
> wrote:
> >> Right, we were doing the same... but Flash was crashing with an
> >> invalid context immediately after a resize.
> >
> > ah, well from the 1-4 steps you sent earlier, I was under the impression
> > that painting (4) was what crashed, not resize.
>
> Sorry, poor wording: Flash is crashing on the very first call to paint
> after we resize (which is what causes us to change graphics contexts,
> because we have a newly-sized transport buffer).
>
> > As I said previously and the his change refers to, it's only bad when
> > calling NPP_SetWindow for the first time with a valid context during a
> > paint.  You can call it with a null context, or a second time with a
> valid
> > context.
>
> Okay... I'm not entirely sure how we would fix the bootstrapping
> problem of the first call to SetWindow though. And I'm very leery of
> assuming that Flash won't ever try to use the context other than from
> Paint, and the event handling routines have no way of knowing that the
> context has changed.
>
> > Really?  cg_context_ and window_.window don't look like they get nulled
> out
> > after a paint?
>
> They don't; I apparently don't understand your suggestion. Could you
> pseudo-code it so we are on the same page?
>

I meant setting cg_context_.context = NULL at the end of
WebPluginDelegateImpl::WindowlessPaint, but that didn't work.

I played around this changelist, to see how far it could be reduced while
still making switching back and forth from HD not crash.  I got it down only
to these lines:


*483* // If we somehow get an event before we've set up the plugin window,
bail. 484 if (!cg_context_.context) 485 return false;
The other lines might be for cleanup (i.e. doesn't look like
windowless_needs_set_window_ stuff does anything, since window_.window is
always non-null).  But the "if (!cg_context_.context)" check that's being
added seems impossible to trigger, since this data member is checked in
WebPluginProxy before calling WebPluginDeledgate::Paint.

Also, the version of Chrome that I have running on my mac (3.0.198.1)
doesn't have this crash.  So this seems like a regression?  I don't know
where the mac builds are kept so I can't narrow it down, but it seems
worthwhile to figure out what exactly broke this..

-Stuart
>


http://codereview.chromium.org/165344

[Amanda Walker, 2009-08-13 16:40:30]

On Wed, Aug 12, 2009 at 10:35 PM, John Abd-El-Malek<jam@chromium.org> wrote:
> Also, the version of Chrome that I have running on my mac (3.0.198.1)
> doesn't have this crash.  So this seems like a regression?  I don't know
> where the mac builds are kept so I can't narrow it down, but it seems
> worthwhile to figure out what exactly broke this..

Turning on plugins broke it.  3.0.198.1 does have it.  I have gotten
this crash in every plugin-enabled build we've pushed out.

Stuart's description of catching Flash in gdb red-handed with a stale
CGContextRef is convincing to me--if the struct is up to date, but
Flash is calling CG APIs with the old value, it must ipso facto be
caching it somewhere.  It is not valid to conclude that since the
windows version of flash doesn't do so, that the Mac version must not
either.  In fact, Flash tries to deduce which browser it's running in
and adjusts its behavior accordingly--it may well be caching the
context in order to work around some past bug in Mac Safari or
Firefox.

The latest version of the patch LGTM, since it affects only the mac
build, and fixes a very reproducible crash.  I would prefer to land
this fix, perhaps with a TODO to investigate it more, than to leave
the current code crashing.

--Amanda

[darin (slow to review), 2009-08-13 22:12:10]

Shouldn't we have a test case for this?  We have several good frameworks for
testing plugin nasties.  For example, we have a test plugin that allows you to
synthesize problem scenarios.  There are also ways to exercise specialized Flash
content.

[stuartmorgan, 2009-08-13 22:35:06]

On 2009/08/13 22:12:10, darin wrote:
> Shouldn't we have a test case for this?  We have several good frameworks for
> testing plugin nasties.  For example, we have a test plugin that allows you to
> synthesize problem scenarios.  There are also ways to exercise specialized
Flash
> content.

I didn't realize we had test environments for Flash (the problem requires
specific behavior on the plugin's part, so the test plugin wouldn't trigger it);
I'll definitely follow up with a test case.

[Amanda Walker, 2009-08-13 22:47:28]

We do have, for example, layout tests that exercise flash (and other
plugins).  Doing it as a unit test is an interesting question, but we
should be able to write one that exercises this specific failure case
without a lot of difficulty.

--Amanda

On Thu, Aug 13, 2009 at 6:35 PM, <stuartmorgan@chromium.org> wrote:
> On 2009/08/13 22:12:10, darin wrote:
>>
>> Shouldn't we have a test case for this? =C2=A0We have several good
>
> frameworks for
>>
>> testing plugin nasties. =C2=A0For example, we have a test plugin that
>
> allows you to
>>
>> synthesize problem scenarios. =C2=A0There are also ways to exercise
>
> specialized Flash
>>
>> content.
>
> I didn't realize we had test environments for Flash (the problem
> requires specific behavior on the plugin's part, so the test plugin
> wouldn't trigger it); I'll definitely follow up with a test case.
>
> http://codereview.chromium.org/165344
>



--=20
"Portability is generally the result of advance planning rather than trench
warfare involving #ifdef" -- Henry Spencer (1992)

[darin (slow to review), 2009-08-13 23:58:47]

The PluginTests framework allows you to exercise a SWF file.
-Darin


On Thu, Aug 13, 2009 at 3:47 PM, Amanda Walker <amanda@chromium.org> wrote:

> We do have, for example, layout tests that exercise flash (and other
> plugins).  Doing it as a unit test is an interesting question, but we
> should be able to write one that exercises this specific failure case
> without a lot of difficulty.
>
> --Amanda
>
> On Thu, Aug 13, 2009 at 6:35 PM, <stuartmorgan@chromium.org> wrote:
> > On 2009/08/13 22:12:10, darin wrote:
> >>
> >> Shouldn't we have a test case for this?  We have several good
> >
> > frameworks for
> >>
> >> testing plugin nasties.  For example, we have a test plugin that
> >
> > allows you to
> >>
> >> synthesize problem scenarios.  There are also ways to exercise
> >
> > specialized Flash
> >>
> >> content.
> >
> > I didn't realize we had test environments for Flash (the problem
> > requires specific behavior on the plugin's part, so the test plugin
> > wouldn't trigger it); I'll definitely follow up with a test case.
> >
> > http://codereview.chromium.org/165344
> >
>
>
>
> --
> "Portability is generally the result of advance planning rather than trench
> warfare involving #ifdef" -- Henry Spencer (1992)
>

[darin (slow to review), 2009-08-13 23:59:47]

Sorry... that didn't really answer your question.  I meant to add:  I don't
think we have any layout tests that use Flash.-Darin

On Thu, Aug 13, 2009 at 4:58 PM, Darin Fisher <darin@chromium.org> wrote:

> The PluginTests framework allows you to exercise a SWF file.
> -Darin
>
>
> On Thu, Aug 13, 2009 at 3:47 PM, Amanda Walker <amanda@chromium.org>wrote:
>
>> We do have, for example, layout tests that exercise flash (and other
>> plugins).  Doing it as a unit test is an interesting question, but we
>> should be able to write one that exercises this specific failure case
>> without a lot of difficulty.
>>
>> --Amanda
>>
>> On Thu, Aug 13, 2009 at 6:35 PM, <stuartmorgan@chromium.org> wrote:
>> > On 2009/08/13 22:12:10, darin wrote:
>> >>
>> >> Shouldn't we have a test case for this?  We have several good
>> >
>> > frameworks for
>> >>
>> >> testing plugin nasties.  For example, we have a test plugin that
>> >
>> > allows you to
>> >>
>> >> synthesize problem scenarios.  There are also ways to exercise
>> >
>> > specialized Flash
>> >>
>> >> content.
>> >
>> > I didn't realize we had test environments for Flash (the problem
>> > requires specific behavior on the plugin's part, so the test plugin
>> > wouldn't trigger it); I'll definitely follow up with a test case.
>> >
>> > http://codereview.chromium.org/165344
>> >
>>
>>
>>
>> --
>> "Portability is generally the result of advance planning rather than
>> trench
>> warfare involving #ifdef" -- Henry Spencer (1992)
>>
>
>

[Amanda Walker, 2009-08-14 00:06:27]

You're right--there are tests that check regressions on behavior, but
they use the test plugin to do it.  I was thinking of the set of links
that come up when you launch test_shell as an application, but those
aren't actual tests.

--Amanda

On Thu, Aug 13, 2009 at 7:59 PM, Darin Fisher<darin@chromium.org> wrote:
> Sorry... that didn't really answer your question. =C2=A0I meant to add: =
=C2=A0I don't
> think we have any layout tests that use Flash.
> -Darin
>
> On Thu, Aug 13, 2009 at 4:58 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>> The PluginTests framework allows you to exercise a SWF file.
>> -Darin
>>
>> On Thu, Aug 13, 2009 at 3:47 PM, Amanda Walker <amanda@chromium.org>
>> wrote:
>>>
>>> We do have, for example, layout tests that exercise flash (and other
>>> plugins). =C2=A0Doing it as a unit test is an interesting question, but=
 we
>>> should be able to write one that exercises this specific failure case
>>> without a lot of difficulty.
>>>
>>> --Amanda
>>>
>>> On Thu, Aug 13, 2009 at 6:35 PM, <stuartmorgan@chromium.org> wrote:
>>> > On 2009/08/13 22:12:10, darin wrote:
>>> >>
>>> >> Shouldn't we have a test case for this? =C2=A0We have several good
>>> >
>>> > frameworks for
>>> >>
>>> >> testing plugin nasties. =C2=A0For example, we have a test plugin tha=
t
>>> >
>>> > allows you to
>>> >>
>>> >> synthesize problem scenarios. =C2=A0There are also ways to exercise
>>> >
>>> > specialized Flash
>>> >>
>>> >> content.
>>> >
>>> > I didn't realize we had test environments for Flash (the problem
>>> > requires specific behavior on the plugin's part, so the test plugin
>>> > wouldn't trigger it); I'll definitely follow up with a test case.
>>> >
>>> > http://codereview.chromium.org/165344
>>> >
>>>
>>>
>>>
>>> --
>>> "Portability is generally the result of advance planning rather than
>>> trench
>>> warfare involving #ifdef" -- Henry Spencer (1992)
>>
>
>



--=20
"Portability is generally the result of advance planning rather than trench
warfare involving #ifdef" -- Henry Spencer (1992)

[darin (slow to review), 2009-08-14 00:09:44]

Right!  Also, PluginTests (the one that runs on a couple of the bots) is
defined here: chrome/test/plugin/plugin_tests.cpp

On Thu, Aug 13, 2009 at 5:06 PM, Amanda Walker <amanda@chromium.org> wrote:

> You're right--there are tests that check regressions on behavior, but
> they use the test plugin to do it.  I was thinking of the set of links
> that come up when you launch test_shell as an application, but those
> aren't actual tests.
>
> --Amanda
>
> On Thu, Aug 13, 2009 at 7:59 PM, Darin Fisher<darin@chromium.org> wrote:
> > Sorry... that didn't really answer your question.  I meant to add:  I
> don't
> > think we have any layout tests that use Flash.
> > -Darin
> >
> > On Thu, Aug 13, 2009 at 4:58 PM, Darin Fisher <darin@chromium.org>
> wrote:
> >>
> >> The PluginTests framework allows you to exercise a SWF file.
> >> -Darin
> >>
> >> On Thu, Aug 13, 2009 at 3:47 PM, Amanda Walker <amanda@chromium.org>
> >> wrote:
> >>>
> >>> We do have, for example, layout tests that exercise flash (and other
> >>> plugins).  Doing it as a unit test is an interesting question, but we
> >>> should be able to write one that exercises this specific failure case
> >>> without a lot of difficulty.
> >>>
> >>> --Amanda
> >>>
> >>> On Thu, Aug 13, 2009 at 6:35 PM, <stuartmorgan@chromium.org> wrote:
> >>> > On 2009/08/13 22:12:10, darin wrote:
> >>> >>
> >>> >> Shouldn't we have a test case for this?  We have several good
> >>> >
> >>> > frameworks for
> >>> >>
> >>> >> testing plugin nasties.  For example, we have a test plugin that
> >>> >
> >>> > allows you to
> >>> >>
> >>> >> synthesize problem scenarios.  There are also ways to exercise
> >>> >
> >>> > specialized Flash
> >>> >>
> >>> >> content.
> >>> >
> >>> > I didn't realize we had test environments for Flash (the problem
> >>> > requires specific behavior on the plugin's part, so the test plugin
> >>> > wouldn't trigger it); I'll definitely follow up with a test case.
> >>> >
> >>> > http://codereview.chromium.org/165344
> >>> >
> >>>
> >>>
> >>>
> >>> --
> >>> "Portability is generally the result of advance planning rather than
> >>> trench
> >>> warfare involving #ifdef" -- Henry Spencer (1992)
> >>
> >
> >
>
>
>
> --
> "Portability is generally the result of advance planning rather than trench
> warfare involving #ifdef" -- Henry Spencer (1992)
>