Skip to content

Commit

Permalink
Reland "[mparch] Update ContentFaviconDriver"
Browse files Browse the repository at this point in the history 
This is a reland of 37e51f1

Changes in this reland
 - We now ignore manifest URL updates in ContentFaviconDriver if they
   arrive prior to the onload handler firing to match current behavior.
   See crbug.com/1205018 for details.
 - Added tests for the above.
 - I have removed the unused WCI::favicon_urls_

Original change's description:
> [mparch] Update ContentFaviconDriver
>
> With this change, we only process favicon updates upon activation
> (i.e., in the primary main frame). To make this work I've done the
> following:
>  - moved per-document state to RFHI rather than the WCI or the content
>    favicon driver to avoid clobbering while prerendering
>  - ensured that this state is cleared when necessary (thanks to lfg@
>    for advice on when to do this)
>  - calling UpdateFaviconURLs after activation making use of the state
>    stored on the RFHI.
>  - Exposed two new functions on NavigationHandle:
>    - IsInPrimaryMainFrame, and
>    - IsPrerenderedPageActivation.
>
> I'd hoped that taking the approach above would replicate the order of
> events that other favicon-related code would see during a normal
> activation, but there may still be some work required if they're
> listening to navigation events.
>
> Testing this required exposing some prerender testing machinery to
> chrome/browser. As suggested by lfg@, I've bundled the prerendering
> test helper machinery into an object that can be aggregated by test
> classes (i.e., no inheritance required).
>
> I also needed to switch some of the WCI unittests over to RFHI tests
> since I'm now storing state at that level.
>
> Bug: 1190827
> Change-Id: I3f2bbbaf19769f216119d048f60e73feab3c35d4
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2787388
> Commit-Queue: Ian Vollick <vollick@chromium.org>
> Reviewed-by: Alexander Timin <altimin@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
> Reviewed-by: Matt Falkenhagen <falken@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: Kevin McNee <mcnee@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#875938}

Bug: 1190827
Change-Id: I90b26e2fd528b09eaa6c343f5ca1c963b86edf04
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2852141
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Ian Vollick <vollick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#879437}
  • Loading branch information
