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

expose WebFrame#routingId and start passing it to WebContents frame specific events #12614

Merged
merged 2 commits into from Apr 26, 2018

Conversation

Projects
None yet
3 participants
@bughit
Contributor

bughit commented Apr 13, 2018

addresses #12604

no docs yet, may not be the right impl, here for review

it's probably better to use FrameTreeNodeId rather than RoutingId, but it wasn't clear how to get a WebLocalFrame in the renderer from the FrameTreeNodeId

@bughit bughit requested a review from electron/reviewers as a code owner Apr 13, 2018

return v8::Number::New(
isolate(),
content::RenderFrame::GetRoutingIdForWebFrame(web_frame_)
);

This comment has been minimized.

@zcbenz

zcbenz Apr 16, 2018

Contributor

Linter warning:

Closing ) should be moved to the previous line  [whitespace/parens] [2]
@bughit

This comment has been minimized.

Contributor

bughit commented Apr 16, 2018

@zcbenz What are your thoughts about using FrameTreeNodeId instead, which I am assuming is better in the long run? In order to make FindFrameById work with it, I'd need to:

  1. convert FrameTreeNodeId to a processId and routingId
  2. look up the current renderer processId (is this a chromium specific id?)
  3. if the processIds match lookup the frame by the routing id

Is this worth doing? Do you know how to do 1 and 2?

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Apr 17, 2018

@bughit For the purpose of getting a web frame in renderer process, using routingId is the best choice. The FrameTreeNodeId is only meaningful in the main process, and it can be mapped to WebContents with WebContents::FromFrameTreeNodeId.

So for long term I think we should use routingId for the WebFrame APIs, but explicitly use routingId in API names instead of id.

@bughit

This comment has been minimized.

Contributor

bughit commented Apr 18, 2018

@zcbenz

  • changed id to routingId
  • for all WebContents frame specific events I'm passing both the frameProcessId and the frameRoutingId
    • because it's worth knowing that the event is for a frame in a different process than
      the main WebContents renderer, and consequently the frameRoutingId is not going to be
      meaningful in there
  • added WebContents#did-start-navigation event because there is currently no frame specific navigation
    start event

@bughit bughit changed the title from expose WebFrame#id and start passing it in WebContents events to expose WebFrame#routingId and start passing it to WebContents frame specific events Apr 18, 2018

@deepak1556

Also can you add some specs, thanks!

= frame_tree_node->render_manager();
content::RenderFrameHostImpl* current_frame_host
= render_manager->current_frame_host();
content::RenderFrameHostImpl* pending_frame_host

This comment has been minimized.

@deepak1556

deepak1556 Apr 18, 2018

Member

pending_frame_host is not required.

= render_manager->current_frame_host();
content::RenderFrameHostImpl* pending_frame_host
= render_manager->pending_frame_host();
content::RenderFrameHostImpl* speculative_frame_host

This comment has been minimized.

@deepak1556

deepak1556 Apr 18, 2018

Member

You can cast these to RenderFrameHost, its best to not rely on *Impl classes directly, there are some use cases of WebContentsImpl in the code base but they are unavoidable. This can simply be

+++ b/atom/browser/api/atom_api_web_contents.cc
@@ -56,9 +56,7 @@
 #include "chrome/browser/printing/print_view_manager_basic.h"
 #include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "content/browser/frame_host/frame_tree_node.h"
-#include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/frame_host/render_frame_host_manager.h"
-#include "content/browser/loader/global_routing_id.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_view_base.h"
 #include "content/common/view_messages.h"
