New issue
Advanced search Search tips

Issue 831155 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Task

Blocked on:
issue 821877



Sign in to add a comment

Call URLLoaderClient::OnStartLoadingResponseBody earlier.

Project Member Reported by arthurso...@chromium.org, Apr 10

Issue description

After the URLLoaderClient interface is unbound in the browser process and bound again in the renderer process, it takes some amount of time to receive URLLoader::OnStartLoadingResponseBody(). It may worth sending the data pipe with the RenderFrameHostImpl::CommitNavigation() message. It would allow the Renderer process to commit the navigation faster.

There are no strong reason to make URLLoader::OnReceiveResponse() and URLLoader::OnStartLoadingResponseBody() separate messages.

This issue tracks progress in this direction.
 
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8fb971332437ff058748c3a0a3130c5a8be1c835

commit 8fb971332437ff058748c3a0a3130c5a8be1c835
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Fri Mar 23 18:05:09 2018

Update navigation tests with client side redirect.

In an effort to commit navigation faster, we are facing some failing
test: https://chromium-review.googlesource.com/c/chromium/src/+/951732

This CL update some of them. In these tests, there are a client-side
redirect. A new navigation is initiated by the document while
it is not fully parsed. These tests are waiting for 2 load stops:
  1) one for the initial navigation,
  2) one for the redirect.
It turns out, the second navigation may be fast enough, so that it
commits before the first document has been loaded.
DocumentLoader::LoadFailed() is called for the first document.
The browser receives DidStopLoading() only once instead of two.

Bug:  705744 
Change-Id: I4201124c0e70d64204e3c9070aae588a684e0913
Reviewed-on: https://chromium-review.googlesource.com/960664
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545517}
[modify] https://crrev.com/8fb971332437ff058748c3a0a3130c5a8be1c835/chrome/browser/devtools/devtools_sanity_browsertest.cc
[modify] https://crrev.com/8fb971332437ff058748c3a0a3130c5a8be1c835/chrome/browser/ssl/ssl_browsertest.cc
[modify] https://crrev.com/8fb971332437ff058748c3a0a3130c5a8be1c835/content/browser/frame_host/navigation_controller_impl_browsertest.cc
Blockedon: 821877
FYI: I ran a benchmark with my temporary patch:
https://ct.skia.org/results/cluster-telemetry/tasks/chromium_perf_runs/arthursonzogni-20180411144049/html/index.html

I am not sure if it means anything. I saw another benchmark with an empty patch, ironically, it shows the same kind of difference:
https://ct.skia.org/results/cluster-telemetry/tasks/chromium_perf_runs/farahcharab-20180410221318/html/index.html
Description: Show this description
Project Member

Comment 5 by bugdroid1@chromium.org, May 3

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

commit c13c16a87cd7bfb4b4883311cd1a2b7343311a57
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Thu May 03 14:53:33 2018

Update two scroll tests.

This updates the following tests:
- NavigationControllerBrowserTest.ReloadWithUrlAnchor
- NavigationControllerBrowserTest.ReloadWithUrlAnchorAndScroll

This two tests are making a <div> to scroll because of a navigation to
an anchor. Then the tests check the scroll position is restored after a
reload.

The issue is that it is not deterministic when the scrolled area is not
the root area. It is working consistently only because the main
resource's response is small (e.g <64ko) and
FrameLoader::ProcessFragment() is called before
FrameLoader::RestoreScrollPositionAndViewState().

This CL make the tests uses the root area instead of the <div>.

  ---

Description of what happens in the current code:

1) When ProcessFragment() is called first:

  a) ProcessFragment():
  Several scrollable layers are scrolled such that the anchor is
  displayed. It may includes the root one, but also some children.

  b) RestoreScrollPositionAndViewState():
  The root layer is scrolled using what is stored in the HistoryItem. It
  overrides the scroll done in a) for the root layer. Children layers
  are not overridden.

2) When RestoreScrollPositionAndViewState() is called first:

  a) RestoreScrollPositionAndViewState():
  The root layer is scrolled using what is stored in the HistoryItem.
  Children layers are not scrolled.

  b) ProcessFragment():
  Nothing happens because InitialScrollState::did_restore_from_history
  has been set to true in a). The ProcessFragment() will skip the
  fragment scroll.

At the end, the difference is that the children layers are scrolled in
1), but not in 2).

  ---

Bug:  821877 , 831155, 839292
Change-Id: I11c74224ba42ad58814c4d4e185d7db189551775
Reviewed-on: https://chromium-review.googlesource.com/1039827
Reviewed-by: Jianpeng Chao <chaopeng@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555727}
[modify] https://crrev.com/c13c16a87cd7bfb4b4883311cd1a2b7343311a57/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/c13c16a87cd7bfb4b4883311cd1a2b7343311a57/content/test/data/navigation_controller/reload-with-url-anchor.html

Project Member

Comment 6 by bugdroid1@chromium.org, May 7

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

commit cfa684720709c0804d65805a1bd1790b695e1ebd
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Mon May 07 17:18:00 2018

Update WPT test: url-in-tags.window.js

The web-platform-test: FileAPI/url/url-in-tags.window.js relies on the
load event to be dispatched after the page has scrolled to the anchor.

This is not guaranteed. It is currently non-deterministic in Chrome.
The page: https://scrolly-in-onload-event.glitch.me/ shows a case where
the opposite happens.

Doing task https://crbug.com/831155 caused this test to fail.

Note: This test was working only on Chrome.
Current status: https://wpt.fyi/FileAPI/url/url-in-tags.window.html
There is a chance applying this patch will make it work on Safari as
well.

Related CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1042191

Bug: 831155
Change-Id: Ifa3f8e3d35cdb2635261248c3c9e9fd959f44340
Reviewed-on: https://chromium-review.googlesource.com/1046596
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556479}
[modify] https://crrev.com/cfa684720709c0804d65805a1bd1790b695e1ebd/third_party/WebKit/LayoutTests/external/wpt/FileAPI/url/url-in-tags.window.js

