Move LoadProgressTracker to the browser process.

Move LoadProgressTracker to the browser process.

BUG=369531
TEST=everything still works

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271421

[nasko, 2014-05-02 22:48:23]

Mostly there, but I think it is broken when we have real out-of-process iframes.
See comments.

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/l...
File content/browser/frame_host/load_progress_tracker_browsertest.cc (right):

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/l...
content/browser/frame_host/load_progress_tracker_browsertest.cc:54:
test_server()->GetURL("files/site_isolation/blank.html"));
It will be nice if this test actually does real cross-site iframes, so it
verifies tracking works across processes. CrossSiteIframe is a test that does
that and can be used as a basis for this.

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/l...
content/browser/frame_host/load_progress_tracker_browsertest.cc:55:
WaitForLoadStop(shell()->web_contents());
This wait seems redundant, as NavigateToURL will wait until load stopped.

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_delegate.h:109: // The page made progress
loading.
nit: Why break up the line in the middle, it has lots more space to fit text
from below.

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:407: if (delegate_)
Don't we have to track that all frames are complete loading before dispatching
this to the delegate? If we have two frames in two processes, each will send
DidStopLoading. This will cause the delegate to think everything is loaded
whenever the first frame is complete.

https://codereview.chromium.org/263973003/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/263973003/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:2920: if (!render_view_->is_loading())
Shouldn't this counting move to the browser process as well? When a frame is in
another process, this one will not know of the loading state.

[jam, 2014-05-02 22:50:21]

per my email on the site isolation mailing list, i think this stuff should hang
off WebContents and not Navigator.

[jam, 2014-05-02 23:22:33]

On 2014/05/02 22:50:21, jam wrote:
> per my email on the site isolation mailing list, i think this stuff should
hang
> off WebContents and not Navigator.

to expand on the above:
-navigator adds extra work (i.e. each new method has to be added to navigator,
navigator_impl.*, and then to talk back to WC there is the delegate interface).
so let's keep the current code until we get webview working as a nested
webcontents, and then we can look at the code with/without navigator to see
which is better

-for new code, like this load progress stuff, there's no reason to put it in
navigator. the WebContentsObserver here should be dispatching the IPCs
themselves. wait for https://codereview.chromium.org/253013002/ to land and then
it'll be more convenient; and the cl should be smaller.

[Avi (use Gerrit), 2014-05-05 15:15:02]

I don't have full answers yet. Some easy things can be dispatched, though.

Nasko: some confusion comes into play where RenderFrameHostDelegate claims that
DidStart/StopLoading is only called on the top-level frames. However, I cannot
figure out where that happens. From what I can see, they're called from every
frame.

Nasko/John: How do we decide where this goes?

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/l...
File content/browser/frame_host/load_progress_tracker_browsertest.cc (right):

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/l...
content/browser/frame_host/load_progress_tracker_browsertest.cc:54:
test_server()->GetURL("files/site_isolation/blank.html"));
On 2014/05/02 22:48:23, nasko wrote:
> It will be nice if this test actually does real cross-site iframes, so it
> verifies tracking works across processes. CrossSiteIframe is a test that does
> that and can be used as a basis for this.

I'm looking at that test and I'm not convinced that I fully understand how it's
doing its setup. I don't want to cargo-cult it. Can we have a common setup that
forces a cross-process scenario?

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/l...
content/browser/frame_host/load_progress_tracker_browsertest.cc:55:
WaitForLoadStop(shell()->web_contents());
On 2014/05/02 22:48:23, nasko wrote:
> This wait seems redundant, as NavigateToURL will wait until load stopped.

Good point.

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_delegate.h:109: // The page made progress
loading.
On 2014/05/02 22:48:23, nasko wrote:
> nit: Why break up the line in the middle, it has lots more space to fit text
> from below.

