Shorten SingletonSocket filename, makes socket length limit harder to hit.

Shorten SingletonSocket filename, makes socket length limit harder to hit.

(This only affects the name of the actual socket file which is located in a
temp directory. The SingletonSocket symlink in the user-data-dir keeps the full
length name for comprehensibility.)

BUG=698759

[mattm, 2017-03-15 21:14:00]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 21:14:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-03-15 21:14:45]

Description was changed from

==========
Shorten SingletonSocket filename, makes socket length limit slightly harder to
hit.

(This only affects the name of the actual socket file which is located in a
temp directory. The SingletonSocket symlink in the user-data-dir keeps the full
length name for comprehensibility.)

BUG=698759
==========

to

==========
Shorten SingletonSocket filename, makes socket length limit harder to hit.

(This only affects the name of the actual socket file which is located in a
temp directory. The SingletonSocket symlink in the user-data-dir keeps the full
length name for comprehensibility.)

BUG=698759
==========

[mattm, 2017-03-15 21:17:40]

mattm@chromium.org changed reviewers:
+ thakis@chromium.org

[commit-bot: I haz the power, 2017-03-15 22:33:59]

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

[commit-bot: I haz the power, 2017-03-15 22:34:00]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-03-17 17:00:56]

ping (should be a simple review)

Thanks!

[Nico, 2017-03-17 19:31:49]

Sorry!

The question below possibly suggests that I'm not qualified to review this, up
to you to find someone more qualified if you want a real review :-)

https://codereview.chromium.org/2751123003/diff/1/chrome/browser/process_sing...
File chrome/browser/process_singleton_posix.cc (right):

https://codereview.chromium.org/2751123003/diff/1/chrome/browser/process_sing...
chrome/browser/process_singleton_posix.cc:722: socket_path_ =
user_data_dir.Append(chrome::kSingletonSocketFilename);
Why does this one keep using the old path? Why not just change the contents of
kSingletonSocketFilename? Having two paths for this seems confusing, and
apparently no tests need updating, so there aren't any tests that explain why
one place uses one name and another the other.

[mattm, 2017-03-17 22:20:19]

https://codereview.chromium.org/2751123003/diff/1/chrome/browser/process_sing...
File chrome/browser/process_singleton_posix.cc (right):

https://codereview.chromium.org/2751123003/diff/1/chrome/browser/process_sing...
chrome/browser/process_singleton_posix.cc:722: socket_path_ =
user_data_dir.Append(chrome::kSingletonSocketFilename);
On 2017/03/17 19:31:49, Nico wrote:
> Why does this one keep using the old path? Why not just change the contents of
> kSingletonSocketFilename? Having two paths for this seems confusing, and
> apparently no tests need updating, so there aren't any tests that explain why
> one place uses one name and another the other.

(I did mention in the CL description, let me know if that isn't sufficient.)
There are two places the names come up since one is the name of the symlink in
the userdatadir, and the other is the actual name of the socket file.
I wanted to leave the name of the file in the userdata dir as the longer version
just so it's less confusing. Only the length of the actual socket file's name
matters. Does that seem reasonable, or do yo u think having them both be the
short version would be cleaner?

Re: tests, since it's currently a CHECK on the file length it's not easy to
test.. but maybe it would be good to have a more user-friendly  error anyway? It
seems like on mac even if you run from the console you don't see the CHECK
failure message..

[Nico, 2017-03-17 22:43:00]

Ok, I read the bug. This seems like it doesn't help all that much and seems
pretty hacky, and it looks like it was introduced by iannucci's TMPDIR thing.
How about we do this instead:

- use CHROMIUM_TEMPDIR_FOR_BOTS if it's set
- use NSTemporaryDirectory() if not as before
- set CHROMIUM_TEMPDIR_FOR_BOTS on bots

That way, we don't change behavior in prod and probably dodge a whole list of
issues, and bots still get to override this dir.

[mattm, 2017-03-17 22:53:49]

On 2017/03/17 22:43:00, Nico wrote:
> Ok, I read the bug. This seems like it doesn't help all that much and seems
> pretty hacky, and it looks like it was introduced by iannucci's TMPDIR thing.
> How about we do this instead:
> 
> - use CHROMIUM_TEMPDIR_FOR_BOTS if it's set
> - use NSTemporaryDirectory() if not as before
> - set CHROMIUM_TEMPDIR_FOR_BOTS on bots
> 
> That way, we don't change behavior in prod and probably dodge a whole list of
> issues, and bots still get to override this dir.

That sounds look a good plan for that bug, if mac people are fine with TMPDIR
not being honored.

We still do honor TMPDIR on linux, so I think this could be generally useful
anyway. Though the socket length limit hasn't caused too much noise before, so
I'm not too set on this CL if we fix the mac issue a different way.

[iannucci, 2017-03-24 00:28:52]

On 2017/03/17 22:53:49, mattm wrote:
> On 2017/03/17 22:43:00, Nico wrote:
> > Ok, I read the bug. This seems like it doesn't help all that much and seems
> > pretty hacky, and it looks like it was introduced by iannucci's TMPDIR
thing.
> > How about we do this instead:
> > 
> > - use CHROMIUM_TEMPDIR_FOR_BOTS if it's set
> > - use NSTemporaryDirectory() if not as before
> > - set CHROMIUM_TEMPDIR_FOR_BOTS on bots
> > 
> > That way, we don't change behavior in prod and probably dodge a whole list
of
> > issues, and bots still get to override this dir.
> 
> That sounds look a good plan for that bug, if mac people are fine with TMPDIR
> not being honored.
> 
> We still do honor TMPDIR on linux, so I think this could be generally useful
> anyway. Though the socket length limit hasn't caused too much noise before, so
> I'm not too set on this CL if we fix the mac issue a different way.

FWIW, we could definitely set a different envvar too; the only reason I used
TMPDIR
was because we do honor it on linux and I thought consistency might be good.

Should I make that change?

[iannucci, 2017-03-24 00:42:40]

On 2017/03/24 00:28:52, iannucci wrote:
> On 2017/03/17 22:53:49, mattm wrote:
> > On 2017/03/17 22:43:00, Nico wrote:
> > > Ok, I read the bug. This seems like it doesn't help all that much and
seems
> > > pretty hacky, and it looks like it was introduced by iannucci's TMPDIR
> thing.
> > > How about we do this instead:
> > > 
> > > - use CHROMIUM_TEMPDIR_FOR_BOTS if it's set
> > > - use NSTemporaryDirectory() if not as before
> > > - set CHROMIUM_TEMPDIR_FOR_BOTS on bots
> > > 
> > > That way, we don't change behavior in prod and probably dodge a whole list
> of
> > > issues, and bots still get to override this dir.
> > 
> > That sounds look a good plan for that bug, if mac people are fine with
TMPDIR
> > not being honored.
> > 
> > We still do honor TMPDIR on linux, so I think this could be generally useful
> > anyway. Though the socket length limit hasn't caused too much noise before,
so
> > I'm not too set on this CL if we fix the mac issue a different way.
> 
> FWIW, we could definitely set a different envvar too; the only reason I used
> TMPDIR
> was because we do honor it on linux and I thought consistency might be good.
> 
> Should I make that change?

Basically; should we land this change, or should I add a funky envvar instead?