chrome/browser/extensions/api/declarative/declarative_apitest.cc - Issue 52743002: Declarative rules should be removed on uninstalling, not unloading

Side by Side Diff: chrome/browser/extensions/api/declarative/declarative_apitest.cc

Issue 52743002: Declarative rules should be removed on uninstalling, not unloading (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Avoid shooting oneself in a foot + browser test cleaned & pruned Created 7 years, 1 month ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

OLD	NEW
1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.	1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "base/bind.h"	5 #include "base/bind.h"

6 #include "base/bind_helpers.h"	6 #include "base/bind_helpers.h"

7 #include "base/command_line.h"	7 #include "base/command_line.h"

8 #include "base/memory/scoped_ptr.h"	8 #include "base/memory/scoped_ptr.h"

	9 #include "base/strings/stringprintf.h"

9 #include "base/strings/utf_string_conversions.h"	10 #include "base/strings/utf_string_conversions.h"

	11 #include "base/test/thread_test_helper.h"

10 #include "chrome/browser/extensions/api/declarative/rules_registry_service.h"	12 #include "chrome/browser/extensions/api/declarative/rules_registry_service.h"

11 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_consta nts.h"	13 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_consta nts.h"

12 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_ registry.h"	14 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_ registry.h"

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

14 #include "chrome/browser/extensions/extension_system_factory.h"	16 #include "chrome/browser/extensions/extension_system_factory.h"

	17 #include "chrome/browser/extensions/extension_test_message_listener.h"

	18 #include "chrome/browser/extensions/test_extension_dir.h"

15 #include "chrome/browser/profiles/profile.h"	19 #include "chrome/browser/profiles/profile.h"

16 #include "chrome/browser/ui/browser.h"	20 #include "chrome/browser/ui/browser.h"

17 #include "chrome/browser/ui/tabs/tab_strip_model.h"	21 #include "chrome/browser/ui/tabs/tab_strip_model.h"

18 #include "chrome/test/base/ui_test_utils.h"	22 #include "chrome/test/base/ui_test_utils.h"

19 #include "content/public/browser/browser_thread.h"	23 #include "content/public/browser/browser_thread.h"

20 #include "extensions/common/extension.h"	24 #include "extensions/common/extension.h"

21	25

	26 using content::BrowserThread;

	27 using extensions::Extension;

22 using extensions::RulesRegistry;	28 using extensions::RulesRegistry;

23 using extensions::RulesRegistryService;	29 using extensions::RulesRegistryService;

	30 using extensions::TestExtensionDir;

24 using extensions::WebRequestRulesRegistry;	31 using extensions::WebRequestRulesRegistry;

25	32

26 namespace {	33 namespace {

27	34

28 const char kArbitraryUrl[] = "http://www.example.com";	35 const char kArbitraryUrl[] = "http://www.example.com"; // Must be http://.

29	36

30 // The extension in "declarative/redirect_to_data" redirects every navigation to	37 // The extension in "declarative/redirect_to_data" redirects every navigation to

31 // a page with title \|kTestTitle\|.	38 // a page with title \|kTestTitle\|.

32 const char kTestTitle[] = ":TEST:";	39 #define TEST_TITLE_STRING ":TEST:"

	40 const char kTestTitle[] = TEST_TITLE_STRING;

	41

	42 // All methods and constands below containing "RedirectToData" in their names

	43 // are parts of a test extension "Redirect to 'data:'".

	44 std::string GetRedirectToDataManifestWithVersion(unsigned version) {

	45 return base::StringPrintf(

	46 "{\n"

	47 " \"name\": \"Redirect to 'data:' (declarative apitest)\",\n"

	48 " \"version\": \"%d\",\n"

	49 " \"manifest_version\": 2,\n"

	50 " \"description\": \"Redirects all requests to a fixed data: URI.\",\n"

	51 " \"background\": {\n"

	52 " \"scripts\": [\"background.js\"]\n"

	53 " },\n"

	54 " \"permissions\": [\n"

	55 " \"declarativeWebRequest\", \"<all_urls>\"\n"

	56 " ]\n"

	57 "}\n",

	58 version);

	59 }

	60

	61 const char kRedirectToDataConstants[] =

	62 "var redirectDataURI =\n"

	63 " 'data:text/html;charset=utf-8,<html><head><title>' +\n"

	64 " '" TEST_TITLE_STRING "' +\n"

	65 " '<%2Ftitle><%2Fhtml>';\n";

	66 #undef TEST_TITLE_STRING

	67

	68 const char kRedirectToDataRules[] =

	69 "var rules = [{\n"

	70 " conditions: [\n"

	71 " new chrome.declarativeWebRequest.RequestMatcher({\n"

	72 " url: {schemes: ['http']}})\n"

	73 " ],\n"

	74 " actions: [\n"

	75 " new chrome.declarativeWebRequest.RedirectRequest({\n"

	76 " redirectUrl: redirectDataURI\n"

	77 " })\n"

	78 " ]\n"

	79 "}];\n";

	80

	81 const char kRedirectToDataInstallRules[] =

	82 "function report(details) {\n"

	83 " if (chrome.extension.lastError) {\n"

	84 " chrome.test.log(chrome.extension.lastError.message);\n"

	85 " } else {\n"

	86 " chrome.test.sendMessage(\"ready\", function(reply) {})\n"

	87 " }\n"

	88 "}\n"

	89 "\n"

	90 "chrome.runtime.onInstalled.addListener(function(details) {\n"

	91 " if (details.reason == 'install')\n"

	92 " chrome.declarativeWebRequest.onRequest.addRules(rules, report);\n"

	93 "});\n";

	94

	95 const char kRedirectToDataNoRules[] =

	96 "chrome.runtime.onInstalled.addListener(function(details) {\n"

	97 " chrome.test.sendMessage(\"ready\", function(reply) {})\n"

	98 "});\n";

33	99

34 } // namespace	100 } // namespace

35	101

36 class DeclarativeApiTest : public ExtensionApiTest {	102 class DeclarativeApiTest : public ExtensionApiTest {

37 public:	103 public:

38 std::string GetTitle() {	104 std::string GetTitle() {

39 string16 title(	105 string16 title(

40 browser()->tab_strip_model()->GetActiveWebContents()->GetTitle());	106 browser()->tab_strip_model()->GetActiveWebContents()->GetTitle());

41 return base::UTF16ToUTF8(title);	107 return base::UTF16ToUTF8(title);

42 }	108 }

	109

	110 // Reports the number of rules registered for the \|extension_id\| with the

	111 // non-webview rules registry.

	112 size_t NumberOfRegisteredRules(const std::string& extension_id) {

	113 RulesRegistryService* rules_registry_service =

	114 extensions::RulesRegistryService::Get(browser()->profile());

	115 scoped_refptr<RulesRegistry> rules_registry =

	116 rules_registry_service->GetRulesRegistry(

	117 RulesRegistry::WebViewKey(0, 0),

	118 extensions::declarative_webrequest_constants::kOnRequest);

	119 std::vector<linked_ptr<RulesRegistry::Rule> > rules;

	120 BrowserThread::PostTask(

	121 BrowserThread::IO,

	122 FROM_HERE,

	123 base::Bind(

	124 &RulesRegistry::GetAllRules, rules_registry, extension_id, &rules));

	125 scoped_refptr<base::ThreadTestHelper> io_helper(new base::ThreadTestHelper(

	126 BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO).get()));

	127 EXPECT_TRUE(io_helper->Run());

	128 return rules.size();

	129 }

43 };	130 };

44	131

45 IN_PROC_BROWSER_TEST_F(DeclarativeApiTest, DeclarativeApi) {	132 IN_PROC_BROWSER_TEST_F(DeclarativeApiTest, DeclarativeApi) {

46 ASSERT_TRUE(RunExtensionTest("declarative/api")) << message_;	133 ASSERT_TRUE(RunExtensionTest("declarative/api")) << message_;

47	134

48 // Check that unloading the page has removed all rules.	135 // Check that uninstalling the extension has removed all rules.

49 std::string extension_id = GetSingleLoadedExtension()->id();	136 std::string extension_id = GetSingleLoadedExtension()->id();

50 UnloadExtension(extension_id);	137 UninstallExtension(extension_id);

51	138

52 // UnloadExtension posts a task to the owner thread of the extension	139 // UnloadExtension posts a task to the owner thread of the extension

53 // to process this unloading. The next task to retrive all rules	140 // to process this unloading. The next task to retrive all rules

54 // is therefore processed after the UnloadExtension task has been executed.	141 // is therefore processed after the UnloadExtension task has been executed.

55	142 EXPECT_EQ(0u, NumberOfRegisteredRules(extension_id));

56 RulesRegistryService* rules_registry_service =

57 extensions::RulesRegistryService::Get(browser()->profile());

58 scoped_refptr<RulesRegistry> rules_registry =

59 rules_registry_service->GetRulesRegistry(

60 RulesRegistry::WebViewKey(0, 0),

61 extensions::declarative_webrequest_constants::kOnRequest);

62

63 std::vector<linked_ptr<RulesRegistry::Rule> > known_rules;

64

65 content::BrowserThread::PostTask(

66 rules_registry->owner_thread(),

67 FROM_HERE,

68 base::Bind(base::IgnoreResult(&RulesRegistry::GetAllRules),

69 rules_registry, extension_id, &known_rules));

70

71 content::RunAllPendingInMessageLoop(rules_registry->owner_thread());

72

73 EXPECT_TRUE(known_rules.empty());

74 }	143 }

75	144

76 // PersistRules test first installs an extension, which registers some rules.	145 // PersistRules test first installs an extension, which registers some rules.

77 // Then after browser restart, it checks that the rules are still in effect.	146 // Then after browser restart, it checks that the rules are still in effect.

78 IN_PROC_BROWSER_TEST_F(DeclarativeApiTest, PRE_PersistRules) {	147 IN_PROC_BROWSER_TEST_F(DeclarativeApiTest, PRE_PersistRules) {

	148 // Note that we cannot use an extension generated by GetRedirectToData

	149 // helpers in a TestExtensionDir, because we need the extension to persist

	150 // until the PersistRules test is run.

79 ASSERT_TRUE(RunExtensionTest("declarative/redirect_to_data")) << message_;	151 ASSERT_TRUE(RunExtensionTest("declarative/redirect_to_data")) << message_;

80 }	152 }

81	153

82 IN_PROC_BROWSER_TEST_F(DeclarativeApiTest, PersistRules) {	154 IN_PROC_BROWSER_TEST_F(DeclarativeApiTest, PersistRules) {

83 ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));	155 ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));