Done.

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:407: if (delegate_)
On 2014/05/02 22:48:23, nasko wrote:
> Don't we have to track that all frames are complete loading before dispatching
> this to the delegate? If we have two frames in two processes, each will send
> DidStopLoading. This will cause the delegate to think everything is loaded
> whenever the first frame is complete.

Let me think about this one for a while.

https://codereview.chromium.org/263973003/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/263973003/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:2920: if (!render_view_->is_loading())
On 2014/05/02 22:48:23, nasko wrote:
> Shouldn't this counting move to the browser process as well? When a frame is
in
> another process, this one will not know of the loading state.

Probably. Hmmm....

[nasko, 2014-05-05 15:30:56]

DidStartLoading/DidStopLoading were dispatched through WebViewCLient, so they
were only sent through the top-level RenderFrameHost. We need to update that
comment.
In general, the logic that was in RenderViewImpl to balance them out, should
move to the browser side and only dispatch stop event when all started frames
are stopped.

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/l...
File content/browser/frame_host/load_progress_tracker_browsertest.cc (right):

https://codereview.chromium.org/263973003/diff/1/content/browser/frame_host/l...
content/browser/frame_host/load_progress_tracker_browsertest.cc:54:
test_server()->GetURL("files/site_isolation/blank.html"));
On 2014/05/05 15:15:02, Avi wrote:
> On 2014/05/02 22:48:23, nasko wrote:
> > It will be nice if this test actually does real cross-site iframes, so it
> > verifies tracking works across processes. CrossSiteIframe is a test that
does
> > that and can be used as a basis for this.
> 
> I'm looking at that test and I'm not convinced that I fully understand how
it's
> doing its setup. I don't want to cargo-cult it. Can we have a common setup
that
> forces a cross-process scenario?

Long term I think we will have such setup, Nick's been thinking about how to do
it. The more I think abou it, CrossSiteIframe will not load the two frames in
separate processes in the initial load, so that might not be the best test.

[Avi (use Gerrit), 2014-05-05 15:53:21]

On 2014/05/05 15:30:56, nasko wrote:
> DidStartLoading/DidStopLoading were dispatched through WebViewCLient, so they
> were only sent through the top-level RenderFrameHost. We need to update that
> comment.

Comment update is in http://crrev.com/262173008. But my question is now how does
this work? If that IPC/delegate call is indeed per-frame and not top-level, how
does the loading indicator work properly today?

[nasko, 2014-05-05 16:07:28]

On 2014/05/05 15:53:21, Avi wrote:
> On 2014/05/05 15:30:56, nasko wrote:
> > DidStartLoading/DidStopLoading were dispatched through WebViewCLient, so
they
> > were only sent through the top-level RenderFrameHost. We need to update that
> > comment.
> 
> Comment update is in http://crrev.com/262173008. But my question is now how
does
> this work? If that IPC/delegate call is indeed per-frame and not top-level,
how
> does the loading indicator work properly today?

The logic in RenderView only allows it to be dispatched once, no? It just
happens that the last frame that stops loading sends it, not the top-level one.
In default Chrome, all frames are in the same process, so it works out fine. I
don't think it works properly in --site-per-process.

[Avi (use Gerrit), 2014-05-05 16:37:47]

Yeah, I see it now.

I was hoping to not have to move over frames_in_progress_ but ugh it doesn't
look like I have much of a choice.

[Avi (use Gerrit), 2014-05-05 16:39:24]

On 2014/05/02 23:22:33, jam wrote:
> so let's keep the current code until we get webview working as a nested
> webcontents, and then we can look at the code with/without navigator to see
> which is better

Are you asking me to hold off on this CL until that time?

> the WebContentsObserver here should be dispatching the IPCs
> themselves.

There is no WebContentsObserver here. What are you referring to?

[jam, 2014-05-05 17:48:34]

On 2014/05/05 16:39:24, Avi wrote:
> On 2014/05/02 23:22:33, jam wrote:
> > so let's keep the current code until we get webview working as a nested
> > webcontents, and then we can look at the code with/without navigator to see
> > which is better
> 
> Are you asking me to hold off on this CL until that time?