Project Member

Comment 7 by bugdroid1@chromium.org, May 11

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

commit 95336cb92b1337d4ad5d2a7fcc7409ec25b4a8ee
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Fri May 11 08:34:35 2018

Fix test: back-to-redirect-with-frame.php

In this test, the following lines looks legitimate, but they are not:
~~~
window.onload = setTimeout(function() {
    window.location = "resources/go-back.html"
}, 10);
~~~
The issue is that setTimeout is executed immediately instead of on load.

This CL wraps it into a function. It also reduces the timeout from 10ms
to 0ms.

Bug: 831155
Change-Id: I7e35fcaeedf918a29fa12ae1f8292d07e08ac213
Reviewed-on: https://chromium-review.googlesource.com/1051908
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557816}
[modify] https://crrev.com/95336cb92b1337d4ad5d2a7fcc7409ec25b4a8ee/third_party/WebKit/LayoutTests/http/tests/navigation/back-to-redirect-with-frame.php

Project Member

Comment 8 by bugdroid1@chromium.org, May 14

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

commit 4835a7de8159e7abde78afe271dc83ee08482971
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Mon May 14 08:55:12 2018

Fix tests: window.onload = setTimeout.

This is similar to a previous CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1051908

All of these tests are using:
window.onload = setTimeout([...], 0);
The issue is that setTimeout is executed immediately instead of on load.

Bug: 831155
Change-Id: I913b8c04de5d583ef87c91dfc23d4ba8be8ba642
Reviewed-on: https://chromium-review.googlesource.com/1055391
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558229}
[modify] https://crrev.com/4835a7de8159e7abde78afe271dc83ee08482971/third_party/WebKit/LayoutTests/http/tests/history/replaceState-then-fragment.html
[modify] https://crrev.com/4835a7de8159e7abde78afe271dc83ee08482971/third_party/WebKit/LayoutTests/http/tests/history/resources/push-state-in-grandchild-grandchild.html
[modify] https://crrev.com/4835a7de8159e7abde78afe271dc83ee08482971/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/async-xhr-revalidate-after-sync-xhr.html
[modify] https://crrev.com/4835a7de8159e7abde78afe271dc83ee08482971/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/sync-xhr-revalidate-after-async-xhr-expected.txt
[modify] https://crrev.com/4835a7de8159e7abde78afe271dc83ee08482971/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/sync-xhr-revalidate-after-async-xhr.html

Project Member

Comment 9 by bugdroid1@chromium.org, May 14

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

commit b3a06c1ba0f43f88e3764eb7b1d09b12f43a45c9
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Mon May 14 11:52:35 2018

Fix flaky HeadlessWebContentsTest focus test.

This test is a bit flaky:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=headless_browsertests&tests=FocusOfHeadlessWebContents_IsIndependent

Furthermore, I am working on https://crbug.com/831155. It causes the
frame to commit a navigation more quickly. As a result the frame may
stop loading sooner. It causes this test to become even flakier.

Why this test is flaky?
It waits for the main frame to stop loading by calling WaitForLoad().
Then it waits for the main frame to gain focus by calling
WaitForFocus(). In reality, it may gain focus before it stops loading.
Since WaitForFocus() doesn't check whether or not the main frame is
already focused before waiting, it may wait forever.

The CL makes the tests wait for both event to happen, no matter the
order.

Related CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1026993

Bug: 831155
Change-Id: Id9002b3ee2a06ef17d670858688d28959f334e03
Reviewed-on: https://chromium-review.googlesource.com/1053777
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Eric Seckler <eseckler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558262}
[modify] https://crrev.com/b3a06c1ba0f43f88e3764eb7b1d09b12f43a45c9/headless/lib/headless_web_contents_browsertest.cc
[modify] https://crrev.com/b3a06c1ba0f43f88e3764eb7b1d09b12f43a45c9/headless/test/headless_browser_test.cc
[modify] https://crrev.com/b3a06c1ba0f43f88e3764eb7b1d09b12f43a45c9/headless/test/headless_browser_test.h

Project Member

Comment 10 by bugdroid1@chromium.org, May 18

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

commit 986ffe005c730dbdbd55bf3a1d9b7fc05b691c39
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Fri May 18 07:37:27 2018

FirstMeaningfulPaint: Ensure event is sent.

The FirstMeaningfulPaintDetector method |CheckNetworkStable| is called
whenever an active connection is removed.
If the number of connection is low enough and is stable for long enough,
the FirstMeaningfulPaint event will be sent. The current implementation
does nothing if the document has not been parsed yet.

In particular, for the main resource, this function is called when the
load is completed. When it happens, the document may not have been
parsed yet. It depends on how things are scheduled in blink. If there
are no other sub resources to load, |CheckNetworkStable()| will not be
called again and the FirstMeaningfulPaint will never be send.

I am working on https://crbug.com/831155. It modifies how things are
scheduled in Blink. Several tests related to this issue are failing:
* SessionRestorePageLoadMetricsBrowserTest.LoadingAfterSessionRestore
* SessionRestorePageLoadMetricsBrowserTest.MultipleSessionRestores
* SessionRestorePageLoadMetricsBrowserTest.MultipleTabsSessionRestore
* SessionRestorePageLoadMetricsBrowserTest.RestoreForeignTab

This CL fixes this issue by calling |CheckNetworkStable()| when the
document has been parsed.