Ian Vollick authored and Chromium LUCI CQ committed May 5, 2021
1 parent 68f2211 commit 6d75ac3
Show file tree
Hide file tree
Showing 28 changed files with 593 additions and 318 deletions.
176 changes: 174 additions & 2 deletions chrome/browser/favicon/content_favicon_driver_browsertest.cc
View file
Expand Up @@ -25,6 +25,7 @@
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_commands.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h"
#include "components/favicon/core/favicon_handler.h"
Expand All @@ -36,10 +37,13 @@
#include "content/public/test/browser_test.h"
#include "content/public/test/browser_test_utils.h"
#include "content/public/test/browsing_data_remover_test_util.h"
#include "content/public/test/prerender_test_util.h"
#include "content/public/test/url_loader_interceptor.h"
#include "net/base/load_flags.h"
#include "net/dns/mock_host_resolver.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "net/test/embedded_test_server/http_request.h"
#include "net/test/embedded_test_server/http_response.h"
#include "net/url_request/url_request.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "third_party/blink/public/common/features.h"
Expand All @@ -51,6 +55,18 @@ namespace {

using testing::ElementsAre;

std::unique_ptr<net::test_server::HttpResponse> NoContentResponseHandler(
const std::string& path,
const net::test_server::HttpRequest& request) {
if (path != request.relative_url)
return nullptr;

std::unique_ptr<net::test_server::BasicHttpResponse> http_response(
new net::test_server::BasicHttpResponse);
http_response->set_code(net::HTTP_NO_CONTENT);
return std::move(http_response);
}

// Tracks which URLs are loaded and whether the requests bypass the cache.
class TestURLLoaderInterceptor {
public:
Expand Down Expand Up @@ -214,11 +230,15 @@ class PageLoadStopper : public content::WebContentsObserver {

class ContentFaviconDriverTest : public InProcessBrowserTest {
public:
ContentFaviconDriverTest() {}
~ContentFaviconDriverTest() override {}
ContentFaviconDriverTest()
: prerender_helper_(
base::BindRepeating(&ContentFaviconDriverTest::web_contents,
base::Unretained(this))) {}
~ContentFaviconDriverTest() override = default;

void SetUpOnMainThread() override {
host_resolver()->AddRule("*", "127.0.0.1");
prerender_helper_.SetUpOnMainThread(embedded_test_server());
}

void SetUpCommandLine(base::CommandLine* command_line) override {
Expand Down Expand Up @@ -267,10 +287,162 @@ class ContentFaviconDriverTest : public InProcessBrowserTest {
return GetFaviconForPageURL(url, icon_type, /*desired_size_in_dip=*/0);
}

content::test::PrerenderTestHelper& prerender_helper() {
return prerender_helper_;
}

private:
content::test::PrerenderTestHelper prerender_helper_;

DISALLOW_COPY_AND_ASSIGN(ContentFaviconDriverTest);
};

IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest,
DoNotLoadFaviconsWhilePrerendering) {
ASSERT_TRUE(embedded_test_server()->Start());
GURL prerender_url =
embedded_test_server()->GetURL("/favicon/page_with_favicon.html");
GURL icon_url = embedded_test_server()->GetURL("/favicon/icon.png");
GURL initial_url =
embedded_test_server()->GetURL("/prerender/add_prerender.html");
prerender_helper().NavigatePrimaryPage(initial_url);

{
PendingTaskWaiter waiter(web_contents());
prerender_helper().AddPrerender(prerender_url);
waiter.Wait();
}

// We should not fetch the URL while prerendering.
prerender_helper().WaitForRequest(prerender_url, 1);
EXPECT_EQ(prerender_helper().GetRequestCount(icon_url), 0);
prerender_helper().NavigatePrimaryPage(prerender_url);

// Check that we've fetched the URL upon activation. Should not hang.
EXPECT_EQ(prerender_helper().GetRequestCount(prerender_url), 1);
prerender_helper().WaitForRequest(icon_url, 1);
}

class NoCommittedEntryWebContentsObserver
: public content::WebContentsObserver {
public:
explicit NoCommittedEntryWebContentsObserver(
content::WebContents* web_contents) {
Observe(web_contents);
}

~NoCommittedEntryWebContentsObserver() override = default;

bool DidUpdateFaviconURLWithNoCommittedEntry() const {
return did_update_favicon_url_with_no_committed_entry_;
}

protected:
// WebContentsObserver:
void DidUpdateFaviconURL(
content::RenderFrameHost* rfh,
const std::vector<blink::mojom::FaviconURLPtr>& candidates) override {
auto* web_contents = content::WebContents::FromRenderFrameHost(rfh);
if (!web_contents->GetController().GetLastCommittedEntry()) {
did_update_favicon_url_with_no_committed_entry_ = true;
}
}

private:
bool did_update_favicon_url_with_no_committed_entry_ = false;
};

// Observes the creation of new tabs and, upon creation, sets up both a pending
// task waiter (to ensure that ContentFaviconDriver tasks complete) and a
// NoCommittedEntryWebContentsObserver (to ensure that we observe the expected
// function calls).
class FaviconUpdateNoLastCommittedEntryTabStripObserver
: public TabStripModelObserver {
public:
explicit FaviconUpdateNoLastCommittedEntryTabStripObserver(
TabStripModel* model)
: model_(model) {
model_->AddObserver(this);
}
~FaviconUpdateNoLastCommittedEntryTabStripObserver() override {
model_->RemoveObserver(this);
}

void WaitForNewTab() {
if (!pending_task_waiter_)
run_loop_.Run();
}

bool DidUpdateFaviconURLWithNoCommittedEntry() const {
return observer_->DidUpdateFaviconURLWithNoCommittedEntry();
}

PendingTaskWaiter* pending_task_waiter() {
return pending_task_waiter_.get();
}

protected:
// TabStripModelObserver:
void OnTabStripModelChanged(
TabStripModel* tab_strip_model,
const TabStripModelChange& change,
const TabStripSelectionChange& selection) override {
if (change.type() != TabStripModelChange::kInserted)
return;
auto* web_contents = model_->GetActiveWebContents();
pending_task_waiter_ = std::make_unique<PendingTaskWaiter>(web_contents);
observer_ =
std::make_unique<NoCommittedEntryWebContentsObserver>(web_contents);
run_loop_.Quit();
}

private:
base::RunLoop run_loop_;
TabStripModel* model_ = nullptr;
std::unique_ptr<PendingTaskWaiter> pending_task_waiter_;
std::unique_ptr<NoCommittedEntryWebContentsObserver> observer_;
};

// Tests that ContentFaviconDriver can handle being sent updated favicon URLs
// if there is no last committed entry. This occurs when script is injected in
// about:blank in a newly created window. See crbug.com/520759 for more
// details.
IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest,
FaviconUpdateNoLastCommittedEntry) {
const char kNoContentPath[] = "/nocontent";
embedded_test_server()->RegisterRequestHandler(
base::BindRepeating(&NoContentResponseHandler, kNoContentPath));

ASSERT_TRUE(embedded_test_server()->Start());
GURL empty_url = embedded_test_server()->GetURL("/empty.html");
GURL no_content_url = embedded_test_server()->GetURL("/nocontent");

FaviconUpdateNoLastCommittedEntryTabStripObserver observer(
browser()->tab_strip_model());

auto* rfh = ui_test_utils::NavigateToURLWithDisposition(
browser(), empty_url, WindowOpenDisposition::CURRENT_TAB,
ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);

EXPECT_TRUE(content::ExecJs(rfh, content::JsReplace(R"(
let w = window.open();
w.document.write('abc');
w.document.close();
w.location.href = $1;)",
no_content_url)));

// Ensure that we have created our tab and set up the pending task waiter and
// web contents observer.
observer.WaitForNewTab();

// Wait for WebContentsObsever::DidUpdateFaviconURL() call and for any
// subsequent ContentFaviconDriver tasks to finish.
observer.pending_task_waiter()->Wait();

// We expect DidUpdateFaviconURL to be called and for no crash to ensue.
EXPECT_TRUE(observer.DidUpdateFaviconURLWithNoCommittedEntry());
}

// Test that when a user reloads a page ignoring the cache that the favicon is
// is redownloaded and (not returned from either the favicon cache or the HTTP
// cache).
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/ssl/security_state_tab_helper_browsertest.cc
View file
Expand Up @@ -2224,15 +2224,15 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperPrerenderTest, InvalidPrerender) {
// Try to prerender the page with an invalid certificate.
auto prerender_url = test_server->GetURL("/title1.html");
content::test::PrerenderHostRegistryObserver registry_observer(
web_contents());
*web_contents());
content::NavigationHandleObserver nav_observer(web_contents(), prerender_url);
prerender_helper_.AddPrerenderAsync(prerender_url);

// Ensure that the prerender has started.
registry_observer.WaitForTrigger(prerender_url);
auto prerender_id = prerender_helper_.GetHostForUrl(prerender_url);
EXPECT_NE(content::RenderFrameHost::kNoFrameTreeNodeId, prerender_id);
content::test::PrerenderHostObserver host_observer(web_contents(),
content::test::PrerenderHostObserver host_observer(*web_contents(),
prerender_id);

// Since the prerender has not yet activated, the state should still be
Expand Down Expand Up @@ -2285,7 +2285,7 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperPrerenderTest,
prerender_helper_.AddPrerender(prerender_url);
auto prerender_id = prerender_helper_.GetHostForUrl(prerender_url);
EXPECT_NE(content::RenderFrameHost::kNoFrameTreeNodeId, prerender_id);
content::test::PrerenderHostObserver host_observer(web_contents(),
content::test::PrerenderHostObserver host_observer(*web_contents(),
prerender_id);

// Since the prerender has not yet activated, the state should still be
Expand Down

0 comments on commit 6d75ac3

Please sign in to comment.