Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2483933003:
blimp: Fix page load status tracking. (Closed)
|
Created:
4 years, 1 month ago by Khushal Modified:
4 years, 1 month ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionblimp: Fix page load status tracking.
Update the PageLoadTracker to use the non-deprecated APIs for tracking
the status of navigations for a RenderFrame. This change also
establishes the policy used for these updates for a tab clearly:
1) The progress bar will be reset when a navigation is initiated. If the
navigation fails, a completion update is sent immediately.
2) If the navigation commits, we wait for at least one frame for this
page to be sent to the client, before sending a completion update.
BUG=648299
Committed: https://crrev.com/1dcad9ce4e13062d7fe0fa8021749e9dab6942a6
Cr-Commit-Position: refs/heads/master@{#432071}
Patch Set 1 #
Total comments: 18
Patch Set 2 : cancelable callback #
Messages
Total messages: 24 (10 generated)
khushalsagar@chromium.org changed reviewers: + dtrainor@chromium.org
This is far from what the final solution for this should look like, but I think fine for now till we have a design for how navigation state is going to be integrated.
khushalsagar@chromium.org changed reviewers: + nasko@chromium.org
+nasko, it would be great if you could check that I'm using the WebContentsObserver APIs correctly, and not missing any case. Thanks!
general blimp code lgtm.
Thanks for converting this code! Few questions to make sure I understand the full picture. https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... File blimp/engine/session/page_load_tracker.cc (right): https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:44: << "There should be only one navigation in the main frame for a tab"; I don't think this is a correct DCHECK. It is quite possible to have a slow navigation to a cross-site URL which will require a cross-process navigation while in the meantime the existing document is invoking pushState, which is classified as "same-page" navigation. https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:57: if (navigation_handle->HasCommitted()) { Does this code care about error page vs real document? https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:58: // Make sure that at least one frame after the navigation commits is sent to nit: Be specific when you say "frame" as it is a very overloaded term. You probably mean a drawing frame, not a RenderFrame, right? https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:63: base::Bind(&PageLoadTracker::DidPaintAfterNavigationCommitted, This seems open to raciness. It could very well be that the browser has updated its state to have committed the navigation, but the IPC channel still has queued up a paint IPC from the old document.
https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... File blimp/engine/session/page_load_tracker.cc (right): https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:44: << "There should be only one navigation in the main frame for a tab"; On 2016/11/08 16:39:27, nasko wrote: > I don't think this is a correct DCHECK. It is quite possible to have a slow > navigation to a cross-site URL which will require a cross-process navigation > while in the meantime the existing document is invoking pushState, which is > classified as "same-page" navigation. I made sure to explicitly ignore same page navigations above in ShouldIgnoreNavigation. Given that notifications from this code are only being used for the progress bar, does that sound reasonable? https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:57: if (navigation_handle->HasCommitted()) { On 2016/11/08 16:39:27, nasko wrote: > Does this code care about error page vs real document? Not at the moment. This code is very ad-hoc, and should change when a more structured approach to navigation state sync is in place and how that will integrate with the client side UI. https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:58: // Make sure that at least one frame after the navigation commits is sent to On 2016/11/08 16:39:27, nasko wrote: > nit: Be specific when you say "frame" as it is a very overloaded term. You > probably mean a drawing frame, not a RenderFrame, right? Yes, it means an update to the compositor's content for this RenderFrame on the client. I'll change it to that and make it more explicit. https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:63: base::Bind(&PageLoadTracker::DidPaintAfterNavigationCommitted, On 2016/11/08 16:39:27, nasko wrote: > This seems open to raciness. It could very well be that the browser has updated > its state to have committed the navigation, but the IPC channel still has queued > up a paint IPC from the old document. Good point. Could I use a cancelable callback then? It can be reset each time a new navigation is committed? I think in the long run we would want tie compositor content updates with the navigation they correspond to. So this logic can be driven completely on the client. For instance, the client will have its own pending navigation, while still receiving content updates from the previous one.
https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... File blimp/engine/session/page_load_tracker.cc (right): https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:63: base::Bind(&PageLoadTracker::DidPaintAfterNavigationCommitted, On 2016/11/08 20:52:25, Khushal wrote: > On 2016/11/08 16:39:27, nasko wrote: > > This seems open to raciness. It could very well be that the browser has > updated > > its state to have committed the navigation, but the IPC channel still has > queued > > up a paint IPC from the old document. > > Good point. Could I use a cancelable callback then? It can be reset each time a > new navigation is committed? Sorry, I meant each time we get a new pending navigation. > > I think in the long run we would want tie compositor content updates with the > navigation they correspond to. So this logic can be driven completely on the > client. For instance, the client will have its own pending navigation, while > still receiving content updates from the previous one.
https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... File blimp/engine/session/page_load_tracker.cc (right): https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:44: << "There should be only one navigation in the main frame for a tab"; On 2016/11/08 20:52:25, Khushal wrote: > On 2016/11/08 16:39:27, nasko wrote: > > I don't think this is a correct DCHECK. It is quite possible to have a slow > > navigation to a cross-site URL which will require a cross-process navigation > > while in the meantime the existing document is invoking pushState, which is > > classified as "same-page" navigation. > > I made sure to explicitly ignore same page navigations above in > ShouldIgnoreNavigation. Given that notifications from this code are only being > used for the progress bar, does that sound reasonable? I still think it is possible without same page navigations. Feel free to leave it in, but don't be surprised if this DCHECK is hit. https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:57: if (navigation_handle->HasCommitted()) { On 2016/11/08 20:52:25, Khushal wrote: > On 2016/11/08 16:39:27, nasko wrote: > > Does this code care about error page vs real document? > > Not at the moment. This code is very ad-hoc, and should change when a more > structured approach to navigation state sync is in place and how that will > integrate with the client side UI. Acknowledged. https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:63: base::Bind(&PageLoadTracker::DidPaintAfterNavigationCommitted, On 2016/11/08 21:20:57, Khushal wrote: > On 2016/11/08 20:52:25, Khushal wrote: > > On 2016/11/08 16:39:27, nasko wrote: > > > This seems open to raciness. It could very well be that the browser has > > updated > > > its state to have committed the navigation, but the IPC channel still has > > queued > > > up a paint IPC from the old document. > > > > Good point. Could I use a cancelable callback then? It can be reset each time > a > > new navigation is committed? > > Sorry, I meant each time we get a new pending navigation. > > > > > I think in the long run we would want tie compositor content updates with the > > navigation they correspond to. So this logic can be driven completely on the > > client. For instance, the client will have its own pending navigation, while > > still receiving content updates from the previous one. > I'm not sure it will fully eliminate the race. If you have a document A and a navigation to document B, when document B commits, you might still have a drawing frame from document A in the IPC queue. kenrb@ has hit various different cases where navigations correlation to painting is hard, so maybe you can pull him in for a more expert review of that specific code.
khushalsagar@chromium.org changed reviewers: + kenrb@chromium.org
https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... File blimp/engine/session/page_load_tracker.cc (right): https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:44: << "There should be only one navigation in the main frame for a tab"; On 2016/11/08 21:55:15, nasko wrote: > On 2016/11/08 20:52:25, Khushal wrote: > > On 2016/11/08 16:39:27, nasko wrote: > > > I don't think this is a correct DCHECK. It is quite possible to have a slow > > > navigation to a cross-site URL which will require a cross-process navigation > > > while in the meantime the existing document is invoking pushState, which is > > > classified as "same-page" navigation. > > > > I made sure to explicitly ignore same page navigations above in > > ShouldIgnoreNavigation. Given that notifications from this code are only being > > used for the progress bar, does that sound reasonable? > > I still think it is possible without same page navigations. Feel free to leave > it in, but don't be surprised if this DCHECK is hit. Oh oh. I already came here trying to fix one DCHECK. I'll remove it then. https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:63: base::Bind(&PageLoadTracker::DidPaintAfterNavigationCommitted, On 2016/11/08 21:55:15, nasko wrote: > On 2016/11/08 21:20:57, Khushal wrote: > > On 2016/11/08 20:52:25, Khushal wrote: > > > On 2016/11/08 16:39:27, nasko wrote: > > > > This seems open to raciness. It could very well be that the browser has > > > updated > > > > its state to have committed the navigation, but the IPC channel still has > > > queued > > > > up a paint IPC from the old document. > > > > > > Good point. Could I use a cancelable callback then? It can be reset each > time > > a > > > new navigation is committed? > > > > Sorry, I meant each time we get a new pending navigation. > > > > > > > > I think in the long run we would want tie compositor content updates with > the > > > navigation they correspond to. So this logic can be driven completely on the > > > client. For instance, the client will have its own pending navigation, while > > > still receiving content updates from the previous one. > > > > I'm not sure it will fully eliminate the race. If you have a document A and a > navigation to document B, when document B commits, you might still have a > drawing frame from document A in the IPC queue. kenrb@ has hit various different > cases where navigations correlation to painting is hard, so maybe you can pull > him in for a more expert review of that specific code. Its true, we could have a drawing frame in the IPC queue for document A, but the callback corresponding to that frame should get dropped the moment that navigation is committed in the browser. We attach the id for the callback with the IPC message that is expected to be reflected back by the renderer. So the browser will still process the message, but the callback shouldn't run. +kenrb to take a look too. In case there is some other case we're missing.
https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... File blimp/engine/session/page_load_tracker.cc (right): https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:63: base::Bind(&PageLoadTracker::DidPaintAfterNavigationCommitted, On 2016/11/08 23:54:06, Khushal wrote: > On 2016/11/08 21:55:15, nasko wrote: > > On 2016/11/08 21:20:57, Khushal wrote: > > > On 2016/11/08 20:52:25, Khushal wrote: > > > > On 2016/11/08 16:39:27, nasko wrote: > > > > > This seems open to raciness. It could very well be that the browser has > > > > updated > > > > > its state to have committed the navigation, but the IPC channel still > has > > > > queued > > > > > up a paint IPC from the old document. > > > > > > > > Good point. Could I use a cancelable callback then? It can be reset each > > time > > > a > > > > new navigation is committed? > > > > > > Sorry, I meant each time we get a new pending navigation. > > > > > > > > > > > I think in the long run we would want tie compositor content updates with > > the > > > > navigation they correspond to. So this logic can be driven completely on > the > > > > client. For instance, the client will have its own pending navigation, > while > > > > still receiving content updates from the previous one. > > > > > > > I'm not sure it will fully eliminate the race. If you have a document A and a > > navigation to document B, when document B commits, you might still have a > > drawing frame from document A in the IPC queue. kenrb@ has hit various > different > > cases where navigations correlation to painting is hard, so maybe you can pull > > him in for a more expert review of that specific code. > > Its true, we could have a drawing frame in the IPC queue for document A, but the > callback corresponding to that frame should get dropped the moment that > navigation is committed in the browser. We attach the id for the callback with > the IPC message that is expected to be reflected back by the renderer. So the > browser will still process the message, but the callback shouldn't run. > > +kenrb to take a look too. In case there is some other case we're missing. The issues I had to deal with (and still haven't entirely fixed) relating to FirstPaintAfterLoad came from the fact that DidCommitProvisionalLoad and the first CompositorFrameSwap are not necessarily ordered, and also not necessarily paired (because for example the renderer could crash after sending one and never send the other). It sounds like associating an identifier with the callback should make it possible to solve this.
https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... File blimp/engine/session/page_load_tracker.cc (right): https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:44: << "There should be only one navigation in the main frame for a tab"; On 2016/11/08 23:54:06, Khushal wrote: > On 2016/11/08 21:55:15, nasko wrote: > > On 2016/11/08 20:52:25, Khushal wrote: > > > On 2016/11/08 16:39:27, nasko wrote: > > > > I don't think this is a correct DCHECK. It is quite possible to have a > slow > > > > navigation to a cross-site URL which will require a cross-process > navigation > > > > while in the meantime the existing document is invoking pushState, which > is > > > > classified as "same-page" navigation. > > > > > > I made sure to explicitly ignore same page navigations above in > > > ShouldIgnoreNavigation. Given that notifications from this code are only > being > > > used for the progress bar, does that sound reasonable? > > > > I still think it is possible without same page navigations. Feel free to leave > > it in, but don't be surprised if this DCHECK is hit. > > Oh oh. I already came here trying to fix one DCHECK. I'll remove it then. Done. https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:58: // Make sure that at least one frame after the navigation commits is sent to On 2016/11/08 20:52:25, Khushal wrote: > On 2016/11/08 16:39:27, nasko wrote: > > nit: Be specific when you say "frame" as it is a very overloaded term. You > > probably mean a drawing frame, not a RenderFrame, right? > > Yes, it means an update to the compositor's content for this RenderFrame on the > client. I'll change it to that and make it more explicit. Done. https://codereview.chromium.org/2483933003/diff/1/blimp/engine/session/page_l... blimp/engine/session/page_load_tracker.cc:63: base::Bind(&PageLoadTracker::DidPaintAfterNavigationCommitted, On 2016/11/10 18:11:42, kenrb wrote: > On 2016/11/08 23:54:06, Khushal wrote: > > On 2016/11/08 21:55:15, nasko wrote: > > > On 2016/11/08 21:20:57, Khushal wrote: > > > > On 2016/11/08 20:52:25, Khushal wrote: > > > > > On 2016/11/08 16:39:27, nasko wrote: > > > > > > This seems open to raciness. It could very well be that the browser > has > > > > > updated > > > > > > its state to have committed the navigation, but the IPC channel still > > has > > > > > queued > > > > > > up a paint IPC from the old document. > > > > > > > > > > Good point. Could I use a cancelable callback then? It can be reset each > > > time > > > > a > > > > > new navigation is committed? > > > > > > > > Sorry, I meant each time we get a new pending navigation. > > > > > > > > > > > > > > I think in the long run we would want tie compositor content updates > with > > > the > > > > > navigation they correspond to. So this logic can be driven completely on > > the > > > > > client. For instance, the client will have its own pending navigation, > > while > > > > > still receiving content updates from the previous one. > > > > > > > > > > I'm not sure it will fully eliminate the race. If you have a document A and > a > > > navigation to document B, when document B commits, you might still have a > > > drawing frame from document A in the IPC queue. kenrb@ has hit various > > different > > > cases where navigations correlation to painting is hard, so maybe you can > pull > > > him in for a more expert review of that specific code. > > > > Its true, we could have a drawing frame in the IPC queue for document A, but > the > > callback corresponding to that frame should get dropped the moment that > > navigation is committed in the browser. We attach the id for the callback with > > the IPC message that is expected to be reflected back by the renderer. So the > > browser will still process the message, but the callback shouldn't run. > > > > +kenrb to take a look too. In case there is some other case we're missing. > > The issues I had to deal with (and still haven't entirely fixed) relating to > FirstPaintAfterLoad came from the fact that DidCommitProvisionalLoad and the > first CompositorFrameSwap are not necessarily ordered, and also not necessarily > paired (because for example the renderer could crash after sending one and never > send the other). It sounds like associating an identifier with the callback > should make it possible to solve this. Thanks Ken. The id is already attached by the RenderFrameHostImpl when queuing the callback. I was just going to rely on that, and cancel the queued callback when receiving a DidStartNavigation for any new navigations. Do you mind taking another look?
The CQ bit was checked by khushalsagar@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.
I'll just go ahead and land this. I think the ids attached to IPC messages sent to the renderer should make sure that drawing frames from the previous navigation are handled correctly. Thanks for the input! :)
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2483933003/#ps20001 (title: "cancelable callback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== blimp: Fix page load status tracking. Update the PageLoadTracker to use the non-deprecated APIs for tracking the status of navigations for a RenderFrame. This change also establishes the policy used for these updates for a tab clearly: 1) The progress bar will be reset when a navigation is initiated. If the navigation fails, a completion update is sent immediately. 2) If the navigation commits, we wait for at least one frame for this page to be sent to the client, before sending a completion update. BUG=648299 ========== to ========== blimp: Fix page load status tracking. Update the PageLoadTracker to use the non-deprecated APIs for tracking the status of navigations for a RenderFrame. This change also establishes the policy used for these updates for a tab clearly: 1) The progress bar will be reset when a navigation is initiated. If the navigation fails, a completion update is sent immediately. 2) If the navigation commits, we wait for at least one frame for this page to be sent to the client, before sending a completion update. BUG=648299 Committed: https://crrev.com/1dcad9ce4e13062d7fe0fa8021749e9dab6942a6 Cr-Commit-Position: refs/heads/master@{#432071} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1dcad9ce4e13062d7fe0fa8021749e9dab6942a6 Cr-Commit-Position: refs/heads/master@{#432071} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