Bug: 831155
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I22c814280087dffd99e2eb6c02918dc1b8201fea
Reviewed-on: https://chromium-review.googlesource.com/1064055
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559829}
[modify] https://crrev.com/986ffe005c730dbdbd55bf3a1d9b7fc05b691c39/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/page/page-lifecycleEvents-expected.txt
[modify] https://crrev.com/986ffe005c730dbdbd55bf3a1d9b7fc05b691c39/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/986ffe005c730dbdbd55bf3a1d9b7fc05b691c39/third_party/blink/renderer/core/paint/first_meaningful_paint_detector.cc
[modify] https://crrev.com/986ffe005c730dbdbd55bf3a1d9b7fc05b691c39/third_party/blink/renderer/core/paint/first_meaningful_paint_detector_test.cc

Project Member

Comment 11 by bugdroid1@chromium.org, May 22

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

commit c875e5d1fa83c83cf8cf3258454717c3b740e8b6
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Tue May 22 15:16:02 2018

navigate_to_204.html: Wait for window.load before navigating.

I am working on https://crbug.com/831155. Some tests that are relying
on how things are scheduled in Blink are failing.

This CL makes the following test to keep working:
 * ExecuteScriptApiTest.FrameWithHttp204

What happened with this test?

1) Javascript test is started:
   test/data/extensions/api_test/executescript/http204/background.js

2) Function navigateToFrameAndWaitUntil204Loaded is executed.
   It navigates to:
   /extensions/api_test/executescript/http204/navigate_to_204.html?b.com

3) |location.href = "[...]page204.html"| happens before the document is
   fully loaded.

4) The navigation to page204.html cancels the previous one.
   FrameMsg_DroppedNavigation IPC is sent.

5) Test fails, it expected "page204.html" load to fail, but
   "navigate_to_204.html" failed first.

Solution is to wait for the document to be fully loaded before
navigating to page204.html.

Bug: 831155
Change-Id: I8780a48877a8a6a67338aa4a1ae167952cb35fb8
Reviewed-on: https://chromium-review.googlesource.com/1065972
Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560592}
[modify] https://crrev.com/c875e5d1fa83c83cf8cf3258454717c3b740e8b6/chrome/test/data/extensions/api_test/executescript/http204/navigate_to_204.html

Project Member

Comment 12 by bugdroid1@chromium.org, May 24

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

commit 48471218ba9ad3dffcd7657b670dfc17c5638022
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Thu May 24 22:34:53 2018

Fix race condition in frameElement.html.

I am working on https://crbug.com/831155. Some tests that are relying
on how things are scheduled in Blink are failing.

There was a race condition in this test. The iframe "#fr2" was asked to
navigate to a first URL, but soon after, a javascript function
modifies the iframe.src to navigate elsewhere.

What happened when it didn't work?
1) The first navigation starts.
2) The second navigation is scheduled (See blink's NavigationScheduler)
3) The first navigation commits. FrameLoader::CommitProvisionalLoad()
   executes: frame_->GetNavigationScheduler().Cancel(). It cancels the
   second navigation.

This CL fixes the race condition.

Bug: 831155
Change-Id: I2dd951140b4c5a671c749348ca0247f1901d8b77
Reviewed-on: https://chromium-review.googlesource.com/1069013
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561659}
[rename] https://crrev.com/48471218ba9ad3dffcd7657b670dfc17c5638022/third_party/WebKit/LayoutTests/external/wpt/html/browsers/windows/nested-browsing-contexts/frameElement.sub.html

Project Member

Comment 13 by bugdroid1@chromium.org, May 25

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

commit 80348445e449a30d6f4d13b6592fee4044edab3b
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Fri May 25 14:29:54 2018

Refresh frameElement.html test.

- Removes unused things like <div id="log"></div>. It probably made
  sense at some point, but no more after the semi-automatic conversion
  to a WPT test.
- Use more meaningful names.
- Group elements (iframes/embed/objects) by tests.

Bug: 831155
Change-Id: I7bb36132bb3e6be0fc952f4d2d7c1edb847878c1
Reviewed-on: https://chromium-review.googlesource.com/1069089
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561865}
[modify] https://crrev.com/80348445e449a30d6f4d13b6592fee4044edab3b/third_party/WebKit/LayoutTests/external/wpt/html/browsers/windows/nested-browsing-contexts/frameElement.sub.html

Project Member

Comment 14 by bugdroid1@chromium.org, May 28

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

commit 2d1846bed03be552223791562288aea46c7c0c8a
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Mon May 28 12:28:02 2018

Ensure CSS :target is set before calling the load event.

I am working on https://crbug.com/831155. It causes the frame to commit
a navigation more quickly. As a result, some tests that are relying on
how things are scheduled in blink are failling.

This CL makes sure at least two WPT tests keep working:
* external/wpt/dom/nodes/Element-matches.html
* external/wpt/dom/nodes/Element-webkitMatchesSelector.html

What happens in theses tests:
In the frame.load event, document.querySelector(":target") is used.
When an URL has an anchor, like in http://example.com#anchor, it returns
the element with the "anchor" ID.

The problem is that blink::Document::SetCSSTarget(Element*) may
currently be called after the 'load' event is executed.

The solution is simply to exchange two lines:
1) The one that will maybe send the 'load' event.
   frame_->GetDocument()->CheckCompleted();
2) The one that will call Document::SetCSSTarget()
   ProcessFragment([...])

This CL is similar to:
https://chromium-review.googlesource.com/c/chromium/src/+/1042191
It is the same solutioa to a different problem.

This test is disabled because of  http://crbug.com/843192 
* svg/animations/viewspec-checkaspectparams.html

Bug: 831155,  843523 ,  843192 
Change-Id: I8ecedd212d3ce30d252ec9c1b1f22214f7bc012a
Reviewed-on: https://chromium-review.googlesource.com/1057507
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562235}
[modify] https://crrev.com/2d1846bed03be552223791562288aea46c7c0c8a/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/2d1846bed03be552223791562288aea46c7c0c8a/third_party/blink/renderer/core/loader/frame_loader.cc

Project Member

Comment 15 by bugdroid1@chromium.org, May 28

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

