Add acknowledgement messages for PluginMsg_UpdateGeometry

Add acknowledgement messages for PluginMsg_UpdateGeometry.  On the Mac, use
these acknowledgements to know when it's safe to dump old TransportDIBs in the
renderer process.  The Mac TransportDIB implementation uses
base::SharedMemory, which cannot be disposed of if an in-flight UpdateGeometry
message refers to the shared memory file descriptor.

BUG=27510, 26754
TEST=1. From bug 25710:
         a. Visit http://www.dkmsoftware.com/Yubotu.htm
         b. Click "Play Now"
         c. Resize vigorously.
        Expect: no sad plug-in icon.
     2. Test case from bug 26754 (affected machines only):
         a. Visit http://news.google.com/
         b. Click the [+] to the left of a YouTube link.  On affected
            machines, you'll get a sad plug-in icon.
         c. Click the [+] to the left of a different YouTube link.
        Expect: no crash.
     3. Test case from bug 26754 comment 9 (affected machines only):
         a. Have lots of bookmarks (import Safari defaults)
         b. Right-click on bookmark bar, and choose "Open All Bookmarks"
        Expect: no crash.
        This change may not actually fix the third test case.

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

[darin (slow to review), 2009-11-20 07:16:53]

It's very unfortunate (perf wise) to have to use this sync IPC here.  Does
it make page scrolling (for pages with flash content) more janky?

-Darin


On Thu, Nov 19, 2009 at 10:46 PM, <mark@chromium.org> wrote:

> Reviewers: stuartmorgan, Amanda Walker,
>
> Description:
> Use PluginMsg_UpdateGeometrySync instead of PluginMsg_UpdateGeometry on
> Mac.
>
> This fixes bug 27510.  I'm not positive that it fixes bug 26754, because I
> can't reliably reproduce that bug locally, but that bug is similar.
>
> BUG=27510, maybe 26754
> TEST=1. From bug 25710:
>         a. Visit http://www.dkmsoftware.com/Yubotu.htm
>         b. Click "Play Now"
>         c. Resize vigorously.
>        Expect: no sad plug-in icon.
>     2. Test case from bug 26754 comment 9 (affected machines only):
>         a. Have lots of bookmarks (import Safari defaults)
>         b. Right-click on bookmark bar, and choose "Open All Bookmarks"
>        Expect: no crash.
>     3. Test case from bug 26754 (affected machines only):
>         a. Visit http://news.google.com/
>         b. Click the [+] to the left of a YouTube link.  On affected
>            machines, you'll get a sad plug-in icon.
>         c. Click the [+] to the left of a different YouTube link.
>        Expect: no crash.
>
> Please review this at http://codereview.chromium.org/417005
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     chrome/renderer/webplugin_delegate_proxy.cc
>
>
> Index: chrome/renderer/webplugin_delegate_proxy.cc
> ===================================================================
> --- chrome/renderer/webplugin_delegate_proxy.cc (revision 32535)
> +++ chrome/renderer/webplugin_delegate_proxy.cc (working copy)
> @@ -444,18 +444,28 @@
>       param.background_buffer = background_store_->handle();
>   }
>
> -  IPC::Message* msg;
> -#if defined (OS_WIN)
> -  std::wstring filename =
> StringToLowerASCII(info_.path.BaseName().value());
> +  bool sync_message = false;
> +#if defined(OS_MACOSX)
> +  // On the Mac, TransportDIB is implemented on top of base::SharedMemory.
> +  // When a TransportDIB is replaced during a resize, a race condition
> exists
> +  // if any asynchronous operations reference the old shared memory file
> +  // descriptor, because the descriptor may be closed or may even refer to
> +  // another file when the asynchronous operation tries to use it.  The
> file
> +  // descriptor must be transmitted synchronously to avoid this problem.
> +  sync_message = true;
> +#elif defined(OS_WIN)
>   if (info_.name.find(L"Windows Media Player") != std::wstring::npos) {
>     // Need to update geometry synchronously with WMP, otherwise if a site
>     // scripts the plugin to start playing while it's in the middle of
> handling
>     // an update geometry message, videos don't play.  See urls in bug
> 20260.
> +    sync_message = true;
> +  }
> +#endif  // OS_*
> +
> +  IPC::Message* msg;
> +  if (sync_message) {
>     msg = new PluginMsg_UpdateGeometrySync(instance_id_, param);
> -  }
> -  else
> -#endif
> -  {
> +  } else {
>     msg = new PluginMsg_UpdateGeometry(instance_id_, param);
>     msg->set_unblock(true);
>   }
>
>
>

