PlzNavigate: preserve SourceLocation when navigating

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 4af9b671725620814b3f5d10384a881f4c68d83a..68465f26085c8c1ede979a5127e4f6ec561a950d 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -675,6 +675,13 @@ CommonNavigationParams MakeCommonNavigationParams(
       navigation_type = FrameMsg_Navigate_Type::RELOAD;
   }
 
+  base::Optional<SourceLocation> source_location;
+  if (info.hasSourceLocation) {
+    source_location = SourceLocation(info.sourceLocation.url.latin1(),
+                                     info.sourceLocation.lineNumber,
+                                     info.sourceLocation.columnNumber);
+  }
+
   const RequestExtraData* extra_data =
       static_cast<RequestExtraData*>(info.urlRequest.getExtraData());
   DCHECK(extra_data);
@@ -684,7 +691,7 @@ CommonNavigationParams MakeCommonNavigationParams(
       report_type, GURL(), GURL(),
       static_cast<PreviewsState>(info.urlRequest.getPreviewsState()),
       base::TimeTicks::Now(), info.urlRequest.httpMethod().latin1(),
-      GetRequestBodyForWebURLRequest(info.urlRequest));
+      GetRequestBodyForWebURLRequest(info.urlRequest), source_location);
 }
 
 media::Context3D GetSharedMainThreadContext3D(
@@ -3415,6 +3422,20 @@ void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
     // UnloadEventStart and UnloadEventEnd are still missing.
   }
 
+  // PlzNavigate: update the source location before processing the navigation
+  // commit.
+  if (IsBrowserSideNavigationEnabled() &&
+      navigation_state->common_params().source_location.has_value()) {
+    blink::WebSourceLocation source_location;
+    source_location.url = WebString::fromLatin1(
+        navigation_state->common_params().source_location->url);
+    source_location.lineNumber =
+        navigation_state->common_params().source_location->line_number;
+    source_location.columnNumber =
+        navigation_state->common_params().source_location->column_number;
+    datasource->setSourceLocation(source_location);
+  }
+
   // Create the serviceworker's per-document network observing object if it
   // does not exist (When navigation happens within a page, the provider already
   // exists).
@@ -6079,6 +6100,14 @@ void RenderFrameImpl::NavigateInternal(
 
   // In case LoadRequest failed before didCreateDataSource was called.
   pending_navigation_params_.reset();
+
+  // PlzNavigate: reset the source location now that the commit checks have been
+  // processed.
+  if (IsBrowserSideNavigationEnabled()) {
+    frame_->dataSource()->resetSourceLocation();

[Nate Chapin, 2017/03/01 23:22:19]

Rather than needing to expose resetSourceLocation() and calling it here, could
we just clear it in DocumentLoader on commit?

[clamy, 2017/03/02 13:34:32]

The problem is that we're going to do an IPC before committing the navigation.
If any other Javascript runs in the meantime and ends up emitting a console
message, we will have the wrong SourceLocation set.

[Nate Chapin, 2017/03/03 00:17:05]

So the flow of that case would be:
* Start navigation that sets a SourceLocation on the provisionalDataSource().
* setTimeout() timer fires, JS runs and sends console message
* Data arrives from browser process, data source commits.

Is that right? If so, wouldn't the JS be associated with the committed
WebDataSource/DocumentLoader, instead of the provisional, so it would safely
ignore the SourceLocation? Or did I misunderstand?

[clamy, 2017/03/06 13:27:53]

That's the case I'm concerned about. Given how the code is written, I don't see
why we would associate it with the committed DataSource, since we override the
location when the provisional DataSource has a SourceLocation.

I do think that the correct thing to do is to only have the SourceLocation info
in the provisionalDataSource for one iteration of the message loop, which is
what this code is doing. After all, IIUC in the current code, you would not get
the SourceLocation for the navigation after the Javascript that triggered it
stopped executing, that is we switched to a new iteration in the message loop?

[Nate Chapin, 2017/03/13 17:45:42]

Ok, I think I can live with this. The combination of the special case in
FrameConsole.cpp and restricting it to a single message loop here is a bit
difficult to reason about. But I think the alternative is to audit all callers
of FrameConsole::addMessage() and figure out which ones should be sensitive to
the provisionalDocumentLoader()'s SourceLocation, and there may be some gray
areas.

+    if (frame_->provisionalDataSource())
+      frame_->provisionalDataSource()->resetSourceLocation();
+  }
 }
 
 void RenderFrameImpl::UpdateEncoding(WebFrame* frame,