I was referring to the existing code in content/browser/frame_host/navigator*.
for this new code, no need to put it there. i.e. putting additional code in
navigator would only add extra work now (because of the interfaces back/to it)
and later (if we remove it).

> 
> > the WebContentsObserver here should be dispatching the IPCs
> > themselves.
> 
> There is no WebContentsObserver here. What are you referring to?

[Avi (use Gerrit), 2014-05-05 18:24:13]

Nasko, what do you think of this approach?

The upshot (as I understand it) is that we would send the IPCs from RenderFrame,
but receive (and manage them) directly in a WebContentsObserver owned by the
NavigatorImpl. No changes to Navigator would be needed.

[Avi (use Gerrit), 2014-05-05 18:28:01]

On 2014/05/05 18:24:13, Avi wrote:
> Nasko, what do you think of this approach?

"this approach" being the architecture that John proposed.

[Avi (use Gerrit), 2014-05-05 22:28:42]

PTAL. This should be better.

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:981: // LoadingProgressMap maps
FrameTreeNode IDs to a double representing that
A FrameTreeNode ID rather than a routing ID like before. These frames are
eventually going to be from random renderer processes, so we need something
globally unique.

[jam, 2014-05-05 23:19:10]

lgtm

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:341: // Called once when the
first frame on the page has started loading.
nit: in general comments go in the interface and not in implementations

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:845: void
SendLoadProgressChanged();
since these two methods are called internally only, they should move to the
private section

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:848: void
DidStopLoading(RenderFrameHost* render_frame_host);
this is only called by OnDidStopLoading() right? if so seems like it should just
be inlined

[Avi (use Gerrit), 2014-05-05 23:34:27]

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:341: // Called once when the
first frame on the page has started loading.
On 2014/05/05 23:19:10, jam wrote:
> nit: in general comments go in the interface and not in implementations

Done.

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:845: void
SendLoadProgressChanged();
On 2014/05/05 23:19:10, jam wrote:
> since these two methods are called internally only, they should move to the
> private section

This *is* the private section.

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:848: void
DidStopLoading(RenderFrameHost* render_frame_host);
On 2014/05/05 23:19:10, jam wrote:
> this is only called by OnDidStopLoading() right?

Nope. This is called by OnDidStopLoading and by OnDialogClosed.

[nasko, 2014-05-05 23:51:11]

A lot better! Just a few comments.

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2329: if (is_main_frame)
Why did you remove this call?

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:341: // Called once when the
first frame on the page has started loading.
This isn't quite correct. It is called when we start a browser-initiated
navigation based on the current code.
Now I wonder if we properly account for that case.

https://codereview.chromium.org/263973003/diff/40001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/263973003/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.h:1009: int frames_in_progress_;
I don't think this should stay here long term, should it? If not a TODO to say
when it can go will be useful.

https://codereview.chromium.org/263973003/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/263973003/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_delegate.h:49: // to force a
loading spinner to show up; TODO(avi): Remove this.
I'm not sure that this is a hack. It seems this is used to intentionally
indicate that a load has started as soon as possible in the case of
browser-initiated navigation.

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:982: typedef
base::hash_map<int64, double> LoadingProgressMap;
I think this is the right choice, but one note here. When doing a cross-process
navigation, we start in one process, then in another, and if the navigation will
commit we stop the first process and let the new one finish the navigation.
I know we see didStartProvisionalLoad from both processes, not sure about
didStartLoading. If that's the case, we will overwrite the map entry.

[Avi (use Gerrit), 2014-05-06 00:03:39]

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/263973003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2329: if (is_main_frame)
On 2014/05/05 23:51:12, nasko wrote:
> Why did you remove this call?

By the time DidStartProvisionalLoad is called, the frames have already started
reporting progress. This was firing a "did make progress 0" after "did make
progress 0.1" was already sent.

