Skip to content

Refactor: Improve Edge process management and JSON handling in patch …#2

Open
lifei6671 wants to merge 1 commit into
xiaox0321:mainfrom
lifei6671:main
Open

Refactor: Improve Edge process management and JSON handling in patch …#2
lifei6671 wants to merge 1 commit into
xiaox0321:mainfrom
lifei6671:main

Conversation

@lifei6671
Copy link
Copy Markdown

…functions

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the Edge patching logic to use generic JSON maps instead of fixed structs, ensuring that non-target fields are preserved during updates. It also introduces unit tests and modularizes process management and file I/O. Feedback highlights the need for atomic file writes to prevent corruption, improved helper process detection on macOS, and a fix for an incorrect error check when waiting for user input at the end of the execution.

Comment thread main.go
Comment on lines +259 to +268
func writeJSONObject(path string, value map[string]any) error {
data, err := json.Marshal(value)
if err != nil {
return fmt.Errorf("marshal JSON file %s: %w", path, err)
}
if err := os.WriteFile(path, data, 0644); err != nil {
return fmt.Errorf("write JSON file %s: %w", path, err)
}
return nil
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This function has two issues:

  1. Non-atomic write: Writing directly to the target file using os.WriteFile is risky. If the process is interrupted or the disk is full during the write, the browser profile file could be corrupted. It's safer to write to a temporary file and then rename it.
  2. JSON formatting: The previous implementation used json.MarshalIndent to maintain readability. Switching to json.Marshal produces minified JSON, which makes manual inspection of browser configuration files much harder.
func writeJSONObject(path string, value map[string]any) error {
	data, err := json.MarshalIndent(value, "", "  ")
	if err != nil {
		return fmt.Errorf("marshal JSON file %s: %w", path, err)
	}

	tmpPath := path + ".tmp"
	if err := os.WriteFile(tmpPath, data, 0644); err != nil {
		return fmt.Errorf("write temp JSON file %s: %w", tmpPath, err)
	}

	if err := os.Rename(tmpPath, path); err != nil {
		_ = os.Remove(tmpPath)
		return fmt.Errorf("rename JSON file %s: %w", path, err)
	}
	return nil
}

Comment thread main.go
// 如果父进程同名,通常说明当前是子进程或 helper,直接跳过。
if ppid, _ := p.Ppid(); ppid > 0 {
if parent, err := process.NewProcess(ppid); err == nil {
if parentName, err := parent.Name(); err == nil && parentName == name {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current heuristic parentName == name to skip helper processes works on Windows and Linux where helpers share the same executable name (msedge). However, on macOS, the main process is typically named Microsoft Edge while helpers are named Microsoft Edge Helper. This check will fail to identify helpers on macOS, causing them to be killed individually. Using isEdgeProcessName for the parent name is a more robust approach across all platforms.

Suggested change
if parentName, err := parent.Name(); err == nil && parentName == name {
if parentName, err := parent.Name(); err == nil && isEdgeProcessName(runtime.GOOS, parentName) {

Comment thread main.go
Comment on lines +372 to +374
if _, err := fmt.Scanln(); err != nil {
fmt.Fprintf(os.Stderr, "Warning: failed to read input: %v\n", err)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

fmt.Scanln() without arguments returns an unexpected newline error when the user presses Enter (as it expects to scan at least one item). This causes the warning message to be printed every time the program exits normally. You should either ignore the error or use a different method to wait for the Enter key.

	fmt.Scanln()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant