Skip to content

Commit

Permalink
Don't re-run content scripts on fragment navigations.
Browse files Browse the repository at this point in the history
This regression was inadvertently introduced by a fix to make executeScript 
run after a fragment navigation (bug 29541). That patch was:

http://codereview.chromium.org/566041

The problem is that on frame navigations (didChangeLocationWithinPage), we end
up creating a new UserScriptIdleScheduler, so this patch keeps track of the 
previous has_run state and propagates that to the new UserScriptIdleScheduler.


BUG=35924
TEST=Steps to verify are outlined in bug report


Review URL: http://codereview.chromium.org/646017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@39939 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
asargent@chromium.org committed Feb 24, 2010
1 parent a6e82fc commit f6c2459
Show file tree
Hide file tree
Showing 12 changed files with 216 additions and 12 deletions.
19 changes: 19 additions & 0 deletions chrome/browser/extensions/fragment_navigation_apitest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (c) 2010 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "chrome/browser/extensions/extension_apitest.h"


IN_PROC_BROWSER_TEST_F(ExtensionApiTest, ContentScriptFragmentNavigation) {
StartHTTPServer();
const char* extension_name = "content_scripts/fragment";
ASSERT_TRUE(RunExtensionTest(extension_name)) << message_;
}

IN_PROC_BROWSER_TEST_F(ExtensionApiTest, ExecuteScriptFragmentNavigation) {
StartHTTPServer();
const char* extension_name = "executescript/fragment";
ASSERT_TRUE(RunExtensionTest(extension_name)) << message_;
}