commit 6023a17012ff4e86517c14ca0123cd1d345e7b53
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Mon May 28 13:03:03 2018

Headless tests: Update navigation tracking to avoid a race condition.

Why there is a the race condition:
==================================

Given this document:
~~~
<html>
  <head>
    <script>
      document.location = 'http://www.example.com'
    </script>
  </head>
  <body>
    [...]
  </body>
</html>
~~~

The scheduled navigation to http://www.example.com may starts before the
document has finish loading. It depends on the size of the document and
how things are scheduled in blink.

FrameLoader::StartLoad() executes: ProgressTracker::ProgressStarted()
which executes probe::frameStartedLoading() if and only if the frame is
not loading currently.

Since it can be called or not, it causes the HeadlessRenderTest to drop
some navigations and a test to fail.

I am working on https://crbug.com/831155. It makes some navigations to
start sooner. This test is failing:
HeadlessRenderBrowserTestClientRedirectChain.RunAsyncTest
This CL keeps it working.

Description of the patch:
=========================

Using:
 1) frameScheduledNavigation
 2) frameStartedLoading
 3) frameClearScheduledNavigation
is not a correct way to track navigation. A new navigation can start
before the previous one has finished loading. So the second
frameStartedLoading may or may not be sent.

This patch makes tests to continue to track whether or not some
navigation have been scheduled using 1), but it stops trying to get
informations from 2) and 3).

More generally, I think using frameScheduledNavigation and
frameClearScheduledNavigation should be avoided. At a first sight, I
think it will miss some navigations, at least every browser initiated
ones and maybe some renderer initiated too.

Bug: 831155
Change-Id: I3bd18a6b20ee5a73e0116244066ee699822dec33
Reviewed-on: https://chromium-review.googlesource.com/1071630
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562241}
[modify] https://crrev.com/6023a17012ff4e86517c14ca0123cd1d345e7b53/headless/test/headless_render_browsertest.cc
[modify] https://crrev.com/6023a17012ff4e86517c14ca0123cd1d345e7b53/headless/test/headless_render_test.cc
[modify] https://crrev.com/6023a17012ff4e86517c14ca0123cd1d345e7b53/headless/test/headless_render_test.h

Project Member

Comment 16 by bugdroid1@chromium.org, May 28

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

commit a777927d9ca90acd0590bf12fc340f038657705f
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Mon May 28 15:43:02 2018

Update inspector protocol test that relied on timing.

I am working on https://crbug.com/831155. Some tests that are relying
on how things are scheduled in Blink are failing.

This CL makes 2 tests to keep working.
 *  inspector-protocol/page/frameAttachedDetached.js
 *  inspector-protocol/sessions/page-frame-events.js

In these two tests, a frame is navigated twice. Immediately after the first
load starts, a second navigation is requested.

The tests expect Page.OnFrameStartedLoading to be called twice. The
issue is the first load may not be finished when the second navigation
is started.

ProgressTracker::ProgressStarted() doesn't send the
frameStartedLoading() if the frame is already loading something when
FrameLoader::StartLoading is called.

To fix the issue, the tests now wait for the frame to stop loading
before navigating again.

Bug: 831155
Change-Id: Iee71767be6d15dbf53c370bb414fe57015dd1df6
Reviewed-on: https://chromium-review.googlesource.com/1071791
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562267}
[modify] https://crrev.com/a777927d9ca90acd0590bf12fc340f038657705f/third_party/WebKit/LayoutTests/inspector-protocol/page/frameAttachedDetached.js
[modify] https://crrev.com/a777927d9ca90acd0590bf12fc340f038657705f/third_party/WebKit/LayoutTests/inspector-protocol/sessions/page-frame-events.js

Project Member

Comment 17 by bugdroid1@chromium.org, Jun 13

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

commit 250cef84d714b3386a3910845842ea12c3a7abed
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Wed Jun 13 11:37:25 2018

Fix test: drag-files-to-editable-element.html

The tested feature regressed, but no one realized it, because the test continued
to pass.

Instead of displaying the filenames in the <div>, a new navigation to the file
happened. Starting the navigation causes the current Document to stop and the
test to finish early. The tests finished before it has executed any assertions.

This CL fixes the test, but not the regression.

Bug: 821455, 831155
Change-Id: I61b9cca80677ca2caed72b0e9cf6074f73c2ad6a
Reviewed-on: https://chromium-review.googlesource.com/1094879
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566800}
[modify] https://crrev.com/250cef84d714b3386a3910845842ea12c3a7abed/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/250cef84d714b3386a3910845842ea12c3a7abed/third_party/WebKit/LayoutTests/editing/pasteboard/drag-files-to-editable-element.html
[add] https://crrev.com/250cef84d714b3386a3910845842ea12c3a7abed/third_party/WebKit/LayoutTests/editing/pasteboard/resources/drag-files-to-editable-element-fail

Project Member

Comment 18 by bugdroid1@chromium.org, Jun 15

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

commit 3ae920b18b96d7fd53c32d9d70abfeba49c5754c
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Fri Jun 15 14:30:21 2018

Update DomTreeExtractionBrowserTest.

Assigned Node ID may not be the same for each executions of the test
DomTreeExtractionBrowserTest.RunAsyncTest. This CL clears the value of
backendNodeId to make this test more deterministic.

backendNodeId uses DOMNodIds::IdForNode(). Here are two stack traces
where the ID number 3 is assigned to a different Node.

02 blink::DOMNodeIds::IdForNode()
03 blink::InspectorDOMSnapshotAgent::VisitNode()
04 blink::InspectorDOMSnapshotAgent::VisitContainerChildren()
05 blink::InspectorDOMSnapshotAgent::VisitNode()
06 blink::InspectorDOMSnapshotAgent::getSnapshot()
07 blink::InspectorDOMSnapshotAgent::getSnapshot()
08 blink::protocol::DOMSnapshot::DispatcherImpl::getSnapshot()
09 blink::protocol::DOMSnapshot::DispatcherImpl::dispatch()
10 blink::protocol::UberDispatcher::dispatch()
11 blink::InspectorSession::DispatchProtocolMessage()
12 blink::WebDevToolsAgentImpl::Session::DispatchProtocolCommand()
13 blink::mojom::blink::DevToolsSessionStubDispatch::Accept()

