Make SandboxIPCProcess a thread.

Make SandboxIPCProcess a thread.

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

[Jorge Lucangeli Obes, 2014-04-29 00:18:48]

Hi guys,

This is my proposal to turn SandboxIPCProcess into a thread in the browser
process. Some comments about the decisions:

-The Sandbox IPC mechanism is already quite low-level and independent from the
rest of Chrome, so it made sense to use SimpleThread for this (SimpleThread's
does not have a MessageLoop).

-Thread lifetime: I'm calling Join() in the destructor for RSHL. However
SandboxIPCHandler::Run does not return at this  point (except on LOG(FATAL), but
it's not really returning there anyways). Since we need this thread to be as
long-lived as the entire browser process, I think this is fine.

-Ownership: DelegateSimpleThread does the following with the |delegate| pointer:
void DelegateSimpleThread::Run() {
  DCHECK(delegate_) << ...
  delegate_->Run();
  delegate_ = NULL;

Therefore I think it's reasonable to keep two scoped_ptr's and use get() to
allow DST to call Run() on the handler. There's no transfer of ownership nor
risk of UaF -- the SandboxIPCHandler object and the DelegateSimpleThread object
are destroyed after Run() and Start() return.

I also got rid of the only use of Pid() in RSHL. There's a couple of constants
left that I can remove if/once this lands.

I'd like to keep this as simple as possible; I'm mostly doing it because the
previous implementation was confusing crash reporting and showing
SandboxIPCProcess as the (incorrect) culprit, which caused the sandbox to be
blamed for crashes.

Thanks for taking a look!

[piman, 2014-04-29 00:25:45]

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
File content/browser/renderer_host/render_sandbox_host_linux.cc (left):

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
content/browser/renderer_host/render_sandbox_host_linux.cc:52: #endif  //
!defined(THREAD_SANITIZER)
Has the zygote been launched at this point?

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
File content/browser/renderer_host/sandbox_ipc_linux.cc (right):

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
content/browser/renderer_host/sandbox_ipc_linux.cc:158: return;
How do we ever exit the thread?

[Jorge Lucangeli Obes, 2014-04-29 01:00:10]

On 2014/04/29 00:25:45, piman wrote:
>
https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
> File content/browser/renderer_host/render_sandbox_host_linux.cc (left):
> 
>
https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
> content/browser/renderer_host/render_sandbox_host_linux.cc:52: #endif  //
> !defined(THREAD_SANITIZER)
> Has the zygote been launched at this point?
> 
>
https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
> 
>
https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
> content/browser/renderer_host/sandbox_ipc_linux.cc:158: return;
> How do we ever exit the thread?

Before, SandboxIPC would exit its *process* when reading from a pipe returned
EOF, meaning that the browser process had died. Therefore, its lifetime was
always at least as long as the browser process.

Now that SandboxIPC is a thread, it obviously makes no sense to use the same
mechanism: if the browser process dies, this specific thread dies too and will
not be reading from any pipe. However, if SandboxIPC is a thread, its lifetime
should match the lifetime of the entire browser process. I'm not sure we want
SandboxIPC::Run to return, but maybe we want to be able to stop the thread and
join it before exiting the browser process?

Thanks for the comments.

[piman, 2014-04-29 01:47:02]

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
File content/browser/renderer_host/sandbox_ipc_linux.cc (right):

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
content/browser/renderer_host/sandbox_ipc_linux.cc:151: const int r =
HANDLE_EINTR(poll(pfds, 2, -1 /* no timeout */));
Actually, we should only poll 1 fd, not 2.

[piman, 2014-04-29 01:47:18]

On Mon, Apr 28, 2014 at 6:00 PM, <jorgelo@chromium.org> wrote:

> On 2014/04/29 00:25:45, piman wrote:
>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 40001/content/browser/renderer_host/render_sandbox_host_linux.cc
>
>> File content/browser/renderer_host/render_sandbox_host_linux.cc (left):
>>
>
>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 40001/content/browser/renderer_host/render_sandbox_host_linux.cc#oldcode52
>
>> content/browser/renderer_host/render_sandbox_host_linux.cc:52: #endif  //
>> !defined(THREAD_SANITIZER)
>> Has the zygote been launched at this point?
>>
>
>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc
>
>> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
>>
>
>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode158
>
>> content/browser/renderer_host/sandbox_ipc_linux.cc:158: return;
>> How do we ever exit the thread?
>>
>
> Before, SandboxIPC would exit its *process* when reading from a pipe
> returned
> EOF, meaning that the browser process had died. Therefore, its lifetime was
> always at least as long as the browser process.
>
> Now that SandboxIPC is a thread, it obviously makes no sense to use the
> same
> mechanism: if the browser process dies, this specific thread dies too and
> will
> not be reading from any pipe. However, if SandboxIPC is a thread, its
> lifetime
> should match the lifetime of the entire browser process. I'm not sure we
> want
> SandboxIPC::Run to return, but maybe we want to be able to stop the thread
> and
> join it before exiting the browser process?
>

Well, right now, RenderSandboxHostLinux::~RenderSandboxHostLinux joins the
thread, but the thread can't exit... isn't it a problem?

I'd be more comfortable if we joined the thread before exiting. We can use
the fact that the browser and all the renderers closed their end of the
socket for exiting the thread? poll() should report the fd as having an
error condition.


>
>
> Thanks for the comments.
>
> https://chromiumcodereview.appspot.com/255693002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jorge Lucangeli Obes, 2014-04-29 18:34:13]

On 2014/04/29 01:47:18, piman wrote:
> On Mon, Apr 28, 2014 at 6:00 PM, <mailto:jorgelo@chromium.org> wrote:
> 
> > On 2014/04/29 00:25:45, piman wrote:
> >
> > https://chromiumcodereview.appspot.com/255693002/diff/
> > 40001/content/browser/renderer_host/render_sandbox_host_linux.cc
> >
> >> File content/browser/renderer_host/render_sandbox_host_linux.cc (left):
> >>
> >
> >
> > https://chromiumcodereview.appspot.com/255693002/diff/
> > 40001/content/browser/renderer_host/render_sandbox_host_linux.cc#oldcode52
> >
> >> content/browser/renderer_host/render_sandbox_host_linux.cc:52: #endif  //
> >> !defined(THREAD_SANITIZER)
> >> Has the zygote been launched at this point?
> >>
> >
> >
> > https://chromiumcodereview.appspot.com/255693002/diff/
> > 40001/content/browser/renderer_host/sandbox_ipc_linux.cc
> >
> >> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
> >>
> >
> >
> > https://chromiumcodereview.appspot.com/255693002/diff/
> > 40001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode158
> >
> >> content/browser/renderer_host/sandbox_ipc_linux.cc:158: return;
> >> How do we ever exit the thread?
> >>
> >
> > Before, SandboxIPC would exit its *process* when reading from a pipe
> > returned
> > EOF, meaning that the browser process had died. Therefore, its lifetime was
> > always at least as long as the browser process.
> >
> > Now that SandboxIPC is a thread, it obviously makes no sense to use the
> > same
> > mechanism: if the browser process dies, this specific thread dies too and
> > will
> > not be reading from any pipe. However, if SandboxIPC is a thread, its
> > lifetime
> > should match the lifetime of the entire browser process. I'm not sure we
> > want
> > SandboxIPC::Run to return, but maybe we want to be able to stop the thread
> > and
> > join it before exiting the browser process?
> >
> 
> Well, right now, RenderSandboxHostLinux::~RenderSandboxHostLinux joins the
> thread, but the thread can't exit... isn't it a problem?
> 
> I'd be more comfortable if we joined the thread before exiting. We can use
> the fact that the browser and all the renderers closed their end of the
> socket for exiting the thread? poll() should report the fd as having an
> error condition.
> 

I'll do that.

> 
> >
> >
> > Thanks for the comments.
> >
> > https://chromiumcodereview.appspot.com/255693002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[jln (very slow on Chromium), 2014-04-29 19:45:27]

The change looks good in general, thanks for taking this on!

> > Well, right now, RenderSandboxHostLinux::~RenderSandboxHostLinux joins the
> > thread, but the thread can't exit... isn't it a problem?
> > 
> > I'd be more comfortable if we joined the thread before exiting. We can use
> > the fact that the browser and all the renderers closed their end of the
> > socket for exiting the thread? poll() should report the fd as having an
> > error condition.
> > 
> 
> I'll do that.

This looks fine to me, even though in practice I fear it'll be a little fragile
to rely on all the renderers to have closed their end, so we should brace
ourselves for some crash reports at Join() time :)

The current version could also be made to work by using the fact that poll is a
cancellation point, but we don't use pthread_cancel() in base/ (probably a good
idea).

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
File content/browser/renderer_host/sandbox_ipc_linux.cc (right):

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
content/browser/renderer_host/sandbox_ipc_linux.cc:151: const int r =
HANDLE_EINTR(poll(pfds, 2, -1 /* no timeout */));
s/2/arraysize(pfds)/