1 change: 1 addition & 0 deletions chrome/chrome_tests.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -1161,6 +1161,7 @@
'browser/extensions/extension_tabs_apitest.cc',
'browser/extensions/extension_toolbar_model_unittest.cc',
'browser/extensions/extension_toolstrip_apitest.cc',
'browser/extensions/fragment_navigation_apitest.cc',
'browser/extensions/incognito_noscript_apitest.cc',
'browser/extensions/isolated_world_apitest.cc',
'browser/extensions/page_action_apitest.cc',
Expand Down
18 changes: 14 additions & 4 deletions chrome/renderer/render_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2681,6 +2681,13 @@ void RenderView::didFinishLoad(WebFrame* frame) {

void RenderView::didChangeLocationWithinPage(
WebFrame* frame, bool is_new_navigation) {

// Determine if the UserScriptIdleScheduler already ran scripts on this page,
// since a new one gets created by didCreateDataSource.
NavigationState* state =
NavigationState::FromDataSource(frame->dataSource());
bool idle_scheduler_ran = state->user_script_idle_scheduler()->has_run();

// If this was a reference fragment navigation that we initiated, then we
// could end up having a non-null pending navigation state. We just need to
// update the ExtraData on the datasource so that others who read the
Expand All @@ -2690,13 +2697,16 @@ void RenderView::didChangeLocationWithinPage(
// DidCreateDataSource conveniently takes care of this for us.
didCreateDataSource(frame, frame->dataSource());

if (idle_scheduler_ran) {
// Update the new UserScriptIdleScheduler so we don't re-run scripts.
NavigationState* new_state =
NavigationState::FromDataSource(frame->dataSource());
new_state->user_script_idle_scheduler()->set_has_run(true);
}

didCommitProvisionalLoad(frame, is_new_navigation);

UpdateTitle(frame, frame->view()->mainFrame()->dataSource()->pageTitle());

NavigationState* navigation_state = NavigationState::FromDataSource(
frame->dataSource());
navigation_state->user_script_idle_scheduler()->DidChangeLocationWithinPage();
}

void RenderView::didUpdateCurrentHistoryItem(WebFrame* frame) {
Expand Down
4 changes: 0 additions & 4 deletions chrome/renderer/user_script_idle_scheduler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ void UserScriptIdleScheduler::DidFinishLoad() {
method_factory_.NewRunnableMethod(&UserScriptIdleScheduler::MaybeRun));
}

void UserScriptIdleScheduler::DidChangeLocationWithinPage() {
DidFinishLoad();
}

void UserScriptIdleScheduler::Cancel() {
view_ = NULL;
frame_ = NULL;
Expand Down
7 changes: 3 additions & 4 deletions chrome/renderer/user_script_idle_scheduler.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,21 @@ class WebFrame;
//
// The intent of this mechanism is to prevent user scripts from slowing down
// fast pages (run after load), while still allowing them to run relatively
// timelily for pages with lots of slow subresources.
// timely for pages with lots of slow subresources.
class UserScriptIdleScheduler {
public:
UserScriptIdleScheduler(RenderView* view, WebKit::WebFrame* frame);

bool has_run() { return has_run_; }

void set_has_run(bool has_run) { has_run_ = has_run; }

// Called when the DOM has been completely constructed.
void DidFinishDocumentLoad();

// Called when the document has completed loading.
void DidFinishLoad();

// Called when the document has navigated to a fragment.
void DidChangeLocationWithinPage();

// Called when the client has gone away and we should no longer run scripts.
void Cancel();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<script>
var failed = false;

function did_fail() {
return failed;
}

function set_failed(val) {
failed = val;
}

function fail() {
set_failed(true);
if (!did_fail()) {
chrome.test.fail();
}
}

var test_url = "http://localhost:1337/files/extensions/test_file.html";

// For running in normal chrome (ie outside of the browser_tests environment),
// set debug to 1 here.
var debug = 0;
if (debug) {
test_url = "http://www.google.com";
chrome.test.log = function(msg) { console.log(msg) };
chrome.test.runTests = function(tests) {
for (var i in tests) {
tests[i]();
}
};
chrome.test.succeed = function(){ console.log("succeed"); };
chrome.test.fail = function(){ console.log("fail"); };
}

chrome.test.runTests([
function test1() {
chrome.extension.onRequest.addListener(function(req, sender) {
chrome.test.log("got request: " + JSON.stringify(req));
if (req == "fail") {
fail();
} else if (req == "content_script_start") {
var tab = sender.tab;
if (tab.url.indexOf("#") != -1) {
fail();
} else {
chrome.tabs.update(tab.id, {"url": tab.url + "#foo"});
}
}
});
chrome.tabs.onUpdated.addListener(function(tabid, info, tab) {
chrome.test.log("onUpdated status: " + info.status + " url:" + tab.url);
if (info.status == "complete" && tab.url.indexOf("#foo") != -1) {
setTimeout(function() {
if (!did_fail()) {
chrome.test.succeed();
}
}, 750);
}
});
chrome.test.log("creating tab");
chrome.tabs.create({"url": test_url});
}
]);


</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) 2010 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// Tests that we don't re-inject scripts after fragment navigation.

// The background page should only see this once - it will then use tab.update
// to navigate this page to #foo.
chrome.extension.sendRequest("content_script_start");

if (location.href.indexOf("#foo") != -1) {
// This means the content script ran again.
chrome.extension.sendRequest("fail");
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "content_scripts_fragment_navigation",
"version": "1.0",
"description": "Tests content scripts running with navigation to fragments within a page",
"background_page": "background.html",
"permissions": ["tabs"],
"content_scripts": [
{ "matches": ["http://*/*"], "js": ["content_script.js"] }
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<script>
var got_request = false;

var test_url = "http://localhost:1337/files/extensions/test_file.html";

// For running in normal chrome (ie outside of the browser_tests environment),
// set debug to 1 here.
var debug = 0;
if (debug) {
test_url = "http://www.google.com";
chrome.test.log = function(msg) { console.log(msg) };
chrome.test.runTests = function(tests) {
for (var i in tests) {
tests[i]();
}
};
chrome.test.succeed = function(){ console.log("succeed"); };
chrome.test.fail = function(){ console.log("fail"); };
}

function navigate_to_fragment(tab, callback) {
var new_url = test_url + "#foo";
chrome.test.log("navigating tab to " + new_url);
chrome.tabs.update(tab.id, {"url": new_url}, callback);
}

var succeeded = false;

function do_execute(tab) {
chrome.tabs.executeScript(tab.id, {"file": "execute_script.js"});
setTimeout(function() {
if (!succeeded) {
chrome.test.fail("timed out");
}
}, 10000);
}


chrome.test.runTests([
// When the tab is created, a content script will send a request letting
// know the onload has fired. Then we navigate to a fragment, and try
// running chrome.tabs.executeScript.
function test1() {
chrome.extension.onRequest.addListener(function(req, sender) {
chrome.test.log("got request: " + JSON.stringify(req));
if (req == "content_script") {
navigate_to_fragment(sender.tab, do_execute);
} else if (req == "execute_script") {
suceeded = true;
chrome.test.succeed();
}
});
chrome.test.log("creating tab");
chrome.tabs.create({"url": test_url});
}
]);


</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) 2010 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

function notify() {
chrome.extension.sendRequest("content_script");
}

if (document.readyState) {
notify();
} else {
document.onload = notify;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Copyright (c) 2010 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// Notify the background page that we ran.
chrome.extension.sendRequest("execute_script");
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "execute_script_fragment_navigation",
"version": "1.0",
"description": "Tests execute script running after navigation to a fragments within a page",
"background_page": "background.html",
"permissions": ["tabs", "http://*/*"],
"content_scripts": [
{"matches": ["http://*/*"], "js": ["content_script.js"]}
]
}

0 comments on commit f6c2459

Please sign in to comment.