https://codereview.chromium.org/263973003/diff/40001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/263973003/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.h:1009: int frames_in_progress_;
On 2014/05/05 23:51:12, nasko wrote:
> I don't think this should stay here long term, should it? If not a TODO to say
> when it can go will be useful.

Done.

https://codereview.chromium.org/263973003/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/263973003/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_delegate.h:49: // to force a
loading spinner to show up; TODO(avi): Remove this.
On 2014/05/05 23:51:12, nasko wrote:
> I'm not sure that this is a hack. It seems this is used to intentionally
> indicate that a load has started as soon as possible in the case of
> browser-initiated navigation. 

It feels unbalanced to me. At points we have to force the UI to stop too, and it
doesn't flow well.

I'll revert this comment, but the current structure feels off.

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:982: typedef
base::hash_map<int64, double> LoadingProgressMap;
On 2014/05/05 23:51:12, nasko wrote:
> I think this is the right choice, but one note here. When doing a
cross-process
> navigation, we start in one process, then in another, and if the navigation
will
> commit we stop the first process and let the new one finish the navigation.
> I know we see didStartProvisionalLoad from both processes, not sure about
> didStartLoading. If that's the case, we will overwrite the map entry.

I am not experienced enough in the loading code to know the details of how
things flow here. If you have advice I'd be happy to take it.

[nasko, 2014-05-06 00:08:03]

Adding japhet@ and creis@ to comment on the imbalance of DidStartLoading for
browser-initiated navigations, as both of them have more experience in this area
than myself.

[Nate Chapin, 2014-05-07 20:16:02]

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:982: typedef
base::hash_map<int64, double> LoadingProgressMap;
On 2014/05/05 23:51:12, nasko wrote:
> I think this is the right choice, but one note here. When doing a
cross-process
> navigation, we start in one process, then in another, and if the navigation
will
> commit we stop the first process and let the new one finish the navigation.
> I know we see didStartProvisionalLoad from both processes, not sure about
> didStartLoading. If that's the case, we will overwrite the map entry.

Unless we create a way to notify the new process that it is a continuation of a
load that was initiated in a different process, then yes, we will receive
didStartLoading() from both processes. I would think that it's straightforward
to account for that case in WebContentsImpl?

[Charlie Reis, 2014-05-07 23:50:59]

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:982: typedef
base::hash_map<int64, double> LoadingProgressMap;
On 2014/05/07 20:16:02, Nate Chapin wrote:
> On 2014/05/05 23:51:12, nasko wrote:
> > I think this is the right choice, but one note here. When doing a
> cross-process
> > navigation, we start in one process, then in another, and if the navigation
> will
> > commit we stop the first process and let the new one finish the navigation.
> > I know we see didStartProvisionalLoad from both processes, not sure about
> > didStartLoading. If that's the case, we will overwrite the map entry.
> 
> Unless we create a way to notify the new process that it is a continuation of
a
> load that was initiated in a different process, then yes, we will receive
> didStartLoading() from both processes. I would think that it's straightforward
> to account for that case in WebContentsImpl?

Yes, I don't think it should cause a big problem.  Maybe progress will go
backwards during the swap (depending on whether we start reporting progress
before commit), but we'll have the same FrameTreeNode ID after the swap.

Would make sense to cover this case in a browser test, though, to make sure it
doesn't hit any unexpected checks.

https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:992: int
loading_frames_in_progress_;
Are we updating this if the process crashes?  I'm curious if the throbber will
continue to spin if the renderer dies before hearing DidStopLoading (e.g., with
a slow subframe).

