don't hit NOTREACHED when pasting on non-x11 non-chromeos unix builds

Chromeos and non-X11 unix builds should not hit NOTREACHED when pasting.

BUG=361341
TEST=ClickModifierTest.WindowOpenShiftMiddleClickTest

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

[Mostyn Bramley-Moore, 2014-04-08 08:29:50]

@jam: PTAL at this small patch...

[dcheng, 2014-04-08 08:32:04]

Can you share how you're hitting this? Usually hitting this would mean a
legitimate bug (e.g. we should never see BufferSelection on platforms that don't
support it).

[Mostyn Bramley-Moore, 2014-04-08 08:39:08]

On 2014/04/08 08:32:04, dcheng wrote:
> Can you share how you're hitting this? Usually hitting this would mean a
> legitimate bug (e.g. we should never see BufferSelection on platforms that
don't
> support it).

I have an embedded linux build (with some local patches), middle clicking on any
page hits this assert.  I'll see if I can trace back to see where this
BufferSelection comes from (I don't think we have modified any clipboard code,
but maybe our input code is the problem).

[dcheng, 2014-04-08 08:43:35]

On 2014/04/08 08:39:08, Mostyn Bramley-Moore wrote:
> On 2014/04/08 08:32:04, dcheng wrote:
> > Can you share how you're hitting this? Usually hitting this would mean a
> > legitimate bug (e.g. we should never see BufferSelection on platforms that
> don't
> > support it).
> 
> I have an embedded linux build (with some local patches), middle clicking on
any
> page hits this assert.  I'll see if I can trace back to see where this
> BufferSelection comes from (I don't think we have modified any clipboard code,
> but maybe our input code is the problem).

It probably comes from
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Right now, the code expects that on non-Windows/non-Mac platforms, global
selection is supported:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Mostyn Bramley-Moore, 2014-04-08 10:01:43]

On 2014/04/08 08:43:35, dcheng wrote:
> On 2014/04/08 08:39:08, Mostyn Bramley-Moore wrote:
> > On 2014/04/08 08:32:04, dcheng wrote:
> > > Can you share how you're hitting this? Usually hitting this would mean a
> > > legitimate bug (e.g. we should never see BufferSelection on platforms that
> > don't
> > > support it).
> > 
> > I have an embedded linux build (with some local patches), middle clicking on
> any
> > page hits this assert.  I'll see if I can trace back to see where this
> > BufferSelection comes from (I don't think we have modified any clipboard
code,
> > but maybe our input code is the problem).
> 
> It probably comes from
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Right- EventHandler::handleMouseReleaseEvent calls handlePasteGlobalSelection
and eventually executePasteGlobalSelection.

> Right now, the code expects that on non-Windows/non-Mac platforms, global
> selection is supported:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Should I add an "EditingEmbeddedBehaviour" value for use in
supportsGlobalSelection?  This feels a bit too coarse-grained for my liking. 
NOTIMPLEMENTED feels OK to me, but I don't know which cases you want to catch
here and force developers to fix by using NOTREACHED.

[dcheng, 2014-04-08 21:19:37]

On 2014/04/08 10:01:43, Mostyn Bramley-Moore wrote:
> On 2014/04/08 08:43:35, dcheng wrote:
> > On 2014/04/08 08:39:08, Mostyn Bramley-Moore wrote:
> > > On 2014/04/08 08:32:04, dcheng wrote:
> > > > Can you share how you're hitting this? Usually hitting this would mean a
> > > > legitimate bug (e.g. we should never see BufferSelection on platforms
that
> > > don't
> > > > support it).
> > > 
> > > I have an embedded linux build (with some local patches), middle clicking
on
> > any
> > > page hits this assert.  I'll see if I can trace back to see where this
> > > BufferSelection comes from (I don't think we have modified any clipboard
> code,
> > > but maybe our input code is the problem).
> > 
> > It probably comes from
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Right- EventHandler::handleMouseReleaseEvent calls handlePasteGlobalSelection
> and eventually executePasteGlobalSelection.
> 
> > Right now, the code expects that on non-Windows/non-Mac platforms, global
> > selection is supported:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Should I add an "EditingEmbeddedBehaviour" value for use in
> supportsGlobalSelection?  This feels a bit too coarse-grained for my liking. 
> NOTIMPLEMENTED feels OK to me, but I don't know which cases you want to catch
> here and force developers to fix by using NOTREACHED.