[Mark Mentovai, 2009-11-20 07:25:24]

darin wrote:
> It's very unfortunate (perf wise) to have to use this sync IPC here.  Does
> it make page scrolling (for pages with flash content) more janky?

I agree that async is better than sync when it's possible, but I don't think
it's possible here given the file descriptors being closed and reopened.

Scrolling doesn't feel janky with this patch on my laptop.  In a debug build,
even.

[darin (slow to review), 2009-11-20 07:37:08]

On Thu, Nov 19, 2009 at 11:25 PM, <mark@chromium.org> wrote:

> darin wrote:
>
>> It's very unfortunate (perf wise) to have to use this sync IPC here.  Does
>> it make page scrolling (for pages with flash content) more janky?
>>
>
> I agree that async is better than sync when it's possible, but I don't
> think
> it's possible here given the file descriptors being closed and reopened.
>

I don't follow this at all.  Can you add details to the CL description that
provides
the justification for this change?  (Sorry, I haven't looked at the bugs in
detail.)



>
> Scrolling doesn't feel janky with this patch on my laptop.  In a debug
> build,
> even.


OK.  In that case, perhaps it is less of a worry.

-Darin



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

[darin (slow to review), 2009-11-20 07:38:29]

On Thu, Nov 19, 2009 at 11:36 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Thu, Nov 19, 2009 at 11:25 PM, <mark@chromium.org> wrote:
>
>> darin wrote:
>>
>>> It's very unfortunate (perf wise) to have to use this sync IPC here.
>>>  Does
>>> it make page scrolling (for pages with flash content) more janky?
>>>
>>
>> I agree that async is better than sync when it's possible, but I don't
>> think
>> it's possible here given the file descriptors being closed and reopened.
>>
>
> I don't follow this at all.  Can you add details to the CL description that
> provides
> the justification for this change?  (Sorry, I haven't looked at the bugs in
> detail.)
>
>
^^^ sorry that sounds overly demanding... i can find what i need by looking
through
the bug, but it would probably help others in the future if the 'svn commit'
log
provided more info.



>
>
>>
>> Scrolling doesn't feel janky with this patch on my laptop.  In a debug
>> build,
>> even.
>
>
> OK.  In that case, perhaps it is less of a worry.
>
> -Darin
>
>
>
>>
>>
>> http://codereview.chromium.org/417005
>>
>
>

[Amanda Walker, 2009-11-20 14:51:07]

After reading the description in bug 27510, LGTM as long as we open a new (lower
priority) bug to rework this so that it works more like painting: async
messages, acks, and debouncing so that only one UpdateGeometry call is in flight
at a time.

[darin (slow to review), 2009-11-20 17:03:26]

Is it really too much work to change this to ACK based now?
-Darin

On Fri, Nov 20, 2009 at 6:51 AM, <amanda@chromium.org> wrote:

> After reading the description in bug 27510, LGTM as long as we open a new
> (lower
> priority) bug to rework this so that it works more like painting: async
> messages, acks, and debouncing so that only one UpdateGeometry call is in
> flight
> at a time.
>
>
>
> http://codereview.chromium.org/417005
>

[Amanda Walker, 2009-11-20 17:11:21]

I'm wary of doing so for Mstone-4 at this point, but I'll leave it to Mark
to judge how much entropy it would add.

On Fri, Nov 20, 2009 at 9:01 AM, Darin Fisher <darin@chromium.org> wrote:

> Is it really too much work to change this to ACK based now?
> -Darin
>
>
> On Fri, Nov 20, 2009 at 6:51 AM, <amanda@chromium.org> wrote:
>
>> After reading the description in bug 27510, LGTM as long as we open a new
>> (lower
>> priority) bug to rework this so that it works more like painting: async
>> messages, acks, and debouncing so that only one UpdateGeometry call is in
>> flight
>> at a time.
>>
>>
>>
>> http://codereview.chromium.org/417005
>>
>
>

[Mark Mentovai, 2009-11-29 04:46:58]

This is ready for another look.

I took an ACK-based approach, but did not add anything to limit the number of
outstanding UpdateGeometry messages to one in flight at a time.  I'm more
comfortable not adding that sort of logic so close to M4.  With the
implementation I've chosen here, the ACKs are only used to determine when it's
safe to release an old TransportDIB.