https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_browsertest.cc:450:
IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, LoadProgress) {
This doesn't seem to test that DidStartLoading and DidStopLoading are called,
unlike the RenderViewImplTest.SendProgressCompletionUpdates test that was
removed.  Is there a way to test that here?

https://codereview.chromium.org/263973003/diff/50019/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/263973003/diff/50019/content/renderer/render_...
content/renderer/render_view_impl.cc:1569: FOR_EACH_OBSERVER(RenderViewObserver,
observers_, DidStartLoading());
No need to address this in in this CL, but I'm curious how we're going to handle
this.  A single process won't know when we start or stop loading when some of
its frames are in other processes.  Maybe we'll send a DidStartLoading IPC from
the browser to all of a tab's renderer processes?

[Nate Chapin, 2014-05-08 00:19:44]

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:982: typedef
base::hash_map<int64, double> LoadingProgressMap;
On 2014/05/07 23:51:00, Charlie Reis wrote:
> On 2014/05/07 20:16:02, Nate Chapin wrote:
> > On 2014/05/05 23:51:12, nasko wrote:
> > > I think this is the right choice, but one note here. When doing a
> > cross-process
> > > navigation, we start in one process, then in another, and if the
navigation
> > will
> > > commit we stop the first process and let the new one finish the
navigation.
> > > I know we see didStartProvisionalLoad from both processes, not sure about
> > > didStartLoading. If that's the case, we will overwrite the map entry.
> > 
> > Unless we create a way to notify the new process that it is a continuation
of
> a
> > load that was initiated in a different process, then yes, we will receive
> > didStartLoading() from both processes. I would think that it's
straightforward
> > to account for that case in WebContentsImpl?
> 
> Yes, I don't think it should cause a big problem.  Maybe progress will go
> backwards during the swap (depending on whether we start reporting progress
> before commit), but we'll have the same FrameTreeNode ID after the swap.
> 
> Would make sense to cover this case in a browser test, though, to make sure it
> doesn't hit any unexpected checks.

We should be able to avoid going backwards in the continuation case. Currently,
we set a minimum completion of 0.1 at load start, and I don't think we increase
it only when data is received (which should mean no sooner than commit time,
when we're in the correct process).

[Avi (use Gerrit), 2014-05-13 21:08:07]

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:982: typedef
base::hash_map<int64, double> LoadingProgressMap;
**THIS ISSUE REMAINS UNADDRESSED**

In the continuation case, does the old frame indicate that its loading has
ended? If so, this shouldn't be horrible. OTOH, if it doesn't, then things will
wait forever.

You refer to a browser test. I need one for this case. Is there a good way to
construct one? We need a good way to churn those out, so that we can test that
case in key places.

https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:992: int
loading_frames_in_progress_;
On 2014/05/07 23:51:00, Charlie Reis(OOO as of May 17) wrote:
> Are we updating this if the process crashes?  I'm curious if the throbber will
> continue to spin if the renderer dies before hearing DidStopLoading (e.g.,
with
> a slow subframe).

What happens today? Today we'll get a FrameHostMsg_DidStartLoading without a
FrameHostMsg_DidStopLoading, but WebContentsImpl::RenderViewTerminated does do
some cleanup by calling SetIsLoading(false).

I will address this.

BTW, what does it mean to be "render process crashed" in the future, when
there's more than one render process servicing a page?

https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_browsertest.cc:450:
IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, LoadProgress) {
On 2014/05/07 23:51:00, Charlie Reis(OOO as of May 17) wrote:
> This doesn't seem to test that DidStartLoading and DidStopLoading are called,
> unlike the RenderViewImplTest.SendProgressCompletionUpdates test that was
> removed.  Is there a way to test that here?

Done.

https://codereview.chromium.org/263973003/diff/50019/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/263973003/diff/50019/content/renderer/render_...
content/renderer/render_view_impl.cc:1569: FOR_EACH_OBSERVER(RenderViewObserver,
observers_, DidStartLoading());
On 2014/05/07 23:51:00, Charlie Reis(OOO as of May 17) wrote:
> I'm curious how we're going to handle this.

So am I :) I'll figure this out later, but your thoughts seem reasonable.

The big question is who is relying on this; can we remove the need for it
entirely?

[Avi (use Gerrit), 2014-05-13 22:31:08]

(I am looking into why this times out on a whole bunch of tests. It's
reproducible, though :)

[Charlie Reis, 2014-05-13 22:44:35]

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:982: typedef
base::hash_map<int64, double> LoadingProgressMap;
On 2014/05/13 21:08:08, Avi wrote:
> **THIS ISSUE REMAINS UNADDRESSED**
> 
> In the continuation case, does the old frame indicate that its loading has
> ended? If so, this shouldn't be horrible. OTOH, if it doesn't, then things
will
> wait forever.
> 

We may need to try this out to be sure, since I'm not sure if we'll always hear
a stop from the old frame.  I think transfers should start in one process, issue
a DidFail + DidStop, and then be taken over by the new process. 
Browser-initiated cross-process navigations take a different path, and Nasko
thinks that may have an extra DidStart, but he's checking.  This is all just
speculation on my part, though.

> You refer to a browser test. I need one for this case. Is there a good way to
> construct one? We need a good way to churn those out, so that we can test that
> case in key places.

Yes, I think WebContentsImplBrowserTest has a set of LoadingStateChanged tests. 
Nick may be able to help you with setting one up for cross-process subframes if
we want to exercise that path.

https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:992: int
loading_frames_in_progress_;
On 2014/05/13 21:08:08, Avi wrote:
> On 2014/05/07 23:51:00, Charlie Reis(OOO as of May 17) wrote:
> > Are we updating this if the process crashes?  I'm curious if the throbber
will
> > continue to spin if the renderer dies before hearing DidStopLoading (e.g.,
> with
> > a slow subframe).
> 
> What happens today? Today we'll get a FrameHostMsg_DidStartLoading without a
> FrameHostMsg_DidStopLoading, but WebContentsImpl::RenderViewTerminated does do
> some cleanup by calling SetIsLoading(false).

Yes, that's probably the reason the throbber stops spinning.

> 
> I will address this.
> 
> BTW, what does it mean to be "render process crashed" in the future, when
> there's more than one render process servicing a page?

Right, the whole tab won't necessarily be a sad tab when one of its renderer
processes dies.  We partly deal with this in FrameTree::RenderProcessGone, which
is called just before WebContentsImpl::RenderViewTerminated.  That clears out
any subframes of RenderFrameHosts with the crashed RenderViewHost, leaving a sad
frame the RenderFrameHosts themselves.  (We may end up needing to update
RenderViewTerminated further, though.)

https://codereview.chromium.org/263973003/diff/50019/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/263973003/diff/50019/content/renderer/render_...
content/renderer/render_view_impl.cc:1569: FOR_EACH_OBSERVER(RenderViewObserver,
observers_, DidStartLoading());
On 2014/05/13 21:08:08, Avi wrote:
> On 2014/05/07 23:51:00, Charlie Reis(OOO as of May 17) wrote:
> > I'm curious how we're going to handle this.
> 
> So am I :) I'll figure this out later, but your thoughts seem reasonable.
> 
> The big question is who is relying on this; can we remove the need for it
> entirely?