02 blink::DOMNodeIds::IdForNode()
03 blink::CompositedLayerMapping::CreateGraphicsLayer()
04 blink::CompositedLayerMapping::CreatePrimaryGraphicsLayer()
05 blink::CompositedLayerMapping::CompositedLayerMapping()
06 blink::PaintLayer::EnsureCompositedLayerMapping()
07 blink::PaintLayerCompositor::AllocateOrClearCompositedLayerMapping()
08 blink::CompositingLayerAssigner::AssignLayersToBackingsInternal()
09 blink::CompositingLayerAssigner::Assign()
10 blink::PaintLayerCompositor::UpdateIfNeeded()
11 blink::PaintLayerCompositor::UpdateIfNeededRecursiveInternal()
12 blink::PaintLayerCompositor::UpdateIfNeededRecursive()
13 blink::LocalFrameView::UpdateLifecyclePhasesInternal()
14 blink::LocalFrameView::UpdateAllLifecyclePhases()
15 blink::PageAnimator::UpdateAllLifecyclePhases()
16 blink::PageWidgetDelegate::UpdateLifecycle()
17 blink::WebViewImpl::UpdateLifecycle()
18 blink::WebViewFrameWidget::UpdateLifecycle()
19 content::RenderWidget::UpdateVisualState()
20 content::RenderWidgetCompositor::UpdateLayerTreeHost()
21 cc::LayerTreeHost::RequestMainFrameUpdate()

Currently, this test doesn't appear to be flaky. I am working on:
https://crbug.com/831155. It causes the navigation to commit slightly
faster. The test fails one time out of four, all the backendNodeId in
getSnapshot() are shifted by one.

Bug: 831155
Change-Id: I83a30202877bb65953957438284062f13fabeb6c
Reviewed-on: https://chromium-review.googlesource.com/1101693
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567640}
[modify] https://crrev.com/3ae920b18b96d7fd53c32d9d70abfeba49c5754c/headless/lib/dom_tree_extraction_expected_nodes.txt
[modify] https://crrev.com/3ae920b18b96d7fd53c32d9d70abfeba49c5754c/headless/lib/headless_devtools_client_browsertest.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Jun 18

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

commit 743262940430fc58261bbcf28658d500a74721b6
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Jun 18 08:45:55 2018

Update test event-source-constructor.html

This CL update the blink layout test:
fast/eventsource/eventsource-constructor.html

Depending on timing, the third EventSource:
~~~
   new EventSource("http://disallowed.example.com/");
~~~
may start before the test finishes. It results in a new console error
message displayed.

This CL prevent the console error message to be displayed.

Note: I am working on https://crbug.com/831155. It causes the frame to commit
a navigation slightly faster. As a result, some tests that are relying on
how things are scheduled in blink are failing.

Bug: 831155
Change-Id: I8a6bf0c1f1203be1e529b7eb6e3192849f7c518d
Reviewed-on: https://chromium-review.googlesource.com/1102324
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567952}
[modify] https://crrev.com/743262940430fc58261bbcf28658d500a74721b6/third_party/WebKit/LayoutTests/fast/eventsource/eventsource-constructor.html

Project Member

Comment 20 by bugdroid1@chromium.org, Jun 19

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

commit 3241281561a9eaa49d4471babfb6fa4ff562513a
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Tue Jun 19 09:13:24 2018

Fix potentially flaky inspector-protocol tests.

Update the blink layout test:
http/tests/inspector-protocol/frameScheduledNavigation.js

This CL is similar to:
 * https://chromium-review.googlesource.com/c/chromium/src/+/1071630
 * https://chromium-review.googlesource.com/c/chromium/src/+/1071791

Depending on timing and the size of the main resource, new navigations
initiated in <script> may starts before the document is fully loaded.
In this case, frameStartedLoading is not fired because the load didn't
stop in between.

Bug: 831155
Change-Id: I872b7ff9b332893c0b57f921f992e348276e9d42
Reviewed-on: https://chromium-review.googlesource.com/1102681
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568376}
[modify] https://crrev.com/3241281561a9eaa49d4471babfb6fa4ff562513a/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/page/frameScheduledNavigation-expected.txt
[modify] https://crrev.com/3241281561a9eaa49d4471babfb6fa4ff562513a/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/page/frameScheduledNavigation.js

Project Member

Comment 21 by bugdroid1@chromium.org, Jun 25

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

commit c0327d09b6e4444caca64f3bd16ec0bd20034049
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Jun 25 10:40:27 2018

Call Document::CheckCompleted() after CancelParsing..

Document::CancelParsing() may cause Document::ShouldComplete() to return
true. Document::CheckCompleted() needs to be called so that its
DocumentLoader is marked to have finished its load.

A lot of tests don't expect DidStopLoading() and DidStartLoading() to be
fired when a new DocumentLoader starts and replaces the previous one.
In FrameLoader::StartLoad(), Document::CancelParsing() is moved after
the creation of the new provisional DocumentLoader so that the
FrameLoader isn't temporarily without any non loading Document. If that
happened, DidStopLoading() would be fired.

There was also one small bug with site isolation. The condition
|have_to_check_if_parent_is_completed| in
HTMLFrameOwnerElement::DisconnectContentFrame() was too strict. The
parent document may have completed its load, but not have sent
DidStopLoading yet because its childs were still loading.