[Jorge Lucangeli Obes, 2014-04-29 20:51:10]

PTAL. I replaced the lifeline_fd check with a check for POLLERR.

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
File content/browser/renderer_host/sandbox_ipc_linux.cc (right):

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
content/browser/renderer_host/sandbox_ipc_linux.cc:151: const int r =
HANDLE_EINTR(poll(pfds, 2, -1 /* no timeout */));
On 2014/04/29 19:45:28, jln wrote:
> s/2/arraysize(pfds)/

Done.

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
content/browser/renderer_host/sandbox_ipc_linux.cc:158: return;
On 2014/04/29 00:25:46, piman wrote:
> How do we ever exit the thread?

Done.

[piman, 2014-04-29 21:23:51]

https://chromiumcodereview.appspot.com/255693002/diff/60001/content/browser/r...
File content/browser/renderer_host/sandbox_ipc_linux.cc (right):

https://chromiumcodereview.appspot.com/255693002/diff/60001/content/browser/r...
content/browser/renderer_host/sandbox_ipc_linux.cc:169: LOG(ERROR) << "poll:
Error condition in one or more fds.";
Why LOG? This is the normal exit path.

[piman, 2014-04-29 21:28:03]

On Tue, Apr 29, 2014 at 12:45 PM, <jln@chromium.org> wrote:

> The change looks good in general, thanks for taking this on!
>
>
>  > Well, right now, RenderSandboxHostLinux::~RenderSandboxHostLinux joins
>> the
>> > thread, but the thread can't exit... isn't it a problem?
>> >
>> > I'd be more comfortable if we joined the thread before exiting. We can
>> use
>> > the fact that the browser and all the renderers closed their end of the
>> > socket for exiting the thread? poll() should report the fd as having an
>> > error condition.
>> >
>>
>
>  I'll do that.
>>
>
> This looks fine to me, even though in practice I fear it'll be a little
> fragile
> to rely on all the renderers to have closed their end, so we should brace
> ourselves for some crash reports at Join() time :)
>


It actually bugs me a lot that all the renderers share the same pipe.
How do we ensure that requests don't interleave and replies aren't picked
up by the wrong renderer? What am I missing?


>
> The current version could also be made to work by using the fact that poll
> is a
> cancellation point, but we don't use pthread_cancel() in base/ (probably a
> good
> idea).


>
>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc
> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode151
> content/browser/renderer_host/sandbox_ipc_linux.cc:151: const int r =
> HANDLE_EINTR(poll(pfds, 2, -1 /* no timeout */));
> s/2/arraysize(pfds)/
>
> https://chromiumcodereview.appspot.com/255693002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-04-29 21:32:53]

On Tue, Apr 29, 2014 at 2:27 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Tue, Apr 29, 2014 at 12:45 PM, <jln@chromium.org> wrote:
>
>> The change looks good in general, thanks for taking this on!
>>
>>
>>  > Well, right now, RenderSandboxHostLinux::~RenderSandboxHostLinux
>>> joins the
>>> > thread, but the thread can't exit... isn't it a problem?
>>> >
>>> > I'd be more comfortable if we joined the thread before exiting. We can
>>> use
>>> > the fact that the browser and all the renderers closed their end of the
>>> > socket for exiting the thread? poll() should report the fd as having an
>>> > error condition.
>>> >
>>>
>>
>>  I'll do that.
>>>
>>
>> This looks fine to me, even though in practice I fear it'll be a little
>> fragile
>> to rely on all the renderers to have closed their end, so we should brace
>> ourselves for some crash reports at Join() time :)
>>
>
>
> It actually bugs me a lot that all the renderers share the same pipe.
> How do we ensure that requests don't interleave and replies aren't picked
> up by the wrong renderer? What am I missing?
>

It would actually be much better to port all this to use standard chrome
IPC, don't you think? We wouldn't need a new thread, requests could be
handled by the IO thread, possibly delegating to the FILE thread for things
that need filesystem access.

>
>
>>
>> The current version could also be made to work by using the fact that
>> poll is a
>> cancellation point, but we don't use pthread_cancel() in base/ (probably
>> a good
>> idea).
>
>
>>
>>
>> https://chromiumcodereview.appspot.com/255693002/diff/
>> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc
>> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
>>
>> https://chromiumcodereview.appspot.com/255693002/diff/
>> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode151
>> content/browser/renderer_host/sandbox_ipc_linux.cc:151: const int r =
>> HANDLE_EINTR(poll(pfds, 2, -1 /* no timeout */));
>> s/2/arraysize(pfds)/
>>
>> https://chromiumcodereview.appspot.com/255693002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jorge Lucangeli Obes, 2014-04-29 21:40:15]

https://chromiumcodereview.appspot.com/255693002/diff/60001/content/browser/r...
File content/browser/renderer_host/sandbox_ipc_linux.cc (right):

https://chromiumcodereview.appspot.com/255693002/diff/60001/content/browser/r...
content/browser/renderer_host/sandbox_ipc_linux.cc:169: LOG(ERROR) << "poll:
Error condition in one or more fds.";
On 2014/04/29 21:23:51, piman wrote:
> Why LOG? This is the normal exit path.

Mostly to make sure that if something is happening that causes POLLERR *before*
the browser exits, we had a way to know. I can turn it into a VLOG.

[Jorge Lucangeli Obes, 2014-04-29 21:43:49]