That's a great question.  Looks like only 4 clients listen for it, and it may
make sense to move some/all of them to the browser process.

[nasko, 2014-05-13 23:05:53]

On 2014/05/13 22:44:35, Charlie Reis(OOO as of May 17) wrote:
>
https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
> File content/browser/web_contents/web_contents_impl.h (right):
> 
>
https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
> content/browser/web_contents/web_contents_impl.h:982: typedef
> base::hash_map<int64, double> LoadingProgressMap;
> On 2014/05/13 21:08:08, Avi wrote:
> > **THIS ISSUE REMAINS UNADDRESSED**
> > 
> > In the continuation case, does the old frame indicate that its loading has
> > ended? If so, this shouldn't be horrible. OTOH, if it doesn't, then things
> will
> > wait forever.
> > 
> 
> We may need to try this out to be sure, since I'm not sure if we'll always
hear
> a stop from the old frame.  I think transfers should start in one process,
issue
> a DidFail + DidStop, and then be taken over by the new process. 
> Browser-initiated cross-process navigations take a different path, and Nasko
> thinks that may have an extra DidStart, but he's checking.  

The extra DidStart is from the browser process in browser-initiated navigations
and it is ok, as it doesn't touch the counter.

> This is all just
> speculation on my part, though.
> 
> > You refer to a browser test. I need one for this case. Is there a good way
to
> > construct one? We need a good way to churn those out, so that we can test
that
> > case in key places.
> 
> Yes, I think WebContentsImplBrowserTest has a set of LoadingStateChanged
tests. 
> Nick may be able to help you with setting one up for cross-process subframes
if
> we want to exercise that path.
> 
>
https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
> File content/browser/web_contents/web_contents_impl.h (right):
> 
>
https://codereview.chromium.org/263973003/diff/50019/content/browser/web_cont...
> content/browser/web_contents/web_contents_impl.h:992: int
> loading_frames_in_progress_;
> On 2014/05/13 21:08:08, Avi wrote:
> > On 2014/05/07 23:51:00, Charlie Reis(OOO as of May 17) wrote:
> > > Are we updating this if the process crashes?  I'm curious if the throbber
> will
> > > continue to spin if the renderer dies before hearing DidStopLoading (e.g.,
> > with
> > > a slow subframe).
> > 
> > What happens today? Today we'll get a FrameHostMsg_DidStartLoading without a
> > FrameHostMsg_DidStopLoading, but WebContentsImpl::RenderViewTerminated does
do
> > some cleanup by calling SetIsLoading(false).
> 
> Yes, that's probably the reason the throbber stops spinning.
> 
> > 
> > I will address this.
> > 
> > BTW, what does it mean to be "render process crashed" in the future, when
> > there's more than one render process servicing a page?
> 
> Right, the whole tab won't necessarily be a sad tab when one of its renderer
> processes dies.  We partly deal with this in FrameTree::RenderProcessGone,
which
> is called just before WebContentsImpl::RenderViewTerminated.  That clears out
> any subframes of RenderFrameHosts with the crashed RenderViewHost, leaving a
sad
> frame the RenderFrameHosts themselves.  (We may end up needing to update
> RenderViewTerminated further, though.)
> 
>
https://codereview.chromium.org/263973003/diff/50019/content/renderer/render_...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/263973003/diff/50019/content/renderer/render_...
> content/renderer/render_view_impl.cc:1569:
FOR_EACH_OBSERVER(RenderViewObserver,
> observers_, DidStartLoading());
> On 2014/05/13 21:08:08, Avi wrote:
> > On 2014/05/07 23:51:00, Charlie Reis(OOO as of May 17) wrote:
> > > I'm curious how we're going to handle this.
> > 
> > So am I :) I'll figure this out later, but your thoughts seem reasonable.
> > 
> > The big question is who is relying on this; can we remove the need for it
> > entirely?
> 
> That's a great question.  Looks like only 4 clients listen for it, and it may
> make sense to move some/all of them to the browser process.