Note: I am working on https://crbug.com/831155. Some tests that are
relying on how things are scheduled in Blink are failing. This CL makes
DownloadTest.TestMultipleDownloadsRequests to keep working. Here, while
the current document is loading, a new navigation happens and cancels
the parser of the current document without calling
Document::CheckCompleted() on it. Then, it turns out the navigation is a
download. The navigation is dropped. In the end the main frame doesn't
know its iframe has finished loading and the test fails.

Bug: 831155

Change-Id: Ib2de108e25991349b44ec4071fe61d57ca6e85a1
Reviewed-on: https://chromium-review.googlesource.com/1107808
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570002}
[modify] https://crrev.com/c0327d09b6e4444caca64f3bd16ec0bd20034049/content/browser/download/download_browsertest.cc
[modify] https://crrev.com/c0327d09b6e4444caca64f3bd16ec0bd20034049/third_party/blink/renderer/core/html/html_frame_owner_element.cc
[modify] https://crrev.com/c0327d09b6e4444caca64f3bd16ec0bd20034049/third_party/blink/renderer/core/loader/frame_loader.cc

Project Member

Comment 22 by bugdroid1@chromium.org, Jul 9

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

commit 7521deccbb7d3d4a7700e6e007e679a8706d8be0
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Jul 09 12:31:04 2018

Transmit the response's body datapipe in CommitNavigation().

# Summary of the current behavior:

When a navigation occurs, the mojo::URLLoaderClient interface is first
bound to the content::NavigationURLLoaderImpl in the browser process.
When the browser knows in which renderer process the navigation will
commit, the interface is bound to the content::URLLoaderClientImpl in
the renderer process to continue the navigation. The switching from one
to another happens when mojo::URLLoaderClient::OnReceiveResponse() is
called. This is described here: https://goo.gl/Rrrc7n

# What are we trying to do:

After the mojo::URLLoaderClient interface is unbound in the browser
process and bound again in the renderer process, it takes some amount of
time to receive URLLoader::OnStartLoadingResponseBody(). It may be worth
sending the data pipe with RenderFrameHostImpl::CommitNavigation() and
give it to the renderer process immediatly instead of waiting for it. It
would allow the Renderer process to get the main resource's data
earlier, especially when the renderer process is busy.

Note: It doesn't look like there are any strong reasons to make
      URLLoaderClient::OnReceiveResponse(response_head) and
      URLLoaderClient::OnStartLoadingResponseBody(response_body)
      separate messages, maybe at some point, they could be merged into:
      URLLoaderClient::OnReceiveResponse(response_head, response_body)

# What this CL does:

This CL makes the switch to happen in
URLLoaderClient::OnStartLoadingResponseBody(). The data pipe is
transmitted in CommitNavigation().

This is enabled/disabled using the NavigationImmediateResponse experiment
The goal is to look for performance improvement or regressions on
Canary.

Bug: 831155
Change-Id: Id6cf667fdc1482baf27f41aab754e58d9a5a569b
Reviewed-on: https://chromium-review.googlesource.com/1100830
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573276}
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/browser_side_navigation_browsertest.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/loader/navigation_url_loader_delegate.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/loader/navigation_url_loader_impl.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/common/frame.mojom
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/common/navigation_client.mojom
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/public/common/browser_side_navigation_policy.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/public/common/browser_side_navigation_policy.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/public/common/content_features.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/public/common/content_features.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/renderer/loader/navigation_response_override_parameters.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/renderer/navigation_client.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/renderer/navigation_client.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/renderer/render_frame_impl.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/test/mock_navigation_client_impl.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/test/mock_navigation_client_impl.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/test/test_navigation_url_loader.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/test/test_navigation_url_loader_delegate.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/test/test_navigation_url_loader_delegate.h
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/content/test/test_render_frame.cc
[modify] https://crrev.com/7521deccbb7d3d4a7700e6e007e679a8706d8be0/third_party/blink/renderer/platform/loader/fetch/raw_resource.cc

Project Member

Comment 23 by bugdroid1@chromium.org, Jul 9

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

commit 7178f5c1387636c7e5dfec90a533bcadb5c0c0c1
Author: Philip Rogers <pdr@chromium.org>
Date: Mon Jul 09 18:52:59 2018

Revert "Transmit the response's body datapipe in CommitNavigation()."

This reverts commit 7521deccbb7d3d4a7700e6e007e679a8706d8be0.

Reason for revert: Caused failures on fast/history/history-back-initial-vs-final-url.html (and possibly other tests, see  https://crbug.com/861808 )

Original change's description:
> Transmit the response's body datapipe in CommitNavigation().
> 
> # Summary of the current behavior:
> 
> When a navigation occurs, the mojo::URLLoaderClient interface is first
> bound to the content::NavigationURLLoaderImpl in the browser process.
> When the browser knows in which renderer process the navigation will
> commit, the interface is bound to the content::URLLoaderClientImpl in
> the renderer process to continue the navigation. The switching from one
> to another happens when mojo::URLLoaderClient::OnReceiveResponse() is
> called. This is described here: https://goo.gl/Rrrc7n
> 
> # What are we trying to do:
> 
> After the mojo::URLLoaderClient interface is unbound in the browser
> process and bound again in the renderer process, it takes some amount of
> time to receive URLLoader::OnStartLoadingResponseBody(). It may be worth
> sending the data pipe with RenderFrameHostImpl::CommitNavigation() and
> give it to the renderer process immediatly instead of waiting for it. It
> would allow the Renderer process to get the main resource's data
> earlier, especially when the renderer process is busy.
> 
> Note: It doesn't look like there are any strong reasons to make
>       URLLoaderClient::OnReceiveResponse(response_head) and
>       URLLoaderClient::OnStartLoadingResponseBody(response_body)
>       separate messages, maybe at some point, they could be merged into:
>       URLLoaderClient::OnReceiveResponse(response_head, response_body)
> 
> # What this CL does:
> 
> This CL makes the switch to happen in
> URLLoaderClient::OnStartLoadingResponseBody(). The data pipe is
> transmitted in CommitNavigation().
> 
> This is enabled/disabled using the NavigationImmediateResponse experiment
> The goal is to look for performance improvement or regressions on
> Canary.
> 
> Bug: 831155
> Change-Id: Id6cf667fdc1482baf27f41aab754e58d9a5a569b
> Reviewed-on: https://chromium-review.googlesource.com/1100830
> Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Peter Kasting <pkasting@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573276}

