Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1978793002:
RenderViewImpl::page_id_ isn't initialized for cross-process replacement navigations. (Closed)
|
Created:
4 years, 7 months ago by Andrey Kraynov Modified:
4 years, 7 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRenderViewImpl::page_id_ isn't initialized for cross-process replacement
navigations.
There can be a situation, when |RenderViewImpl::page_id_| isn't initialized
for a navigation that should replace the current entry. And then it will
trigger a DCHECK in |RenderFrameImpl::NavigateInternal|.
It happens when a new page is loaded with |should_replace_current_entry| flag
set to true and a new renderer is used for this navigation (i.e. the new entry
uses a different SiteInstance from the previous entry's one).
BUG=611679
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/4b2ec6d51e135654911cc38d0193b221ac042a08
Cr-Commit-Position: refs/heads/master@{#394515}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address review remarks #
Total comments: 4
Patch Set 3 : Fix nits and remove testing condition. #
Messages
Total messages: 21 (11 generated)
Description was changed from ========== RenderViewImpl::page_id_ isn't initialized for some special navigation. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens if the first navigation for WebContents occurs in a separate SiteInstance (and a renderer process), but second navigation will be initiated with flag |should_replace_current_entry| == true. BUG=611679 ========== to ========== RenderViewImpl::page_id_ isn't initialized for some special navigation. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens if the first navigation for WebContents occurs in a separate SiteInstance (and a renderer process), but second navigation will be initiated with flag |should_replace_current_entry| == true. BUG=611679 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
iceman@yandex-team.ru changed reviewers: + esprehn@chromium.org
Hi! Here are some tests that fails without my patch, to demonstrate the problem. And I tried to fix it with a small patch. Could you take a look at this CL, please?
Description was changed from ========== RenderViewImpl::page_id_ isn't initialized for some special navigation. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens if the first navigation for WebContents occurs in a separate SiteInstance (and a renderer process), but second navigation will be initiated with flag |should_replace_current_entry| == true. BUG=611679 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== RenderViewImpl::page_id_ isn't initialized for some special navigation. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens when a new page is loaded with |should_replace_current_entry| flag set to true and a new renderer is used for this navigation (i.e. the new entry uses a different SiteInstance from the previous entry's one). BUG=611679 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
iceman@yandex-team.ru changed reviewers: + creis@chromium.org
+ creis@ PTAL
Thanks for tracking this down, and for including a test! LGTM with nits. Can you also update the CL description to say "for cross-process replacement navigations" rather than "for some special navigation"? (For reference, we'll be removing the page ID logic in the near future, but it does make sense to have this fixed in the meantime.) https://codereview.chromium.org/1978793002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1978793002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:456: IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, Can you put a comment linking to https://crbug.com/611679 before the test? https://codereview.chromium.org/1978793002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:464: NavigateToURL(shell(), GURL("data:text/html,page1")); nit: EXPECT_TRUE https://codereview.chromium.org/1978793002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:465: EXPECT_TRUE(WaitForLoadStop(shell()->web_contents())); This isn't necessary, since NavigateToURL will wait long enough. https://codereview.chromium.org/1978793002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:483: EXPECT_NE(-1, shell()->web_contents()->GetMaxPageID()); Also check EXPECT_TRUE(shell()->web_contents()->GetMainFrame()->IsRenderFrameLive()); https://codereview.chromium.org/1978793002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1978793002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:3260: const auto& request_params = navigation_state->request_params(); nit: I think it's useful documentation to leave the type (RequestNavigationParams) here. There are a lot of params objects floating around this class, so it's nice to know at a glance. https://codereview.chromium.org/1978793002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:3262: bool should_init_page_id = render_view_->page_id_ == -1 && Let's add a comment above this: Ensure that we allocate a page ID if this is the first navigation for the page in this process. This can happen even when is_new_navigation is false, such as after a cross-process location.replace navigation.
On 2016/05/13 19:14:32, Charlie Reis (slow to reply) wrote: > Thanks for tracking this down, and for including a test! LGTM with nits. Thanks for the review! I will address these nits ASAP and update this CL.
I fixed nits. Could you take another look at this CL to verify that I did it right? Thanks in advance! https://codereview.chromium.org/1978793002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1978793002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:456: IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, On 2016/05/13 19:14:31, Charlie Reis (slow to reply) wrote: > Can you put a comment linking to https://crbug.com/611679 before the test? Done. https://codereview.chromium.org/1978793002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:464: NavigateToURL(shell(), GURL("data:text/html,page1")); On 2016/05/13 19:14:31, Charlie Reis (slow to reply) wrote: > nit: EXPECT_TRUE Done. https://codereview.chromium.org/1978793002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:465: EXPECT_TRUE(WaitForLoadStop(shell()->web_contents())); On 2016/05/13 19:14:31, Charlie Reis (slow to reply) wrote: > This isn't necessary, since NavigateToURL will wait long enough. Done. https://codereview.chromium.org/1978793002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:483: EXPECT_NE(-1, shell()->web_contents()->GetMaxPageID()); On 2016/05/13 19:14:31, Charlie Reis (slow to reply) wrote: > Also check > EXPECT_TRUE(shell()->web_contents()->GetMainFrame()->IsRenderFrameLive()); Done. https://codereview.chromium.org/1978793002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1978793002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:3260: const auto& request_params = navigation_state->request_params(); On 2016/05/13 19:14:32, Charlie Reis (slow to reply) wrote: > nit: I think it's useful documentation to leave the type > (RequestNavigationParams) here. There are a lot of params objects floating > around this class, so it's nice to know at a glance. Done. https://codereview.chromium.org/1978793002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:3262: bool should_init_page_id = render_view_->page_id_ == -1 && On 2016/05/13 19:14:32, Charlie Reis (slow to reply) wrote: > Let's add a comment above this: > > Ensure that we allocate a page ID if this is the first navigation for the page > in this process. This can happen even when is_new_navigation is false, such as > after a cross-process location.replace navigation. Done.
esprehn@chromium.org changed reviewers: - esprehn@chromium.org
Description was changed from ========== RenderViewImpl::page_id_ isn't initialized for some special navigation. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens when a new page is loaded with |should_replace_current_entry| flag set to true and a new renderer is used for this navigation (i.e. the new entry uses a different SiteInstance from the previous entry's one). BUG=611679 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== RenderViewImpl::page_id_ isn't initialized for some special navigation. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens when a new page is loaded with |should_replace_current_entry| flag set to true and a new renderer is used for this navigation (i.e. the new entry uses a different SiteInstance from the previous entry's one). BUG=611679 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Sorry for the delayed reply. LGTM with one fix below. https://codereview.chromium.org/1978793002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1978793002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:456: nit: Only one blank line between tests. https://codereview.chromium.org/1978793002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1978793002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:3271: if (is_new_navigation || (should_init_page_id && false)) { Oops-- I assume this was just for testing? Remove the "&& false" before landing.
And thank you again for taking your time! I will try to land this, if you don't mind. https://codereview.chromium.org/1978793002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1978793002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:456: On 2016/05/18 17:33:59, Charlie Reis (slow to reply) wrote: > nit: Only one blank line between tests. Done. https://codereview.chromium.org/1978793002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1978793002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:3271: if (is_new_navigation || (should_init_page_id && false)) { On 2016/05/18 17:33:59, Charlie Reis (slow to reply) wrote: > Oops-- I assume this was just for testing? Remove the "&& false" before > landing. Done. Sorry about that, you are quite right, it was for testing only!
The CQ bit was checked by iceman@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1978793002/#ps40001 (title: "Fix nits and remove testing condition.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1978793002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1978793002/40001
Description was changed from ========== RenderViewImpl::page_id_ isn't initialized for some special navigation. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens when a new page is loaded with |should_replace_current_entry| flag set to true and a new renderer is used for this navigation (i.e. the new entry uses a different SiteInstance from the previous entry's one). BUG=611679 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== RenderViewImpl::page_id_ isn't initialized for cross-process replacement navigations. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens when a new page is loaded with |should_replace_current_entry| flag set to true and a new renderer is used for this navigation (i.e. the new entry uses a different SiteInstance from the previous entry's one). BUG=611679 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Description was changed from ========== RenderViewImpl::page_id_ isn't initialized for cross-process replacement navigations. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens when a new page is loaded with |should_replace_current_entry| flag set to true and a new renderer is used for this navigation (i.e. the new entry uses a different SiteInstance from the previous entry's one). BUG=611679 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== RenderViewImpl::page_id_ isn't initialized for cross-process replacement navigations. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens when a new page is loaded with |should_replace_current_entry| flag set to true and a new renderer is used for this navigation (i.e. the new entry uses a different SiteInstance from the previous entry's one). BUG=611679 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== RenderViewImpl::page_id_ isn't initialized for cross-process replacement navigations. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens when a new page is loaded with |should_replace_current_entry| flag set to true and a new renderer is used for this navigation (i.e. the new entry uses a different SiteInstance from the previous entry's one). BUG=611679 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== RenderViewImpl::page_id_ isn't initialized for cross-process replacement navigations. There can be a situation, when |RenderViewImpl::page_id_| isn't initialized for a navigation that should replace the current entry. And then it will trigger a DCHECK in |RenderFrameImpl::NavigateInternal|. It happens when a new page is loaded with |should_replace_current_entry| flag set to true and a new renderer is used for this navigation (i.e. the new entry uses a different SiteInstance from the previous entry's one). BUG=611679 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/4b2ec6d51e135654911cc38d0193b221ac042a08 Cr-Commit-Position: refs/heads/master@{#394515} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4b2ec6d51e135654911cc38d0193b221ac042a08 Cr-Commit-Position: refs/heads/master@{#394515} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
