Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528813002:
Fix Self-Referencing OOPIF Infinite Loop (Closed)
|
Created:
4 years ago by davidsac (gone - try alexmos) Modified:
3 years, 10 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix infinite loop of frames being created when one or more frames
contain nested IFrames to one another.
To reproduce this issue, run Chrome in site-per-process mode and visit
either http://dudeguy409.github.io/coreferencingframe1 or
https://davidsac.github.io/coreferencingframe2.html.
The current check refuses to load a URL if any two ancestors in the
frame tree have the same URL. It is currently inadequate for two
reasons:
1) This doesn't work for OOPIFs. If the parent frames are in a
different process, they will be remote frames which don't have URLs,
so the check is simply skipped.
(2) it's only applied in a subset of cases and doesn't handle
redirects, for example.
To fix these issues, this CL moves these checks to the browser
process, where it's applied inside NavigationHandleImpl any time a
request is started or a redirect is received.
BUG=650332, 527367
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2528813002
Cr-Commit-Position: refs/heads/master@{#450728}
Committed: https://chromium.googlesource.com/chromium/src/+/04fdd09f342badeb2af3a639d62a4da65d4baaa1
Patch Set 1 #Patch Set 2 : added tests and html files for coreferencing iframes #Patch Set 3 : add tests for self-referencing iframes w/ and w/o site-per-process #Patch Set 4 : remove blink self-referencing solution #Patch Set 5 : add self-referencing oopif check to core #Patch Set 6 : add fix to find and renderframe tests #
Total comments: 48
Patch Set 7 : Add tests and small fixes #Patch Set 8 : Add |state_| change in |WillStartRequest| and |WillRedirectRequest| #
Total comments: 45
Patch Set 9 : small fixes and renaming #Patch Set 10 : small fixes and renaming #Patch Set 11 : made small fix to test #Patch Set 12 : added log stmts #Patch Set 13 : rebased and added log stmts #Patch Set 14 : small fix #Patch Set 15 : fix broken test case and remove log statements #Patch Set 16 : actually fix broken test #
Total comments: 31
Patch Set 17 : commented and cleaned up tests #Patch Set 18 : commented and cleaned up tests #
Total comments: 2
Patch Set 19 : make EqualIgnoringFragmentIdentifier more efficient #
Total comments: 2
Patch Set 20 : rename variables and remove FIXME #Patch Set 21 : replace old EqualsIgnoringRef with new version #Patch Set 22 : rebase #
Total comments: 2
Patch Set 23 : refactor allowedToLoadFrame conditional #Messages
Total messages: 87 (59 generated)
Description was changed from ========== add tests and html files for infinitely nesting coreferencing iframes BUG=650332 ========== to ========== add tests and html files for infinitely nesting coreferencing iframes BUG=650332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by davidsac@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by davidsac@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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by davidsac@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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== add tests and html files for infinitely nesting coreferencing iframes BUG=650332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix infinite loop of frames being created when one or more frames contain nested IFrames to one another. To reproduce this issue, run Chrome in site-per-process mode and visit either http://dudeguy409.github.io/coreferencingframe1 or https://davidsac.github.io/coreferencingframe2.html. The logic for checking if a site navigation is to an already-visited page should be moved to the core logic of the browser, rather than in blink. Currently, the check in blink can not check the origin of other parent frames since they are in separate processes. BUG=650332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
davidsac@chromium.org changed reviewers: + alexmos@chromium.org, nasko@chromium.org
Hi Nasko and Alex, Could you please review this CL? A few lengthy notes: I am considering renaming isURLBlocked to is URLAllowed and switching the boolean values to avoid the confusion of double negatives. Let me know what your thoughts are. I edited the description of this issue, but I wasn't sure exactly why it was failing before the fix, only that it was failing. Let me know if my assumption was correct. I needed to change a line in find_request_manager_browsertest.cc. As I understood it, a test was failing, but only run while not in site-per-process mode. The test seems to be putting iframes from the same site nested within each other and testing the find feature on the text. In order to do this, a complete binary tree datastructure is used to create the frame tree. A depth of two was used, implying three layers, with four on the bottom layer. IsURLBlocked was returning true exactly four times when not in site-per-process mode. The writer of the test seems to have been exploiting bug 527367 or something. When the test was run in site-per-process mode, each level of the tree was composed from different origins, in order to make the test work, I assume. However, this was disabled when not in site-per-process mode. Since the logic to handle this was moved to apply to both modes, I simply removed the toggle in the test in order to always ensure that these nested iframes appear to be from different origins in the test. I assume this is not an issue, but let me know if you think I'm wrong.
[+site-isolation-reviews] Thanks! Some initial comments below. Also, one correction in the description: > Currently, the check in blink can not check the origin of > other parent frames since they are in separate processes. We actually replicate origins so we do have the origins of both local and remote frames. But the check uses full URLs and not origins, and we don't have URLs for remote frames, which is why the old Blink check didn't work for them (it simply skipped them). https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... File content/browser/find_request_manager_browsertest.cc (left): https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... content/browser/find_request_manager_browsertest.cc:322: if (cross_process) Can you explain why this change is needed? I remember you mentioned some tests were somehow affected by self-referential iframes, but don't recall the details. But this seems to mess with the way find-in-page tests work (i.e., always create cross-process frames), which is unexpected. https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... File content/browser/find_request_manager_browsertest.cc (right): https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... content/browser/find_request_manager_browsertest.cc:323: GURL url(embedded_test_server()->GetURL( Please fix the indent in lines 322-336. https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... content/browser/find_request_manager_browsertest.cc:575: new blank line not needed https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:437: LOG(INFO) << "Will start request"; Don't forget to remove the LOGs, here and below https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:457: // return false; Not needed https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:458: if (url_.SchemeIs("about")) Let's keep the comment about excluding about: from the self-reference checks. The original one in Blink wasn't very helpful, but I found that it was added in https://codereview.chromium.org/1410093006, but https://crbug.com/341858. Let's add that bug number (and/or some of the description in that CL) to the comment. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:463: bool foundSelfReference = false; nit: rename this according to Chromium style, i.e., found_self_reference. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:464: for (const FrameTreeNode* node = frame_tree_node_; node; I think there might be a bug here, in that this should start on frame_tree_node_'s parent instead. The original isURLAllowed was called on the parent frame in HTMLFrameElementBase::isURLAllowed, but this will start from the actual navigating frame, so the first comparison will be incorrect. The specific scenario in which this would fail would be if the navigating frame's current (last committed) URL is the same as the target URL. Then the new check might disallow the new navigation (if the URL also occurs in one of the ancestors) unnecessarily, whereas the previous check would've allowed it. It'd be nice to add a test for this. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:466: if (node->current_url() == url_) { This also doesn't look the same as the old check. The check in Blink used equalIgnoringFragmentIdentifier, which ignores the part of the URL after the #, but this won't do that. Let's add that functionality along with a test. (ref() is the GURL way of getting the part after the #; you can look into GURL::ReplaceComponents/ClearRef to clear that out in both URLs before comparison.) https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:503: RunCompleteCallback(NavigationThrottle::CANCEL); Looking at how CheckWill(Start|Redirect)Request works, I wonder if we should also add a "state_ = CANCELING;" before this, both here in and WillStartRequest. It'd be good to double-check with Nasko on this; I'm not sure if it's important to keep state_ correct for canceling. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:227: // its ancestors nit: end with period. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:227: // its ancestors This comment isn't entirely accurate, as it only tells whether the navigation URL is referenced in ancestors, but it doesn't actually prevent the navigation. Perhaps just tweak the wording to say that this is a helper function that's used by WillStartRequest and WillRedirectRequest to prevent the navigation? https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:228: bool isURLBlocked(); Let's use a name that conveys what specific check is done here. For example, IsSelfReferentialURL. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:228: bool isURLBlocked(); Let's not insert this in between WillStartRequest and WillRedirectRequest, which make sense to keep next to each other. Also, can this be private, as it's not really intended to be used outside of here? So for example this could go just after RegisterNavigationThrottles(), here and in in .cc to match header order. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3035: // TODO(davidsac): This test should be broken until issue 527367 is resolved. I don't see how issue 527367 is related to this test. That looks like a problem with <meta> refresh, which this doesn't use. It'd be good to follow up on that issue separately and try out their repro to see if your patch indeed fixes it. If so, we can just add a test; if not, we can figure out what's needed beyond this CL and whether we should do it. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3063: // The Frame Tree contains two successful instances of each site plus an nit: s/Frame Tree/FrameTree/g (to match class name) https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3066: // iframes nit: unnecessary line break https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3068: // mode. I think you can just put this comment above the if, and remove its other copy in the other clause. It explains both if branches pretty well. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3074: " +--Site A\nWhere A = http://a.com/", nit: line break after \n https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3079: root->child_at(0)->child_at(0)->child_at(0)->child_at(0)->current_url(), You can use GURL::is_empty() for this, along with EXPECT_TRUE. It might also be nice to check that has_committed_real_load() is false on the same bottom-most FTN. https://codereview.chromium.org/2528813002/diff/100001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2528813002/diff/100001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:8987: IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, CoReferencingFrames) { This test looks identical to the one above in RenderFrameHostManagerTest, which has coverage both without and with --site-per-process (the latter on our FYI bots and try bot). Can it be removed, or was there a reason for keeping it? https://codereview.chromium.org/2528813002/diff/100001/content/test/data/core... File content/test/data/coreferencingframe_1.html (right): https://codereview.chromium.org/2528813002/diff/100001/content/test/data/core... content/test/data/coreferencingframe_1.html:4: A - This page has co-referencing nested iframes. Or does it? nit: "Or does it?" not needed :)
https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... File content/browser/find_request_manager_browsertest.cc (left): https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... content/browser/find_request_manager_browsertest.cc:322: if (cross_process) On 2016/12/28 00:25:59, alexmos wrote: > Can you explain why this change is needed? I remember you mentioned some tests > were somehow affected by self-referential iframes, but don't recall the details. > But this seems to mess with the way find-in-page tests work (i.e., always > create cross-process frames), which is unexpected. Sorry for the miscommunication. I sent a description of why I did this when I requested that you review this. I was correctly concerned that it could easily be missed. Sorry about that. Let me know if you think this approach or my understanding is incorrect or if you need further clarification.
https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... File content/browser/find_request_manager_browsertest.cc (left): https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... content/browser/find_request_manager_browsertest.cc:322: if (cross_process) On 2017/01/03 19:29:35, davidsac wrote: > On 2016/12/28 00:25:59, alexmos wrote: > > Can you explain why this change is needed? I remember you mentioned some > tests > > were somehow affected by self-referential iframes, but don't recall the > details. > > But this seems to mess with the way find-in-page tests work (i.e., always > > create cross-process frames), which is unexpected. > > Sorry for the miscommunication. I sent a description of why I did this when I > requested that you review this. I was correctly concerned that it could easily > be missed. Sorry about that. Let me know if you think this approach or my > understanding is incorrect or if you need further clarification. Ah, yes, I missed your explanation in the initial message, sorry! (A tip is that you can add a self-review comment like that on the right line (i.e., by double-clicking), and then it'll be easy for reviewers to see.) So, IIUC, the tree or origins that test creates for height=2 seems to be: With --site-per-process: a(aa(aaa,aaa),aa(aaa,aaa)) Without --site-per-process: a(a(a,a),a(a,a)) And indeed, it seems without --site-per-process, we'll indeed hit the self-referencing case with each grandchild frame for height >= 2. I suppose previously that was not a problem because this is a browser-initiated frame navigation (using NavigateFrameToURL), and the Blink check only happened for renderer-initiated navigations. But now that your moved check applies to both browser-initiated and renderer-initiated navigations, we need to worry about this. I don't think we should just remove |cross_process| though, since then we'll lose coverage for non-cross-process subframes. A couple of other options are (1) fix the test to not rely on nesting the same URL arbitrarily deep in the non-SPP case (e.g., by appending a query string or something), or (2) not enforce the self-reference check for browser-initiated navigations.
https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... File content/browser/find_request_manager_browsertest.cc (left): https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... content/browser/find_request_manager_browsertest.cc:322: if (cross_process) On 2017/01/03 22:12:05, alexmos wrote: > On 2017/01/03 19:29:35, davidsac wrote: > > On 2016/12/28 00:25:59, alexmos wrote: > > > Can you explain why this change is needed? I remember you mentioned some > > tests > > > were somehow affected by self-referential iframes, but don't recall the > > details. > > > But this seems to mess with the way find-in-page tests work (i.e., always > > > create cross-process frames), which is unexpected. > > > > Sorry for the miscommunication. I sent a description of why I did this when I > > requested that you review this. I was correctly concerned that it could > easily > > be missed. Sorry about that. Let me know if you think this approach or my > > understanding is incorrect or if you need further clarification. > > Ah, yes, I missed your explanation in the initial message, sorry! (A tip is > that you can add a self-review comment like that on the right line (i.e., by > double-clicking), and then it'll be easy for reviewers to see.) > > So, IIUC, the tree or origins that test creates for height=2 seems to be: > With --site-per-process: a(aa(aaa,aaa),aa(aaa,aaa)) > Without --site-per-process: a(a(a,a),a(a,a)) > > And indeed, it seems without --site-per-process, we'll indeed hit the > self-referencing case with each grandchild frame for height >= 2. I suppose > previously that was not a problem because this is a browser-initiated frame > navigation (using NavigateFrameToURL), and the Blink check only happened for > renderer-initiated navigations. But now that your moved check applies to both > browser-initiated and renderer-initiated navigations, we need to worry about > this. I don't think we should just remove |cross_process| though, since then > we'll lose coverage for non-cross-process subframes. A couple of other options > are (1) fix the test to not rely on nesting the same URL arbitrarily deep in the > non-SPP case (e.g., by appending a query string or something), or (2) not > enforce the self-reference check for browser-initiated navigations. That is a good point. Let me know which approach you think will work better. In the mean time, I will admit that I am a bit confused by what you mean when you say that we will lose test coverage. As far as I can tell, we are not losing coverage. The test still runs and passes in both non-spp and spp mode. The only change is that the two trees look identical in both non-spp and spp, or: a(aa(aaa,aaa),aa(aaa,aaa)). In other words, I believe that by removing that line, I am performing the fix that you mentioned in 1). Let me know if this is incorrect.
https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... File content/browser/find_request_manager_browsertest.cc (left): https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... content/browser/find_request_manager_browsertest.cc:322: if (cross_process) On 2017/01/03 23:14:18, davidsac wrote: > On 2017/01/03 22:12:05, alexmos wrote: > > On 2017/01/03 19:29:35, davidsac wrote: > > > On 2016/12/28 00:25:59, alexmos wrote: > > > > Can you explain why this change is needed? I remember you mentioned some > > > tests > > > > were somehow affected by self-referential iframes, but don't recall the > > > details. > > > > But this seems to mess with the way find-in-page tests work (i.e., always > > > > create cross-process frames), which is unexpected. > > > > > > Sorry for the miscommunication. I sent a description of why I did this when > I > > > requested that you review this. I was correctly concerned that it could > > easily > > > be missed. Sorry about that. Let me know if you think this approach or my > > > understanding is incorrect or if you need further clarification. > > > > Ah, yes, I missed your explanation in the initial message, sorry! (A tip is > > that you can add a self-review comment like that on the right line (i.e., by > > double-clicking), and then it'll be easy for reviewers to see.) > > > > So, IIUC, the tree or origins that test creates for height=2 seems to be: > > With --site-per-process: a(aa(aaa,aaa),aa(aaa,aaa)) > > Without --site-per-process: a(a(a,a),a(a,a)) > > > > And indeed, it seems without --site-per-process, we'll indeed hit the > > self-referencing case with each grandchild frame for height >= 2. I suppose > > previously that was not a problem because this is a browser-initiated frame > > navigation (using NavigateFrameToURL), and the Blink check only happened for > > renderer-initiated navigations. But now that your moved check applies to both > > browser-initiated and renderer-initiated navigations, we need to worry about > > this. I don't think we should just remove |cross_process| though, since then > > we'll lose coverage for non-cross-process subframes. A couple of other > options > > are (1) fix the test to not rely on nesting the same URL arbitrarily deep in > the > > non-SPP case (e.g., by appending a query string or something), or (2) not > > enforce the self-reference check for browser-initiated navigations. > > That is a good point. Let me know which approach you think will work better. > In the mean time, I will admit that I am a bit confused by what you mean when > you say that we will lose test coverage. As far as I can tell, we are not > losing coverage. The test still runs and passes in both non-spp and spp mode. > The only change is that the two trees look identical in both non-spp and spp, > or: a(aa(aaa,aaa),aa(aaa,aaa)). In other words, I believe that by removing > that line, I am performing the fix that you mentioned in 1). Let me know if > this is incorrect. Let's go with option (2), since we've confirmed with Nasko that browser-initiated subframe navigations should only happen in tests or history navigations (and the latter shouldn't have problematic self-references as we would've blocked them before they get into history). That better matches the scope of the existing Blink check anyway. For losing test coverage, I was just worried that this will remove same-origin iframes from the tests, so we won't be testing find-in-page for those cases anymore (with or without OOPIFs turned on). I don't know if that really matters (paulmeyer@ would know), but regardless, I hope that we won't need to modify this test anymore once we add the check for (2).
The CQ bit was checked by davidsac@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 checked by davidsac@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...
Hi Alex, I have addressed all of your comments. I think we are ready for round 2 of the review! https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... File content/browser/find_request_manager_browsertest.cc (left): https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... content/browser/find_request_manager_browsertest.cc:322: if (cross_process) On 2017/01/04 22:35:10, alexmos wrote: > On 2017/01/03 23:14:18, davidsac wrote: > > On 2017/01/03 22:12:05, alexmos wrote: > > > On 2017/01/03 19:29:35, davidsac wrote: > > > > On 2016/12/28 00:25:59, alexmos wrote: > > > > > Can you explain why this change is needed? I remember you mentioned > some > > > > tests > > > > > were somehow affected by self-referential iframes, but don't recall the > > > > details. > > > > > But this seems to mess with the way find-in-page tests work (i.e., > always > > > > > create cross-process frames), which is unexpected. > > > > > > > > Sorry for the miscommunication. I sent a description of why I did this > when > > I > > > > requested that you review this. I was correctly concerned that it could > > > easily > > > > be missed. Sorry about that. Let me know if you think this approach or > my > > > > understanding is incorrect or if you need further clarification. > > > > > > Ah, yes, I missed your explanation in the initial message, sorry! (A tip is > > > that you can add a self-review comment like that on the right line (i.e., by > > > double-clicking), and then it'll be easy for reviewers to see.) > > > > > > So, IIUC, the tree or origins that test creates for height=2 seems to be: > > > With --site-per-process: a(aa(aaa,aaa),aa(aaa,aaa)) > > > Without --site-per-process: a(a(a,a),a(a,a)) > > > > > > And indeed, it seems without --site-per-process, we'll indeed hit the > > > self-referencing case with each grandchild frame for height >= 2. I suppose > > > previously that was not a problem because this is a browser-initiated frame > > > navigation (using NavigateFrameToURL), and the Blink check only happened for > > > renderer-initiated navigations. But now that your moved check applies to > both > > > browser-initiated and renderer-initiated navigations, we need to worry about > > > this. I don't think we should just remove |cross_process| though, since > then > > > we'll lose coverage for non-cross-process subframes. A couple of other > > options > > > are (1) fix the test to not rely on nesting the same URL arbitrarily deep in > > the > > > non-SPP case (e.g., by appending a query string or something), or (2) not > > > enforce the self-reference check for browser-initiated navigations. > > > > That is a good point. Let me know which approach you think will work better. > > In the mean time, I will admit that I am a bit confused by what you mean when > > you say that we will lose test coverage. As far as I can tell, we are not > > losing coverage. The test still runs and passes in both non-spp and spp mode. > > > The only change is that the two trees look identical in both non-spp and spp, > > or: a(aa(aaa,aaa),aa(aaa,aaa)). In other words, I believe that by removing > > that line, I am performing the fix that you mentioned in 1). Let me know if > > this is incorrect. > > Let's go with option (2), since we've confirmed with Nasko that > browser-initiated subframe navigations should only happen in tests or history > navigations (and the latter shouldn't have problematic self-references as we > would've blocked them before they get into history). That better matches the > scope of the existing Blink check anyway. > > For losing test coverage, I was just worried that this will remove same-origin > iframes from the tests, so we won't be testing find-in-page for those cases > anymore (with or without OOPIFs turned on). I don't know if that really matters > (paulmeyer@ would know), but regardless, I hope that we won't need to modify > this test anymore once we add the check for (2). Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... File content/browser/find_request_manager_browsertest.cc (right): https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... content/browser/find_request_manager_browsertest.cc:323: GURL url(embedded_test_server()->GetURL( On 2016/12/28 00:25:59, alexmos wrote: > Please fix the indent in lines 322-336. Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/find_r... content/browser/find_request_manager_browsertest.cc:575: On 2016/12/28 00:25:58, alexmos wrote: > new blank line not needed Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:437: LOG(INFO) << "Will start request"; On 2016/12/28 00:25:59, alexmos wrote: > Don't forget to remove the LOGs, here and below Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:457: // return false; On 2016/12/28 00:25:59, alexmos wrote: > Not needed Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:458: if (url_.SchemeIs("about")) On 2016/12/28 00:25:59, alexmos wrote: > Let's keep the comment about excluding about: from the self-reference checks. > The original one in Blink wasn't very helpful, but I found that it was added in > https://codereview.chromium.org/1410093006, but https://crbug.com/341858. Let's > add that bug number (and/or some of the description in that CL) to the comment. Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:463: bool foundSelfReference = false; On 2016/12/28 00:25:59, alexmos wrote: > nit: rename this according to Chromium style, i.e., found_self_reference. Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:464: for (const FrameTreeNode* node = frame_tree_node_; node; On 2016/12/28 00:25:59, alexmos wrote: > I think there might be a bug here, in that this should start on > frame_tree_node_'s parent instead. The original isURLAllowed was called on the > parent frame in HTMLFrameElementBase::isURLAllowed, but this will start from the > actual navigating frame, so the first comparison will be incorrect. > > The specific scenario in which this would fail would be if the navigating > frame's current (last committed) URL is the same as the target URL. Then the > new check might disallow the new navigation (if the URL also occurs in one of > the ancestors) unnecessarily, whereas the previous check would've allowed it. > It'd be nice to add a test for this. Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:466: if (node->current_url() == url_) { On 2016/12/28 00:25:59, alexmos wrote: > This also doesn't look the same as the old check. The check in Blink used > equalIgnoringFragmentIdentifier, which ignores the part of the URL after the #, > but this won't do that. Let's add that functionality along with a test. (ref() > is the GURL way of getting the part after the #; you can look into > GURL::ReplaceComponents/ClearRef to clear that out in both URLs before > comparison.) Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:503: RunCompleteCallback(NavigationThrottle::CANCEL); On 2016/12/28 00:25:59, alexmos wrote: > Looking at how CheckWill(Start|Redirect)Request works, I wonder if we should > also add a "state_ = CANCELING;" before this, both here in and WillStartRequest. > It'd be good to double-check with Nasko on this; I'm not sure if it's important > to keep state_ correct for canceling. Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:227: // its ancestors On 2016/12/28 00:25:59, alexmos wrote: > nit: end with period. Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:227: // its ancestors On 2016/12/28 00:25:59, alexmos wrote: > This comment isn't entirely accurate, as it only tells whether the navigation > URL is referenced in ancestors, but it doesn't actually prevent the navigation. > Perhaps just tweak the wording to say that this is a helper function that's used > by WillStartRequest and WillRedirectRequest to prevent the navigation? Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:228: bool isURLBlocked(); On 2016/12/28 00:25:59, alexmos wrote: > Let's use a name that conveys what specific check is done here. For example, > IsSelfReferentialURL. Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:228: bool isURLBlocked(); On 2016/12/28 00:25:59, alexmos wrote: > Let's not insert this in between WillStartRequest and WillRedirectRequest, which > make sense to keep next to each other. Also, can this be private, as it's not > really intended to be used outside of here? So for example this could go just > after RegisterNavigationThrottles(), here and in in .cc to match header order. Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3035: // TODO(davidsac): This test should be broken until issue 527367 is resolved. On 2016/12/28 00:25:59, alexmos wrote: > I don't see how issue 527367 is related to this test. That looks like a problem > with <meta> refresh, which this doesn't use. > > It'd be good to follow up on that issue separately and try out their repro to > see if your patch indeed fixes it. If so, we can just add a test; if not, we > can figure out what's needed beyond this CL and whether we should do it. Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3063: // The Frame Tree contains two successful instances of each site plus an On 2016/12/28 00:25:59, alexmos wrote: > nit: s/Frame Tree/FrameTree/g (to match class name) Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3066: // iframes On 2016/12/28 00:25:59, alexmos wrote: > nit: unnecessary line break Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3068: // mode. On 2016/12/28 00:25:59, alexmos wrote: > I think you can just put this comment above the if, and remove its other copy in > the other clause. It explains both if branches pretty well. Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3074: " +--Site A\nWhere A = http://a.com/", On 2016/12/28 00:25:59, alexmos wrote: > nit: line break after \n Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3079: root->child_at(0)->child_at(0)->child_at(0)->child_at(0)->current_url(), On 2016/12/28 00:25:59, alexmos wrote: > You can use GURL::is_empty() for this, along with EXPECT_TRUE. > It might also be nice to check that has_committed_real_load() is false on the > same bottom-most FTN. Done. https://codereview.chromium.org/2528813002/diff/100001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2528813002/diff/100001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:8987: IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, CoReferencingFrames) { On 2016/12/28 00:25:59, alexmos wrote: > This test looks identical to the one above in RenderFrameHostManagerTest, which > has coverage both without and with --site-per-process (the latter on our FYI > bots and try bot). Can it be removed, or was there a reason for keeping it? It has been removed. :) I think when I first wrote both tests, I didn't realize that RenderFrameHostManagerTest was run in both modes. https://codereview.chromium.org/2528813002/diff/100001/content/test/data/core... File content/test/data/coreferencingframe_1.html (right): https://codereview.chromium.org/2528813002/diff/100001/content/test/data/core... content/test/data/coreferencingframe_1.html:4: A - This page has co-referencing nested iframes. Or does it? On 2016/12/28 00:25:59, alexmos wrote: > nit: "Or does it?" not needed :) Done.
Thanks, this is getting there! A few more comments below. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:824: // and cannot be the source of infinite recursion. BUG=341858 nit: For referencing bug numbers, it's more common to use "See https://crbug.com/341858." https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:837: url::Replacements<char> replacements; I think this might become more readable if you declared a helper (e.g., EqualIgnoringFragmentIdentifier(url1, url2)) in the anonymous namespace, similar to the one we had in Blink, and used it here. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:839: replacements.ClearUsername(); Why do you clear out username and password as well? Just clearing the # should be sufficient. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:843: if (found_self_reference) { nit: { not necessary https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:350: // than once in its ancestors. This is a helper function used by nit: s/its/the frame's/ https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3033: // Ensure that loading a page with a cross-site coreferencing iframe nit: s/a cross-site coreferencing iframe/cross-site coreferencing iframes/ (since there are lots of them :)) https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3035: // TODO(davidsac): This test should be broken until issue 527367 is resolved. I think we can also remove the TODO, right? I don't think this test should be referencing 527367. Or maybe I'm not understanding what you meant by the TODO? Instead, I think you can add a reference to https://crbug.com/650332, which is exactly what the test is for. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3037: // Load a page with a cross-site coreferencing iframe. Perhaps add a comment here to clarify what "coreferencing" means. I.e., the page's cross-site iframe also has an iframe with its source set to the top frame's URL. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3049: // unsuccessfully-navigated third instance of B with a blank URL. The nit: I'd move "when outside of site-per-process mode" to the beginning of the sentence. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3073: EXPECT_TRUE(root->child_at(0) You could declare "FrameTreeNode* bottom_child = root->child_at(0)->child_at(0)->child_at(0)->child_at(0);" and use in both places below to save a bunch of lines :) https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3087: // Ensure that loading a page with fragments Finish the comment to explain what the test is doing :) And similarly to above, reference 650332 and remove the TODO. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3101: TestFrameNavigationObserver observer1(child); Let's add a comment that it's important to use renderer-initiated navigations, as the self-referencing URL check is bypassed for browser-initiated ones. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3108: TestFrameNavigationObserver observer2(grandchild); Add a comment that this navigation should be blocked. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3121: Can you include an EXPECT_EQ() for the grandchild's URL? Presumably it should be about:blank, i.e. GURL(url::kAboutBlankURL)? https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3125: // Ensure that loading a page with a meta refresh iframe nit: rewrap comment to 80 lines. Your editor should be able to do this automatically on the fly for you. :) https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3127: // TODO(davidsac): This test should be broken until issue 527367 is resolved. Instead of TODO, just reference https://crbug.com/527367. Also, might be nice to give a bit more detail, something like "This test loads a page with an about:blank iframe, injects html containing a meta refresh to the iframe, and ensures that this does not cause..." https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3128: IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, MetaRefreshFrames) { Perhaps add "SelfReferencing" prefix to the test name? so it's clear that this is related to the other tests? https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3150: EXPECT_FALSE(root->child_at(0)->current_url().is_empty()); Instead of this, can we check that current_url() is equal to about:blank? https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3184: TestFrameNavigationObserver observer2(child); This setup is very similar to the test in SelfReferencingFragmentFrames. Can we just move lines 3184-3196 there (before the grandchild checks), and check both things in one test? (And then you won't need this test.) https://codereview.chromium.org/2528813002/diff/140001/content/test/data/ifra... File content/test/data/iframe.html (right): https://codereview.chromium.org/2528813002/diff/140001/content/test/data/ifra... content/test/data/iframe.html:1: <html><head><title>iframe test</title></head> There's already content/test/data/page_with_iframe.html, which looks like it should work, so no need to add this one. https://codereview.chromium.org/2528813002/diff/140001/content/test/data/page... File content/test/data/page_with_meta_refresh_frame.html (right): https://codereview.chromium.org/2528813002/diff/140001/content/test/data/page... content/test/data/page_with_meta_refresh_frame.html:11: " <meta http-equiv=\"refresh\" content=\"3\">" + Let's use "0" here to not slow down the tests on the bots :) https://codereview.chromium.org/2528813002/diff/140001/content/test/data/page... content/test/data/page_with_meta_refresh_frame.html:19: doc.open(); Let's add a comment that this will overwrite the iframe document's URL with the URL of the parent document. (Since this is the most non-obvious part of this code.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by davidsac@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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by davidsac@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Oh, before I forget, let's also add 527367 as a bug reference in the description.
Description was changed from ========== Fix infinite loop of frames being created when one or more frames contain nested IFrames to one another. To reproduce this issue, run Chrome in site-per-process mode and visit either http://dudeguy409.github.io/coreferencingframe1 or https://davidsac.github.io/coreferencingframe2.html. The logic for checking if a site navigation is to an already-visited page should be moved to the core logic of the browser, rather than in blink. Currently, the check in blink can not check the origin of other parent frames since they are in separate processes. BUG=650332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix infinite loop of frames being created when one or more frames contain nested IFrames to one another. To reproduce this issue, run Chrome in site-per-process mode and visit either http://dudeguy409.github.io/coreferencingframe1 or https://davidsac.github.io/coreferencingframe2.html. The logic for checking if a site navigation is to an already-visited page should be moved to the core logic of the browser, rather than in blink. Currently, the check in blink can not check the origin of other parent frames since they are in separate processes. BUG=650332, 527367 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix infinite loop of frames being created when one or more frames contain nested IFrames to one another. To reproduce this issue, run Chrome in site-per-process mode and visit either http://dudeguy409.github.io/coreferencingframe1 or https://davidsac.github.io/coreferencingframe2.html. The logic for checking if a site navigation is to an already-visited page should be moved to the core logic of the browser, rather than in blink. Currently, the check in blink can not check the origin of other parent frames since they are in separate processes. BUG=650332, 527367 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix infinite loop of frames being created when one or more frames contain nested IFrames to one another. To reproduce this issue, run Chrome in site-per-process mode and visit either http://dudeguy409.github.io/coreferencingframe1 or https://davidsac.github.io/coreferencingframe2.html. The logic for checking if a site navigation is to an already-visited page should be moved to the core logic of the browser, rather than in blink. Currently, the check in blink can not check the full URLs of parent frames because they are remote frames, so the check is simply skipped. BUG=650332, 527367 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by davidsac@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.
Hey Alex, I have addressed all of your comments and got the tests working again. This CL is ready for another round of reviews. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:824: // and cannot be the source of infinite recursion. BUG=341858 On 2017/01/06 02:27:29, alexmos wrote: > nit: For referencing bug numbers, it's more common to use "See > https://crbug.com/341858.%22 Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:837: url::Replacements<char> replacements; On 2017/01/06 02:27:29, alexmos wrote: > I think this might become more readable if you declared a helper (e.g., > EqualIgnoringFragmentIdentifier(url1, url2)) in the anonymous namespace, similar > to the one we had in Blink, and used it here. Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:839: replacements.ClearUsername(); On 2017/01/06 02:27:29, alexmos wrote: > Why do you clear out username and password as well? Just clearing the # should > be sufficient. Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:843: if (found_self_reference) { On 2017/01/06 02:27:29, alexmos wrote: > nit: { not necessary Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:350: // than once in its ancestors. This is a helper function used by On 2017/01/06 02:27:29, alexmos wrote: > nit: s/its/the frame's/ Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3033: // Ensure that loading a page with a cross-site coreferencing iframe On 2017/01/06 02:27:29, alexmos wrote: > nit: s/a cross-site coreferencing iframe/cross-site coreferencing iframes/ > (since there are lots of them :)) Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3035: // TODO(davidsac): This test should be broken until issue 527367 is resolved. On 2017/01/06 02:27:29, alexmos wrote: > I think we can also remove the TODO, right? I don't think this test should be > referencing 527367. Or maybe I'm not understanding what you meant by the TODO? > > Instead, I think you can add a reference to https://crbug.com/650332, which is > exactly what the test is for. Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3037: // Load a page with a cross-site coreferencing iframe. On 2017/01/06 02:27:29, alexmos wrote: > Perhaps add a comment here to clarify what "coreferencing" means. I.e., the > page's cross-site iframe also has an iframe with its source set to the top > frame's URL. Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3049: // unsuccessfully-navigated third instance of B with a blank URL. The On 2017/01/06 02:27:29, alexmos wrote: > nit: I'd move "when outside of site-per-process mode" to the beginning of the > sentence. Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3073: EXPECT_TRUE(root->child_at(0) On 2017/01/06 02:27:29, alexmos wrote: > You could declare "FrameTreeNode* bottom_child = > root->child_at(0)->child_at(0)->child_at(0)->child_at(0);" and use in both > places below to save a bunch of lines :) Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3087: // Ensure that loading a page with fragments On 2017/01/06 02:27:29, alexmos wrote: > Finish the comment to explain what the test is doing :) > > And similarly to above, reference 650332 and remove the TODO. Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3101: TestFrameNavigationObserver observer1(child); On 2017/01/06 02:27:29, alexmos wrote: > Let's add a comment that it's important to use renderer-initiated navigations, > as the self-referencing URL check is bypassed for browser-initiated ones. Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3108: TestFrameNavigationObserver observer2(grandchild); On 2017/01/06 02:27:29, alexmos wrote: > Add a comment that this navigation should be blocked. Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3121: On 2017/01/06 02:27:29, alexmos wrote: > Can you include an EXPECT_EQ() for the grandchild's URL? Presumably it should > be about:blank, i.e. GURL(url::kAboutBlankURL)? Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3125: // Ensure that loading a page with a meta refresh iframe On 2017/01/06 02:27:29, alexmos wrote: > nit: rewrap comment to 80 lines. Your editor should be able to do this > automatically on the fly for you. :) Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3127: // TODO(davidsac): This test should be broken until issue 527367 is resolved. On 2017/01/06 02:27:29, alexmos wrote: > Instead of TODO, just reference https://crbug.com/527367. Also, might be nice > to give a bit more detail, something like "This test loads a page with an > about:blank iframe, injects html containing a meta refresh to the iframe, and > ensures that this does not cause..." Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3128: IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, MetaRefreshFrames) { On 2017/01/06 02:27:29, alexmos wrote: > Perhaps add "SelfReferencing" prefix to the test name? so it's clear that this > is related to the other tests? Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3150: EXPECT_FALSE(root->child_at(0)->current_url().is_empty()); On 2017/01/06 02:27:29, alexmos wrote: > Instead of this, can we check that current_url() is equal to about:blank? Done. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3184: TestFrameNavigationObserver observer2(child); On 2017/01/06 02:27:29, alexmos wrote: > This setup is very similar to the test in SelfReferencingFragmentFrames. Can we > just move lines 3184-3196 there (before the grandchild checks), and check both > things in one test? (And then you won't need this test.) I feel like that would make it a bit confusing what was getting tested. Although both tests use URL fragments, this test checks a pretty different bug. That being said, I have renamed this test to be a bit more clear that it is related to the other tests. https://codereview.chromium.org/2528813002/diff/140001/content/test/data/ifra... File content/test/data/iframe.html (right): https://codereview.chromium.org/2528813002/diff/140001/content/test/data/ifra... content/test/data/iframe.html:1: <html><head><title>iframe test</title></head> On 2017/01/06 02:27:29, alexmos wrote: > There's already content/test/data/page_with_iframe.html, which looks like it > should work, so no need to add this one. Done. https://codereview.chromium.org/2528813002/diff/140001/content/test/data/page... File content/test/data/page_with_meta_refresh_frame.html (right): https://codereview.chromium.org/2528813002/diff/140001/content/test/data/page... content/test/data/page_with_meta_refresh_frame.html:11: " <meta http-equiv=\"refresh\" content=\"3\">" + On 2017/01/06 02:27:29, alexmos wrote: > Let's use "0" here to not slow down the tests on the bots :) Done. https://codereview.chromium.org/2528813002/diff/140001/content/test/data/page... content/test/data/page_with_meta_refresh_frame.html:19: doc.open(); On 2017/01/06 02:27:30, alexmos wrote: > Let's add a comment that this will overwrite the iframe document's URL with the > URL of the parent document. (Since this is the most non-obvious part of this > code.) Done.
Thanks, this is almost there! Just a few more minor comments. https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/2528813002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3184: TestFrameNavigationObserver observer2(child); On 2017/01/19 18:26:24, davidsac wrote: > On 2017/01/06 02:27:29, alexmos wrote: > > This setup is very similar to the test in SelfReferencingFragmentFrames. Can > we > > just move lines 3184-3196 there (before the grandchild checks), and check both > > things in one test? (And then you won't need this test.) > > I feel like that would make it a bit confusing what was getting tested. > Although both tests use URL fragments, this test checks a pretty different bug. > That being said, I have renamed this test to be a bit more clear that it is > related to the other tests. Acknowledged. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:840: if (url_.SchemeIs("about")) { nit: { not needed https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:845: if (!is_renderer_initiated_) { nit: { not needed https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:855: if (found_self_reference) { nit: { not needed https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3036: // See https://crbug.com/650332 . nit: rewrap comment to 80 chars. Also no space needed after bug number (same for all other places below) https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3105: I'd suggest moving up the definition of |expected_url| here, and doing the EXPECT_EQ(expected_url, grandchild->current_url()) here, in addition to doing it after the failed navigation below. This way it's very clear that the grandchild starts at this URL, and that this URL isn't expected to change. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3109: TestNavigationManager delayer(web_contents, stalled_url); Let's not call this "delayer", as we aren't actually delaying any requests here; maybe just "manager" will do. Similarly, let's rename |stalled_url| to something like "blocked_url". "Stalled" would imply that the request is going to hang up waiting for a network reply, which isn't what you're doing here. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3112: EXPECT_FALSE(delayer.WaitForRequestStart()); I'd suggest to add a comment to explain this a bit. Something like "Wait for WillStartRequest and verify that it aborts the request before starting it." https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3130: // an about:blank iframe which injects html containing a meta refresh into the nit: s/which/where the page/ (it's the page injecting the html, not the iframe) https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3139: // TODO(davidsac): add in an expect true here for something? There isn't a good way to expect anything from that helper at the moment. :( I think you can remove this TODO - you've got it covered with the two last EXPECTs below. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3162: // Ensure that navigating a subframe to the same url as its parent twice in a nit: s/url/URL/ https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3176: TestFrameNavigationObserver observer1(child); I'd suggest defining url_expected here, perhaps as GURL second_url(url.spec() + "#123"). Then use it both in ExecuteScript and in the EXPECT_EQ below. You could also rename |url| to |first_url| and it will be clear that the child's URL should've changed from |second_url| back to |first_url|. (right now this was a bit hard to follow for me.) https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3192: TestFrameNavigationObserver observer2(child); Perhaps add a comment that this navigation shouldn't be blocked, because the blocking only happens when more than one ancestor has the same URL (excluding fragments), and the frame's current URL shouldn't count toward that. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3202: FrameTreeVisualizer().DepictFrameTree(root)); I feel like the DepictFrameTree checks in this test aren't adding much here, given all the coverage you've got for it in other tests. If the second navigation were to be incorrectly blocked, the frame tree would look the same way. I'd suggest removing both of them here -- just checking that the child navigated to the URL correctly should be sufficient for what this test is doing. https://codereview.chromium.org/2528813002/diff/300001/content/test/data/core... File content/test/data/coreferencingframe_2.html (right): https://codereview.chromium.org/2528813002/diff/300001/content/test/data/core... content/test/data/coreferencingframe_2.html:4: B - This page has co-referencing nested iframes. Or does it? "Or does it?" - yeah, it does :) https://codereview.chromium.org/2528813002/diff/300001/content/test/data/page... File content/test/data/page_with_meta_refresh_frame.html (right): https://codereview.chromium.org/2528813002/diff/300001/content/test/data/page... content/test/data/page_with_meta_refresh_frame.html:9: "<html>" + nit: indent the string lines
The CQ bit was checked by davidsac@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.
https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:840: if (url_.SchemeIs("about")) { On 2017/01/19 23:45:53, alexmos wrote: > nit: { not needed Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:845: if (!is_renderer_initiated_) { On 2017/01/19 23:45:53, alexmos wrote: > nit: { not needed Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:855: if (found_self_reference) { On 2017/01/19 23:45:53, alexmos wrote: > nit: { not needed Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3036: // See https://crbug.com/650332 . On 2017/01/19 23:45:53, alexmos wrote: > nit: rewrap comment to 80 chars. > > Also no space needed after bug number (same for all other places below) Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3105: On 2017/01/19 23:45:54, alexmos wrote: > I'd suggest moving up the definition of |expected_url| here, and doing the > EXPECT_EQ(expected_url, grandchild->current_url()) here, in addition to doing it > after the failed navigation below. This way it's very clear that the grandchild > starts at this URL, and that this URL isn't expected to change. Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3109: TestNavigationManager delayer(web_contents, stalled_url); On 2017/01/19 23:45:54, alexmos wrote: > Let's not call this "delayer", as we aren't actually delaying any requests here; > maybe just "manager" will do. Similarly, let's rename |stalled_url| to > something like "blocked_url". "Stalled" would imply that the request is going > to hang up waiting for a network reply, which isn't what you're doing here. Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3112: EXPECT_FALSE(delayer.WaitForRequestStart()); On 2017/01/19 23:45:54, alexmos wrote: > I'd suggest to add a comment to explain this a bit. Something like "Wait for > WillStartRequest and verify that it aborts the request before starting it." Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3130: // an about:blank iframe which injects html containing a meta refresh into the On 2017/01/19 23:45:54, alexmos wrote: > nit: s/which/where the page/ (it's the page injecting the html, not the iframe) Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3139: // TODO(davidsac): add in an expect true here for something? On 2017/01/19 23:45:53, alexmos wrote: > There isn't a good way to expect anything from that helper at the moment. :( I > think you can remove this TODO - you've got it covered with the two last EXPECTs > below. Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3162: // Ensure that navigating a subframe to the same url as its parent twice in a On 2017/01/19 23:45:54, alexmos wrote: > nit: s/url/URL/ Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3176: TestFrameNavigationObserver observer1(child); On 2017/01/19 23:45:53, alexmos wrote: > I'd suggest defining url_expected here, perhaps as GURL second_url(url.spec() + > "#123"). Then use it both in ExecuteScript and in the EXPECT_EQ below. You > could also rename |url| to |first_url| and it will be clear that the child's URL > should've changed from |second_url| back to |first_url|. (right now this was a > bit hard to follow for me.) Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3192: TestFrameNavigationObserver observer2(child); On 2017/01/19 23:45:53, alexmos wrote: > Perhaps add a comment that this navigation shouldn't be blocked, because the > blocking only happens when more than one ancestor has the same URL (excluding > fragments), and the frame's current URL shouldn't count toward that. Done. https://codereview.chromium.org/2528813002/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:3202: FrameTreeVisualizer().DepictFrameTree(root)); On 2017/01/19 23:45:53, alexmos wrote: > I feel like the DepictFrameTree checks in this test aren't adding much here, > given all the coverage you've got for it in other tests. If the second > navigation were to be incorrectly blocked, the frame tree would look the same > way. I'd suggest removing both of them here -- just checking that the child > navigated to the URL correctly should be sufficient for what this test is doing. Good point! Done. https://codereview.chromium.org/2528813002/diff/300001/content/test/data/core... File content/test/data/coreferencingframe_2.html (right): https://codereview.chromium.org/2528813002/diff/300001/content/test/data/core... content/test/data/coreferencingframe_2.html:4: B - This page has co-referencing nested iframes. Or does it? On 2017/01/19 23:45:54, alexmos wrote: > "Or does it?" - yeah, it does :) Done. https://codereview.chromium.org/2528813002/diff/300001/content/test/data/core... content/test/data/coreferencingframe_2.html:4: B - This page has co-referencing nested iframes. Or does it? On 2017/01/19 23:45:54, alexmos wrote: > "Or does it?" - yeah, it does :) Whoops! I fixed this a while back on one, but not both. https://codereview.chromium.org/2528813002/diff/300001/content/test/data/page... File content/test/data/page_with_meta_refresh_frame.html (right): https://codereview.chromium.org/2528813002/diff/300001/content/test/data/page... content/test/data/page_with_meta_refresh_frame.html:9: "<html>" + On 2017/01/19 23:45:54, alexmos wrote: > nit: indent the string lines Done.
Thanks! LGTM
Description was changed from ========== Fix infinite loop of frames being created when one or more frames contain nested IFrames to one another. To reproduce this issue, run Chrome in site-per-process mode and visit either http://dudeguy409.github.io/coreferencingframe1 or https://davidsac.github.io/coreferencingframe2.html. The logic for checking if a site navigation is to an already-visited page should be moved to the core logic of the browser, rather than in blink. Currently, the check in blink can not check the full URLs of parent frames because they are remote frames, so the check is simply skipped. BUG=650332, 527367 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix infinite loop of frames being created when one or more frames contain nested IFrames to one another. To reproduce this issue, run Chrome in site-per-process mode and visit either http://dudeguy409.github.io/coreferencingframe1 or https://davidsac.github.io/coreferencingframe2.html. The current check refuses to load a URL if any two ancestors in the frame tree have the same URL. It is currently inadequate for two reasons: 1) This doesn't work for OOPIFs. If the parent frames are in a different process, they will be remote frames which don't have URLs, so the check is simply skipped. (2) it's only applied in a subset of cases and doesn't handle redirects, for example. To fix these issues, this CL moves these checks to the browser process, where it's applied inside NavigationHandleImpl any time a request is started or a redirect is received. BUG=650332, 527367 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
davidsac@chromium.org changed reviewers: + alexmos (at blinkon until 2-2); nasko; dcheng@chromium.org - alexmos@chromium.org, nasko@chromium.org
Hi Daniel, Could you please review this CL for OWNER approval of the modified WebKit files?
davidsac@chromium.org changed reviewers: + alexmos@chromium.org, dcheng@chromium.org, nasko@chromium.org - alexmos (at blinkon until 2-2); nasko; dcheng@chromium.org
Hi Daniel, Could you please review this CL for OWNER approval of the modified WebKit files?
Description was changed from ========== Fix infinite loop of frames being created when one or more frames contain nested IFrames to one another. To reproduce this issue, run Chrome in site-per-process mode and visit either http://dudeguy409.github.io/coreferencingframe1 or https://davidsac.github.io/coreferencingframe2.html. The current check refuses to load a URL if any two ancestors in the frame tree have the same URL. It is currently inadequate for two reasons: 1) This doesn't work for OOPIFs. If the parent frames are in a different process, they will be remote frames which don't have URLs, so the check is simply skipped. (2) it's only applied in a subset of cases and doesn't handle redirects, for example. To fix these issues, this CL moves these checks to the browser process, where it's applied inside NavigationHandleImpl any time a request is started or a redirect is received. BUG=650332, 527367 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix infinite loop of frames being created when one or more frames contain nested IFrames to one another. To reproduce this issue, run Chrome in site-per-process mode and visit either http://dudeguy409.github.io/coreferencingframe1 or https://davidsac.github.io/coreferencingframe2.html. The current check refuses to load a URL if any two ancestors in the frame tree have the same URL. It is currently inadequate for two reasons: 1) This doesn't work for OOPIFs. If the parent frames are in a different process, they will be remote frames which don't have URLs, so the check is simply skipped. (2) it's only applied in a subset of cases and doesn't handle redirects, for example. To fix these issues, this CL moves these checks to the browser process, where it's applied inside NavigationHandleImpl any time a request is started or a redirect is received. BUG=650332, 527367 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
https://codereview.chromium.org/2528813002/diff/340001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2528813002/diff/340001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:61: url2.ReplaceComponents(replacements)); We (or more precisely, csharrison@) has gone to a lot of trouble to reduce the number of string copies when working with urls. Can we do the same thing that the Blink equivalent of this function does, so we can avoid copies here?
Hey dcheng, could you take a look at my attempt to make EqualIgnoringFragmentIdentifier more efficient?
drive-by comment. https://codereview.chromium.org/2528813002/diff/360001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2528813002/diff/360001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:59: bool EqualIgnoringFragmentIdentifier(const GURL& a, const GURL& b) { This is actually being implemented in GURL itself as part of another CL. Maybe just wait for it to land (likely today/tomorrow) and use that directly. https://codereview.chromium.org/2584513003/diff/1240001/url/gurl.h
On 2017/02/07 19:51:20, nasko (very slow) wrote: > drive-by comment. > > https://codereview.chromium.org/2528813002/diff/360001/content/browser/frame_... > File content/browser/frame_host/navigation_handle_impl.cc (right): > > https://codereview.chromium.org/2528813002/diff/360001/content/browser/frame_... > content/browser/frame_host/navigation_handle_impl.cc:59: bool > EqualIgnoringFragmentIdentifier(const GURL& a, const GURL& b) { > This is actually being implemented in GURL itself as part of another CL. Maybe > just wait for it to land (likely today/tomorrow) and use that directly. > > https://codereview.chromium.org/2584513003/diff/1240001/url/gurl.h Cool, nice to know that's in the works. If that CL doesn't land tomorrow, maybe we can pull the GURL bits and land them independently?
On 2017/02/08 04:35:17, dcheng wrote: > On 2017/02/07 19:51:20, nasko (very slow) wrote: > > drive-by comment. > > > > > https://codereview.chromium.org/2528813002/diff/360001/content/browser/frame_... > > File content/browser/frame_host/navigation_handle_impl.cc (right): > > > > > https://codereview.chromium.org/2528813002/diff/360001/content/browser/frame_... > > content/browser/frame_host/navigation_handle_impl.cc:59: bool > > EqualIgnoringFragmentIdentifier(const GURL& a, const GURL& b) { > > This is actually being implemented in GURL itself as part of another CL. Maybe > > just wait for it to land (likely today/tomorrow) and use that directly. > > > > https://codereview.chromium.org/2584513003/diff/1240001/url/gurl.h > > Cool, nice to know that's in the works. > If that CL doesn't land tomorrow, maybe we can pull the GURL bits and land them > independently? Extracted into https://codereview.chromium.org/2681113002/, which should land soon.
The CQ bit was checked by davidsac@chromium.org to run a CQ dry run
Hi Daniel, This CL is ready for another round of reviews. Thanks. https://codereview.chromium.org/2528813002/diff/340001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2528813002/diff/340001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:61: url2.ReplaceComponents(replacements)); On 2017/01/28 01:51:44, dcheng wrote: > We (or more precisely, csharrison@) has gone to a lot of trouble to reduce the > number of string copies when working with urls. > > Can we do the same thing that the Blink equivalent of this function does, so we > can avoid copies here? Done. https://codereview.chromium.org/2528813002/diff/360001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2528813002/diff/360001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:59: bool EqualIgnoringFragmentIdentifier(const GURL& a, const GURL& b) { On 2017/02/07 19:51:20, nasko wrote: > This is actually being implemented in GURL itself as part of another CL. Maybe > just wait for it to land (likely today/tomorrow) and use that directly. > > https://codereview.chromium.org/2584513003/diff/1240001/url/gurl.h Acknowledged.
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 checked by davidsac@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.
LGTM with a minor nit https://codereview.chromium.org/2528813002/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2528813002/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:472: return true; Nit: it's a really long return, but the usual convention is to just: return !condition; instead of if (condition) return false; return true;
The CQ bit was checked by davidsac@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...
Hi Nasko, Please feel free to take another look at this CL if you would like, before it gets landed.
Hi Dcheng, I made the fix https://codereview.chromium.org/2528813002/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2528813002/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:472: return true; On 2017/02/12 08:34:59, dcheng wrote: > Nit: it's a really long return, but the usual convention is to just: > > return !condition; > > instead of > > if (condition) > return false; > return true; Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by davidsac@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2528813002/#ps440001 (title: "refactor allowedToLoadFrame conditional")
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by alexmos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1487172736202960,
"parent_rev": "183dc82ce88ae08056adfcc97e48937c0fe1be59", "commit_rev":
"04fdd09f342badeb2af3a639d62a4da65d4baaa1"}
Message was sent while issue was closed.
Description was changed from ========== Fix infinite loop of frames being created when one or more frames contain nested IFrames to one another. To reproduce this issue, run Chrome in site-per-process mode and visit either http://dudeguy409.github.io/coreferencingframe1 or https://davidsac.github.io/coreferencingframe2.html. The current check refuses to load a URL if any two ancestors in the frame tree have the same URL. It is currently inadequate for two reasons: 1) This doesn't work for OOPIFs. If the parent frames are in a different process, they will be remote frames which don't have URLs, so the check is simply skipped. (2) it's only applied in a subset of cases and doesn't handle redirects, for example. To fix these issues, this CL moves these checks to the browser process, where it's applied inside NavigationHandleImpl any time a request is started or a redirect is received. BUG=650332, 527367 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix infinite loop of frames being created when one or more frames contain nested IFrames to one another. To reproduce this issue, run Chrome in site-per-process mode and visit either http://dudeguy409.github.io/coreferencingframe1 or https://davidsac.github.io/coreferencingframe2.html. The current check refuses to load a URL if any two ancestors in the frame tree have the same URL. It is currently inadequate for two reasons: 1) This doesn't work for OOPIFs. If the parent frames are in a different process, they will be remote frames which don't have URLs, so the check is simply skipped. (2) it's only applied in a subset of cases and doesn't handle redirects, for example. To fix these issues, this CL moves these checks to the browser process, where it's applied inside NavigationHandleImpl any time a request is started or a redirect is received. BUG=650332, 527367 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2528813002 Cr-Commit-Position: refs/heads/master@{#450728} Committed: https://chromium.googlesource.com/chromium/src/+/04fdd09f342badeb2af3a639d62a... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001) as https://chromium.googlesource.com/chromium/src/+/04fdd09f342badeb2af3a639d62a... |