I'd prefer not to change this from NOTREACHED() to NOTIMPLEMENTED(), because we
really just shouldn't be hitting that case normally.

Perhaps we can figure out how CrOS is avoiding this case in
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/common/webp...
and emulate what they do?

[Mostyn Bramley-Moore, 2014-04-08 21:23:43]

> > Should I add an "EditingEmbeddedBehaviour" value for use in
> > supportsGlobalSelection?  This feels a bit too coarse-grained for my liking.

> > NOTIMPLEMENTED feels OK to me, but I don't know which cases you want to
catch
> > here and force developers to fix by using NOTREACHED.
> 
> I'd prefer not to change this from NOTREACHED() to NOTIMPLEMENTED(), because
we
> really just shouldn't be hitting that case normally.
> 
> Perhaps we can figure out how CrOS is avoiding this case in
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/common/webp...
> and emulate what they do?

CrOS simply returns false from an ifdef block in this function ~7 lines up
without hitting NOTREACHED, so it looks non-serious to that target at least.

[dcheng, 2014-04-08 21:55:05]

Ah. Sorry for missing that.

Is there some combination of defines that we can gate this on so we can adjust
the #if block to encompass your situation as well?

[Mostyn Bramley-Moore, 2014-04-08 22:13:48]

On 2014/04/08 21:55:05, dcheng wrote:
> Ah. Sorry for missing that.
> 
> Is there some combination of defines that we can gate this on so we can adjust
> the #if block to encompass your situation as well?

There is am "embedded" gyp variable, that we could use add a new define.  

@rjkroege, spang: what do you think?  Do your platforms hit this NOTREACHED
here?

[spang, 2014-04-08 22:51:20]

On 2014/04/08 22:13:48, Mostyn Bramley-Moore wrote:
> On 2014/04/08 21:55:05, dcheng wrote:
> > Ah. Sorry for missing that.
> > 
> > Is there some combination of defines that we can gate this on so we can
adjust
> > the #if block to encompass your situation as well?
> 
> There is am "embedded" gyp variable, that we could use add a new define.  

I don't think we should add an EMBEDDED ifdef. You can use the USE_OZONE ifdef
if its really necessary.

> @rjkroege, spang: what do you think?  Do your platforms hit this NOTREACHED
> here?

Changing USE_X11 to OS_LINUX might work. However, I agree with adding

#else
  NOTIMPLEMENTED()
  return false;
#endif

However it should be under the BufferSelection case rather than the default
case. 

NOTIMPLEMENTED is much kinder to porters.

[spang, 2014-04-08 22:57:21]

https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
File content/renderer/webclipboard_impl.cc (right):

https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
content/renderer/webclipboard_impl.cc:215: #endif
This code can preprocess to 

    case BufferSelection:
    default:
      NOTREACHED();
      return false;

That seems rather unexpected. Especially since there's no "fallthrough" comment
here.

[rjkroege, 2014-04-09 14:56:07]

https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
File content/renderer/webclipboard_impl.cc (right):

https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
content/renderer/webclipboard_impl.cc:215: #endif
On 2014/04/08 22:57:21, spang wrote:
> This code can preprocess to 
> 
>     case BufferSelection:
>     default:
>       NOTREACHED();
>       return false;
> 
> That seems rather unexpected. Especially since there's no "fallthrough"
comment
> here.

I'm with spang here. Why does this code not fire on Windows or Mac etc.?  Is it
possible that somewhere up the calling chain, there is a missing #ifdef that
makes sure that buffer is set to BufferStandard on embedded=1 builds?