Limiting outstanding UpdateGeometry messages to one in flight at a time is
probably still a good idea, and should be done as a post-M4 follow-up.

[jam, 2009-11-29 18:37:03]

Since the new message and new parameter are only needed for mac, can they
all be behind OS_MAC ifdefs to make it clear that the other platforms don't
need to do anything?

On Sat, Nov 28, 2009 at 8:46 PM, <mark@chromium.org> wrote:

> This is ready for another look.
>
> I took an ACK-based approach, but did not add anything to limit the number
> of
> outstanding UpdateGeometry messages to one in flight at a time.  I'm more
> comfortable not adding that sort of logic so close to M4.  With the
> implementation I've chosen here, the ACKs are only used to determine when
> it's
> safe to release an old TransportDIB.
>
> Limiting outstanding UpdateGeometry messages to one in flight at a time is
> probably still a good idea, and should be done as a post-M4 follow-up.
>
> http://codereview.chromium.org/417005
>

[mmentovai_google.com, 2009-11-29 19:02:24]

John Abd-El-Malek wrote:
> Since the new message and new parameter are only needed for mac, can they
> all be behind OS_MAC ifdefs to make it clear that the other platforms don't
> need to do anything?

I did that initially, but things got pretty cluttered.  See patch set
2, http://codereview.chromium.org/417005/diff/4001/5002 and friends.

I don't think we usually put single parameters behind #ifdefs, but if
you really think it's better, I can go back to doing it that way.

Mark

[jam, 2009-11-30 02:15:26]

On Sun, Nov 29, 2009 at 11:02 AM, Mark Mentovai <mmentovai@google.com>wrote:

> John Abd-El-Malek wrote:
> > Since the new message and new parameter are only needed for mac, can they
> > all be behind OS_MAC ifdefs to make it clear that the other platforms
> don't
> > need to do anything?
>
> I did that initially, but things got pretty cluttered.  See patch set
> 2, http://codereview.chromium.org/417005/diff/4001/5002 and friends.
>
> I don't think we usually put single parameters behind #ifdefs, but if
> you really think it's better, I can go back to doing it that way.
>

I really like it because it makes it clear to people working on the
windows/linux code, or refactoring all of it, that this flag is only used
for Mac.  One thing I sometimes do in these situations which makes the ifdef
clutter a little smaller is to not put the platform specific variable at the
end (because of the awkward && and commas).


>
> Mark
>

[mmentovai_google.com, 2009-11-30 02:50:55]

I uploaded a version that restores the #ifdefs.

Mark

John Abd-El-Malek wrote:
> On Sun, Nov 29, 2009 at 11:02 AM, Mark Mentovai <mmentovai@google.com>
> wrote:
>>
>> John Abd-El-Malek wrote:
>> > Since the new message and new parameter are only needed for mac, can
>> > they
>> > all be behind OS_MAC ifdefs to make it clear that the other platforms
>> > don't
>> > need to do anything?
>>
>> I did that initially, but things got pretty cluttered.  See patch set
>> 2, http://codereview.chromium.org/417005/diff/4001/5002 and friends.
>>
>> I don't think we usually put single parameters behind #ifdefs, but if
>> you really think it's better, I can go back to doing it that way.
>
> I really like it because it makes it clear to people working on the
> windows/linux code, or refactoring all of it, that this flag is only used
> for Mac.  One thing I sometimes do in these situations which makes the ifdef
> clutter a little smaller is to not put the platform specific variable at the
> end (because of the awkward && and commas).

[Amanda Walker, 2009-11-30 03:24:09]

LGTM.

I'm not at all convinced that we only want it on the Mac (seems to me that
debouncing resizes is helpful for the same reasons as debouncing paints, which
we do everywhere), but I don't think that discussion should hold up this fix.

[Mark Mentovai, 2009-11-30 03:27:05]

amanda@chromium.org wrote:
> I'm not at all convinced that we only want it on the Mac (seems to me that
> debouncing resizes is helpful for the same reasons as debouncing paints, which
> we do everywhere), but I don't think that discussion should hold up this fix.

I agree.  When we have something that debounces, it should be
operative on all platforms.

I don't think debouncing is in scope for M4.  And with debouncing,
there will only be one UpdateGeometry in flight at a time, so the
ack_key stuff can come out.

Mark