TBR=pkasting@chromium.org,kinuko@chromium.org,nasko@chromium.org,yhirano@chromium.org,clamy@chromium.org,arthursonzogni@chromium.org

Change-Id: I22eac2d660f9cf1d8128331b547c304af729a85a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 831155
Reviewed-on: https://chromium-review.googlesource.com/1129659
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573392}
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/browser_side_navigation_browsertest.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/loader/navigation_url_loader_delegate.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/loader/navigation_url_loader_impl.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/common/frame.mojom
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/common/navigation_client.mojom
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/public/common/browser_side_navigation_policy.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/public/common/browser_side_navigation_policy.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/public/common/content_features.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/public/common/content_features.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/renderer/loader/navigation_response_override_parameters.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/renderer/navigation_client.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/renderer/navigation_client.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/renderer/render_frame_impl.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/test/mock_navigation_client_impl.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/test/mock_navigation_client_impl.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/test/test_navigation_url_loader.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/test/test_navigation_url_loader_delegate.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/test/test_navigation_url_loader_delegate.h
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/content/test/test_render_frame.cc
[modify] https://crrev.com/7178f5c1387636c7e5dfec90a533bcadb5c0c0c1/third_party/blink/renderer/platform/loader/fetch/raw_resource.cc

Project Member

Comment 24 by bugdroid1@chromium.org, Jul 12

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

commit ae5ca38f28d3e136c7def1fe7444afed1db7ea9a
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Thu Jul 12 18:50:48 2018

FrameLoader: move CancelParsing and CheckCompleted earlier

This way we do it before creating new provisional loader, which
will help in the future when there would be no provisional loader.

Added Document::Abort method which encapsulates CancelParsing
and CheckCompleted and also does not issue undesired DidFinishNavigation.

Bug: 831155
Change-Id: Ica5d968508d7aed7bb07125702ceeaeb6f389497
Reviewed-on: https://chromium-review.googlesource.com/1119262
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574650}
[modify] https://crrev.com/ae5ca38f28d3e136c7def1fe7444afed1db7ea9a/third_party/WebKit/LayoutTests/http/tests/loading/onreadystatechange-detach-expected.txt
[add] https://crrev.com/ae5ca38f28d3e136c7def1fe7444afed1db7ea9a/third_party/WebKit/LayoutTests/http/tests/loading/onreadystatechange-window-stop.html
[modify] https://crrev.com/ae5ca38f28d3e136c7def1fe7444afed1db7ea9a/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/ae5ca38f28d3e136c7def1fe7444afed1db7ea9a/third_party/blink/renderer/core/dom/document.h
[modify] https://crrev.com/ae5ca38f28d3e136c7def1fe7444afed1db7ea9a/third_party/blink/renderer/core/loader/frame_loader.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Jul 30

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

commit 83b84428f235e8fb47c72acb621d1d382ee866f0
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Jul 30 08:08:36 2018

Fix flaky xssAuditor test.

The layout test: object-src-inject.html is flaky.
Sometimes, this line is printed:
```
   CONSOLE MESSAGE: Blink Test Plugin: initializing
```

It is fired from BlinkTestInstance::Init(...);

It is racing with the end of the layout test.
This CL disables console error dump to avoid this issue.

Bug:  862959 , 831155
Change-Id: I912e71b9ff8b7b5ccd7a4716f111f2017493e37a
Reviewed-on: https://chromium-review.googlesource.com/1135003
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578994}
[modify] https://crrev.com/83b84428f235e8fb47c72acb621d1d382ee866f0/third_party/WebKit/LayoutTests/http/tests/security/xssAuditor/object-src-inject.html

Project Member

Comment 26 by bugdroid1@chromium.org, Jul 30

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

commit 16edd27467d3490c9f7f46b2b6661d492e780c57
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Jul 30 08:58:42 2018

Fix test: http/tests/navigation/back-send-referrer.html

The navigation may be triggered before the load event. When it happens,
the history entry must replace the current one instead of being appended
to it. Then history.go(-1) fails in the next document.

This CL make the navigation to happen after the load event.

Bug:  862580 , 831155
Change-Id: I4a48d50f5899e3fd65c74d9a89ab836ae94ba7be
Reviewed-on: https://chromium-review.googlesource.com/1133261
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578998}
[modify] https://crrev.com/16edd27467d3490c9f7f46b2b6661d492e780c57/third_party/WebKit/LayoutTests/http/tests/navigation/back-send-referrer.html

Project Member

Comment 27 by bugdroid1@chromium.org, Jul 30

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

commit 5f32e29a69aaeb7c3cb9a29835669cc8116ed404
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Jul 30 09:42:52 2018

Fix 2 flaky blink layout tests.

The test http/tests/misc/adopt-iframe-src-attr-after-remove.html as
always been a bit flaky:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fmisc%2Fadopt-iframe-src-attr-after-remove.html&testType=webkit_layout_tests

The reason is that navigations triggered by a script before the onload
handler don't generate back/forward entries.
See third_party/blink/public/web/web_history_commit_type.h

Sometimes, this test was fast enough so that no history entry was created.
Then history.back() failed and it ended with a timeout.

This is the same for:
http/tests/misc/resource-timing-iframe-restored-from-history.html