On 2014/04/29 21:32:53, piman wrote:
> On Tue, Apr 29, 2014 at 2:27 PM, Antoine Labour <mailto:piman@chromium.org>
wrote:
> 
> >
> >
> >
> > On Tue, Apr 29, 2014 at 12:45 PM, <mailto:jln@chromium.org> wrote:
> >
> >> The change looks good in general, thanks for taking this on!
> >>
> >>
> >>  > Well, right now, RenderSandboxHostLinux::~RenderSandboxHostLinux
> >>> joins the
> >>> > thread, but the thread can't exit... isn't it a problem?
> >>> >
> >>> > I'd be more comfortable if we joined the thread before exiting. We can
> >>> use
> >>> > the fact that the browser and all the renderers closed their end of the
> >>> > socket for exiting the thread? poll() should report the fd as having an
> >>> > error condition.
> >>> >
> >>>
> >>
> >>  I'll do that.
> >>>
> >>
> >> This looks fine to me, even though in practice I fear it'll be a little
> >> fragile
> >> to rely on all the renderers to have closed their end, so we should brace
> >> ourselves for some crash reports at Join() time :)
> >>
> >
> >
> > It actually bugs me a lot that all the renderers share the same pipe.
> > How do we ensure that requests don't interleave and replies aren't picked
> > up by the wrong renderer? What am I missing?
> >
> 
> It would actually be much better to port all this to use standard chrome
> IPC, don't you think? We wouldn't need a new thread, requests could be
> handled by the IO thread, possibly delegating to the FILE thread for things
> that need filesystem access.
> 

I believe the reason we had a separate mechanism in the first place is that we
couldn't use the normal IPC channel because of blocking issues (IIRC the browser
is blocked waiting on the renderer so the renderer needed another way to access
a resource outside of the sandbox). This predates my time in Chrome though so I
could be wrong.

In any case, the reason to do this is to fix an issue with crash reporting
(we're seeing a spurious crash in the SandboxIPC process, instead of the real
browser process crash). The interleaving would be happening already, and this
change looks easier than rewriting this IPC.

However, if we think this is definitely a step in the wrong direction, it would
make sense to reconsider the approach.

> >
> >
> >>
> >> The current version could also be made to work by using the fact that
> >> poll is a
> >> cancellation point, but we don't use pthread_cancel() in base/ (probably
> >> a good
> >> idea).
> >
> >
> >>
> >>
> >> https://chromiumcodereview.appspot.com/255693002/diff/
> >> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc
> >> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
> >>
> >> https://chromiumcodereview.appspot.com/255693002/diff/
> >> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode151
> >> content/browser/renderer_host/sandbox_ipc_linux.cc:151: const int r =
> >> HANDLE_EINTR(poll(pfds, 2, -1 /* no timeout */));
> >> s/2/arraysize(pfds)/
> >>
> >> https://chromiumcodereview.appspot.com/255693002/
> >>
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[jln (very slow on Chromium), 2014-04-29 21:48:41]

On 2014/04/29 21:28:03, piman wrote:
> On Tue, Apr 29, 2014 at 12:45 PM, <mailto:jln@chromium.org> wrote:
> 
> > The change looks good in general, thanks for taking this on!
> >
> >
> >  > Well, right now, RenderSandboxHostLinux::~RenderSandboxHostLinux joins
> >> the
> >> > thread, but the thread can't exit... isn't it a problem?
> >> >
> >> > I'd be more comfortable if we joined the thread before exiting. We can
> >> use
> >> > the fact that the browser and all the renderers closed their end of the
> >> > socket for exiting the thread? poll() should report the fd as having an
> >> > error condition.
> >> >
> >>
> >
> >  I'll do that.
> >>
> >
> > This looks fine to me, even though in practice I fear it'll be a little
> > fragile
> > to rely on all the renderers to have closed their end, so we should brace
> > ourselves for some crash reports at Join() time :)
> >
> 
> 
> It actually bugs me a lot that all the renderers share the same pipe.
> How do we ensure that requests don't interleave and replies aren't picked
> up by the wrong renderer? What am I missing?

Yes, this IPC mechanism is implemented with ancient code, really awful and we
should aim to get rid of it.

The browser side should never write to it, renderers should always attach a new
file descriptor to reply on (we shoudld shutdown(SHUT_WR) the browsers' end if
that's not already done). Also it's using a SOCK_SEQPACKET socket, so message
boundaries should be preserved.

But I think the current CL is a net benefit, the correctness of the IPC
mechanism is a separate issue.

[jln (very slow on Chromium), 2014-04-29 21:49:10]

+cc: mdempsky FYI

[mdempsky, 2014-04-29 21:56:55]

On 2014/04/29 21:28:03, piman wrote:
> It actually bugs me a lot that all the renderers share the same pipe.
> How do we ensure that requests don't interleave and replies aren't picked
> up by the wrong renderer? What am I missing?

The messages from renderer->browser are sent using SOCK_SEQPACKET, which ensures
the message boundaries will be preserved.  Additionally, each of these sandbox
IPC messages include one end of a newly allocated transient socket pair, and the
browser->renderer response is sent over this socket instead of the main one.

So it's sketchy and not a pattern to emulate, but assuming the kernel works, it
should work.

[piman, 2014-04-29 21:57:53]

On Tue, Apr 29, 2014 at 2:43 PM, <jorgelo@chromium.org> wrote:

> On 2014/04/29 21:32:53, piman wrote:
>
>> On Tue, Apr 29, 2014 at 2:27 PM, Antoine Labour <mailto:
>> piman@chromium.org>
>>
> wrote:
>
>
>  >
>> >
>> >
>> > On Tue, Apr 29, 2014 at 12:45 PM, <mailto:jln@chromium.org> wrote:
>> >
>> >> The change looks good in general, thanks for taking this on!
>> >>
>> >>
>> >>  > Well, right now, RenderSandboxHostLinux::~RenderSandboxHostLinux
>> >>> joins the
>> >>> > thread, but the thread can't exit... isn't it a problem?
>> >>> >
>> >>> > I'd be more comfortable if we joined the thread before exiting. We
>> can
>> >>> use
>> >>> > the fact that the browser and all the renderers closed their end of
>> the
>> >>> > socket for exiting the thread? poll() should report the fd as
>> having an
>> >>> > error condition.
>> >>> >
>> >>>
>> >>
>> >>  I'll do that.
>> >>>
>> >>
>> >> This looks fine to me, even though in practice I fear it'll be a little
>> >> fragile
>> >> to rely on all the renderers to have closed their end, so we should
>> brace
>> >> ourselves for some crash reports at Join() time :)
>> >>
>> >
>> >
>> > It actually bugs me a lot that all the renderers share the same pipe.
>> > How do we ensure that requests don't interleave and replies aren't
>> picked
>> > up by the wrong renderer? What am I missing?
>> >
>>
>
>  It would actually be much better to port all this to use standard chrome
>> IPC, don't you think? We wouldn't need a new thread, requests could be
>> handled by the IO thread, possibly delegating to the FILE thread for
>> things
>> that need filesystem access.
>>
>
>
> I believe the reason we had a separate mechanism in the first place is
> that we
> couldn't use the normal IPC channel because of blocking issues (IIRC the
> browser
> is blocked waiting on the renderer so the renderer needed another way to
> access
> a resource outside of the sandbox). This predates my time in Chrome though
> so I
> could be wrong.
>

