Switch Linux Auru ports over to POSIX SHM instead of legacy SYSV SHM.

Switch Linux Aura ports over to POSIX SHM instead of legacy SYSV SHM.

BUG=147622
R=jln@chromium.org, piman@chromium.org, skaslev@chromium.org

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

[jln (very slow on Chromium), 2013-04-06 00:22:23]

Looks good in general. I didn't look closely at every #ifdef, but tests and bots
will do a much better job at this anyways.

https://codereview.chromium.org/13529027/diff/2002/content/common/sandbox_sec...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://codereview.chromium.org/13529027/diff/2002/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:1389: #if defined(TOOLKIT_GTK)
Please, make an IfUsingToolKitGtk() function above, like we do have for
IsArchitecture* instead of using #ifdef.

The less #ifdefs we have, the better off we are.

https://codereview.chromium.org/13529027/diff/2002/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:1418: #if defined(TOOLKIT_GTK)
Same remark here.

[jln (very slow on Chromium), 2013-04-06 00:48:53]

lgtm, on to owners reviews!

[Chris Evans, 2013-04-06 02:00:49]

On 2013/04/06 00:48:53, Julien Tinnes wrote:
> lgtm, on to owners reviews!

[+piman]

John suggested he might be a better reviewer.

(Linux bots in PS#2 try run are now all green which is nice)

[piman, 2013-04-06 02:02:13]

Mechanical things look ok, but at a high level there's 2 things:
- https://chromiumcodereview.appspot.com/13255002 landed recently, is a new user
of TransportDIB::Create from the renderer, that would now fail. I guess it needs
to go through RenderProcessImpl::CreateTransportDIB . It does that on the
compositor thread, which means that we'd need to go through the
SyncMessageFilter rather than directly to the SyncChannel in that case.
- long time ago, I fixed an issue with Flash that was creating a lot of
ImageData, hitting the limit of 4k sysv segments. I lowered to 2k. On Chrome OS
though, the fd limit is 1k, and I suspect that would cause Flash and/or the
renderer to crash (out of fds). I will try to find the repro case, I don't have
it handy. I suspect we'll need to fix Flash for that.

[cevans, 2013-04-06 02:08:49]

On Fri, Apr 5, 2013 at 7:02 PM, <piman@chromium.org> wrote:

> Mechanical things look ok, but at a high level there's 2 things:
> -
https://chromiumcodereview.**appspot.com/13255002<https://chromiumcodereview....
recently, is a new user
> of TransportDIB::Create from the renderer, that would now fail. I guess it
> needs
> to go through RenderProcessImpl::**CreateTransportDIB . It does that on
> the
> compositor thread, which means that we'd need to go through the
> SyncMessageFilter rather than directly to the SyncChannel in that case.
>

Interesting. Is there an easy way for me to trigger this code path so I can
test it?
--disable-gpu or some such?

I can get on this first piece right away.



> - long time ago, I fixed an issue with Flash that was creating a lot of
> ImageData, hitting the limit of 4k sysv segments. I lowered to 2k. On
> Chrome OS
> though, the fd limit is 1k, and I suspect that would cause Flash and/or the
> renderer to crash (out of fds). I will try to find the repro case, I don't
> have
> it handy. I suspect we'll need to fix Flash for that.
>
>
https://codereview.chromium.**org/13529027/<https://codereview.chromium.org/1...
>

[piman, 2013-04-06 03:32:52]

On Fri, Apr 5, 2013 at 7:08 PM, Chris Evans <cevans@google.com> wrote:

> On Fri, Apr 5, 2013 at 7:02 PM, <piman@chromium.org> wrote:
>
>> Mechanical things look ok, but at a high level there's 2 things:
>> -
https://chromiumcodereview.**appspot.com/13255002<https://chromiumcodereview....
recently, is a new user
>> of TransportDIB::Create from the renderer, that would now fail. I guess
>> it needs
>> to go through RenderProcessImpl::**CreateTransportDIB . It does that on
>> the
>> compositor thread, which means that we'd need to go through the
>> SyncMessageFilter rather than directly to the SyncChannel in that case.
>>
>
> Interesting. Is there an easy way for me to trigger this code path so I
> can test it?
> --disable-gpu or some such?
>
> I can get on this first piece right away.
>

Try --enable-software-compositing-gl-adapter on an aura build.

I was just thinking more about it and there might be a bit more to it wrt
the way we manage handles going back and forth between the browser and the
renderer. I can look at it / help you on monday


>
>
>> - long time ago, I fixed an issue with Flash that was creating a lot of
>> ImageData, hitting the limit of 4k sysv segments. I lowered to 2k. On
>> Chrome OS
>> though, the fd limit is 1k, and I suspect that would cause Flash and/or
>> the
>> renderer to crash (out of fds). I will try to find the repro case, I
>> don't have
>> it handy. I suspect we'll need to fix Flash for that.
>>
>>
https://codereview.chromium.**org/13529027/<https://codereview.chromium.org/1...
>>
>
>

[rjkroege, 2013-05-16 19:29:42]

drive by comments. feel free to ignore.

https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
File ui/surface/transport_dib_posix.cc (right):

https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
ui/surface/transport_dib_posix.cc:8: #if !defined(TOOLKIT_GTK)
perhaps get rid of the #define and force-exclude _posix on gtk builds from .gyp?

https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
File ui/surface/transport_dib_sysvipc.cc (right):

https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
ui/surface/transport_dib_sysvipc.cc:8: #if defined(TOOLKIT_GTK)
rename this file to _gtk and you don't need the #define

[Chris Evans, 2013-05-16 22:45:27]

On 2013/05/16 19:29:42, rjkroege wrote:
> drive by comments. feel free to ignore.
> 
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> File ui/surface/transport_dib_posix.cc (right):
> 
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> ui/surface/transport_dib_posix.cc:8: #if !defined(TOOLKIT_GTK)
> perhaps get rid of the #define and force-exclude _posix on gtk builds from
.gyp?

I don't think we can generally do that, because GTK is still a POSIX platform.

> 
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> File ui/surface/transport_dib_sysvipc.cc (right):
> 
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> ui/surface/transport_dib_sysvipc.cc:8: #if defined(TOOLKIT_GTK)
> rename this file to _gtk and you don't need the #define

Interesting idea, I'll try it.

[slavi, 2013-05-16 22:50:00]

lgtm

[slavi, 2013-05-16 23:15:47]

https://codereview.chromium.org/13529027/diff/33015/content/renderer/render_p...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/13529027/diff/33015/content/renderer/render_p...
content/renderer/render_process_impl.cc:129: return TransportDIB::Create(size,
transport_dib_next_sequence_number_++);
Shouldn't we also map the new dib to match the semantics of
RenderProcessImpl::CreateTransportDIB on POSIX (above)?

[rjkroege, 2013-05-16 23:20:17]

On 2013/05/16 22:45:27, Chris Evans wrote:
> On 2013/05/16 19:29:42, rjkroege wrote:
> > drive by comments. feel free to ignore.
> > 
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > File ui/surface/transport_dib_posix.cc (right):
> > 
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > ui/surface/transport_dib_posix.cc:8: #if !defined(TOOLKIT_GTK)
> > perhaps get rid of the #define and force-exclude _posix on gtk builds from
> .gyp?
> 
> I don't think we can generally do that, because GTK is still a POSIX platform.

I was thinking making a change to surface.gyp:

      'conditions': [
        ['use_gtk=1', {
          'sources!': [
            'transport_dib_posix.cc',
          ],
        }],

which ought to exclude from the gtk compile

and then the rename of transport_dib_sysvipc -> transport_dib_gtk would have it
picked up instead?

> 
> > 
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > File ui/surface/transport_dib_sysvipc.cc (right):
> > 
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > ui/surface/transport_dib_sysvipc.cc:8: #if defined(TOOLKIT_GTK)
> > rename this file to _gtk and you don't need the #define
> 
> Interesting idea, I'll try it.

[Chris Evans, 2013-05-17 00:15:04]

On 2013/05/16 23:20:17, rjkroege wrote:
> On 2013/05/16 22:45:27, Chris Evans wrote:
> > On 2013/05/16 19:29:42, rjkroege wrote:
> > > drive by comments. feel free to ignore.
> > > 
> > >
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > > File ui/surface/transport_dib_posix.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > > ui/surface/transport_dib_posix.cc:8: #if !defined(TOOLKIT_GTK)
> > > perhaps get rid of the #define and force-exclude _posix on gtk builds from
> > .gyp?
> > 
> > I don't think we can generally do that, because GTK is still a POSIX
platform.
> 
> I was thinking making a change to surface.gyp:
> 
>       'conditions': [
>         ['use_gtk=1', {
>           'sources!': [
>             'transport_dib_posix.cc',
>           ],
>         }],
> 
> which ought to exclude from the gtk compile
> 
> and then the rename of transport_dib_sysvipc -> transport_dib_gtk would have
it
> picked up instead?
> 
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > > File ui/surface/transport_dib_sysvipc.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > > ui/surface/transport_dib_sysvipc.cc:8: #if defined(TOOLKIT_GTK)
> > > rename this file to _gtk and you don't need the #define
> > 
> > Interesting idea, I'll try it.

Yeah, it works. Cool. Thanks / done.

[Chris Evans, 2013-05-17 00:40:03]

On 2013/05/16 23:20:17, rjkroege wrote:
> On 2013/05/16 22:45:27, Chris Evans wrote:
> > On 2013/05/16 19:29:42, rjkroege wrote:
> > > drive by comments. feel free to ignore.
> > > 
> > >
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > > File ui/surface/transport_dib_posix.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > > ui/surface/transport_dib_posix.cc:8: #if !defined(TOOLKIT_GTK)
> > > perhaps get rid of the #define and force-exclude _posix on gtk builds from
> > .gyp?
> > 
> > I don't think we can generally do that, because GTK is still a POSIX
platform.
> 
> I was thinking making a change to surface.gyp:
> 
>       'conditions': [
>         ['use_gtk=1', {
>           'sources!': [
>             'transport_dib_posix.cc',
>           ],
>         }],
> 
> which ought to exclude from the gtk compile

Ah, I see. I'm fairly ambivalent as to which is less complexity. I'm not 100%
comfortable fiddling with posix vs. gtk in the build files, because the
semantics of _gtk.cc and _posix.cc are well defined. I'm leaning towards leaving
it as-is.

> 
> and then the rename of transport_dib_sysvipc -> transport_dib_gtk would have
it
> picked up instead?

I did do the rename in the latest patchset, which I think makes things cleaner.

> 
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > > File ui/surface/transport_dib_sysvipc.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/13529027/diff/15001/ui/surface/transport_dib_...
> > > ui/surface/transport_dib_sysvipc.cc:8: #if defined(TOOLKIT_GTK)
> > > rename this file to _gtk and you don't need the #define
> > 
> > Interesting idea, I'll try it.

[piman, 2013-05-17 01:12:31]

LGTM. I can't find the bug that had the repro case for Flash, so we'll go with
that and see if anyone screams.

[Chris Evans, 2013-05-17 07:31:47]

Committed patchset #11 manually as r200741.