84 EXPECT_EQ(kTestTitle, GetTitle());	156 EXPECT_EQ(kTestTitle, GetTitle());

85 }	157 }

	158

	159 // Test that the rules are correctly persisted and (de)activated during

	160 // changing the "installed" and "enabled" status of an extension.

	161 IN_PROC_BROWSER_TEST_F(DeclarativeApiTest, ExtensionLifetimeRulesHandling) {

	162 TestExtensionDir ext_dir;

	163

	164 // 1. Install the extension. Rules should become active.

	165 ext_dir.WriteManifest(GetRedirectToDataManifestWithVersion(1));

	166 ext_dir.WriteFile(FILE_PATH_LITERAL("background.js"),

	167 base::StringPrintf("%s%s%s",

	168 kRedirectToDataConstants,

	169 kRedirectToDataRules,

	170 kRedirectToDataInstallRules));

	171 ExtensionTestMessageListener ready("ready", /will_reply=/true);
	Jeffrey Yasskin 2013/11/22 18:42:44 nit: I think will_reply should actually be false h nit: I think will_reply should actually be false here since you never call Reply(). vabr (Chromium) 2013/11/22 21:16:44 Done. Show quoted text On 2013/11/22 18:42:44, Jeffrey Yasskin wrote: > nit: I think will_reply should actually be false here since you never call > Reply(). Done.
	172 const Extension* extension = InstallExtensionWithUIAutoConfirm(

	173 ext_dir.Pack(), 1 /+1 installed extension/, browser());

	174 ASSERT_TRUE(extension);

	175 std::string extension_id(extension->id());

	176 ASSERT_TRUE(ready.WaitUntilSatisfied());

	177 ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));

	178 EXPECT_EQ(kTestTitle, GetTitle());

	179 EXPECT_EQ(1u, NumberOfRegisteredRules(extension_id));

	180

	181 // 2. Disable the extension. Rules no more active, but are still registered.
	Jeffrey Yasskin 2013/11/22 18:42:44 grammar nit: "Rules are no longer active" grammar nit: "Rules are no longer active" vabr (Chromium) 2013/11/22 21:16:44 Done. Show quoted text On 2013/11/22 18:42:44, Jeffrey Yasskin wrote: > grammar nit: "Rules are no longer active" Done.
	182 DisableExtension(extension_id);

	183 ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));

	184 EXPECT_NE(kTestTitle, GetTitle());

	185 EXPECT_EQ(1u, NumberOfRegisteredRules(extension_id));

	186

	187 // 3. Enable the extension again. Rules are active again.

	188 EnableExtension(extension_id);

	189 ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));

	190 EXPECT_EQ(kTestTitle, GetTitle());

	191 EXPECT_EQ(1u, NumberOfRegisteredRules(extension_id));

	192

	193 // 4. Reload the extension. Rules are still active.
	Jeffrey Yasskin 2013/11/22 18:42:44 I think this comment isn't quite describing what a I think this comment isn't quite describing what actually happens. Looking at your previous section 7, the message that was sent was "ready", not "ready_noop", indicating that the reload re-added the rules. If the reload actually doesn't remove the rules, but still runs the onInstalled(reason="install"), then do this after the "without the code" (i.e. after step 5) to be clear that reload actually doesn't drop the rules. vabr (Chromium) 2013/11/22 21:16:44 The previous patch was still broken, and part 7 wa Show quoted text On 2013/11/22 18:42:44, Jeffrey Yasskin wrote: > I think this comment isn't quite describing what actually happens. Looking at > your previous section 7, the message that was sent was "ready", not > "ready_noop", indicating that the reload re-added the rules. If the reload > actually doesn't remove the rules, but still runs the > onInstalled(reason="install"), then do this after the "without the code" (i.e. > after step 5) to be clear that reload actually doesn't drop the rules. The previous patch was still broken, and part 7 was one of the broken parts. Reload does not invoke onInstalled(reason="install"). I think it might invoke onInstalled(reason="update") when the source of the extension is upgraded in between. In this case it should not invoke onInstalled at all. On the other hand, rules are now not flushed on unload (and uninstalled is not called during reload). Does the updated comment make sense now? Jeffrey Yasskin 2013/11/22 21:39:52 Given that behavior, you should move the Reload se Show quoted text On 2013/11/22 21:16:44, vabr (Chromium) wrote: > On 2013/11/22 18:42:44, Jeffrey Yasskin wrote: > > I think this comment isn't quite describing what actually happens. Looking at > > your previous section 7, the message that was sent was "ready", not > > "ready_noop", indicating that the reload re-added the rules. If the reload > > actually doesn't remove the rules, but still runs the > > onInstalled(reason="install"), then do this after the "without the code" (i.e. > > after step 5) to be clear that reload actually doesn't drop the rules. > > The previous patch was still broken, and part 7 was one of the broken parts. > Reload does not invoke onInstalled(reason="install"). I think it might invoke > onInstalled(reason="update") when the source of the extension is upgraded in > between. In this case it should not invoke onInstalled at all. > > On the other hand, rules are now not flushed on unload (and uninstalled is not > called during reload). > > Does the updated comment make sense now? Given that behavior, you should move the Reload section after section 5, so that there's no way the onInstalled event could possible re-add the rules. then if the test passes, we've proven that reload doesn't flush the rules. vabr (Chromium) 2013/11/22 21:52:39 Done. Show quoted text On 2013/11/22 21:39:52, Jeffrey Yasskin wrote: > On 2013/11/22 21:16:44, vabr (Chromium) wrote: > > On 2013/11/22 18:42:44, Jeffrey Yasskin wrote: > > > I think this comment isn't quite describing what actually happens. Looking > at > > > your previous section 7, the message that was sent was "ready", not > > > "ready_noop", indicating that the reload re-added the rules. If the reload > > > actually doesn't remove the rules, but still runs the > > > onInstalled(reason="install"), then do this after the "without the code" > (i.e. > > > after step 5) to be clear that reload actually doesn't drop the rules. > > > > The previous patch was still broken, and part 7 was one of the broken parts. > > Reload does not invoke onInstalled(reason="install"). I think it might invoke > > onInstalled(reason="update") when the source of the extension is upgraded in > > between. In this case it should not invoke onInstalled at all. > > > > On the other hand, rules are now not flushed on unload (and uninstalled is not > > called during reload). > > > > Does the updated comment make sense now? > > Given that behavior, you should move the Reload section after section 5, so that > there's no way the onInstalled event could possible re-add the rules. then if > the test passes, we've proven that reload doesn't flush the rules. Done.
	194 ReloadExtension(extension_id);

	195 ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));

	196 EXPECT_EQ(kTestTitle, GetTitle());

	197 EXPECT_EQ(1u, NumberOfRegisteredRules(extension_id));

	198

	199 // 5. Bump the version and update, without the code to add the rules. Rules

	200 // are still active, because the registry does not drop them unless the

	201 // extension gets uninstalled.

	202 ext_dir.WriteManifest(GetRedirectToDataManifestWithVersion(2));

	203 ext_dir.WriteFile(

	204 FILE_PATH_LITERAL("background.js"),

	205 base::StringPrintf(

	206 "%s%s", kRedirectToDataConstants, kRedirectToDataNoRules));

	207 ExtensionTestMessageListener ready_after_update("ready",

	208 /will_reply=/true);

	209 EXPECT_TRUE(UpdateExtension(

	210 extension_id, ext_dir.Pack(), 0 /no new installed extension/));

	211 ASSERT_TRUE(ready_after_update.WaitUntilSatisfied());

	212 ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));

	213 EXPECT_EQ(kTestTitle, GetTitle());

	214 EXPECT_EQ(1u, NumberOfRegisteredRules(extension_id));

	215

	216 // 6. Uninstall the extension. Rules are gone.

	217 UninstallExtension(extension_id);

	218 ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));

	219 EXPECT_NE(kTestTitle, GetTitle());

	220 EXPECT_EQ(0u, NumberOfRegisteredRules(extension_id));

	221 }

OLD	NEW

« no previous file with comments | « no previous file | chrome/browser/extensions/api/declarative/rules_cache_delegate.h » ('j') | no next file with comments »