@@ -908,22 +906,18 @@ void WebContents::DidStartNavigation(
     content::FrameTreeNode::GloballyFindByID(frame_tree_node_id);
   content::RenderFrameHostManager* render_manager
     = frame_tree_node->render_manager();
-  content::RenderFrameHostImpl* current_frame_host
+  content::RenderFrameHost* current_frame_host
     = render_manager->current_frame_host();
-  content::RenderFrameHostImpl* pending_frame_host
-    = render_manager->pending_frame_host();
-  content::RenderFrameHostImpl* speculative_frame_host
+  content::RenderFrameHost* speculative_frame_host
     = render_manager->speculative_frame_host();
-  content::RenderFrameHostImpl* frame_host =
+  content::RenderFrameHost* frame_host =
     speculative_frame_host ? speculative_frame_host : current_frame_host;
 
   bool is_main_frame = navigation_handle->IsInMainFrame();
   int frame_process_id = -1, frame_routing_id = -1;
   if (frame_host) {
-    content::GlobalFrameRoutingId global_routing_id =
-      frame_host->GetGlobalFrameRoutingId();
-    frame_process_id = global_routing_id.child_id;
-    frame_routing_id = global_routing_id.frame_routing_id;
+    frame_process_id = frame_host->GetProcess()->GetID();
+    frame_routing_id = frame_host->GetRoutingID();
   }
   bool is_same_document = navigation_handle->IsSameDocument();
   auto url = navigation_handle->GetURL();

This comment has been minimized.

@bughit

bughit Apr 18, 2018

Contributor

I am calling RenderFrameHostImp::GetGlobalFrameRoutingId, unless there's some equivalent on RenderFrameHost, it seems I need the Impl.

This comment has been minimized.

@bughit

bughit Apr 18, 2018

Contributor

sorry didn't notice your example

void WebContents::DidFinishNavigation(
content::NavigationHandle* navigation_handle) {
bool is_main_frame = navigation_handle->IsInMainFrame();
content::RenderFrameHost* render_frame_host =
navigation_handle->GetRenderFrameHost();

This comment has been minimized.

@bughit

bughit Apr 18, 2018

Contributor

@zcbenz or @deepak1556

it looks like this GetRenderFrameHost call is problematic (failing ci)

[6815:0418/031343.941870:FATAL:navigation_handle_impl.cc(318)] Check failed: state_ >= WILL_PROCESS_RESPONSE (1 vs. 6)This accessor should only be called after a response has been delivered for processing.
#0 0x7f833624db2d base::debug::StackTrace::StackTrace()
#1 0x7f833624bf5c base::debug::StackTrace::StackTrace()
#2 0x7f83362d2dba logging::LogMessage::~LogMessage()
#3 0x7f833ec55c96 content::NavigationHandleImpl::GetRenderFrameHost()
#4 0x000000c3e34b atom::api::WebContents::DidFinishNavigation()

Is this expected? I can do the same thing here as in DidStartNavigation

@bughit

This comment has been minimized.

Contributor

bughit commented Apr 18, 2018

  • extracted frame host lookup from tree node id to FrameHostFromTreeNodeId
  • using it in both DidStartNavigation and DidFinishNavigation
  • avoiding Impl
@bughit

This comment has been minimized.

Contributor

bughit commented Apr 19, 2018

Also can you add some specs, thanks!

Please be more specific, what needs to be specced? did-start-navigation? anything else?

There is still an issue with DidFinishNavigation which I don't understand, from ci:

Received signal 11 SEGV_MAPERR 000000000090
#0 0x7fcdb7eafb2d base::debug::StackTrace::StackTrace()
#1 0x7fcdb7eadf5c base::debug::StackTrace::StackTrace()
#2 0x7fcdb7eaf4e5 <unknown>
#3 0x7fcdad4d5390 <unknown>
#4 0x000000c5526c content::RenderFrameHostManager::speculative_frame_host()
#5 0x000000c3e2d5 atom::api::(anonymous namespace)::FrameHostFromTreeNodeId()
#6 0x000000c3e393 atom::api::WebContents::DidFinishNavigation()

I am doing the same thing as in DidStartNavigation, it's working in my manual tests.

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Apr 19, 2018

Please be more specific, what needs to be specced? did-start-navigation? anything else?

The specs need to verify the behavior of new webFrame apis using the event parameters created in this PR. Also need to add specs for did-start-navigation event.

For the DidFinishNavigation case, you cannot rely on retrieving speculative render frame host as it will be destroyed, at this point in navigation cycle the current frame host will already take either speculative render frame host or new one depending on the site instance. You can simply rely on the render frame host returned by the navigation handle in this case.

@bughit

This comment has been minimized.

Contributor

bughit commented Apr 19, 2018

You can simply rely on the render frame host returned by the navigation handle in this case.

I was already doing that in the previous iteration and ci testa were failing:

[6815:0418/031343.941870:FATAL:navigation_handle_impl.cc(318)] Check failed: state_ >= WILL_PROCESS_RESPONSE (1 vs. 6)This accessor should only be called after a response has been delivered for processing.
#0 0x7f833624db2d base::debug::StackTrace::StackTrace()
#1 0x7f833624bf5c base::debug::StackTrace::StackTrace()
#2 0x7f83362d2dba logging::LogMessage::~LogMessage()
#3 0x7f833ec55c96 content::NavigationHandleImpl::GetRenderFrameHost()
#4 0x000000c3e34b atom::api::WebContents::DidFinishNavigation()

So should I revert to this or modify FrameHostFromTreeNodeId to optionally skip speculative?

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Apr 19, 2018

I was already doing that in the previous iteration and ci testa were failing:

Yup that was because NavigationHandle::GetRenderFrameHost cannot be used before it has committed.

So should I revert to this or modify FrameHostFromTreeNodeId to optionally skip speculative?

Modify DidFinishNavigation to the following

void WebContents::DidFinishNavigation(
    content::NavigationHandle* navigation_handle) {
  bool is_main_frame = navigation_handle->IsInMainFrame();
  if (navigation_handle->HasCommitted()) {
    content::RenderFrameHost* frame_host =
        navigation_handle->GetRenderFrameHost();
    int frame_process_id = -1, frame_routing_id = -1;
    if (frame_host) {
      frame_process_id = frame_host->GetProcess()->GetID();
      frame_routing_id = frame_host->GetRoutingID();
    }
    if (!navigation_handle->IsErrorPage()) {
      auto url = navigation_handle->GetURL();
      bool is_same_document = navigation_handle->IsSameDocument();
      if (is_main_frame && !is_same_document) {
        Emit("did-navigate", url);
      } else if (is_same_document) {
        Emit("did-navigate-in-page", url, is_main_frame, frame_process_id,
             frame_routing_id);
      }
    } else {
      auto url = navigation_handle->GetURL();
      int code = navigation_handle->GetNetErrorCode();
      auto description = net::ErrorToShortString(code);
      Emit("did-fail-provisional-load", code, description, url, is_main_frame,
           frame_process_id, frame_routing_id);

      // Do not emit "did-fail-load" for canceled requests.
      if (code != net::ERR_ABORTED)
        Emit("did-fail-load", code, description, url, is_main_frame,
             frame_process_id, frame_routing_id);
    }
  }
}

Also the helper function FrameHostFromTreeNodeId is not needed, as it is only useful inside DidStartNavigation, can pull the implementation inside it.

@bughit

This comment has been minimized.

Contributor

bughit commented Apr 19, 2018

Modify DidFinishNavigation to the following

This changes the behavior in the following way:

  • before: if navigation_handle has not committed it would emit did-fail- events
  • after: won't emit anything

Is this what you intended? Was the old behavior a bug?

What does nav handle committed mean?
What is the difference between DidFinishNavigation with committed handle and without?

@bughit

This comment has been minimized.

Contributor

bughit commented Apr 19, 2018

given this explanation

  // Whether the navigation has committed. Navigations that end up being
  // downloads or return 204/205 response codes do not commit (i.e. the
  // WebContents stays at the existing URL).
  // This returns true for either successful commits or error pages that
  // replace the previous page (distinguished by |IsErrorPage|), and false for
  // errors that leave the user on the previous page.

the old (current) behavior does seem buggy as it would emit fails for uncommitted successes

but doesn't the new ignore uncommitted fails?

@bughit

This comment has been minimized.

Contributor

bughit commented Apr 20, 2018

@deepak1556 Will you suggested edit miss "errors that leave the user on the previous page." or will DidFailLoad catch those?

@bughit

This comment has been minimized.

Contributor

bughit commented Apr 23, 2018

@zcbenz The current WebContents::DidFinishNavigation code tries to handle uncommitted navigations (perhaps incorrectly). Changes suggested by @deepak1556, fix my issue but ignore both uncommitted successes and failures. I am happy to include them, but i don't want to introduce bugs and just need a confirmation that ignoring uncommitted in DidFinishNavigation is correct (would also be good to understand why)

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Apr 25, 2018

@bughit The change looks good to me.

@zcbenz

zcbenz approved these changes Apr 25, 2018

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Apr 25, 2018

Circle CI is having some infrastructure problems, I'm holding on merging until it is resolved.

bughit and others added some commits Apr 13, 2018

expose WebFrame#routingId and pass it to WebContents frame specific e…
…vents along with frameProcessId; add WebContets.did-start-navigation event

@zcbenz zcbenz closed this Apr 26, 2018

@zcbenz zcbenz reopened this Apr 26, 2018

@zcbenz zcbenz merged commit 4fcd178 into electron:master Apr 26, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@bughit bughit deleted the bughit:web_frame_id branch May 1, 2018

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