New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 622509 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

!source_url.is_empty() in render_frame_impl.cc

Project Member Reported by ClusterFuzz, Jun 22 2016

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6404769052884992

Fuzzer: inferno_layout_test_unmodified
Job Type: linux_debug_content_shell_drt
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  !source_url.is_empty() in render_frame_impl.cc
  content::RenderFrameImpl::decidePolicyForNavigation
  blink::FrameLoaderClientImpl::decidePolicyForNavigation
  

Minimized Testcase (0.16 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv97ij_XuPApJ-FvU0LRns4vwjYeV1K2uJ8hPCWVKknhxrjiCzIUUJGmMiXaem1Vgasw1v7G0ZvzeDZEtgo_XzYARSvQcGg3NYNU4-BJcpSVSulbXYILitIb8mbAK1nw6jTo7C6CjUlQiO4gtBsFUW8-q-7Nz9g?testcase_id=6404769052884992
<script>
var __v_212 = window.open("resources/reentrant-beforeunload-helper.html");
__v_212.open( "resources/xmlhttprequest-no-content-type-with-text.php");
</script>


Filer: mummareddy

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Cc: ben@chromium.org
Labels: Te-Logged M-52
Owner: creis@chromium.org
Status: Assigned (was: Available)
As per code search on file render_frame_impl.cc, recent related changes done by creis@. could you please have a look and help us to find correct owner.

Thank you

Comment 2 by creis@chromium.org, Jun 22 2016

Cc: creis@chromium.org nasko@chromium.org
Components: UI>Browser>Navigation
Owner: tsepez@chromium.org
Looks like it's failing this DCHECK?

    if (!should_fork && url.SchemeIs(url::kFileScheme)) {
      // Fork non-file to file opens.  Check the opener URL if this is the
      // initial navigation in a newly opened window.
      GURL source_url(old_url);
      if (is_initial_navigation && source_url.is_empty() && frame_->opener())
        source_url = frame_->opener()->top()->document().url();
      DCHECK(!source_url.is_empty());
      should_fork = !source_url.SchemeIs(url::kFileScheme);
    }

tsepez@ added that in https://codereview.chromium.org/10517009.  Tom, can you take a look?

Looks like just a DCHECK failure, but that code does look like it has a bug.  The source URL could still be empty if the window had a chain of multiple openers or if the opener were cleared.  Matters in the case that the navigation happens before the first commit, and it might bypass the intent of the security check in release builds (when DCHECKs aren't present).
Project Member

Comment 3 by sheriffbot@chromium.org, Jun 23 2016

Labels: -M-52 M-53 MovedFrom-52
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by sheriffbot@chromium.org, Jul 3 2016

Labels: -M-53 -Pri-1 M-54 MovedFrom-53 Pri-2
This issue is Pri-1 but has already been moved once. Lowering the priority and moving to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by tsepez@chromium.org, Jul 14 2016

Should be OK to just remove the DCHECK(), then.
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6573f1eaf729320beeaf776674b124bfe5c330bb

commit 6573f1eaf729320beeaf776674b124bfe5c330bb
Author: tsepez <tsepez@chromium.org>
Date: Thu Jul 14 22:35:57 2016

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

Review-Url: https://codereview.chromium.org/2153573002
Cr-Commit-Position: refs/heads/master@{#405609}

[modify] https://crrev.com/6573f1eaf729320beeaf776674b124bfe5c330bb/content/renderer/render_frame_impl.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/11f3ed430923329885682d50057217041ce661e1

commit 11f3ed430923329885682d50057217041ce661e1
Author: tkent <tkent@chromium.org>
Date: Fri Jul 15 04:16:37 2016

Revert of Simplify RenderFrameImpl::decidePolicyForNavigation and avoid DCHECK (patchset #2 id:20001 of https://codereview.chromium.org/2153573002/ )

Reason for revert:
Broke a layout test fast/events/popup-allowed-from-gesture-initiated-form-submit.html .

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&showLargeExpectations=true&tests=popup-allowed-from-gesture-initiated-form-submit.html

Original issue's description:
> 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}

TBR=creis@chromium.org,tsepez@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 622509 ,  628478 

Review-Url: https://codereview.chromium.org/2150243002
Cr-Commit-Position: refs/heads/master@{#405694}

[modify] https://crrev.com/11f3ed430923329885682d50057217041ce661e1/content/renderer/render_frame_impl.cc

Project Member

Comment 8 by ClusterFuzz, Jul 15 2016

ClusterFuzz has detected this issue as fixed in range 405563:405613.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6404769052884992

Fuzzer: inferno_layout_test_unmodified
Job Type: linux_debug_content_shell_drt
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  !source_url.is_empty() in render_frame_impl.cc
  content::RenderFrameImpl::decidePolicyForNavigation
  blink::FrameLoaderClientImpl::decidePolicyForNavigation
  
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_debug_content_shell_drt&range=405563:405613

Minimized Testcase (0.16 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv97ij_XuPApJ-FvU0LRns4vwjYeV1K2uJ8hPCWVKknhxrjiCzIUUJGmMiXaem1Vgasw1v7G0ZvzeDZEtgo_XzYARSvQcGg3NYNU4-BJcpSVSulbXYILitIb8mbAK1nw6jTo7C6CjUlQiO4gtBsFUW8-q-7Nz9g?testcase_id=6404769052884992
<script>
var __v_212 = window.open("resources/reentrant-beforeunload-helper.html");
__v_212.open( "resources/xmlhttprequest-no-content-type-with-text.php");
</script>


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 9 by ClusterFuzz, Jul 15 2016

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase is verified as fixed, closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Status: Assigned (was: Verified)
Reverted, bug reopened
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/77b02a9e18d0a359c570e871d970fc515d54a9aa

commit 77b02a9e18d0a359c570e871d970fc515d54a9aa
Author: tsepez <tsepez@chromium.org>
Date: Fri Jul 15 22:21:11 2016

Remove DCHECK(!source_url.is_empty()) from RenderFrameImpl::decidePolicyForNavigation

BUG= 622509 

Review-Url: https://codereview.chromium.org/2152353002
Cr-Commit-Position: refs/heads/master@{#405877}

[modify] https://crrev.com/77b02a9e18d0a359c570e871d970fc515d54a9aa/content/renderer/render_frame_impl.cc

Status: Fixed (was: Assigned)
Project Member

Comment 13 by sheriffbot@chromium.org, Nov 22 2016

Labels: -Restrict-View-EditIssue
Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment