Skip to content

GitLab

T
tangled
Commit 0e604b32 authored by Patrick Noland's avatar Patrick Noland Committed by Commit Bot
Browse files
Options

[Chromeshine] Stop using the page-provided canonical URL 

This means we won't handle AMP pages that don't use SXG(signed exchanges).
Trusting the canonical URL blindly has potential privacy risks. Trusting only
Google-provided AMP urls is also the wrong solution; we want to rely on the
long term solution that is SXG.

Bug: 961814
Change-Id: Ib3544e8c478a3a9089d8c7414f7ea42bb65b8794
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1659511Reviewed-by: default avatarSky Malice <skym@chromium.org>
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669854}
parent 95f50ad2
Hide whitespace changes
Inline Side-by-side
Showing with 10 additions and 90 deletions
chrome/android/java/src/org/chromium/chrome/browser/usage_stats/PageViewObserver.java
View file @ 0e604b32
......@@ -21,7 +21,6 @@ import org.chromium.chrome.browser.tabmodel.TabLaunchType;
import org.chromium.chrome.browser.tabmodel.TabModelSelector;
import org.chromium.chrome.browser.tabmodel.TabModelSelectorTabModelObserver;
import org.chromium.chrome.browser.tabmodel.TabSelectionType;
import org.chromium.content_public.browser.NavigationHandle;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
......@@ -58,7 +57,7 @@ public class PageViewObserver {
@Override
public void onShown(Tab tab, @TabSelectionType int type) {
if (!tab.isLoading() && !tab.isBeingRestored()) {
updateUsingCanonicalUrl(tab);
updateUrl(tab.getUrl());
}
}
......@@ -82,25 +81,13 @@ public class PageViewObserver {
public void didFirstVisuallyNonEmptyPaint(Tab tab) {
assert tab == mCurrentTab;
updateUsingCanonicalUrl(tab);
updateUrl(tab.getUrl());
}
@Override
public void onCrash(Tab tab) {
updateUrl(null);
}
@Override
public void onDidFinishNavigation(Tab tab, NavigationHandle navigation) {
// We only check isLikelySubframeAmpNavigation here because this is the only
// observer method that fires for subframe navigations. The reason we have the
// isLikelySubframeAmpNavigation at all is that onDidFinishNavigation triggers very
// often, and we only want to bother getting the canonical URL if we think there's a
// good chance that it will actually be present.
if (isLikelySubframeAmpNavigation(navigation)) {
updateUsingCanonicalUrl(tab);
}
}
};
mTabModelObserver = new TabModelSelectorTabModelObserver(tabModelSelector) {
......@@ -146,22 +133,6 @@ public class PageViewObserver {
}
}
private void updateUsingCanonicalUrl(Tab tab) {
if (tab.getWebContents() == null || tab.getWebContents().getMainFrame() == null) {
updateUrl(tab.getUrl());
return;
}
tab.getWebContents().getMainFrame().getCanonicalUrlForSharing((canonicalUrl) -> {
if (tab != mCurrentTab) return;
String urlToUse = tab.getUrl();
if (canonicalUrl != null && canonicalUrl.length() > 0) {
urlToUse = canonicalUrl;
}
updateUrl(urlToUse);
});
}
/**
* Updates our state from the previous url to {@code newUrl}. This can result in any/all of the
* following:
......@@ -232,7 +203,7 @@ public class PageViewObserver {
// If the newly active tab is hidden, we don't want to check its URL yet; we'll wait until
// the onShown event fires.
if (mCurrentTab != null && !tab.isHidden()) {
updateUsingCanonicalUrl(tab);
updateUrl(tab.getUrl());
}
}
......@@ -274,18 +245,4 @@ public class PageViewObserver {
String host = Uri.parse(url).getHost();
return host == null ? "" : host;
}
private boolean isLikelySubframeAmpNavigation(NavigationHandle navigation) {
if (navigation.isInMainFrame()) return false;
String subframeUrl = navigation.getUrl();
if (subframeUrl == null || subframeUrl.length() == 0) return false;
// Our heuristic for AMP pages, based on AMPPageLoadMetricsObserver, is to look for a
// subframe navigation that contains "amp_js_v" in the query. This might produce a false
// positive, but we're OK with that; we'll just call updateUrl an extra time. This is a
// no-op if the URL hasn't actually changed.
String query = Uri.parse(subframeUrl).getQuery();
if (query == null) return false;
return query.contains(AMP_QUERY_PARAM);
}
}
chrome/android/javatests/src/org/chromium/chrome/browser/usage_stats/TabSuspensionTest.java
View file @ 0e604b32
......@@ -42,15 +42,12 @@ import org.chromium.chrome.test.util.MenuUtils;
import org.chromium.components.safe_browsing.SafeBrowsingApiBridge;
import org.chromium.content_public.browser.LoadUrlParams;
import org.chromium.content_public.browser.test.util.CriteriaHelper;
import org.chromium.content_public.browser.test.util.JavaScriptUtils;
import org.chromium.content_public.browser.test.util.TestThreadUtils;
import org.chromium.content_public.common.ContentSwitches;
import org.chromium.net.test.EmbeddedTestServer;
import org.chromium.ui.base.PageTransition;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
/**
* Integration tests for {@link PageViewObserver} and {@link SuspendedTab}
......@@ -63,9 +60,6 @@ import java.util.concurrent.TimeoutException;
public class TabSuspensionTest {
private static final String STARTING_FQDN = "example.com";
private static final String DIFFERENT_FQDN = "www.google.com";
private static final String MAINFRAME_TEST_PAGE =
"/chrome/test/data/android/usage_stats/amp_root.html";
private static final String SUBFRAME_TEST_PAGE = "/chrome/test/data/android/test.html";
@Rule
public ChromeTabbedActivityTestRule mActivityTestRule = new ChromeTabbedActivityTestRule();
......@@ -75,13 +69,12 @@ public class TabSuspensionTest {
@Mock
private UsageStatsBridge mUsageStatsBridge;
@Mock
private EventTracker mEventTracker;
@Mock
private SuspensionTracker mSuspensionTracker;
private ChromeTabbedActivity mActivity;
private PageViewObserver mPageViewObserver;
private TokenTracker mTokenTracker;
private EventTracker mEventTracker;
private Tab mTab;
private EmbeddedTestServer mTestServer;
private String mStartingUrl;
......@@ -90,10 +83,12 @@ public class TabSuspensionTest {
@Before
public void setUp() throws InterruptedException {
MockitoAnnotations.initMocks(this);
// TokenTracker holds a promise, and Promises can only be used on a single thread, so we
// have to initialize it on the thread where it will be used.
TestThreadUtils.runOnUiThreadBlocking(
() -> { mTokenTracker = new TokenTracker(mUsageStatsBridge); });
// TokenTracker and EventTracker hold a promise, and Promises can only be used on a single
// thread, so we have to initialize them on the thread where they will be used.
TestThreadUtils.runOnUiThreadBlocking(() -> {
mTokenTracker = new TokenTracker(mUsageStatsBridge);
mEventTracker = new EventTracker(mUsageStatsBridge);
});
mTestServer = EmbeddedTestServer.createAndStartServer(InstrumentationRegistry.getContext());
mStartingUrl = mTestServer.getURLWithHostName(STARTING_FQDN, "/defaultresponse");
mDifferentUrl = mTestServer.getURLWithHostName(DIFFERENT_FQDN, "/defaultresponse");
......@@ -286,38 +281,6 @@ public class TabSuspensionTest {
});
}
@Test
@MediumTest
public void testAmpPage() throws InterruptedException, TimeoutException {
String mainframeUrl = mTestServer.getURLWithHostName(DIFFERENT_FQDN, MAINFRAME_TEST_PAGE);
mActivityTestRule.loadUrl(mainframeUrl);
doReturn(true).when(mSuspensionTracker).isWebsiteSuspended(STARTING_FQDN);
String iframeSrc =
mTestServer.getURLWithHostName(STARTING_FQDN, SUBFRAME_TEST_PAGE + "?amp_js_v=0");
String code = "document.getElementById('iframe_test_id').src='" + iframeSrc + "';";
JavaScriptUtils.executeJavaScriptAndWaitForResult(
mTab.getWebContents(), code, 3, TimeUnit.SECONDS);
waitForSuspendedTabToShow(mTab, STARTING_FQDN);
doReturn(false).when(mSuspensionTracker).isWebsiteSuspended(STARTING_FQDN);
unsuspendDomain(STARTING_FQDN);
assertSuspendedTabHidden(mTab);
// Un-suspension reloads the page, so we need to wait for it to load and re-navigate the
// iframe back to the suspended domain.
ChromeTabUtils.waitForTabPageLoaded(mTab, mainframeUrl);
JavaScriptUtils.executeJavaScriptAndWaitForResult(
mTab.getWebContents(), code, 3, TimeUnit.SECONDS);
doReturn(true).when(mSuspensionTracker).isWebsiteSuspended(STARTING_FQDN);
suspendDomain(STARTING_FQDN);
waitForSuspendedTabToShow(mTab, STARTING_FQDN);
mActivityTestRule.loadUrl(mDifferentUrl);
assertSuspendedTabHidden(mTab);
}
private void startLoadingUrl(Tab tab, String url) {
TestThreadUtils.runOnUiThreadBlocking(
() -> { tab.loadUrl(new LoadUrlParams(url, PageTransition.TYPED)); });
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment