Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2153573002:
Simplify RenderFrameImpl::decidePolicyForNavigation and avoid DCHECK (Closed)
|
Created:
4 years, 5 months ago by Tom Sepez Modified:
4 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimplify RenderFrameImpl::decidePolicyForNavigation and avoid DCHECK.
DCHECK can be tripped when there is a chain of openers or when the
opener has been cleared. The subsequent comparison against file:
fails, and we may make a browser navigation where one could have
been avoided, but it is still correct.
Since it is still correct, we can simplify the check, perhaps
forking in more cases that we used to, but that doesn't matter.
BUG=622509
R=creis@chromium.org
Committed: https://crrev.com/6573f1eaf729320beeaf776674b124bfe5c330bb
Cr-Commit-Position: refs/heads/master@{#405609}
Patch Set 1 #
Total comments: 2
Patch Set 2 : simplify logic #Messages
Total messages: 22 (13 generated)
The CQ bit was checked by tsepez@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...
Charlie, for review.
Thanks! Wondering if we can go one step further. https://codereview.chromium.org/2153573002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/2153573002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4986: source_url = frame_->opener()->top()->document().url(); Do we gain much by having the opener check at all? I ask because it's broken for OOPIFs, in which case top()->document() may be null. (We're going to have to come back to fix this in https://crbug.com/466297.) If it's safe to fork for file-to-file navigations when the opener isn't found, it's probably safe to do it even when there is an opener. And that's good, because I think some tab (or frame) other than the opener might be able to cause this navigation as well. Maybe we can change the comment to something like: // Fork non-file to file opens. Note that this may fork unnecessarily if another tab (hosting a file or not) targeted this one before its initial navigation, but that shouldn't cause a problem.
Description was changed from ========== Remove DCHECK(!source_url.is_empty()) from RenderFrameImpl::decidePolicyForNavigation This can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. BUG=622509 R=creis@chromium.org ========== to ========== Remove DCHECK(!source_url.is_empty()) from RenderFrameImpl::decidePolicyForNavigation This can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. Since it is still correct, we can simplify the check, perhaps forking in more cases that we used to, but that doesn't matter. BUG=622509 R=creis@chromium.org ==========
Description was changed from ========== Remove DCHECK(!source_url.is_empty()) from RenderFrameImpl::decidePolicyForNavigation This can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. Since it is still correct, we can simplify the check, perhaps forking in more cases that we used to, but that doesn't matter. BUG=622509 R=creis@chromium.org ========== to ========== Remove DCHECK(!source_url.is_empty()) from RenderFrameImpl::decidePolicyForNavigation This can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. Since it is still correct, we can simplify the check, perhaps forking in more cases that we used to, but that doesn't matter. BUG=622509 R=creis@chromium.org ==========
https://codereview.chromium.org/2153573002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/2153573002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4986: source_url = frame_->opener()->top()->document().url(); On 2016/07/14 21:16:47, Charlie Reis wrote: > Do we gain much by having the opener check at all? I ask because it's broken > for OOPIFs, in which case top()->document() may be null. (We're going to have > to come back to fix this in https://crbug.com/466297.) > > If it's safe to fork for file-to-file navigations when the opener isn't found, > it's probably safe to do it even when there is an opener. And that's good, > because I think some tab (or frame) other than the opener might be able to cause > this navigation as well. > > Maybe we can change the comment to something like: > // Fork non-file to file opens. Note that this may fork unnecessarily if > another tab (hosting a file or not) targeted this one before its initial > navigation, but that shouldn't cause a problem. Done.
The CQ bit was checked by tsepez@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...
Thanks, LGTM. Don't forget to update the CL description.
Description was changed from ========== Remove DCHECK(!source_url.is_empty()) from RenderFrameImpl::decidePolicyForNavigation This can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. Since it is still correct, we can simplify the check, perhaps forking in more cases that we used to, but that doesn't matter. BUG=622509 R=creis@chromium.org ========== to ========== Simplify RenderFrameImpl::decidePolicyForNavigation and avoid DCHECK. This can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. Since it is still correct, we can simplify the check, perhaps forking in more cases that we used to, but that doesn't matter. BUG=622509 R=creis@chromium.org ==========
Description was changed from ========== Simplify RenderFrameImpl::decidePolicyForNavigation and avoid DCHECK. This can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. Since it is still correct, we can simplify the check, perhaps forking in more cases that we used to, but that doesn't matter. BUG=622509 R=creis@chromium.org ========== to ========== Simplify RenderFrameImpl::decidePolicyForNavigation and avoid DCHECK. DCHECK can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. Since it is still correct, we can simplify the check, perhaps forking in more cases that we used to, but that doesn't matter. BUG=622509 R=creis@chromium.org ==========
The CQ bit was unchecked by tsepez@chromium.org
The CQ bit was checked by tsepez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Simplify RenderFrameImpl::decidePolicyForNavigation and avoid DCHECK. DCHECK can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. Since it is still correct, we can simplify the check, perhaps forking in more cases that we used to, but that doesn't matter. BUG=622509 R=creis@chromium.org ========== to ========== Simplify RenderFrameImpl::decidePolicyForNavigation and avoid DCHECK. DCHECK can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. Since it is still correct, we can simplify the check, perhaps forking in more cases that we used to, but that doesn't matter. BUG=622509 R=creis@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Simplify RenderFrameImpl::decidePolicyForNavigation and avoid DCHECK. DCHECK can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. Since it is still correct, we can simplify the check, perhaps forking in more cases that we used to, but that doesn't matter. BUG=622509 R=creis@chromium.org ========== to ========== Simplify RenderFrameImpl::decidePolicyForNavigation and avoid DCHECK. DCHECK can be tripped when there is a chain of openers or when the opener has been cleared. The subsequent comparison against file: fails, and we may make a browser navigation where one could have been avoided, but it is still correct. Since it is still correct, we can simplify the check, perhaps forking in more cases that we used to, but that doesn't matter. BUG=622509 R=creis@chromium.org Committed: https://crrev.com/6573f1eaf729320beeaf776674b124bfe5c330bb Cr-Commit-Position: refs/heads/master@{#405609} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6573f1eaf729320beeaf776674b124bfe5c330bb Cr-Commit-Position: refs/heads/master@{#405609}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2150243002/ by tkent@chromium.org. The reason for reverting is: Broke a layout test fast/events/popup-allowed-from-gesture-initiated-form-submit.html . http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec... .
Message was sent while issue was closed.
creis@chromium.org changed reviewers: + lukasza@chromium.org
Message was sent while issue was closed.
On 2016/07/15 04:15:17, tkent wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/2150243002/ by mailto:tkent@chromium.org. > > The reason for reverting is: Broke a layout test > fast/events/popup-allowed-from-gesture-initiated-form-submit.html . > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec... > . The crash was due to a POST to a file:// URL: 20:44:37.988 5273 [11930:11930:0714/204437:3914558297:FATAL:navigation_controller_impl.cc(673)] Check failed: false. Http post load must use http(s) scheme. 20:44:37.988 5273 #0 0x000002834c1e base::debug::StackTrace::StackTrace() 20:44:37.988 5273 #1 0x000002849ddb logging::LogMessage::~LogMessage() 20:44:37.988 5273 #2 0x0000023ff03e content::NavigationControllerImpl::LoadURLWithParams() 20:44:37.988 5273 #3 0x0000027f3fa3 content::Shell::OpenURLFromTab() 20:44:37.988 5273 #4 0x0000026c6188 content::WebContentsImpl::RequestOpenURL() 20:44:37.988 5273 #5 0x00000240cc0c content::NavigatorImpl::RequestOpenURL() 20:44:37.988 5273 #6 0x000002419ab3 content::RenderFrameHostImpl::OpenURL() I think lukasza@ was intending to loosen that restriction, so maybe we can check with him when he gets back from vacation next week. Anyway, glad to see there's some test coverage for this. :) |