[spang, 2014-04-09 15:22:04]

On 2014/04/09 14:56:07, rjkroege wrote:
>
https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
> File content/renderer/webclipboard_impl.cc (right):
> 
>
https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
> content/renderer/webclipboard_impl.cc:215: #endif
> On 2014/04/08 22:57:21, spang wrote:
> > This code can preprocess to 
> > 
> >     case BufferSelection:
> >     default:
> >       NOTREACHED();
> >       return false;
> > 
> > That seems rather unexpected. Especially since there's no "fallthrough"
> comment
> > here.
> 
> I'm with spang here. Why does this code not fire on Windows or Mac etc.?  Is
it
> possible that somewhere up the calling chain, there is a missing #ifdef that
> makes sure that buffer is set to BufferStandard on embedded=1 builds?

I think the reason it breaks is that blink defaults to "unix" selection
behavior, but we're assuming unix == x11 here. So we hit the NOTREACHED.

static EditingBehaviorType editingBehaviorTypeForPlatform()
{
    return
#if OS(MACOSX)
    EditingMacBehavior
#elif OS(WIN)
    EditingWindowsBehavior
#elif OS(ANDROID)
    EditingAndroidBehavior
#else // Rest of the UNIX-like systems
    EditingUnixBehavior
#endif
    ;
}

[rjkroege, 2014-04-09 16:09:39]

On 2014/04/09 15:22:04, spang wrote:
> On 2014/04/09 14:56:07, rjkroege wrote:
> >
>
https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
> > File content/renderer/webclipboard_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
> > content/renderer/webclipboard_impl.cc:215: #endif
> > On 2014/04/08 22:57:21, spang wrote:
> > > This code can preprocess to 
> > > 
> > >     case BufferSelection:
> > >     default:
> > >       NOTREACHED();
> > >       return false;
> > > 
> > > That seems rather unexpected. Especially since there's no "fallthrough"
> > comment
> > > here.
> > 
> > I'm with spang here. Why does this code not fire on Windows or Mac etc.?  Is
> it
> > possible that somewhere up the calling chain, there is a missing #ifdef that
> > makes sure that buffer is set to BufferStandard on embedded=1 builds?
> 
> I think the reason it breaks is that blink defaults to "unix" selection
> behavior, but we're assuming unix == x11 here. So we hit the NOTREACHED.