The browser IO thread can never be blocked on the renderer.


> In any case, the reason to do this is to fix an issue with crash reporting
> (we're seeing a spurious crash in the SandboxIPC process, instead of the
> real
> browser process crash). The interleaving would be happening already, and
> this
> change looks easier than rewriting this IPC.
>
> However, if we think this is definitely a step in the wrong direction, it
> would
> make sense to reconsider the approach.
>
>  >
>> >
>> >>
>> >> The current version could also be made to work by using the fact that
>> >> poll is a
>> >> cancellation point, but we don't use pthread_cancel() in base/
>> (probably
>> >> a good
>> >> idea).
>> >
>> >
>> >>
>> >>
>> >> https://chromiumcodereview.appspot.com/255693002/diff/
>> >> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc
>> >> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
>> >>
>> >> https://chromiumcodereview.appspot.com/255693002/diff/
>> >> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode151
>> >> content/browser/renderer_host/sandbox_ipc_linux.cc:151: const int r =
>> >> HANDLE_EINTR(poll(pfds, 2, -1 /* no timeout */));
>> >> s/2/arraysize(pfds)/
>> >>
>> >> https://chromiumcodereview.appspot.com/255693002/
>> >>
>> >
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://chromiumcodereview.appspot.com/255693002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jorge Lucangeli Obes, 2014-04-29 22:00:38]

On 2014/04/29 21:57:53, piman wrote:
> On Tue, Apr 29, 2014 at 2:43 PM, <mailto:jorgelo@chromium.org> wrote:
> 
> > On 2014/04/29 21:32:53, piman wrote:
> >
> >> On Tue, Apr 29, 2014 at 2:27 PM, Antoine Labour <mailto:
> >> mailto:piman@chromium.org>
> >>
> > wrote:
> >
> >
> >  >
> >> >
> >> >
> >> > On Tue, Apr 29, 2014 at 12:45 PM, <mailto:jln@chromium.org> wrote:
> >> >
> >> >> The change looks good in general, thanks for taking this on!
> >> >>
> >> >>
> >> >>  > Well, right now, RenderSandboxHostLinux::~RenderSandboxHostLinux
> >> >>> joins the
> >> >>> > thread, but the thread can't exit... isn't it a problem?
> >> >>> >
> >> >>> > I'd be more comfortable if we joined the thread before exiting. We
> >> can
> >> >>> use
> >> >>> > the fact that the browser and all the renderers closed their end of
> >> the
> >> >>> > socket for exiting the thread? poll() should report the fd as
> >> having an
> >> >>> > error condition.
> >> >>> >
> >> >>>
> >> >>
> >> >>  I'll do that.
> >> >>>
> >> >>
> >> >> This looks fine to me, even though in practice I fear it'll be a little
> >> >> fragile
> >> >> to rely on all the renderers to have closed their end, so we should
> >> brace
> >> >> ourselves for some crash reports at Join() time :)
> >> >>
> >> >
> >> >
> >> > It actually bugs me a lot that all the renderers share the same pipe.
> >> > How do we ensure that requests don't interleave and replies aren't
> >> picked
> >> > up by the wrong renderer? What am I missing?
> >> >
> >>
> >
> >  It would actually be much better to port all this to use standard chrome
> >> IPC, don't you think? We wouldn't need a new thread, requests could be
> >> handled by the IO thread, possibly delegating to the FILE thread for
> >> things
> >> that need filesystem access.
> >>
> >
> >
> > I believe the reason we had a separate mechanism in the first place is
> > that we
> > couldn't use the normal IPC channel because of blocking issues (IIRC the
> > browser
> > is blocked waiting on the renderer so the renderer needed another way to
> > access
> > a resource outside of the sandbox). This predates my time in Chrome though
> > so I
> > could be wrong.
> >
> 
> The browser IO thread can never be blocked on the renderer.
> 

That's good to know. However, I expect there was a reason to have this as a
separate *process* in the first place, which is why my intent was to keep the
mechanism as-is, but get rid of the extra process that's complicating crash
analysis on Linux and CrOS.

> 
> > In any case, the reason to do this is to fix an issue with crash reporting
> > (we're seeing a spurious crash in the SandboxIPC process, instead of the
> > real
> > browser process crash). The interleaving would be happening already, and
> > this
> > change looks easier than rewriting this IPC.
> >
> > However, if we think this is definitely a step in the wrong direction, it
> > would
> > make sense to reconsider the approach.
> >
> >  >
> >> >
> >> >>
> >> >> The current version could also be made to work by using the fact that
> >> >> poll is a
> >> >> cancellation point, but we don't use pthread_cancel() in base/
> >> (probably
> >> >> a good
> >> >> idea).
> >> >
> >> >
> >> >>
> >> >>
> >> >> https://chromiumcodereview.appspot.com/255693002/diff/
> >> >> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc
> >> >> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
> >> >>
> >> >> https://chromiumcodereview.appspot.com/255693002/diff/
> >> >> 40001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode151
> >> >> content/browser/renderer_host/sandbox_ipc_linux.cc:151: const int r =
> >> >> HANDLE_EINTR(poll(pfds, 2, -1 /* no timeout */));
> >> >> s/2/arraysize(pfds)/
> >> >>
> >> >> https://chromiumcodereview.appspot.com/255693002/
> >> >>
> >> >
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> >
> > https://chromiumcodereview.appspot.com/255693002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[mdempsky, 2014-04-29 22:02:06]

On 2014/04/29 21:56:55, mdempsky wrote:
> The messages from renderer->browser are sent using SOCK_SEQPACKET, which
ensures
> the message boundaries will be preserved.  Additionally, each of these sandbox
> IPC messages include one end of a newly allocated transient socket pair, and
the
> browser->renderer response is sent over this socket instead of the main one.

[Oops, this is basically what jln already said.]

[piman, 2014-04-29 22:03:00]

On Tue, Apr 29, 2014 at 2:40 PM, <jorgelo@chromium.org> wrote:

>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 60001/content/browser/renderer_host/sandbox_ipc_linux.cc
> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 60001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode169
> content/browser/renderer_host/sandbox_ipc_linux.cc:169: LOG(ERROR) <<
> "poll: Error condition in one or more fds.";
> On 2014/04/29 21:23:51, piman wrote:
>
>> Why LOG? This is the normal exit path.
>>
>
> Mostly to make sure that if something is happening that causes POLLERR
> *before* the browser exits, we had a way to know. I can turn it into a
> VLOG.'
>

Another way would be for the browser to send a message (through its
"renderer" end) to terminate the loop explicitly. That way we can
discriminate regular shutdown from error conditions.


>
> https://chromiumcodereview.appspot.com/255693002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jln (very slow on Chromium), 2014-04-29 22:05:00]

