New issue
Advanced search Search tips

Issue 866367 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 846235



Sign in to add a comment

Pass browser_tests and content_browsertests when NetS13nSW is on.

Project Member Reported by falken@chromium.org, Jul 23

Issue description

In order to enable NetS13nSW by default, we'll need to make all browser tests pass.

We should prepare a CL that enables by default and see how many tests fail on the CQ and fix all the failures so we can eventually enable by default when ready.

 
FWIW I ran browser_tests and content_browsertests a while ago on my machine.

https://bugs.chromium.org/p/chromium/issues/detail?id=861644
https://bugs.chromium.org/p/chromium/issues/detail?id=861657

At that time above two failures were all failures I found (and fixed).
Ah, I got confused and thought we only did this for ServiceWorker* tests. Thanks, I didn't realize you already did this for everything.

Would you be able to run it on the CQ to see if everything is really green everywhere?
Owner: bashi@chromium.org
Status: Assigned (was: Available)
Sure. Assigning to myself for now.
Cc: yhirano@chromium.org
yhirano: FYI, we expect to already pass almost all tests with --enable-features="ServiceWorkerServicification" (unless there are failures we wouldn't see locally like Android-only failures etc).

We are still working on content_unittests at  issue 860361 .

Comment 5 Deleted

Ran on trybots: https://chromium-review.googlesource.com/c/chromium/src/+/1146529

Looks like we have some failures:

content_browsertests

PreviewsStateBrowserTest.ShouldEnableLoFiModeOn
PreviewsStateBrowserTest.ShouldEnableLoFiModeNavigateBackThenForward
PreviewsStateBrowserTest.ShouldEnableLoFiModeReload

browser_tests on ChromeOS and Linux

ResourceLoadingHintsBrowserTest.ResourceLoadingHintsHttpsWhitelistedRedirectToHttps
ResourceLoadingHintsBrowserTest.ResourceLoadingHintsHttpsWhitelisted
PreviewsNoScriptBrowserTest.NoScriptPreviewsEnabled
PreviewsNoScriptBrowserTest.NoScriptPreviewsEnabledHttpRedirectToHttps
And some layout tests not covered by the virtual test suite? I'm not sure if it's out-of-scope of this issue though.
It looks like the finch config only runs browser_tests with the flag.

At https://chromium-review.googlesource.com/c/chromium/src/+/1147891, the failures are:

content_browsertests on Android (flake?):
MSE_ExternalClearKey/EncryptedMediaTest.FrameSizeChangeVideo/0

browser_tests on others:
PreviewsNoScriptBrowserTest.NoScriptPreviewsEnabled
PreviewsNoScriptBrowserTest.NoScriptPreviewsEnabledHttpRedirectToHttps

Actually not sure about the first sentence of c#8.
Regarding PreviewsNoScriptBrowserTests:

It looks like Previews use NavigationData to store their data. Uploaded a fix.
https://chromium-review.googlesource.com/c/chromium/src/+/1148094

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 30

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

commit a71756341b5f561768144f0ba51b1567783884d5
Author: Kenichi Ishibashi <bashi@chromium.org>
Date: Mon Jul 30 02:08:35 2018

S13nServiceWorker: Populate NavigationData when NetworkService is disabled

When S13nServiceWorker is enabled but NetworkService is disabled,
NavigationURLLoaderImpl actually uses URLRequest when a navigation
is not intercepted. In that case we have a NavigationData and we can
populate it to NavigationHandle. Since some features like Previews
use NavigationData to store their data, it would be better to
populate it if possible.

This fixes following browser_tests:
- PreviewsNoScriptBrowserTest.NoScriptPreviewsEnabled
- PreviewsNoScriptBrowserTest.NoScriptPreviewsEnabledHttpRedirectToHttps

Bug:  866367 
Change-Id: Idc85d272772be81dddde9e9e0bdf76f4fadfef8a
Reviewed-on: https://chromium-review.googlesource.com/1148094
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578963}
[modify] https://crrev.com/a71756341b5f561768144f0ba51b1567783884d5/content/browser/loader/navigation_url_loader_impl.cc

Status: Fixed (was: Assigned)
All browser_tests and content_browsertests pass on my Linux machine.

(Actually some tests are failing but these failures happen w/o S13nSW too).
 
Filed tracking bug for layout test failures ( issue 869302 ).

I'll re-open this if I find other failures on bots etc.

Sign in to add a comment