[Avi (use Gerrit), 2014-05-14 00:14:32]

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:982: typedef
base::hash_map<int64, double> LoadingProgressMap;
This already does a good job of failing just about every CrossSiteTransferTest
and half the SitePerProcessBrowserTests. Can I rely on those?

[Charlie Reis, 2014-05-14 17:18:33]

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:982: typedef
base::hash_map<int64, double> LoadingProgressMap;
On 2014/05/14 00:14:32, Avi wrote:
> This already does a good job of failing just about every CrossSiteTransferTest
> and half the SitePerProcessBrowserTests. Can I rely on those?

Yeah, the SitePerProcessBrowserTests exercise the subframe loading case and
should be sufficient.  Note that there's a TODO in NavigateIframeToURL hacking
around this issue: we're listening for a commit instead of load stop, because of
the duplicate load start/stops.

(I think Nasko's new NavigateFrameToURL utility method also does this.)

I mention this because it's fine if we end up hiding these duplicate start/stops
and make it possible for tests to listen for load stop again.  Not sure if
that'll happen here, though.

[nasko, 2014-05-14 17:36:33]

On 2014/05/14 17:18:33, Charlie Reis(OOO as of May 17) wrote:
>
https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
> File content/browser/web_contents/web_contents_impl.h (right):
> 
>
https://codereview.chromium.org/263973003/diff/60001/content/browser/web_cont...
> content/browser/web_contents/web_contents_impl.h:982: typedef
> base::hash_map<int64, double> LoadingProgressMap;
> On 2014/05/14 00:14:32, Avi wrote:
> > This already does a good job of failing just about every
CrossSiteTransferTest
> > and half the SitePerProcessBrowserTests. Can I rely on those?
> 
> Yeah, the SitePerProcessBrowserTests exercise the subframe loading case and
> should be sufficient.  Note that there's a TODO in NavigateIframeToURL hacking
> around this issue: we're listening for a commit instead of load stop, because
of
> the duplicate load start/stops.
> 
> (I think Nasko's new NavigateFrameToURL utility method also does this.)

The NavigateFrameToURL uses navigation events, not load start/stop, since it
targets a specific frame and start/stop loading is more geared towards page load
(possibly multiple frames).

> I mention this because it's fine if we end up hiding these duplicate
start/stops
> and make it possible for tests to listen for load stop again.  Not sure if
> that'll happen here, though.

[Avi (use Gerrit), 2014-05-16 20:30:36]

OK, Charlie, Nasko, this seems to work.

[nasko, 2014-05-16 22:19:46]

LGTM with a couple of nits.

https://codereview.chromium.org/263973003/diff/130001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/263973003/diff/130001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2643: // It is possible for a
frame to make multiple calls to OnDidStartLoading
nit: It will be nice to qualify "frame" here to distinguish between
FrameTreeNode and RenderFrameHost. We can get only one DidStartLoading per RFH,
but we can have multiple coming from a FrameTreeNode, since there could be two
RFHs at a time under the same node.

https://codereview.chromium.org/263973003/diff/130001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2647: // "swappedout://" URLs
go away and Camille moves navigation network requests
nit: If a specific person is mentioned, shouldn't it be by username@ to avoid
name collisions and make reaching out easier?

[Charlie Reis, 2014-05-16 23:13:45]

LGTM

https://codereview.chromium.org/263973003/diff/130001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/263973003/diff/130001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3414: // Reset the loading
progress. TODO(avi): What does it mean to have a
Yeah, leaving this as a TODO is fine for now, but it's a good question.  With
--site-per-process, it's entirely possible for one subframe to crash while a
different one is loading, such that the tab continues to stay in a loading state
after the crash.  I suppose we should be resetting loading_progresses_ for any
frames affected by the crash (i.e., which have the given rvh).

[Avi (use Gerrit), 2014-05-16 23:29:59]

https://codereview.chromium.org/263973003/diff/130001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/263973003/diff/130001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2643: // It is possible for a
frame to make multiple calls to OnDidStartLoading
Rewritten; please verify.

https://codereview.chromium.org/263973003/diff/130001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2647: // "swappedout://" URLs
go away and Camille moves navigation network requests
On 2014/05/16 22:19:46, nasko wrote:
> nit: If a specific person is mentioned, shouldn't it be by username@ to avoid
> name collisions and make reaching out easier?

Done.

[nasko, 2014-05-16 23:45:58]

We now use RFHs for navigations and progressively more :).

https://codereview.chromium.org/263973003/diff/150001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/263973003/diff/150001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2640: // - Tomorrow, there
might be more than one RenderFrameHost sharing the same
This is true even today.

[Avi (use Gerrit), 2014-05-19 14:39:12]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-05-19 14:39:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/263973003/170001

[commit-bot: I haz the power, 2014-05-19 16:29:26]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-19 18:01:03]

Change committed as 271421