New issue
Advanced search Search tips

Issue 724724 link

Starred by 1 user
Status: Fixed
Owner:
michae...@chromium.org
Closed: Jul 2017
Cc:
r...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug

Blocking:
issue 675413



Sign in to add a comment

AppShell processes delete each other's log files

Project Member Reported by michae...@chromium.org, May 20 2017 
When ShellMainDelegate::InitLogging [1] is executed by a process, it finds the log file (app_shell.log, which is shared by all processes) and deletes it if it exists. So only the last process to run this code will hold a file descriptor that actually points to the file on disk.

We can fix this the same way Chrome does [2]: only the main process should delete the old log file (desired behavior when launching the first instance). Derivative processes should choose to append to that file.

[1] https://cs.chromium.org/chromium/src/extensions/shell/app/shell_main_delegate.cc?q=initlogging+file:shell_main_delegate%5C.cc&sq=package:chromium&l=50&dr=C
[2] https://cs.chromium.org/chromium/src/chrome/app/chrome_main_delegate.cc?sq=package:chromium&dr=C&q=process_type+delete_old_log_file&l=482

content_shell has the same behavior as app_shell does currently. It would be nice if content_shell, app_shell and Chrome could share more of their implementation (see also: issue 719644).
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/865c5b53cdb67074a3c6d177dee052b46022bde6

commit 865c5b53cdb67074a3c6d177dee052b46022bde6
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Tue Jun 06 05:45:52 2017

AppShell: Fix log file location and writing

Make AppShell log to app_shell.log in its data directory (given by
--data-path, defaults to $HOME/.config/content_shell on Linux) instead
of alongside the executable.

Also fix log file overwriting: Each process opens the app_shell.log
file for logging. The file was being replaced each time it was opened
for writing, so only the last process to open the file was actually
writing to a useful file descriptor.

Like Chrome, AppShell should still replace the old log file when a new
app_shell process is staarted from scratch but append to this file
from forked processes so all logging is persisted.

Bug:  724724 
Change-Id: Ifbb8893d476d1cc3688707f517318898c17c9ef8
Reviewed-on: https://chromium-review.googlesource.com/516164
Reviewed-by: Rahul Chaturvedi <rkc@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477204}
[modify] https://crrev.com/865c5b53cdb67074a3c6d177dee052b46022bde6/extensions/shell/DEPS
[modify] https://crrev.com/865c5b53cdb67074a3c6d177dee052b46022bde6/extensions/shell/app/shell_main_delegate.cc
[modify] https://crrev.com/865c5b53cdb67074a3c6d177dee052b46022bde6/extensions/shell/app/shell_main_delegate.h

Comment 2 by michae...@chromium.org, Jul 14 2017

Status: Fixed (was: Started)
fixed last month
►

Sign in to add a comment