Bug:  862580 , 831155
Change-Id: I3b8243c5f43c5043de8e23acfbe561a648c0f115
Reviewed-on: https://chromium-review.googlesource.com/1133170
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579001}
[modify] https://crrev.com/5f32e29a69aaeb7c3cb9a29835669cc8116ed404/third_party/WebKit/LayoutTests/fast/history/history-back-initial-vs-final-url.html
[modify] https://crrev.com/5f32e29a69aaeb7c3cb9a29835669cc8116ed404/third_party/WebKit/LayoutTests/http/tests/misc/resource-timing-iframe-restored-from-history.html

Project Member

Comment 28 by bugdroid1@chromium.org, Aug 31

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

commit 36f5bb5e7c843d8768b86977fc879135da365eea
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Fri Aug 31 08:09:27 2018

Update LayoutTest /history/redirect-js-* (11 tests)

The current behavior of chrome is the following one:
---
When a javascript navigation is triggered, if it happens:
   * before the load event, replace the current history entry.
   * after the load event, append a new history entry.
---

The first case is sometimes called "javascript-redirect".

The current set of tests uses two kind of tests. One with a timeout of 2
seconds. This one is expected to happens after the load event. Another
one with a timeout of 0 seconds. We don't really know this one is
expected to happens. This is flaky. It usually happens after the load
event, but it is non deterministic.

This CLs removes:
  * /http/tests/history/redirect-js-*-0-seconds}.html
  * /http/tests/history/redirect-js-*-2-seconds}.html

And completes:
  * /http/tests/history/redirect-js-*-before-load}.html
  * /http/tests/history/redirect-js-*-after-load}.html

It modifies the tests so that they really depend on the load event, not
on timing, which is not reliable.

Bug:  862591 , 831155
Change-Id: I3d568019c8616e6b52b9db554ad95ed312bd82d9
Reviewed-on: https://chromium-review.googlesource.com/1185094
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587965}
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-document-location-2-seconds.html
[rename] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-document-location-after-load-expected.txt
[add] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-document-location-after-load.html
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-form-submit-0-seconds-expected.txt
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-form-submit-0-seconds.html
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-form-submit-2-seconds-expected.txt
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-form-submit-2-seconds.html
[copy] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-form-submit-after-load-expected.txt
[add] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-form-submit-after-load.html
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-2-seconds-expected.txt
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-2-seconds.html
[rename] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-after-load-expected.txt
[add] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-after-load.html
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-assign-0-seconds.html
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-assign-2-seconds-expected.txt
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-assign-2-seconds.html
[rename] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-assign-after-load-expected.txt
[add] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-assign-after-load.html
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-href-2-seconds.html
[rename] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-href-after-load-expected.txt
[add] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-href-after-load.html
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-replace-0-seconds.html
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-replace-2-seconds.html
[rename] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-replace-after-load-expected.txt
[add] https://crrev.com/36f5bb5e7c843d8768b86977fc879135da365eea/third_party/WebKit/LayoutTests/http/tests/history/redirect-js-location-replace-after-load.html
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/reload-during-load-after-load-event-expected.txt
[delete] https://crrev.com/3b06b81f4b9d5eafc7f65821522bc19bc362a93f/third_party/WebKit/LayoutTests/http/tests/history/reload-during-load-after-load-event.html

Project Member

Comment 29 by bugdroid1@chromium.org, Sep 5

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

commit 2c76a0fac8022f693c0ee86a18676734b79e8512
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Tue Sep 04 16:49:09 2018

Fix flaky LayoutTest: adopt-iframe-src-attr-after-remove.html

This test was:
  1) using non valid HTML syntax (line 3).
  2) using (for no reason) named access on the window object
     https://html.spec.whatwg.org/#named-access-on-the-window-object
     The iframe with id "iframe" is globally accessible using
     "window.iframe" (or implicitly "iframe"). This is more or less
     deprecated in favor of using document.getElementById("iframe"). It
     causes the test to be flaky (see below for understanding why).

This CL removes 1) and 2).

Why this test is flaky?
---------------------------
Line 37 in DOMContentLoaded handler, adoptAfterLoaded is executed after a 1ms
timeout. If the 'load' event is fired before 1ms, the iframe is removed
and window.iframe is now undefined. Then in adoptAfterLoaded, 'ifr' is
undefined and is dereferenced line 24, the script fails just before calling
testRunner.notifyDone().

Bug: 831155,  861808 
Change-Id: I131dfb3b74c6d3fea1c412b219008a1be544a4f7
Reviewed-on: https://chromium-review.googlesource.com/1202282
Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588546}
[modify] https://crrev.com/2c76a0fac8022f693c0ee86a18676734b79e8512/third_party/WebKit/LayoutTests/http/tests/misc/adopt-iframe-src-attr-after-remove.html

Project Member

Comment 30 by bugdroid1@chromium.org, Oct 4

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

commit 4a63ed4ed0dbbd23523479b1732815cf968ad3e5
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Thu Oct 04 16:38:39 2018

Fix flaky test open-with-pending-load2

There is a race in between:
 1) The end of the test.
 2) The XHR 'abort' event handlers executed.

There is a PostTask in between:
 A) blink::XMLHttpRequest::HandleDidCancel()
 B) blink::XMLHttpRequest::HandleRequestError()

FrameLoader::FinishedParsing may be interleaved in between A) and B).

Make use of testRunner.{waitUntilDone, notifyDone} to get reliable
results.

Bug: 831155
Change-Id: Ibe1ad9020319aae7268f331b2dba0911035789e2
Reviewed-on: https://chromium-review.googlesource.com/c/1261520
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596704}
[modify] https://crrev.com/4a63ed4ed0dbbd23523479b1732815cf968ad3e5/third_party/WebKit/LayoutTests/fast/dom/Document/open-with-pending-load2-expected.txt
[modify] https://crrev.com/4a63ed4ed0dbbd23523479b1732815cf968ad3e5/third_party/WebKit/LayoutTests/fast/dom/Document/open-with-pending-load2.html

Sign in to add a comment