this raises two points not really in scope in this CL. (http://crbug.com/361620)
to track

1. (in an ideal world of infinitely available engineering :-) ozone
implementations ought to be able to pick their blink-side editing behaviour.
i.e.: someday there might be ozone=1 on mac and ozone=1 on win. Or something
like that.

2. for ozone=1, chromeos=1, UX should get to pick which editing behaviour they
want right? ChromeOS has many editing / key-binding defaults in non-blink code
that are reminiscent of windows. maybe blink for CrOS/ozone should be like win?

In this particular case however, we have "EditingUnixBehavior" true so maybe
change:

#if defined(USE_X11) 

to 

#if defined(USE_X11) || defined(USE_OZONE)?


> 
> static EditingBehaviorType editingBehaviorTypeForPlatform()
> {
>     return
> #if OS(MACOSX)
>     EditingMacBehavior
> #elif OS(WIN)
>     EditingWindowsBehavior
> #elif OS(ANDROID)
>     EditingAndroidBehavior
> #else // Rest of the UNIX-like systems
>     EditingUnixBehavior
> #endif
>     ;
> }

[spang, 2014-04-09 16:34:47]

On 2014/04/09 16:09:39, rjkroege wrote:
> On 2014/04/09 15:22:04, spang wrote:
> > On 2014/04/09 14:56:07, rjkroege wrote:
> > >
> >
>
https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
> > > File content/renderer/webclipboard_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
> > > content/renderer/webclipboard_impl.cc:215: #endif
> > > On 2014/04/08 22:57:21, spang wrote:
> > > > This code can preprocess to 
> > > > 
> > > >     case BufferSelection:
> > > >     default:
> > > >       NOTREACHED();
> > > >       return false;
> > > > 
> > > > That seems rather unexpected. Especially since there's no "fallthrough"
> > > comment
> > > > here.
> > > 
> > > I'm with spang here. Why does this code not fire on Windows or Mac etc.? 
Is
> > it
> > > possible that somewhere up the calling chain, there is a missing #ifdef
that
> > > makes sure that buffer is set to BufferStandard on embedded=1 builds?
> > 
> > I think the reason it breaks is that blink defaults to "unix" selection
> > behavior, but we're assuming unix == x11 here. So we hit the NOTREACHED.
> 
> this raises two points not really in scope in this CL.
(http://crbug.com/361620)
> to track
> 
> 1. (in an ideal world of infinitely available engineering :-) ozone
> implementations ought to be able to pick their blink-side editing behaviour.
> i.e.: someday there might be ozone=1 on mac and ozone=1 on win. Or something
> like that.
> 
> 2. for ozone=1, chromeos=1, UX should get to pick which editing behaviour they
> want right? ChromeOS has many editing / key-binding defaults in non-blink code
> that are reminiscent of windows. maybe blink for CrOS/ozone should be like
win?
> 
> In this particular case however, we have "EditingUnixBehavior" true so maybe
> change:
> 
> #if defined(USE_X11) 
> 
> to 
> 
> #if defined(USE_X11) || defined(USE_OZONE)?
> 

This is a good alternative if we're worried about being more permissive here.

However the fact that blink defaults to unix but this may result in a
NOTREACHED() here seems bad.

> 
> > 
> > static EditingBehaviorType editingBehaviorTypeForPlatform()
> > {
> >     return
> > #if OS(MACOSX)
> >     EditingMacBehavior
> > #elif OS(WIN)
> >     EditingWindowsBehavior
> > #elif OS(ANDROID)
> >     EditingAndroidBehavior
> > #else // Rest of the UNIX-like systems
> >     EditingUnixBehavior
> > #endif
> >     ;
> > }

[Mostyn Bramley-Moore, 2014-04-09 18:40:27]

https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
File content/renderer/webclipboard_impl.cc (right):

https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
content/renderer/webclipboard_impl.cc:215: #endif
> I'm with spang here. Why does this code not fire on Windows or Mac etc.?  Is
it
> possible that somewhere up the calling chain, there is a missing #ifdef that
> makes sure that buffer is set to BufferStandard on embedded=1 builds?

X is the only platform I know of that copies on selection (I assume that's what
BufferSelection means: "the buffer was filled automatically by selection").

This is only triggered if EditingBehavior::supportsGlobalSelection() returns
true, which it never does for windows or mac, as dcheng mentioned earlier:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

How about we rejig the ifdefs to something like:
#if defined(USE_X11) && !defined(OS_CHROMEOS)
  *result = ui::CLIPBOARD_TYPE_SELECTION;
  break;
#else
  // Chromeos and non-x11 unix builds don't support the X selection clipboard.
  return false;
#endif

[spang, 2014-04-09 18:53:41]

On 2014/04/09 18:40:27, Mostyn Bramley-Moore wrote:
>
https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
> File content/renderer/webclipboard_impl.cc (right):
> 
>
https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
> content/renderer/webclipboard_impl.cc:215: #endif
> > I'm with spang here. Why does this code not fire on Windows or Mac etc.?  Is
> it
> > possible that somewhere up the calling chain, there is a missing #ifdef that
> > makes sure that buffer is set to BufferStandard on embedded=1 builds?
> 
> X is the only platform I know of that copies on selection (I assume that's
what
> BufferSelection means: "the buffer was filled automatically by selection").
> 
> This is only triggered if EditingBehavior::supportsGlobalSelection() returns
> true, which it never does for windows or mac, as dcheng mentioned earlier:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> How about we rejig the ifdefs to something like:
> #if defined(USE_X11) && !defined(OS_CHROMEOS)
>   *result = ui::CLIPBOARD_TYPE_SELECTION;
>   break;
> #else
>   // Chromeos and non-x11 unix builds don't support the X selection clipboard.
>   return false;
> #endif

That sounds fine, at least for now.

However, this seems like the wrong place to introduce this special case. If the
platform doesn't support the selection clipboard, then it can achieve that in
the platform implementation of the clipboard. We shouldn't need any platform
ifdefs for clipboard inside content/.

So in concrete terms, handling this case should be moved to clipboard_aurax11.

[dcheng, 2014-04-09 19:27:56]

On 2014/04/09 18:53:41, spang wrote:
> On 2014/04/09 18:40:27, Mostyn Bramley-Moore wrote:
> >
>
https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
> > File content/renderer/webclipboard_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/224843013/diff/1/content/renderer/webclipboar...
> > content/renderer/webclipboard_impl.cc:215: #endif
> > > I'm with spang here. Why does this code not fire on Windows or Mac etc.? 
Is
> > it
> > > possible that somewhere up the calling chain, there is a missing #ifdef
that
> > > makes sure that buffer is set to BufferStandard on embedded=1 builds?
> > 
> > X is the only platform I know of that copies on selection (I assume that's
> what
> > BufferSelection means: "the buffer was filled automatically by selection").
> > 
> > This is only triggered if EditingBehavior::supportsGlobalSelection() returns
> > true, which it never does for windows or mac, as dcheng mentioned earlier:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > How about we rejig the ifdefs to something like:
> > #if defined(USE_X11) && !defined(OS_CHROMEOS)
> >   *result = ui::CLIPBOARD_TYPE_SELECTION;
> >   break;
> > #else
> >   // Chromeos and non-x11 unix builds don't support the X selection
clipboard.
> >   return false;
> > #endif
> 
> That sounds fine, at least for now.
> 
> However, this seems like the wrong place to introduce this special case. If
the
> platform doesn't support the selection clipboard, then it can achieve that in
> the platform implementation of the clipboard. We shouldn't need any platform
> ifdefs for clipboard inside content/.
> 
> So in concrete terms, handling this case should be moved to clipboard_aurax11.

?

The clipboard code for non-X11 platforms has traditionally DCHECKed that the
clipboard type is not CLIPBOARD_TYPE_SELECTION. I think CrOS is buggy that it
shortcuts in content. By doing so, it actually fires a paste event in web
content on middle click--which it shouldn't, because the operation isn't
supported to begin with.

[Mostyn Bramley-Moore, 2014-04-09 19:34:59]

@dcheng: are you happy with patchset 2 in the meantime?

[dcheng, 2014-04-09 20:45:30]

Other than that, LGTM as a temporary solution.

https://codereview.chromium.org/224843013/diff/20001/content/renderer/webclip...
File content/renderer/webclipboard_impl.cc (right):

https://codereview.chromium.org/224843013/diff/20001/content/renderer/webclip...
content/renderer/webclipboard_impl.cc:210: // Chrome OS and non-X11 unix builds
do not support
Do you mind adding a TODO with a reference to http://crbug.com/361753? We should
clean this up in the long run.

[Mostyn Bramley-Moore, 2014-04-09 21:00:40]

@jam: can you give this an OWNER thumbs up?

https://codereview.chromium.org/224843013/diff/20001/content/renderer/webclip...
File content/renderer/webclipboard_impl.cc (right):

https://codereview.chromium.org/224843013/diff/20001/content/renderer/webclip...
content/renderer/webclipboard_impl.cc:210: // Chrome OS and non-X11 unix builds
do not support
On 2014/04/09 20:45:30, dcheng wrote:
> Do you mind adding a TODO with a reference to http://crbug.com/361753 We
should
> clean this up in the long run.

Done.

[jam, 2014-04-11 18:07:29]

lgtm

[spang, 2014-04-11 19:22:21]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-04-11 19:23:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/224843013/40001

[commit-bot: I haz the power, 2014-04-12 00:07:05]

Change committed as 263420