On 2014/04/29 22:00:38, Jorge Lucangeli Obes wrote:
> On 2014/04/29 21:57:53, piman wrote:
> > On Tue, Apr 29, 2014 at 2:43 PM, <mailto:jorgelo@chromium.org> wrote:
> > 
> > > On 2014/04/29 21:32:53, piman wrote:
> > >
> > >> On Tue, Apr 29, 2014 at 2:27 PM, Antoine Labour <mailto:
> > >> mailto:piman@chromium.org>
> > >>
> > > wrote:
> > >
> > >
> > >  >
> > >> >
> > >> >
> > >> > On Tue, Apr 29, 2014 at 12:45 PM, <mailto:jln@chromium.org> wrote:
> > >> >
> > >> >> The change looks good in general, thanks for taking this on!
> > >> >>
> > >> >>
> > >> >>  > Well, right now, RenderSandboxHostLinux::~RenderSandboxHostLinux
> > >> >>> joins the
> > >> >>> > thread, but the thread can't exit... isn't it a problem?
> > >> >>> >
> > >> >>> > I'd be more comfortable if we joined the thread before exiting. We
> > >> can
> > >> >>> use
> > >> >>> > the fact that the browser and all the renderers closed their end of
> > >> the
> > >> >>> > socket for exiting the thread? poll() should report the fd as
> > >> having an
> > >> >>> > error condition.
> > >> >>> >
> > >> >>>
> > >> >>
> > >> >>  I'll do that.
> > >> >>>
> > >> >>
> > >> >> This looks fine to me, even though in practice I fear it'll be a
little
> > >> >> fragile
> > >> >> to rely on all the renderers to have closed their end, so we should
> > >> brace
> > >> >> ourselves for some crash reports at Join() time :)
> > >> >>
> > >> >
> > >> >
> > >> > It actually bugs me a lot that all the renderers share the same pipe.
> > >> > How do we ensure that requests don't interleave and replies aren't
> > >> picked
> > >> > up by the wrong renderer? What am I missing?
> > >> >
> > >>
> > >
> > >  It would actually be much better to port all this to use standard chrome
> > >> IPC, don't you think? We wouldn't need a new thread, requests could be
> > >> handled by the IO thread, possibly delegating to the FILE thread for
> > >> things
> > >> that need filesystem access.
> > >>
> > >
> > >
> > > I believe the reason we had a separate mechanism in the first place is
> > > that we
> > > couldn't use the normal IPC channel because of blocking issues (IIRC the
> > > browser
> > > is blocked waiting on the renderer so the renderer needed another way to
> > > access
> > > a resource outside of the sandbox). This predates my time in Chrome though
> > > so I
> > > could be wrong.
> > >
> > 
> > The browser IO thread can never be blocked on the renderer.
> > 
> 
> That's good to know. However, I expect there was a reason to have this as a
> separate *process* in the first place, which is why my intent was to keep the
> mechanism as-is, but get rid of the extra process that's complicating crash
> analysis on Linux and CrOS.

The main reason was that this IPC mechanism is used by the horrible "weak
symbols" hack that allows to proxy LIBC functions over the IPC channel. This is
in use even before the real renderers IPC channel is initialized.

