New issue
Advanced search Search tips

Issue 752372 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug
Team-Security-UX

Blocked on:
issue 752370

Blocking:
issue 448486



Sign in to add a comment

Interstitial refactor: adapt SSL browser tests for committed interstitials

Project Member Reported by est...@chromium.org, Aug 4 2017

Issue description

After implementing the new interstitial logic, we'll have to adapt browser tests for it. This includes ssl_browser_tests.cc but possibly others. Basically, we'll need to turn on the flag for the new code path, then run all tests and fix whatever needs to be fixed.

Design doc pointer: https://docs.google.com/document/d/1rEBpw5V-Nn1UIi8CIFa5ZZvwlR08SkY3CogvWE2UMFs/edit#heading=h.enpjxt8bnc0o
 

Comment 1 by est...@chromium.org, Nov 10 2017

Labels: Hotlist-EnamelAndFriendsFixIt
Forgot to put the bug # in the bbug description for this CL:
================================
Start adapting SSL browser tests for committed interstitials.

We do this by introducing a new test class called TransientAndCommittedSSLUITest
to support testing both transient and committed interstitial navigations.

This CL migrates tests that pass under committed interstitials without modification.
Subsequent CLs will fix up additional classes of tests.

Bug: 
Change-Id: I5868e9cc5587dc3b122ea856341f77409d496024
Reviewed-on: https://chromium-review.googlesource.com/769241
Commit-Queue: Lucas Garron <lgarron@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516639}
Owner: lgar...@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 17 2017

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

commit 56e0c61a1190bd6fd5aa8ff085389e58958d2cfb
Author: Lucas Garron <lgarron@chromium.org>
Date: Fri Nov 17 02:48:12 2017

SSL browser tests: Use a WaitForInterstitial() wrapper compatible with committed interstitials.

Bug: 752372
Change-Id: I931681551c7c38a42cfef5c941d620c18c4562a3
Reviewed-on: https://chromium-review.googlesource.com/773563
Commit-Queue: Lucas Garron <lgarron@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517259}
[modify] https://crrev.com/56e0c61a1190bd6fd5aa8ff085389e58958d2cfb/chrome/browser/ssl/ssl_browsertest.cc

Owner: est...@chromium.org
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 8 2017

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

commit 3c5be56d665bc07dec88655b0dfc1e07d838cdd0
Author: Emily Stark <estark@google.com>
Date: Fri Dec 08 17:51:13 2017

Adapt nearly all of ssl_browsertest.cc to committed interstitials

Nearly all of ssl_browsertest.cc are now parameterized to run with committed
interstitials both disabled and enabled. The tests that are not yet converted
are:
- ones that don't involve interstitials
- ones that depend on the Javascript commands being hooked up ( https://crbug.com/779782 )
- ones that are already parameterized (so they need a little extra work to also
  parameterize for committed interstitials)
- the certificate reporting ones, which require a little more refactoring
  ( https://crbug.com/792324 )
- There are additional SSL interstitial browser tests in
  captive_portal_blocking_page_browsertest.cc, policy_browsertest.cc,
  certificate_reporting_service_browsertest.cc, and
  security_state_tab_helper_browsertest.cc that will need to be converted
  separately.

One browser test revealed a bug in SSLErrorTabHelper which this CL also
fixes. (We were improperly cleaning up blocking pages when a navigation finishes
but does not commit.)

TBR=pastarmovj@chromium.org

Bug: 752372
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I16b395ae54c2546c0cc30a502b2da66266c1fe8a
Reviewed-on: https://chromium-review.googlesource.com/809890
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522816}
[modify] https://crrev.com/3c5be56d665bc07dec88655b0dfc1e07d838cdd0/chrome/browser/interstitials/security_interstitial_page_test_utils.cc
[modify] https://crrev.com/3c5be56d665bc07dec88655b0dfc1e07d838cdd0/chrome/browser/interstitials/security_interstitial_page_test_utils.h
[modify] https://crrev.com/3c5be56d665bc07dec88655b0dfc1e07d838cdd0/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/3c5be56d665bc07dec88655b0dfc1e07d838cdd0/chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc
[modify] https://crrev.com/3c5be56d665bc07dec88655b0dfc1e07d838cdd0/chrome/browser/ssl/ssl_blocking_page.h
[modify] https://crrev.com/3c5be56d665bc07dec88655b0dfc1e07d838cdd0/chrome/browser/ssl/ssl_browsertest.cc
[modify] https://crrev.com/3c5be56d665bc07dec88655b0dfc1e07d838cdd0/chrome/browser/ssl/ssl_error_tab_helper.cc
[modify] https://crrev.com/3c5be56d665bc07dec88655b0dfc1e07d838cdd0/chrome/browser/ssl/ssl_error_tab_helper_unittest.cc
[modify] https://crrev.com/3c5be56d665bc07dec88655b0dfc1e07d838cdd0/components/security_interstitials/core/bad_clock_ui.cc
[modify] https://crrev.com/3c5be56d665bc07dec88655b0dfc1e07d838cdd0/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Comment 7 by est...@chromium.org, Feb 18 2018

Labels: -Hotlist-EnamelAndFriendsFixIt
Project Member

Comment 8 by bugdroid1@chromium.org, May 11 2018

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

commit 0ea4e59bf6dd74763e22cd49466cbe5afcb70959
Author: Carlos IL <carlosil@chromium.org>
Date: Fri May 11 01:51:57 2018

Adapt additional SSL interstitial tests to committed interstitials
(Rebase of crrev.com/c/827676)

This adapts remaining SSL interstitial tests in policy_browsertest.cc,
captive_portal_blocking_page_browsertest.cc, and
security_state_tab_helper_browsertest.cc to run with committed interstitials
enabled.

There is a small handful of other relevant tests scattered throughout the
codebase, but those should be straightforward to adapt when we turn on committed
interstitials by default.

Bug: 752372
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ic83cd3be0c2f4f95876d0bafbe8922932fa60f3c
Reviewed-on: https://chromium-review.googlesource.com/1041126
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557763}
[modify] https://crrev.com/0ea4e59bf6dd74763e22cd49466cbe5afcb70959/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/0ea4e59bf6dd74763e22cd49466cbe5afcb70959/chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc
[modify] https://crrev.com/0ea4e59bf6dd74763e22cd49466cbe5afcb70959/chrome/browser/ssl/security_state_tab_helper.cc
[modify] https://crrev.com/0ea4e59bf6dd74763e22cd49466cbe5afcb70959/chrome/browser/ssl/security_state_tab_helper_browsertest.cc
[modify] https://crrev.com/0ea4e59bf6dd74763e22cd49466cbe5afcb70959/chrome/browser/ssl/ssl_browsertest.cc
[modify] https://crrev.com/0ea4e59bf6dd74763e22cd49466cbe5afcb70959/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Project Member

