Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use render client id to track deleted render process hosts #14520

Merged
merged 2 commits into from Sep 11, 2018

Conversation

deepak1556
Copy link
Member

@deepak1556 deepak1556 commented Sep 10, 2018

Description of Change

This fix supersedes #14324

Instead of relying on OS process id, which may not be unique when a process is reused, we rely on the renderer client id (which is the render process host id) passed by the content layer when starting the renderer process which is guaranteed to be unique for the lifetime of the app.

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines
Release Notes

Notes: fix missing remote objects #14054

@deepak1556 deepak1556 requested review from a team and zcbenz September 10, 2018 04:53
// we should define our own.
DCHECK(command_line->HasSwitch(::switches::kRendererClientId));
renderer_client_id_ =
command_line->GetSwitchValueASCII(::switches::kRendererClientId);
Copy link
Member

Choose a reason for hiding this comment

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

can we use RenderThread::GetClientId() here rather than parsing it out manually?

Copy link
Member Author

Choose a reason for hiding this comment

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

RenderThread::Get can only be used when the render thread has been started, it may not be available at this point also it may not be the same thread. To avoid confusion of which might get triggered first render thread creation or script context creation, I am just taking safety first approach here.

Copy link
Member

Choose a reason for hiding this comment

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

RenderThread is created pretty early in initialization, even before the sandbox is enabled: https://chromium.googlesource.com/chromium/src/+/67.0.3396.127/content/renderer/renderer_main.cc#228

I don't see a way that RendererClient could be created before RenderThread is initialized.

Copy link
Member

@nornagon nornagon Sep 10, 2018

Choose a reason for hiding this comment

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

Er, wait, actually I think I was wrong. It definitely is called before RenderThread is initialized:

content_main_runner.cc:543 is what calls CreateContentRendererClient, which will ultimately call this function.

content_main_runner.cc:562 is what will call RendererMain, which initializes RenderThread.

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

The code looks good to me, but so did the last 2 versions of this, both of which were found to be buggy. Can you help me understand what exactly renderer_client_id is supposed to be a unique identifier for?

@deepak1556
Copy link
Member Author

Every render process has a corresponding process host in the browser process and each host is assigned a id which is a globally unique across all the child processes launched in the lifetime of the app https://cs.chromium.org/chromium/src/content/public/browser/render_process_host.h?l=227, renderer-client-id transmits this id to the render process just before its initialization.

Each render process may host multiple script contexts and hence we create a unique identifier for referencing remote objects in the renderer with ${renderer-cleint-id}-${context_counter}, whenever a render view is deleted as a result of navigation we dereference the object using the process host id.

Previously we relied on OS process id which was open to failures when the process id can be reused by the operating system and the patch before that was trying to register the delete listener with process host id in the browser process which can be id of a previous process host when its not finalized during redirects or reloads, this patch fixes all those concerns by syncing the process host ids during the registration of the delete listener.

Currently there is a spec which ensures the remote object gets dereferenced for the right process host id, but we need more specs that can cover these scenarios of navigation. I will try adding them this week.

Instead of relying on OS process id, which may not be unique
when a process is reused, we rely on the renderer client id
passed by the content layer when starting the renderer process
which is guaranteed to be unique for the lifetime of the app.
Ensuring that it doesn't wrap easily with a large number
of context creation on some malformed web pages.
@codebytere codebytere merged commit 14ed71f into master Sep 11, 2018
@release-clerk
Copy link

release-clerk bot commented Sep 11, 2018

Release Notes Persisted

fix missing remote objects #14054

@trop
Copy link
Contributor

trop bot commented Sep 11, 2018

An error occurred while attempting to backport this PR to "3-0-x",
you will need to perform this backport manually

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

Successfully merging this pull request may close these issues.

None yet

6 participants