I dream of getting rid if this. This very channel created a serious
vulnerability in BMM NaCl: crbug.com/367263. It would probably require keeping
this as-is for now (but move it to a thread like this CL does), but then close
it in renderers after some initialization happend. This is not going to be easy
:(

[Jorge Lucangeli Obes, 2014-04-29 22:06:49]

On 2014/04/29 22:03:00, piman wrote:
> On Tue, Apr 29, 2014 at 2:40 PM, <mailto:jorgelo@chromium.org> wrote:
> 
> >
> > https://chromiumcodereview.appspot.com/255693002/diff/
> > 60001/content/browser/renderer_host/sandbox_ipc_linux.cc
> > File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
> >
> > https://chromiumcodereview.appspot.com/255693002/diff/
> > 60001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode169
> > content/browser/renderer_host/sandbox_ipc_linux.cc:169: LOG(ERROR) <<
> > "poll: Error condition in one or more fds.";
> > On 2014/04/29 21:23:51, piman wrote:
> >
> >> Why LOG? This is the normal exit path.
> >>
> >
> > Mostly to make sure that if something is happening that causes POLLERR
> > *before* the browser exits, we had a way to know. I can turn it into a
> > VLOG.'
> >
> 
> Another way would be for the browser to send a message (through its
> "renderer" end) to terminate the loop explicitly. That way we can
> discriminate regular shutdown from error conditions.
> 

I'm happy to do that if we are OK with this CL as a way to fix crbug.com/322185
and as a step in the right direction to get rid of this crazy IPC.

(Step in the right direction because it will tell us if there's any unforeseen
issues with running this code as a thread instead of as a separate process.)

> 
> >
> > https://chromiumcodereview.appspot.com/255693002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[jln (very slow on Chromium), 2014-04-29 22:08:47]

https://chromiumcodereview.appspot.com/255693002/diff/60001/content/browser/r...
File content/browser/renderer_host/sandbox_ipc_linux.cc (right):

https://chromiumcodereview.appspot.com/255693002/diff/60001/content/browser/r...
content/browser/renderer_host/sandbox_ipc_linux.cc:137: 
Do you want to shutdown SHUT_WR browser_socket to make it clear that the browser
should never "reply" there?

This could be done in a separate CL, where we should also SHUT_RD the other end
of that socketpair in the Zygote.

[Jorge Lucangeli Obes, 2014-04-29 22:09:48]

On 2014/04/29 22:08:47, jln wrote:
>
https://chromiumcodereview.appspot.com/255693002/diff/60001/content/browser/r...
> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
> 
>
https://chromiumcodereview.appspot.com/255693002/diff/60001/content/browser/r...
> content/browser/renderer_host/sandbox_ipc_linux.cc:137: 
> Do you want to shutdown SHUT_WR browser_socket to make it clear that the
browser
> should never "reply" there?
> 
> This could be done in a separate CL, where we should also SHUT_RD the other
end
> of that socketpair in the Zygote.

I can do that too. Does it make sense to do it before landing this?

[jln (very slow on Chromium), 2014-04-29 22:11:55]

One example of why getting rid of this IPC channel is, sadly, so hard:

https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...

[jln (very slow on Chromium), 2014-04-29 22:13:12]

On 2014/04/29 22:09:48, Jorge Lucangeli Obes wrote:

> I can do that too. Does it make sense to do it before landing this?

Yes, it would be nice to do it first rather than have that change depend on a CL
that has a higher chance of being reverted (this one).

[piman, 2014-04-29 22:16:03]

On Tue, Apr 29, 2014 at 3:06 PM, <jorgelo@chromium.org> wrote:

> On 2014/04/29 22:03:00, piman wrote:
>
>  On Tue, Apr 29, 2014 at 2:40 PM, <mailto:jorgelo@chromium.org> wrote:
>>
>
>  >
>> > https://chromiumcodereview.appspot.com/255693002/diff/
>> > 60001/content/browser/renderer_host/sandbox_ipc_linux.cc
>> > File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
>> >
>> > https://chromiumcodereview.appspot.com/255693002/diff/
>> > 60001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode169
>> > content/browser/renderer_host/sandbox_ipc_linux.cc:169: LOG(ERROR) <<
>> > "poll: Error condition in one or more fds.";
>> > On 2014/04/29 21:23:51, piman wrote:
>> >
>> >> Why LOG? This is the normal exit path.
>> >>
>> >
>> > Mostly to make sure that if something is happening that causes POLLERR
>> > *before* the browser exits, we had a way to know. I can turn it into a
>> > VLOG.'
>> >
>>
>
>  Another way would be for the browser to send a message (through its
>> "renderer" end) to terminate the loop explicitly. That way we can
>> discriminate regular shutdown from error conditions.
>>
>
>
> I'm happy to do that if we are OK with this CL as a way to fix
> crbug.com/322185
> and as a step in the right direction to get rid of this crazy IPC.
>
> (Step in the right direction because it will tell us if there's any
> unforeseen
> issues with running this code as a thread instead of as a separate
> process.)


Sure, let's do this.

>
>
>
>  >
>> > https://chromiumcodereview.appspot.com/255693002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://chromiumcodereview.appspot.com/255693002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jln (very slow on Chromium), 2014-04-29 22:19:26]

On 2014/04/29 22:11:55, jln wrote:
> One example of why getting rid of this IPC channel is, sadly, so hard:
> 
>
https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...

+cc: mark@ FYI

What I meant to point to is Mark's comment on line 20 here:
https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...

There is third-party code that calls localtime() and needs to get hooked to
perform an IPC. That's why getting rid of this low-level IPC channel is so hard.
It's doable though, once the renderer's channel is up, we could notify the low
level IPC mechanism to use a new wrapper that would then rely on the high level
channel.

[Jorge Lucangeli Obes, 2014-04-29 22:22:35]

On 2014/04/29 22:16:03, piman wrote:
> On Tue, Apr 29, 2014 at 3:06 PM, <mailto:jorgelo@chromium.org> wrote:
> 
> > On 2014/04/29 22:03:00, piman wrote:
> >
> >  On Tue, Apr 29, 2014 at 2:40 PM, <mailto:jorgelo@chromium.org> wrote:
> >>
> >
> >  >
> >> > https://chromiumcodereview.appspot.com/255693002/diff/
> >> > 60001/content/browser/renderer_host/sandbox_ipc_linux.cc
> >> > File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
> >> >
> >> > https://chromiumcodereview.appspot.com/255693002/diff/
> >> > 60001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode169
> >> > content/browser/renderer_host/sandbox_ipc_linux.cc:169: LOG(ERROR) <<
> >> > "poll: Error condition in one or more fds.";
> >> > On 2014/04/29 21:23:51, piman wrote:
> >> >
> >> >> Why LOG? This is the normal exit path.
> >> >>
> >> >
> >> > Mostly to make sure that if something is happening that causes POLLERR
> >> > *before* the browser exits, we had a way to know. I can turn it into a
> >> > VLOG.'
> >> >
> >>
> >
> >  Another way would be for the browser to send a message (through its
> >> "renderer" end) to terminate the loop explicitly. That way we can
> >> discriminate regular shutdown from error conditions.
> >>
> >
> >
> > I'm happy to do that if we are OK with this CL as a way to fix
> > crbug.com/322185
> > and as a step in the right direction to get rid of this crazy IPC.
> >
> > (Step in the right direction because it will tell us if there's any
> > unforeseen
> > issues with running this code as a thread instead of as a separate
> > process.)
> 
> 
> Sure, let's do this.
> 

Alright, I will prepare a CL to disallow using the ends of the channel
incorrectly, and then I will update this CL (at least changing the LOG into
VLOG, but I'll look into writing to the channel to exit the loop).

> >
> >
> >
> >  >
> >> > https://chromiumcodereview.appspot.com/255693002/
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> > https://chromiumcodereview.appspot.com/255693002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Jorge Lucangeli Obes, 2014-04-30 17:58:19]

Rebased on top of the shutdown(2) change. Changed LOG(ERROR) to VLOG. I've been
browsing with this build all morning and haven't seen any strangeness. If we're
still worried about POLLERR happening outside of browser shutdown, I'll look
into having the browser signal the thread via the socket.

Thanks for taking a look!

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
File content/browser/renderer_host/render_sandbox_host_linux.cc (left):

https://chromiumcodereview.appspot.com/255693002/diff/40001/content/browser/r...
content/browser/renderer_host/render_sandbox_host_linux.cc:52: #endif  //
!defined(THREAD_SANITIZER)
On 2014/04/29 00:25:46, piman wrote:
> Has the zygote been launched at this point?

I don't think that would be an issue. The browser keeps the fd that it will pass
to the zygote.

https://chromiumcodereview.appspot.com/255693002/diff/60001/content/browser/r...
File content/browser/renderer_host/sandbox_ipc_linux.cc (right):

https://chromiumcodereview.appspot.com/255693002/diff/60001/content/browser/r...
content/browser/renderer_host/sandbox_ipc_linux.cc:169: LOG(ERROR) << "poll:
Error condition in one or more fds.";
On 2014/04/29 21:40:15, Jorge Lucangeli Obes wrote:
> On 2014/04/29 21:23:51, piman wrote:
> > Why LOG? This is the normal exit path.
> 
> Mostly to make sure that if something is happening that causes POLLERR
*before*
> the browser exits, we had a way to know. I can turn it into a VLOG.

Done.

[jln (very slow on Chromium), 2014-04-30 19:32:03]

lgtm (in truth, this looks horrible, but this CL is a nice improvement :)

[jln (very slow on Chromium), 2014-04-30 20:25:44]

(and again I forgot to send the comments with my reply).

https://chromiumcodereview.appspot.com/255693002/diff/100001/content/browser/...
File content/browser/renderer_host/sandbox_ipc_linux.cc (right):

https://chromiumcodereview.appspot.com/255693002/diff/100001/content/browser/...
content/browser/renderer_host/sandbox_ipc_linux.cc:168: if (pfds[0].revents &
POLLERR) {
I think you are guaranteed (POLLHUP | POLLERR) here (connected unix socket).
POLLERR without POLLHUP would be another error, in case you want to be paranoid
about checking.

Feel free to keep as-is, this should work.

[piman, 2014-04-30 22:26:39]

On Wed, Apr 30, 2014 at 10:58 AM, <jorgelo@chromium.org> wrote:

> Rebased on top of the shutdown(2) change. Changed LOG(ERROR) to VLOG. I've
> been
> browsing with this build all morning and haven't seen any strangeness. If
> we're
> still worried about POLLERR happening outside of browser shutdown, I'll
> look
> into having the browser signal the thread via the socket.
>
>
> Thanks for taking a look!
>
>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 40001/content/browser/renderer_host/render_sandbox_host_linux.cc
> File content/browser/renderer_host/render_sandbox_host_linux.cc (left):
>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 40001/content/browser/renderer_host/render_sandbox_host_linux.cc#oldcode52
>  content/browser/renderer_host/render_sandbox_host_linux.cc:52: #endif
> // !defined(THREAD_SANITIZER)
> On 2014/04/29 00:25:46, piman wrote:
>
>> Has the zygote been launched at this point?
>>
>
> I don't think that would be an issue. The browser keeps the fd that it
> will pass to the zygote.


The issue is that we can't create the zygote if we already have threads,
right?
Just wondering about the ordering.


>
>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 60001/content/browser/renderer_host/sandbox_ipc_linux.cc
> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
>
> https://chromiumcodereview.appspot.com/255693002/diff/
> 60001/content/browser/renderer_host/sandbox_ipc_linux.cc#newcode169
>
> content/browser/renderer_host/sandbox_ipc_linux.cc:169: LOG(ERROR) <<
> "poll: Error condition in one or more fds.";
> On 2014/04/29 21:40:15, Jorge Lucangeli Obes wrote:
>
>> On 2014/04/29 21:23:51, piman wrote:
>> > Why LOG? This is the normal exit path.
>>
>
>  Mostly to make sure that if something is happening that causes POLLERR
>>
> *before*
>
>> the browser exits, we had a way to know. I can turn it into a VLOG.
>>
>
> Done.
>
> https://chromiumcodereview.appspot.com/255693002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-04-30 22:28:03]

(lgtm if there's no zygote issue)

[jln (very slow on Chromium), 2014-04-30 22:29:51]

On 2014/04/30 22:26:39, piman wrote:

> The issue is that we can't create the zygote if we already have threads,
> right?
> Just wondering about the ordering.

No, no problem for the Zygote. Fork + execve is fine. The issue with fork() +
threads is that thread don't exist in the child process. So all kind of things
can go wrong, including locks can be held forever. But execve() cleans all of
this.

base::LaunchProcess() is "somewhat" careful at not doing anything that could
hold locks (including memory allocations) between fork and execve (even though
it's actually quite broken).

Julien

[Jorge Lucangeli Obes, 2014-05-01 01:02:33]

On 2014/04/30 22:29:51, jln wrote:
> On 2014/04/30 22:26:39, piman wrote:
> 
> > The issue is that we can't create the zygote if we already have threads,
> > right?
> > Just wondering about the ordering.
> 
> No, no problem for the Zygote. Fork + execve is fine. The issue with fork() +
> threads is that thread don't exist in the child process. So all kind of things
> can go wrong, including locks can be held forever. But execve() cleans all of
> this.
> 
> base::LaunchProcess() is "somewhat" careful at not doing anything that could
> hold locks (including memory allocations) between fork and execve (even though
> it's actually quite broken).
> 

FYI, we actually *need* to check for POLLHUP to join the thread successfully.
close() on the other side of the socketpair returns POLLHUP in poll(). I've
updated a new PS to add that.

And now trybots pass (or fail to apply the patch, but never fail tests.)

> Julien

[Jorge Lucangeli Obes, 2014-05-01 04:07:01]

The CQ bit was checked by jorgelo@chromium.org

[commit-bot: I haz the power, 2014-05-01 04:07:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jorgelo@chromium.org/255693002/120001

[commit-bot: I haz the power, 2014-05-01 04:07:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 04:07:32]

Failed to apply patch for content/browser/renderer_host/sandbox_ipc_linux.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/renderer_host/sandbox_ipc_linux.cc
  Hunk #1 succeeded at 129 (offset 2 lines).
  Hunk #2 succeeded at 141 (offset 2 lines).
  Hunk #3 FAILED at 163.
  Hunk #4 succeeded at 220 (offset -3 lines).
  Hunk #5 FAILED at 233.
  Hunk #6 FAILED at 272.
  Hunk #7 FAILED at 300.
  Hunk #8 FAILED at 330.
  Hunk #9 FAILED at 358.
  Hunk #10 FAILED at 390.
  Hunk #11 FAILED at 424.
  Hunk #12 FAILED at 443.
  Hunk #13 FAILED at 602.
  Hunk #14 succeeded at 643 (offset 6 lines).
  10 out of 14 hunks FAILED -- saving rejects to file
content/browser/renderer_host/sandbox_ipc_linux.cc.rej

Patch:       content/browser/renderer_host/sandbox_ipc_linux.cc
Index: content/browser/renderer_host/sandbox_ipc_linux.cc
diff --git a/content/browser/renderer_host/sandbox_ipc_linux.cc
b/content/browser/renderer_host/sandbox_ipc_linux.cc
index
ad1747e01736ec41d009c825a9562f8b3b8708d0..ee95f1b3c81527248c3c93a5f27f93f7ab18ad6b
100644
--- a/content/browser/renderer_host/sandbox_ipc_linux.cc
+++ b/content/browser/renderer_host/sandbox_ipc_linux.cc
@@ -127,10 +127,9 @@ static void MSCharSetToFontconfig(FcLangSet* langset,
unsigned fdwCharSet) {
 
 namespace content {
 
-SandboxIPCProcess::SandboxIPCProcess(int lifeline_fd,
-                                     int browser_socket,
+SandboxIPCHandler::SandboxIPCHandler(int browser_socket,
                                      std::string sandbox_cmd)
-    : lifeline_fd_(lifeline_fd), browser_socket_(browser_socket) {
+    : browser_socket_(browser_socket) {
   if (!sandbox_cmd.empty()) {
     sandbox_cmd_.push_back(sandbox_cmd);
     sandbox_cmd_.push_back(base::kFindInodeSwitch);
@@ -140,33 +139,23 @@ SandboxIPCProcess::SandboxIPCProcess(int lifeline_fd,
   // positioning, so we pass the current setting through to WebKit.
   WebFontInfo::setSubpixelPositioning(
       gfx::GetDefaultWebkitSubpixelPositioning());
-
-  CommandLine& command_line = *CommandLine::ForCurrentProcess();
-  command_line.AppendSwitchASCII(switches::kProcessType,
-                                 switches::kSandboxIPCProcess);
-
-  // Update the process title. The argv was already cached by the call to
-  // SetProcessTitleFromCommandLine in content_main_runner.cc, so we can pass
-  // NULL here (we don't have the original argv at this point).
-  SetProcessTitleFromCommandLine(NULL);
 }
 
-void SandboxIPCProcess::Run() {
-  struct pollfd pfds[2];
-  pfds[0].fd = lifeline_fd_;
+void SandboxIPCHandler::Run() {
+  struct pollfd pfds[1];
+  pfds[0].fd = browser_socket_;
   pfds[0].events = POLLIN;
-  pfds[1].fd = browser_socket_;
-  pfds[1].events = POLLIN;
 
   int failed_polls = 0;
   for (;;) {
-    const int r = HANDLE_EINTR(poll(pfds, 2, -1 /* no timeout */));
+    const int r =
+        HANDLE_EINTR(poll(pfds, arraysize(pfds), -1 /* no timeout */));
     // '0' is not a possible return value with no timeout.
     DCHECK_NE(0, r);
     if (r < 0) {
       PLOG(WARNING) << "poll";
       if (failed_polls++ == 3) {
-        LOG(FATAL) << "poll(2) failing. RenderSandboxHostLinux aborting.";
+        LOG(FATAL) << "poll(2) failing. SandboxIPCHandler aborting.";
         return;
       }
       continue;
@@ -174,18 +163,20 @@ void SandboxIPCProcess::Run() {
 
     failed_polls = 0;
 
-    if (pfds[0].revents) {
-      // our parent died so we should too.
-      _exit(0);
+    // If poll(2) reports an error condition in the fd,
+    // we assume the zygote is gone and we return.
+    if (pfds[0].revents & (POLLERR | POLLHUP)) {
+      VLOG(1) << "SandboxIPCHandler stopping.";
+      return;
     }
 
-    if (pfds[1].revents) {
+    if (pfds[0].revents & POLLIN) {
       HandleRequestFromRenderer(browser_socket_);
     }
   }
 }
 
-void SandboxIPCProcess::HandleRequestFromRenderer(int fd) {
+void SandboxIPCHandler::HandleRequestFromRenderer(int fd) {
   std::vector<int> fds;
 
   // A FontConfigIPC::METHOD_MATCH message could be kMaxFontFamilyLength
@@ -234,7 +225,7 @@ error:
   }
 }
 
-int SandboxIPCProcess::FindOrAddPath(const SkString& path) {
+int SandboxIPCHandler::FindOrAddPath(const SkString& path) {
   int count = paths_.count();
   for (int i = 0; i < count; ++i) {
     if (path == *paths_[i])
@@ -244,7 +235,7 @@ int SandboxIPCProcess::FindOrAddPath(const SkString& path) {
   return count;
 }
 
-void SandboxIPCProcess::HandleFontMatchRequest(int fd,
+void SandboxIPCHandler::HandleFontMatchRequest(int fd,
                                                const Pickle& pickle,
                                                PickleIterator iter,
                                                std::vector<int>& fds) {
@@ -283,7 +274,7 @@ void SandboxIPCProcess::HandleFontMatchRequest(int fd,
   SendRendererReply(fds, reply, -1);
 }
 
-void SandboxIPCProcess::HandleFontOpenRequest(int fd,
+void SandboxIPCHandler::HandleFontOpenRequest(int fd,
                                               const Pickle& pickle,
                                               PickleIterator iter,
                                               std::vector<int>& fds) {
@@ -311,12 +302,12 @@ void SandboxIPCProcess::HandleFontOpenRequest(int fd,
   }
 }
 
-void SandboxIPCProcess::HandleGetFontFamilyForChar(int fd,
+void SandboxIPCHandler::HandleGetFontFamilyForChar(int fd,
                                                    const Pickle& pickle,
                                                    PickleIterator iter,
                                                    std::vector<int>& fds) {
   // The other side of this call is
-  // chrome/renderer/renderer_sandbox_support_linux.cc
+  // content/common/child_process_sandbox_support_impl_linux.cc
 
   EnsureWebKitInitialized();
   WebUChar32 c;
@@ -341,7 +332,7 @@ void SandboxIPCProcess::HandleGetFontFamilyForChar(int fd,
   SendRendererReply(fds, reply, -1);
 }
 
-void SandboxIPCProcess::HandleGetStyleForStrike(int fd,
+void SandboxIPCHandler::HandleGetStyleForStrike(int fd,
                                                 const Pickle& pickle,
                                                 PickleIterator iter,
                                                 std::vector<int>& fds) {
@@ -369,7 +360,7 @@ void SandboxIPCProcess::HandleGetStyleForStrike(int fd,
   SendRendererReply(fds, reply, -1);
 }
 
-void SandboxIPCProcess::HandleLocaltime(int fd,
+void SandboxIPCHandler::HandleLocaltime(int fd,
                                         const Pickle& pickle,
                                         PickleIterator iter,
                                         std::vector<int>& fds) {
@@ -401,7 +392,7 @@ void SandboxIPCProcess::HandleLocaltime(int fd,
   SendRendererReply(fds, reply, -1);
 }
 
-void SandboxIPCProcess::HandleGetChildWithInode(int fd,
+void SandboxIPCHandler::HandleGetChildWithInode(int fd,
                                                 const Pickle& pickle,
                                                 PickleIterator iter,
                                                 std::vector<int>& fds) {
@@ -435,7 +426,7 @@ void SandboxIPCProcess::HandleGetChildWithInode(int fd,
   SendRendererReply(fds, reply, -1);
 }
 
-void SandboxIPCProcess::HandleMakeSharedMemorySegment(int fd,
+void SandboxIPCHandler::HandleMakeSharedMemorySegment(int fd,
                                                       const Pickle& pickle,
                                                       PickleIterator iter,
                                                       std::vector<int>& fds) {
@@ -454,7 +445,7 @@ void SandboxIPCProcess::HandleMakeSharedMemorySegment(int
fd,
   SendRendererReply(fds, reply, shm_fd);
 }
 
-void SandboxIPCProcess::HandleMatchWithFallback(int fd,
+void SandboxIPCHandler::HandleMatchWithFallback(int fd,
                                                 const Pickle& pickle,
                                                 PickleIterator iter,
                                                 std::vector<int>& fds) {
@@ -613,7 +604,7 @@ void SandboxIPCProcess::HandleMatchWithFallback(int fd,
   }
 }
 
-void SandboxIPCProcess::SendRendererReply(const std::vector<int>& fds,
+void SandboxIPCHandler::SendRendererReply(const std::vector<int>& fds,
                                           const Pickle& reply,
                                           int reply_fd) {
   struct msghdr msg;
@@ -648,13 +639,13 @@ void SandboxIPCProcess::SendRendererReply(const
std::vector<int>& fds,
     PLOG(ERROR) << "sendmsg";
 }
 
-SandboxIPCProcess::~SandboxIPCProcess() {
+SandboxIPCHandler::~SandboxIPCHandler() {
   paths_.deleteAll();
   if (webkit_platform_support_)
     blink::shutdownWithoutV8();
 }
 
-void SandboxIPCProcess::EnsureWebKitInitialized() {
+void SandboxIPCHandler::EnsureWebKitInitialized() {
   if (webkit_platform_support_)
     return;
   webkit_platform_support_.reset(new BlinkPlatformImpl);

[Jorge Lucangeli Obes, 2014-05-01 05:33:11]

The CQ bit was checked by jorgelo@chromium.org

[commit-bot: I haz the power, 2014-05-01 05:33:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jorgelo@chromium.org/255693002/140001

[commit-bot: I haz the power, 2014-05-01 05:35:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 05:35:28]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium

[jln (very slow on Chromium), 2014-05-01 08:31:19]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-05-01 08:31:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jorgelo@chromium.org/255693002/140001

[commit-bot: I haz the power, 2014-05-01 08:35:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 08:35:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Jorge Lucangeli Obes, 2014-05-01 16:45:08]

Lei: could you review chrome/browser/memory_details.cc for OWNERS? I'm making
the SandboxIPCProcess a thread so there's no need to report its memory anymore.
Thanks!

[Lei Zhang, 2014-05-01 18:06:03]

chrome/ lgtm

[Jorge Lucangeli Obes, 2014-05-01 18:11:15]

The CQ bit was checked by jorgelo@chromium.org

[commit-bot: I haz the power, 2014-05-01 18:11:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jorgelo@chromium.org/255693002/140001

[commit-bot: I haz the power, 2014-05-01 18:15:48]

Change committed as 267583

[Lei Zhang, 2014-05-01 20:38:19]

A revert of this CL has been created in
https://codereview.chromium.org/261783008/ by thestig@chromium.org.

The reason for reverting is: TSANv2 bot reports races on many many tests..

[Jorge Lucangeli Obes, 2014-05-01 20:40:09]

On 2014/05/01 20:38:19, Lei Zhang wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/261783008/ by mailto:thestig@chromium.org.
> 
> The reason for reverting is: TSANv2 bot reports races on many many tests..

These seem to be races in Skia. Will take a look.