Comment 9 by bugdroid1@chromium.org, May 11 2018

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

commit ec7108a6d8e4878ff06a49fe921cd90ffaab0e15
Author: Matt Falkenhagen <falken@chromium.org>
Date: Fri May 11 06:02:51 2018

NetworkService: Reenable some browser tests after r557763.

These still seem to pass locally. I suspect a merge caused them to be
accidentally added in
https://chromium-review.googlesource.com/c/chromium/src/+/1041126

PlatformAppBrowserTest.AppWindowAdjustBoundsToBeVisibleOnScreen
PlatformAppBrowserTest.CreateAndCloseAppWindow
MimeHandlerViewTests/MimeHandlerViewTest.SingleRequest/0

Bug: 752372
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Id2713b0196ed1cfa666a2d62c37da8600b02fbd0
TBR: jam
NOTRY: true
Reviewed-on: https://chromium-review.googlesource.com/1055129
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557803}
[modify] https://crrev.com/ec7108a6d8e4878ff06a49fe921cd90ffaab0e15/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 23

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

commit d87f46c26a7294b03da487908ffc6cf98037bb6d
Author: Carlos IL <carlosil@chromium.org>
Date: Tue Oct 23 19:40:24 2018

Adapt SSLUIDynamicInterstitial tests for committed interstitials.

Bug: 752372
Change-Id: Id083cb05a837169140b2b3b7464b92aebfa7a409
Reviewed-on: https://chromium-review.googlesource.com/c/1297004
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602058}
[modify] https://crrev.com/d87f46c26a7294b03da487908ffc6cf98037bb6d/chrome/browser/ssl/ssl_browsertest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 26

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

commit 414ab4c6515329a5e7815125e028dcb1cb793ac2
Author: Carlos IL <carlosil@chromium.org>
Date: Fri Oct 26 00:11:31 2018

Adapt WebviewTests for committed interstitials

With committed interstitials, interstitials are no longer a special
case of navigation. Adapted interstitial related WebView tests so they
are compatible with committed interstitials.

Bug: 752372
Change-Id: I5df2d2fa6b96632704828df66af0f61e33dc8d1e
Reviewed-on: https://chromium-review.googlesource.com/c/1299929
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602940}
[modify] https://crrev.com/414ab4c6515329a5e7815125e028dcb1cb793ac2/chrome/browser/apps/guest_view/web_view_browsertest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Nov 13

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

commit ad7cb8bd1279ecdbefa58e0c217398542e3665bc
Author: Carlos IL <carlosil@chromium.org>
Date: Tue Nov 13 17:36:51 2018

Disable MessagingInterstitial test for committed interstitials

This test was added as part of a fix for a crash that occurred due to
interstitials not having an associated web contents.
With committed interstitials enabled, interstitials are no longer a
special case, and do have a web contents, so the special conditions
that are checked in this test no longer apply.

Bug: 752372
Change-Id: Id06e38674dd17f97a467f98b5759433aad8cd6ec
Reviewed-on: https://chromium-review.googlesource.com/c/1313675
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607634}
[modify] https://crrev.com/ad7cb8bd1279ecdbefa58e0c217398542e3665bc/chrome/browser/extensions/extension_messages_apitest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Nov 14

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

commit 73defadf883036a56cb541f79c727a0d1f875719
Author: Carlos IL <carlosil@chromium.org>
Date: Wed Nov 14 17:01:37 2018

Adapt HideStarOnNonbookmarkedInterstitial for committed interstitials

Bug: 752372
Change-Id: I11f97243aa8aecd1ae00d55317e31d94d8e0d612
Reviewed-on: https://chromium-review.googlesource.com/c/1334649
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608017}
[modify] https://crrev.com/73defadf883036a56cb541f79c727a0d1f875719/chrome/browser/ui/bookmarks/bookmark_browsertest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Nov 22

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

commit 03630edab20251d635febe1f7196eb238c245104
Author: Lucas Furukawa Gadani <lfg@chromium.org>
Date: Thu Nov 22 23:31:14 2018

Disable WebViewTest.InterstitialPageFocusedWidget with committed interstitials.

Bug: 752372
Change-Id: Icc7b51b58168982b0237cd6a3fe06259c8a6f7ac
Reviewed-on: https://chromium-review.googlesource.com/c/1348971
Commit-Queue: Lucas Gadani <lfg@chromium.org>
Reviewed-by: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610502}
[modify] https://crrev.com/03630edab20251d635febe1f7196eb238c245104/chrome/browser/apps/guest_view/web_view_browsertest.cc

Sign in to add a comment