New issue
Advanced search Search tips

Issue 606594 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug

Blocked on:
issue 595895

Blocking:
issue 477150
issue 646607



Sign in to add a comment

UaF of delegate_ in WebFrameTestClient::willSendRequest

Project Member Reported by lukasza@chromium.org, Apr 26 2016

Issue description

Repro (with ASAN enabled):
DISPLAY=:20 third_party/WebKit/Tools/Scripts/run-webkit-tests -t gn -v --additional-drt-flag=--site-per-process --child-processes=3 --additional-drt-flag=--no-sandbox --iterations=1 http/tests/local/serviceworker/fetch-request-body-file.html

The problem is that WebFrameTestClient gets, stores and uses a global delegate (passed from WebTestInterfaces::CreateWebFrameTestClient) rather than using frame/view-specific delegate via this->web_test_proxy_base_->delegate().

Fixing this will be a bit complicated, because  which (incorrect) delegate is being used has been "baked in" for a while (i.e. delegate_->PrintMessage calls have no effect if accidentally going through a swapped-out delegate).
 
Actually, the issue is not about swapped-out vs not, but about the fact that nobody listens for PrintMessage from secondary windows.  This means that whether PrintMessage is delivered depends on which window first appeared in the given renderer process (main vs secondary window).
Project Member

Comment 2 by bugdroid1@chromium.org, May 3 2016

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

commit 6bd0f3201a2bc6c1d30dfc8a5a23eeeaedf56e2b
Author: lukasza <lukasza@chromium.org>
Date: Tue May 03 16:26:14 2016

Triaging some of the remaining site-per-process layout test failures.

Most of the changes in this CL just moved test expectations around and
added comments with bug pointers.

One exception is that http/tests/appcache/remove-cache.html expectation
has been removed altogether:
- this test passes on the Site Isolation FYI
- flakiness is already called out in  https://crbug.com/518929 
- locally I see a failure with and without --site-per-process, but since
  the bots are happy we should just remove the exception I think

BUG= 582211 ,  608015 ,  608023 ,  606594 ,  607991 ,  607981 

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

[modify] https://crrev.com/6bd0f3201a2bc6c1d30dfc8a5a23eeeaedf56e2b/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Comment 3 by sshru...@google.com, May 18 2016

Labels: Test-Layout

Comment 4 by sshru...@google.com, May 18 2016

Components: -Blink>LayoutTests
Deprecating component:Blink>LayoutTests, to use label Test=Layout instead.
Blocking: 646607
Blockedon: 595895
Always going through process-level messages will make us resilient to the trouble pointed out in #c1.
Owner: lukasza@chromium.org
Status: Assigned (was: Untriaged)
UaF fix proposal: https://codereview.chromium.org/2349063002

As pointed out in #c6, the fix above depends on switching to process-level messages - tentative CL is at https://codereview.chromium.org/1814963002
Cc: jiameng@chromium.org
+jiameng@ as an FYI in case she runs into the UaF as part of the work to migrate layout tests to use mojo.
Cc: lukasza@chromium.org pfeldman@chromium.org dpranke@chromium.org dmazz...@chromium.org
 Issue 766007  has been merged into this issue.
Project Member

Comment 10 by ClusterFuzz, Feb 14 2018

Components: Blink
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

Comment 11 by bokan@chromium.org, Feb 15 2018

Components: -Blink Blink>Internals
Project Member

Comment 12 by ClusterFuzz, Feb 16 2018

Labels: OS-Mac
Project Member

Comment 13 by ClusterFuzz, Mar 21 2018

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

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Sign in to add a comment