Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2830753004:
Pipe the devTools FrameId from blink into the browser for headless (Closed)
|
Created:
3 years, 8 months ago by alex clarke (OOO till 29th) Modified:
3 years, 7 months ago CC:
apavlov+blink_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-frames_chromium.org, caseq+blink_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dcheng, devtools-reviews_chromium.org, dglazkov+blink, jam, kinuko+watch, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, mlamouri+watch-content_chromium.org, nasko, nasko+codewatch_chromium.org, ncarter (slow), pfeldman, pfeldman+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPipe the devTools FrameId from blink into the browser for headless
Headless chrome needs this information because we wish to know
which frames resources came from.
BUG=715541
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2830753004
Cr-Commit-Position: refs/heads/master@{#468932}
Committed: https://chromium.googlesource.com/chromium/src/+/ada1e2d7eb1bace10236e750ca89f4f715d63a37
Patch Set 1 #Patch Set 2 : Fix threadding bug in test #Patch Set 3 : Fix PlzNavigate #Patch Set 4 : Add include for msvc #
Total comments: 10
Patch Set 5 : Changes for Sami and creis@ #
Total comments: 11
Patch Set 6 : Changes for dgozman@ #
Total comments: 30
Patch Set 7 : Changes for creis@ #
Total comments: 2
Patch Set 8 : Rebased #
Total comments: 9
Patch Set 9 : Made the test also load resources #Patch Set 10 : Add a comment #Patch Set 11 : Rebased #Messages
Total messages: 81 (54 generated)
Description was changed from ========== Pipe the devTools FrameId from blink into the browser for headless Headless chrome needs this information because we wish to know which frames resources came from. BUG= ========== to ========== Pipe the devTools FrameId from blink into the browser for headless Headless chrome needs this information because we wish to know which frames resources came from. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Pipe the devTools FrameId from blink into the browser for headless Headless chrome needs this information because we wish to know which frames resources came from. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Pipe the devTools FrameId from blink into the browser for headless Headless chrome needs this information because we wish to know which frames resources came from. BUG=546953 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
alexclarke@chromium.org changed reviewers: + dgozman@chromium.org, skyostil@chromium.org
PTAL
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
We discussed stable frame ids with other content owners, and there is some movement in this direction. How urgent is this? Can we wait a couple of weeks or it's preferable to land now and migrate later?
On 2017/04/21 23:08:59, dgozman wrote: > We discussed stable frame ids with other content owners, and there is some > movement in this direction. How urgent is this? Can we wait a couple of weeks or > it's preferable to land now and migrate later? Can we land this and revert if there's a subsequent better method? As a bonus if we land this, we can get rid of the X-DevTools-RequestId header. While we're not totally blocked if this doesn't land, theres's a bunch of follow on work we'd prefer to do now rather than later.
Thanks, this seems like a workable (intermediate?) solution. Just one thought about the public API surface. https://codereview.chromium.org/2830753004/diff/60001/headless/public/headles... File headless/public/headless_web_contents.h (right): https://codereview.chromium.org/2830753004/diff/60001/headless/public/headles... headless/public/headless_web_contents.h:86: // any. Note this only works after we have received the data from blink. nit: "after we have received the data from blink" is a little hard to interpret at this level. Is there a notification we have to wait for? https://codereview.chromium.org/2830753004/diff/60001/headless/public/headles... headless/public/headless_web_contents.h:87: virtual std::string GetDevToolsFrameIdForFrameTreeNodeId( Could we put this (and the new method on HeadlessWebContents) just on the corresponding impls instead of making them part of the public API? The reason is that there isn't really a way to use this just through public headless API (e.g., the render process ids or frame ids aren't exposed anywhere), and I wouldn't want to encourage folks to start depending on random bits of content/ to get that data.
creis@chromium.org changed reviewers: + creis@chromium.org
I have some concerns about landing this as is. https://codereview.chromium.org/2830753004/diff/60001/content/browser/devtool... File content/browser/devtools/devtools_agent_host_impl.cc (right): https://codereview.chromium.org/2830753004/diff/60001/content/browser/devtool... content/browser/devtools/devtools_agent_host_impl.cc:63: int frame_tree_node_id) { We've been talking about how to make APIs like this safer, since it's too easy at the moment to introduce security bugs this way. The FTN ID doesn't change even when a cross-process navigation has taken place (e.g., between chrome://settings and evil.com), so you might be getting a string here that was sent up from a compromised renderer process when expecting one for a highly privileged page. One way to make it safer is to require the caller to pass in a process ID any time they pass in a FrameTreeNode ID, so we can check that it agrees with the current_frame_host's process inside this call. https://codereview.chromium.org/2830753004/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2830753004/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2409: devtools_frame_id_ = devtools_frame_id; I'm a bit uncomfortable having the browser process make decisions on frame IDs based on a value sent up by a potentially compromised renderer process. I don't think this a good approach for people to be building code on top of. If it's needed as a stopgap until we solve the problem of passing FrameTreeNodes to the renderer, then we at least need to make it clear that it's dangerous and shouldn't be used by other code. https://codereview.chromium.org/2830753004/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2830753004/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:1184: std::string devtools_frame_id_; dgozman@: Is this something that's RFH specific, or is it stable for the lifetime of the FrameTreeNode? (Is RFH the right place for it?)
Thanks for the feedback Charlie. With those adjustments do we think this would be an acceptable solution until there's a content-wide id mechanism in place? I'm asking because first of all I'm glad to hear there's renewed interest solving this id problem more generally, but secondly it's also been around for a long time and I'm a bit hesitant to block things on our side on a new solution without knowing a bit more about it. Does someone have a timeline/design docs/patches? We can explain a bit more offline how this mechanism is used on our side and why we believe the risk from compromised renderers isn't as great. I realize that doesn't stop someone else from using this API in a bad way -- maybe we just need to give it a sufficiently scary name? :)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2830753004/diff/60001/content/browser/devtool... File content/browser/devtools/devtools_agent_host_impl.cc (right): https://codereview.chromium.org/2830753004/diff/60001/content/browser/devtool... content/browser/devtools/devtools_agent_host_impl.cc:63: int frame_tree_node_id) { On 2017/04/24 20:26:42, Charlie Reis wrote: > We've been talking about how to make APIs like this safer, since it's too easy > at the moment to introduce security bugs this way. The FTN ID doesn't change > even when a cross-process navigation has taken place (e.g., between > chrome://settings and http://evil.com), so you might be getting a string here that was > sent up from a compromised renderer process when expecting one for a highly > privileged page. > > One way to make it safer is to require the caller to pass in a process ID any > time they pass in a FrameTreeNode ID, so we can check that it agrees with the > current_frame_host's process inside this call. Done. https://codereview.chromium.org/2830753004/diff/60001/headless/public/headles... File headless/public/headless_web_contents.h (right): https://codereview.chromium.org/2830753004/diff/60001/headless/public/headles... headless/public/headless_web_contents.h:86: // any. Note this only works after we have received the data from blink. On 2017/04/24 19:54:05, Sami wrote: > nit: "after we have received the data from blink" is a little hard to interpret > at this level. Is there a notification we have to wait for? Done. https://codereview.chromium.org/2830753004/diff/60001/headless/public/headles... headless/public/headless_web_contents.h:87: virtual std::string GetDevToolsFrameIdForFrameTreeNodeId( On 2017/04/24 19:54:05, Sami wrote: > Could we put this (and the new method on HeadlessWebContents) just on the > corresponding impls instead of making them part of the public API? The reason is > that there isn't really a way to use this just through public headless API > (e.g., the render process ids or frame ids aren't exposed anywhere), and I > wouldn't want to encourage folks to start depending on random bits of content/ > to get that data. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Let's create a separate bug to make it easier to track various frame ids in case we are going to cleanup. https://codereview.chromium.org/2830753004/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2830753004/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:1184: std::string devtools_frame_id_; On 2017/04/24 20:26:42, Charlie Reis wrote: > dgozman@: Is this something that's RFH specific, or is it stable for the > lifetime of the FrameTreeNode? (Is RFH the right place for it?) This is currently per-{Local,Render}Frame so RFH is a right place. https://codereview.chromium.org/2830753004/diff/80001/content/browser/devtool... File content/browser/devtools/devtools_agent_host_impl.cc (right): https://codereview.chromium.org/2830753004/diff/80001/content/browser/devtool... content/browser/devtools/devtools_agent_host_impl.cc:73: ->current_frame_host() Let's extract a variable for frame_tree_node->current_frame_host(). https://codereview.chromium.org/2830753004/diff/80001/content/browser/devtool... content/browser/devtools/devtools_agent_host_impl.cc:74: ->render_view_host() You can skip render_view_host() call, RFHI has GetProcess(). https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (right): https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:695: frame->Client()->SetDevToolsFrameId(frame_id); Let's do this in FrameAttachedToParent - it comes earlier. https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameClient.h:261: virtual void SetDevToolsFrameId(const blink::WebString& devtools_frame_id) {} I'd prefer content to ask for devtools frame id and send an IPC, instead of pushing from blink, but I failed to find a place where LocalFrame would already exist. The best guess was RenderFrameImpl::BindToWebFrame. @creis: are you aware of a better place?
Description was changed from ========== Pipe the devTools FrameId from blink into the browser for headless Headless chrome needs this information because we wish to know which frames resources came from. BUG=546953 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Pipe the devTools FrameId from blink into the browser for headless Headless chrome needs this information because we wish to know which frames resources came from. BUG=715541 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
PTAL https://codereview.chromium.org/2830753004/diff/80001/content/browser/devtool... File content/browser/devtools/devtools_agent_host_impl.cc (right): https://codereview.chromium.org/2830753004/diff/80001/content/browser/devtool... content/browser/devtools/devtools_agent_host_impl.cc:73: ->current_frame_host() On 2017/04/25 22:04:48, dgozman wrote: > Let's extract a variable for frame_tree_node->current_frame_host(). Done. https://codereview.chromium.org/2830753004/diff/80001/content/browser/devtool... content/browser/devtools/devtools_agent_host_impl.cc:74: ->render_view_host() On 2017/04/25 22:04:48, dgozman wrote: > You can skip render_view_host() call, RFHI has GetProcess(). Done. https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (right): https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:695: frame->Client()->SetDevToolsFrameId(frame_id); On 2017/04/25 22:04:48, dgozman wrote: > Let's do this in FrameAttachedToParent - it comes earlier. I tried doing that, the problem is the main frame is getting created before the DevTools client attaches so probe::frameAttachedToParent doesn't do anything and the FrameIdTest fails. Maybe we should make Page.enable send this event too?
Forgot to mention I've added a specific bug as requested.
headless/ lgtm, thanks! https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:223: void RemoveFrameTreeNode(int render_process_id, int render_frame_id); Probably did not mean to have this here?
Thanks. The frameId stuff is a little better now, though I noticed some other potential problems (e.g., locks, OOPIFs). On 2017/04/25 11:36:07, Sami wrote: > Thanks for the feedback Charlie. With those adjustments do we think this would > be an acceptable solution until there's a content-wide id mechanism in place? I think I'm ok with using the DevTools frame ID as a short term solution as long as we make it sufficiently clear in all the places that it will go away. We should have TODOs with the bug number in all the places it's stored/exposed. > I'm asking because first of all I'm glad to hear there's renewed interest > solving this id problem more generally, but secondly it's also been around for a > long time and I'm a bit hesitant to block things on our side on a new solution > without knowing a bit more about it. Does someone have a timeline/design > docs/patches? No, we're waiting for dcheng@ to return to finalize the ideas, so there isn't a timeline yet. > > We can explain a bit more offline how this mechanism is used on our side and why > we believe the risk from compromised renderers isn't as great. I realize that > doesn't stop someone else from using this API in a bad way -- maybe we just need > to give it a sufficiently scary name? :) A scary name and TODOs should help, thanks. https://codereview.chromium.org/2830753004/diff/60001/content/browser/devtool... File content/browser/devtools/devtools_agent_host_impl.cc (right): https://codereview.chromium.org/2830753004/diff/60001/content/browser/devtool... content/browser/devtools/devtools_agent_host_impl.cc:63: int frame_tree_node_id) { On 2017/04/25 15:01:03, alex clarke wrote: > On 2017/04/24 20:26:42, Charlie Reis wrote: > > We've been talking about how to make APIs like this safer, since it's too easy > > at the moment to introduce security bugs this way. The FTN ID doesn't change > > even when a cross-process navigation has taken place (e.g., between > > chrome://settings and http://evil.com), so you might be getting a string here > that was > > sent up from a compromised renderer process when expecting one for a highly > > privileged page. > > > > One way to make it safer is to require the caller to pass in a process ID any > > time they pass in a FrameTreeNode ID, so we can check that it agrees with the > > current_frame_host's process inside this call. > > Done. Thanks! https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameClient.h:261: virtual void SetDevToolsFrameId(const blink::WebString& devtools_frame_id) {} On 2017/04/25 22:04:48, dgozman wrote: > I'd prefer content to ask for devtools frame id and send an IPC, instead of > pushing from blink, but I failed to find a place where LocalFrame would already > exist. The best guess was RenderFrameImpl::BindToWebFrame. > > @creis: are you aware of a better place? I agree that it's better for content to ask, to make sure that it goes up at a consistent time. Is the idea to do it as early after frame creation as possible? If so, BindToWebFrame may be the right timing, though it feels a bit weird to put an IPC there. I suppose it's ok if it's just temporary, until we send the frame_tree_node_id down. Note: Whether we do this or the Blink-push approach, the browser process is still going to have to deal with a window of time where RenderFrameHost doesn't know the ID yet, including both waiting for the IPC to arrive on the IO thread and get posted to the UI thread. Is the headless code that uses it going to face problems if the ID hasn't arrived yet? https://codereview.chromium.org/2830753004/diff/100001/content/browser/devtoo... File content/browser/devtools/devtools_agent_host_impl.cc (right): https://codereview.chromium.org/2830753004/diff/100001/content/browser/devtoo... content/browser/devtools/devtools_agent_host_impl.cc:74: render_frame_host_impl->GetProcess()->GetID() != process_id) { Thanks-- I appreciate this check. https://codereview.chromium.org/2830753004/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2830753004/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:618: const std::string& devtools_frame_id() const { return devtools_frame_id_; } This should have a comment saying when it's set, that it's untrusted, and that we plan to remove it (with a TODO referencing the bug). https://codereview.chromium.org/2830753004/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:1190: std::string devtools_frame_id_; Similar comment, or at least the TODO about removing it. https://codereview.chromium.org/2830753004/diff/100001/content/public/browser... File content/public/browser/devtools_agent_host.h (right): https://codereview.chromium.org/2830753004/diff/100001/content/public/browser... content/public/browser/devtools_agent_host.h:56: // |frame_tree_node_id|. This is sent by the renderer and shouldn't be fully nit: Remove extra space before "sent". https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_browser_context_impl.h (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:49: int render_frame_id, routing_id or render_frame_routing_id would be less ambiguous, since we now have at least 3 possible meanings for "frame id" (routing ID, frame_tree_node_id, and the DevTools frameId). https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:52: void RemoveFrameTreeNode(int render_process_id, int render_frame_id); Same here. https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:90: // Returns the frame tree node id for the corresponding RenderFrameHost or -1 nit: FrameTreeNode id https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:111: mutable base::Lock frame_tree_node_map_lock_; // Guards frame_tree_node_map_ Why does this need to be locked? We try hard to avoid sharing data structures across threads when possible. https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.h (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.h:109: int GetRenderProcessId() const; A WebContents can have multiple processes, so we shouldn't introduce a new API suggesting it has only one. (WebContents::GetRenderProcessHost() is on its way out in https://crbug.com/666525.) We should at least rename it to GetMainFrameProcessId() (and implement it using GetMainFrame()->GetProcess()->GetID()). Or actually, this appears to only be used in tests? Maybe there's a way to avoid exposing it, or make it specific to tests? https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.h:132: content::RenderProcessHost* render_process_host_; // Not owned. This has a similar problem. It looks like HeadlessWebContentsImpl could have lots of problems when out-of-process iframes are present on a page (e.g., not observing subframe processes)? https://codereview.chromium.org/2830753004/diff/100001/headless/lib/frame_id_... File headless/lib/frame_id_browsertest.cc (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/frame_id_... headless/lib/frame_id_browsertest.cc:206: headless_web_contents_impl->GetRenderProcessId(), Is this assuming that the only loads in the test happen in the main frame (or its process)? It seems like you could have load events in out-of-process iframes that wouldn't match this process ID. https://codereview.chromium.org/2830753004/diff/100001/headless/public/util/g... File headless/public/util/generic_url_request_job.cc (right): https://codereview.chromium.org/2830753004/diff/100001/headless/public/util/g... headless/public/util/generic_url_request_job.cc:214: content::RenderFrameHost::FromID(render_process_id, render_frame_id); I'm trying to understand this a bit more, but I can't find any callers of this method. What thread does this class run on? It sounds like IO thread stuff (URLRequestJobs, protocol handlers, etc), but this can only be called on the UI thread and has a DCHECK to that effect. Is all of this on the UI thread?
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2830753004/diff/100001/content/browser/devtoo... File content/browser/devtools/devtools_agent_host_impl.cc (right): https://codereview.chromium.org/2830753004/diff/100001/content/browser/devtoo... content/browser/devtools/devtools_agent_host_impl.cc:74: render_frame_host_impl->GetProcess()->GetID() != process_id) { On 2017/04/26 19:52:55, Charlie Reis wrote: > Thanks-- I appreciate this check. Acknowledged. https://codereview.chromium.org/2830753004/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2830753004/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:618: const std::string& devtools_frame_id() const { return devtools_frame_id_; } On 2017/04/26 19:52:55, Charlie Reis wrote: > This should have a comment saying when it's set, that it's untrusted, and that > we plan to remove it (with a TODO referencing the bug). Done. https://codereview.chromium.org/2830753004/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:1190: std::string devtools_frame_id_; On 2017/04/26 19:52:55, Charlie Reis wrote: > Similar comment, or at least the TODO about removing it. Done. https://codereview.chromium.org/2830753004/diff/100001/content/public/browser... File content/public/browser/devtools_agent_host.h (right): https://codereview.chromium.org/2830753004/diff/100001/content/public/browser... content/public/browser/devtools_agent_host.h:56: // |frame_tree_node_id|. This is sent by the renderer and shouldn't be fully On 2017/04/26 19:52:55, Charlie Reis wrote: > nit: Remove extra space before "sent". Done. https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_browser_context_impl.h (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:49: int render_frame_id, On 2017/04/26 19:52:55, Charlie Reis wrote: > routing_id or render_frame_routing_id would be less ambiguous, since we now have > at least 3 possible meanings for "frame id" (routing ID, frame_tree_node_id, and > the DevTools frameId). Done. https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:52: void RemoveFrameTreeNode(int render_process_id, int render_frame_id); On 2017/04/26 19:52:55, Charlie Reis wrote: > Same here. Done. https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:90: // Returns the frame tree node id for the corresponding RenderFrameHost or -1 On 2017/04/26 19:52:55, Charlie Reis wrote: > nit: FrameTreeNode id Done. https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:111: mutable base::Lock frame_tree_node_map_lock_; // Guards frame_tree_node_map_ On 2017/04/26 19:52:55, Charlie Reis wrote: > Why does this need to be locked? We try hard to avoid sharing data structures > across threads when possible. That's actually why we're adding this :) We want to call HeadlessBrowserContextImpl::GetFrameTreeNodeId from the IO thread. That means we can't use RenderFrameHost::FromID and have to roll our own thread safe lookup (HeadlessBrowserContextImpl::SetFrameTreeNodeId gets called on the UI thread). https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:223: void RemoveFrameTreeNode(int render_process_id, int render_frame_id); On 2017/04/26 16:21:17, Sami wrote: > Probably did not mean to have this here? Done. https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.h (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.h:109: int GetRenderProcessId() const; On 2017/04/26 19:52:56, Charlie Reis wrote: > A WebContents can have multiple processes, so we shouldn't introduce a new API > suggesting it has only one. (WebContents::GetRenderProcessHost() is on its way > out in https://crbug.com/666525.) > > We should at least rename it to GetMainFrameProcessId() (and implement it using > GetMainFrame()->GetProcess()->GetID()). > > Or actually, this appears to only be used in tests? Maybe there's a way to > avoid exposing it, or make it specific to tests? Done. https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.h:132: content::RenderProcessHost* render_process_host_; // Not owned. On 2017/04/26 19:52:55, Charlie Reis wrote: > This has a similar problem. It looks like HeadlessWebContentsImpl could have > lots of problems when out-of-process iframes are present on a page (e.g., not > observing subframe processes)? Acknowledged. https://codereview.chromium.org/2830753004/diff/100001/headless/lib/frame_id_... File headless/lib/frame_id_browsertest.cc (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/frame_id_... headless/lib/frame_id_browsertest.cc:206: headless_web_contents_impl->GetRenderProcessId(), On 2017/04/26 19:52:56, Charlie Reis wrote: > Is this assuming that the only loads in the test happen in the main frame (or > its process)? It seems like you could have load events in out-of-process > iframes that wouldn't match this process ID. Yes I expect we'll have to make an number of changes for OOPIF. I've filed a bug to track that. https://bugs.chromium.org/p/chromium/issues/detail?id=715924 https://codereview.chromium.org/2830753004/diff/100001/headless/public/util/g... File headless/public/util/generic_url_request_job.cc (right): https://codereview.chromium.org/2830753004/diff/100001/headless/public/util/g... headless/public/util/generic_url_request_job.cc:214: content::RenderFrameHost::FromID(render_process_id, render_frame_id); On 2017/04/26 19:52:56, Charlie Reis wrote: > I'm trying to understand this a bit more, but I can't find any callers of this > method. What thread does this class run on? It sounds like IO thread stuff > (URLRequestJobs, protocol handlers, etc), but this can only be called on the UI > thread and has a DCHECK to that effect. Is all of this on the UI thread? I expect this will be called on the IO thread.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This patch lgtm as a temporary solution, if we are sure to follow up with stable frame ids. Otherwise, I'd look for nice place in RenderFrameImpl to get devtools-specific id and push to browser (ask dcheng@ maybe?) https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameClient.h:261: virtual void SetDevToolsFrameId(const blink::WebString& devtools_frame_id) {} On 2017/04/26 19:52:55, Charlie Reis wrote: > On 2017/04/25 22:04:48, dgozman wrote: > > I'd prefer content to ask for devtools frame id and send an IPC, instead of > > pushing from blink, but I failed to find a place where LocalFrame would > already > > exist. The best guess was RenderFrameImpl::BindToWebFrame. > > > > @creis: are you aware of a better place? > > I agree that it's better for content to ask, to make sure that it goes up at a > consistent time. Is the idea to do it as early after frame creation as > possible? > > If so, BindToWebFrame may be the right timing, though it feels a bit weird to > put an IPC there. I suppose it's ok if it's just temporary, until we send the > frame_tree_node_id down. From my understanding, at the time of BindToWebFrame, LocalFrame* may not be instantiated yet. That's why I failed to find a good place. Perhaps, it doesn't matter that much if we are 100% sure this is temporary. WDYT? > > Note: Whether we do this or the Blink-push approach, the browser process is > still going to have to deal with a window of time where RenderFrameHost doesn't > know the ID yet, including both waiting for the IPC to arrive on the IO thread > and get posted to the UI thread. Is the headless code that uses it going to > face problems if the ID hasn't arrived yet? https://codereview.chromium.org/2830753004/diff/140001/content/browser/devtoo... File content/browser/devtools/devtools_agent_host_impl.cc (right): https://codereview.chromium.org/2830753004/diff/140001/content/browser/devtoo... content/browser/devtools/devtools_agent_host_impl.cc:77: return frame_tree_node->current_frame_host()->untrusted_devtools_frame_id(); nit: use |render_frame_host_impl|. https://codereview.chromium.org/2830753004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (right): https://codereview.chromium.org/2830753004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:687: frame->Client()->SetDevToolsFrameId(frame_id); Let's do this before previous call, so that it's available to browser-process C++ bindings beforehand.
Thanks. Some additional questions below, though I suppose we may be stuck with some of this for now. https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameClient.h:261: virtual void SetDevToolsFrameId(const blink::WebString& devtools_frame_id) {} On 2017/04/27 17:12:06, dgozman wrote: > On 2017/04/26 19:52:55, Charlie Reis wrote: > > On 2017/04/25 22:04:48, dgozman wrote: > > > I'd prefer content to ask for devtools frame id and send an IPC, instead of > > > pushing from blink, but I failed to find a place where LocalFrame would > > already > > > exist. The best guess was RenderFrameImpl::BindToWebFrame. > > > > > > @creis: are you aware of a better place? > > > > I agree that it's better for content to ask, to make sure that it goes up at a > > consistent time. Is the idea to do it as early after frame creation as > > possible? > > > > If so, BindToWebFrame may be the right timing, though it feels a bit weird to > > put an IPC there. I suppose it's ok if it's just temporary, until we send the > > frame_tree_node_id down. > From my understanding, at the time of BindToWebFrame, LocalFrame* may not be > instantiated yet. That's why I failed to find a good place. Perhaps, it doesn't > matter that much if we are 100% sure this is temporary. WDYT? I don't understand. BindToWebFrame is passed a LocalFrame that has just been constructed in each of the three call sites. Looks like it's not null in practice when debugging, both for main frames and subframes. When would it not be instantiated yet? I think this is probably the right timing, whether we do it inside that call or just after each of the call sites. And if we can, I think we should definitely do it this way, rather than having Blink push the value up. > > > > > Note: Whether we do this or the Blink-push approach, the browser process is > > still going to have to deal with a window of time where RenderFrameHost > doesn't > > know the ID yet, including both waiting for the IPC to arrive on the IO thread > > and get posted to the UI thread. Is the headless code that uses it going to > > face problems if the ID hasn't arrived yet? > https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_browser_context_impl.h (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:111: mutable base::Lock frame_tree_node_map_lock_; // Guards frame_tree_node_map_ On 2017/04/27 08:49:43, alex clarke wrote: > On 2017/04/26 19:52:55, Charlie Reis wrote: > > Why does this need to be locked? We try hard to avoid sharing data structures > > across threads when possible. > > That's actually why we're adding this :) We want to call > HeadlessBrowserContextImpl::GetFrameTreeNodeId from the IO thread. That means > we can't use RenderFrameHost::FromID and have to roll our own thread safe lookup > (HeadlessBrowserContextImpl::SetFrameTreeNodeId gets called on the UI thread). Hmm, it seems like we should avoid that when possible. When looking for alternatives, though, I found that extension_api_frame_id_map.h unfortunately does something similar. :( Maybe we need to provide a better way to do this association on the IO thread so that features don't add new locks for it. In the meantime, maybe you can do this within headless if it doesn't affect normal Chrome. I'm curious if the need goes away when PlzNavigate ships (see my other question). https://codereview.chromium.org/2830753004/diff/120001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2830753004/diff/120001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:267: return web_contents()->GetRenderProcessHost()->GetID(); Can you update this to web_contents()->GetMainFrame()->GetProcess()->GetID()? That will give us fewer places to update as we try to remove GetRenderProcessHost(). https://codereview.chromium.org/2830753004/diff/140001/headless/lib/browser/h... File headless/lib/browser/headless_browser_context_impl.h (right): https://codereview.chromium.org/2830753004/diff/140001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:111: mutable base::Lock frame_tree_node_map_lock_; // Guards frame_tree_node_map_ If we need to keep this, please include more comments about how it's modified and accessed and from which threads (e.g., see how extension_api_frame_id_map.h documents its shared map). This will help us find alternatives to remove it in the future if possible. https://codereview.chromium.org/2830753004/diff/140001/headless/public/util/g... File headless/public/util/generic_url_request_job.cc (right): https://codereview.chromium.org/2830753004/diff/140001/headless/public/util/g... headless/public/util/generic_url_request_job.cc:222: return request_resource_info_->GetFrameTreeNodeId(); Will the need for the map go away in PlzNavigate, if you have the FTN ID here?
> I don't understand. BindToWebFrame is passed a LocalFrame that has just been > constructed in each of the three call sites. Looks like it's not null in > practice when debugging, both for main frames and subframes. When would it not > be instantiated yet? > > I think this is probably the right timing, whether we do it inside that call or > just after each of the call sites. > > And if we can, I think we should definitely do it this way, rather than having > Blink push the value up. Seems like for child frames when RenderFrameImpl::CreateChildFrame is called (which does BindToWebFrame), the LocalFrame has not been created yet. See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebLocalFr...
[CC lfg, dcheng] https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameClient.h:261: virtual void SetDevToolsFrameId(const blink::WebString& devtools_frame_id) {} On 2017/04/27 19:42:49, dgozman wrote: > > I don't understand. BindToWebFrame is passed a LocalFrame that has just been > > constructed in each of the three call sites. Looks like it's not null in > > practice when debugging, both for main frames and subframes. When would it > not > > be instantiated yet? > > > > I think this is probably the right timing, whether we do it inside that call > or > > just after each of the call sites. > > > > And if we can, I think we should definitely do it this way, rather than having > > Blink push the value up. > > Seems like for child frames when RenderFrameImpl::CreateChildFrame is called > (which does BindToWebFrame), the LocalFrame has not been created yet. > See > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebLocalFr... Wow, that's some complex initialization! So we have a WebLocalFrameImpl at the time of BindToWebFrame but not a LocalFrame inside it, which would explain why that doesn't work for getting the DevTools frameId. Sigh. Unless lfg@ or dcheng@ have any ideas about how RenderFrameImpl can know when the LocalFrame is created, I guess we have to push the data. :(
On 2017/04/27 20:07:34, Charlie Reis wrote: > [CC lfg, dcheng] > > https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... > File third_party/WebKit/public/web/WebFrameClient.h (right): > > https://codereview.chromium.org/2830753004/diff/80001/third_party/WebKit/publ... > third_party/WebKit/public/web/WebFrameClient.h:261: virtual void > SetDevToolsFrameId(const blink::WebString& devtools_frame_id) {} > On 2017/04/27 19:42:49, dgozman wrote: > > > I don't understand. BindToWebFrame is passed a LocalFrame that has just > been > > > constructed in each of the three call sites. Looks like it's not null in > > > practice when debugging, both for main frames and subframes. When would it > > not > > > be instantiated yet? > > > > > > I think this is probably the right timing, whether we do it inside that call > > or > > > just after each of the call sites. > > > > > > And if we can, I think we should definitely do it this way, rather than > having > > > Blink push the value up. > > > > Seems like for child frames when RenderFrameImpl::CreateChildFrame is called > > (which does BindToWebFrame), the LocalFrame has not been created yet. > > See > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebLocalFr... > > Wow, that's some complex initialization! So we have a WebLocalFrameImpl at the > time of BindToWebFrame but not a LocalFrame inside it, which would explain why > that doesn't work for getting the DevTools frameId. Sigh. > > Unless lfg@ or dcheng@ have any ideas about how RenderFrameImpl can know when > the LocalFrame is created, I guess we have to push the data. :( The LocalFrame gets created in InitializeCoreFrame, and I don't think there's any observable event that content can check that it happened. Out of the top of my head, I can think of WebLocalFrame::CreateChild and WebFrame::Swap that can creates the core frame, but they happen after the content callback to create the RenderFrame, and they won't transfer control back to content after that.
PTAL https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_browser_context_impl.h (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:111: mutable base::Lock frame_tree_node_map_lock_; // Guards frame_tree_node_map_ On 2017/04/27 18:58:02, Charlie Reis wrote: > On 2017/04/27 08:49:43, alex clarke wrote: > > On 2017/04/26 19:52:55, Charlie Reis wrote: > > > Why does this need to be locked? We try hard to avoid sharing data > structures > > > across threads when possible. > > > > That's actually why we're adding this :) We want to call > > HeadlessBrowserContextImpl::GetFrameTreeNodeId from the IO thread. That means > > we can't use RenderFrameHost::FromID and have to roll our own thread safe > lookup > > (HeadlessBrowserContextImpl::SetFrameTreeNodeId gets called on the UI thread). > > Hmm, it seems like we should avoid that when possible. When looking for > alternatives, though, I found that extension_api_frame_id_map.h unfortunately > does something similar. :( Maybe we need to provide a better way to do this > association on the IO thread so that features don't add new locks for it. > > In the meantime, maybe you can do this within headless if it doesn't affect > normal Chrome. I'm curious if the need goes away when PlzNavigate ships (see my > other question). Yes but only for browser side navigation requests. Anything resources fetches from blink will still require us to do this lookup. Incidentally I wonder if this could be avoided if the FrameTreeNode Id was included in URLRequestUserData? https://codereview.chromium.org/2830753004/diff/120001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2830753004/diff/120001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:267: return web_contents()->GetRenderProcessHost()->GetID(); On 2017/04/27 18:58:02, Charlie Reis wrote: > Can you update this to web_contents()->GetMainFrame()->GetProcess()->GetID()? > That will give us fewer places to update as we try to remove > GetRenderProcessHost(). Done. https://codereview.chromium.org/2830753004/diff/140001/content/browser/devtoo... File content/browser/devtools/devtools_agent_host_impl.cc (right): https://codereview.chromium.org/2830753004/diff/140001/content/browser/devtoo... content/browser/devtools/devtools_agent_host_impl.cc:77: return frame_tree_node->current_frame_host()->untrusted_devtools_frame_id(); On 2017/04/27 17:12:06, dgozman wrote: > nit: use |render_frame_host_impl|. Done. https://codereview.chromium.org/2830753004/diff/140001/headless/lib/browser/h... File headless/lib/browser/headless_browser_context_impl.h (right): https://codereview.chromium.org/2830753004/diff/140001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:111: mutable base::Lock frame_tree_node_map_lock_; // Guards frame_tree_node_map_ On 2017/04/27 18:58:02, Charlie Reis wrote: > If we need to keep this, please include more comments about how it's modified > and accessed and from which threads (e.g., see how extension_api_frame_id_map.h > documents its shared map). This will help us find alternatives to remove it in > the future if possible. Done. https://codereview.chromium.org/2830753004/diff/140001/headless/public/util/g... File headless/public/util/generic_url_request_job.cc (right): https://codereview.chromium.org/2830753004/diff/140001/headless/public/util/g... headless/public/util/generic_url_request_job.cc:222: return request_resource_info_->GetFrameTreeNodeId(); On 2017/04/27 18:58:02, Charlie Reis wrote: > Will the need for the map go away in PlzNavigate, if you have the FTN ID here? Only for browser side navigations. We still need the FrameTreeNode ID for renderer initiated requests. I've added a resource to each of the documents in the test to underscore this. https://codereview.chromium.org/2830753004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (right): https://codereview.chromium.org/2830753004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:687: frame->Client()->SetDevToolsFrameId(frame_id); On 2017/04/27 17:12:06, dgozman wrote: > Let's do this before previous call, so that it's available to browser-process > C++ bindings beforehand. Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #9 (id:160001) has been deleted
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ok, I suppose this will have to do for now. LGTM. https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_browser_context_impl.h (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:111: mutable base::Lock frame_tree_node_map_lock_; // Guards frame_tree_node_map_ On 2017/04/28 09:14:44, alex clarke wrote: > On 2017/04/27 18:58:02, Charlie Reis wrote: > > On 2017/04/27 08:49:43, alex clarke wrote: > > > On 2017/04/26 19:52:55, Charlie Reis wrote: > > > > Why does this need to be locked? We try hard to avoid sharing data > > structures > > > > across threads when possible. > > > > > > That's actually why we're adding this :) We want to call > > > HeadlessBrowserContextImpl::GetFrameTreeNodeId from the IO thread. That > means > > > we can't use RenderFrameHost::FromID and have to roll our own thread safe > > lookup > > > (HeadlessBrowserContextImpl::SetFrameTreeNodeId gets called on the UI > thread). > > > > Hmm, it seems like we should avoid that when possible. When looking for > > alternatives, though, I found that extension_api_frame_id_map.h unfortunately > > does something similar. :( Maybe we need to provide a better way to do this > > association on the IO thread so that features don't add new locks for it. > > > > In the meantime, maybe you can do this within headless if it doesn't affect > > normal Chrome. I'm curious if the need goes away when PlzNavigate ships (see > my > > other question). > > Yes but only for browser side navigation requests. Anything resources fetches > from blink will still require us to do this lookup. > > Incidentally I wonder if this could be avoided if the FrameTreeNode Id was > included in URLRequestUserData? Perhaps. That's in content/common, so we can't do that until we finish the plan to expose FrameTreeNode IDs to the renderer process. Once that's done, I would imagine we could add it there, and that could give you a way to eliminate this lock. Can you add a TODO to that effect, along the lines of this? "This might be unnecessary if we can add FrameTreeNode ID to URLRequestUserData in https://crbug.com/715541." https://codereview.chromium.org/2830753004/diff/140001/headless/public/util/g... File headless/public/util/generic_url_request_job.cc (right): https://codereview.chromium.org/2830753004/diff/140001/headless/public/util/g... headless/public/util/generic_url_request_job.cc:222: return request_resource_info_->GetFrameTreeNodeId(); On 2017/04/28 09:14:45, alex clarke wrote: > On 2017/04/27 18:58:02, Charlie Reis wrote: > > Will the need for the map go away in PlzNavigate, if you have the FTN ID here? > > Only for browser side navigations. We still need the FrameTreeNode ID for > renderer initiated requests. > > I've added a resource to each of the documents in the test to underscore this. Acknowledged. Hopefully your URLRequestUserData idea will work out in the future.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
alexclarke@chromium.org changed reviewers: + mkwst@chromium.org
+Mike West could you please review changes in content/common/frame_messages.h and third_party/WebKit/public/web/WebFrameClient.h https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_browser_context_impl.h (right): https://codereview.chromium.org/2830753004/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.h:111: mutable base::Lock frame_tree_node_map_lock_; // Guards frame_tree_node_map_ On 2017/04/28 20:29:03, Charlie Reis wrote: > On 2017/04/28 09:14:44, alex clarke wrote: > > On 2017/04/27 18:58:02, Charlie Reis wrote: > > > On 2017/04/27 08:49:43, alex clarke wrote: > > > > On 2017/04/26 19:52:55, Charlie Reis wrote: > > > > > Why does this need to be locked? We try hard to avoid sharing data > > > structures > > > > > across threads when possible. > > > > > > > > That's actually why we're adding this :) We want to call > > > > HeadlessBrowserContextImpl::GetFrameTreeNodeId from the IO thread. That > > means > > > > we can't use RenderFrameHost::FromID and have to roll our own thread safe > > > lookup > > > > (HeadlessBrowserContextImpl::SetFrameTreeNodeId gets called on the UI > > thread). > > > > > > Hmm, it seems like we should avoid that when possible. When looking for > > > alternatives, though, I found that extension_api_frame_id_map.h > unfortunately > > > does something similar. :( Maybe we need to provide a better way to do > this > > > association on the IO thread so that features don't add new locks for it. > > > > > > In the meantime, maybe you can do this within headless if it doesn't affect > > > normal Chrome. I'm curious if the need goes away when PlzNavigate ships > (see > > my > > > other question). > > > > Yes but only for browser side navigation requests. Anything resources fetches > > from blink will still require us to do this lookup. > > > > Incidentally I wonder if this could be avoided if the FrameTreeNode Id was > > included in URLRequestUserData? > > Perhaps. That's in content/common, so we can't do that until we finish the plan > to expose FrameTreeNode IDs to the renderer process. Once that's done, I would > imagine we could add it there, and that could give you a way to eliminate this > lock. > > Can you add a TODO to that effect, along the lines of this? > "This might be unnecessary if we can add FrameTreeNode ID to URLRequestUserData > in https://crbug.com/715541.%22 Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
Thanks all!
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, dgozman@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2830753004/#ps200001 (title: "Add a comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, creis@chromium.org, dgozman@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2830753004/#ps220001 (title: "Rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1493799528898160,
"parent_rev": "e43e4863433f22fa9316fadb57f965934a20881d", "commit_rev":
"ada1e2d7eb1bace10236e750ca89f4f715d63a37"}
Message was sent while issue was closed.
Description was changed from ========== Pipe the devTools FrameId from blink into the browser for headless Headless chrome needs this information because we wish to know which frames resources came from. BUG=715541 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Pipe the devTools FrameId from blink into the browser for headless Headless chrome needs this information because we wish to know which frames resources came from. BUG=715541 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2830753004 Cr-Commit-Position: refs/heads/master@{#468932} Committed: https://chromium.googlesource.com/chromium/src/+/ada1e2d7eb1bace10236e750ca89... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/ada1e2d7eb1bace10236e750ca89... |
