Implement externally_connectable! Web pages can now communicate directly with

Implement externally_connectable! Web pages can now communicate directly with
extensions via chrome.runtime.connect() and chrome.runtime.sendMessage() without
needing to proxy via a content script.

BUG=55316
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205116

[Jeffrey Yasskin, 2013-06-08 00:28:09]

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.cc:205: bool
is_externally_connectable = true;
Default this to false in order to fail safe. (i.e. if your if's aren't
exhaustive, you want this to block connecting.)

Or extract the decision logic into a (static?) function that can return its
decision.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.cc:220: // Legacy
behaviour. Any extension, no webpages.
What makes this legacy? Is there a plan to require the externally_connectable
key, say in manifest v3? If users want to opt out of the legacy behavior, how do
they do it? (This needs to make its way to the documentation (i.e. add a TODO),
but you can just comment it for now.)

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.cc:226: source,
receiver_port_id, kReceivingEndDoesntExistError);
This message is, of course, a lie. Please comment why we need to lie so someone
doesn't "correct" it.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.h:215: // Immediate
dispatches a disconnect to |source| for |port_id| given |reason|.
Is "reason" a human-readable string or something else? What does extension_id
mean about the behavior?

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
File chrome/browser/extensions/extension_messages_apitest.cc (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/extension_messages_apitest.cc:126: // TODO(kalman):
test with host permissions.
nit: Capitalize comment sentences.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/extension_messages_apitest.cc:129: // Result codes
from the test. These must match up |results| in
s/match up |results|//match up with |results|/ ?

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/extension_messages_apitest.cc:158: "sendMessage"
It's odd to see "sendMessage" in a list of non-messaging APIs.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/extension_messages_apitest.cc:227: GURL path_url =
embedded_test_server()->GetURL(replacements[i].path);
Do this as a helper function (returning the GURL) instead of a loop. That'll
also help you avoid the puzzle of a non-const variable starting with 'k'.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/extension_messages_apitest.cc:235: // When the
extension isn't installed either should fail.
"either"? Maybe "neither domain should be able to connect to the extension."

https://codereview.chromium.org/16174005/diff/34002/chrome/common/extensions/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:74: "contexts": "all",
Can you add a comment saying where the schema for this file is defined? Probably
at the top of the file.

https://codereview.chromium.org/16174005/diff/34002/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:998: RegisterBinding("app", context);
This change is because the lazy binding loader works now?

https://codereview.chromium.org/16174005/diff/34002/chrome/renderer/extension...
File chrome/renderer/extensions/runtime_custom_bindings.cc (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/renderer/extension...
chrome/renderer/extensions/runtime_custom_bindings.cc:53:
ExtensionMsg_ExternalConnectionInfo info;
This is where you'd need to record whether the event was user-initiated, right?
https://code.google.com/p/chromium/issues/detail?id=55316#c66

https://codereview.chromium.org/16174005/diff/34002/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js
(right):

https://codereview.chromium.org/16174005/diff/34002/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js:71:
port.onMessage.addListener(function(response) {
Do you need to add this listener before posting your messages, or is JS
synchronous enough that it doesn't matter?

https://codereview.chromium.org/16174005/diff/34002/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js:73:
if (checkLastError(reply) &&
If the first response produces a lastError, but the second doesn't, I suspect
this will call reply(results.OK) on the second one, producing a duplicate reply.

https://codereview.chromium.org/16174005/diff/34002/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/messaging/externally_connectable/web_connectable/background.js
(right):

https://codereview.chromium.org/16174005/diff/34002/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/messaging/externally_connectable/web_connectable/background.js:12:
chrome.runtime.onConnectExternal.addListener(function(port) {
Both of these tests should include some mention of the sender, since that's
going to have a different format for web senders than extension senders.

Or extend messaging/connect_external to check this.

[not at google - send to devlin, 2013-06-08 02:02:33]

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.cc:205: bool
is_externally_connectable = true;
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> Default this to false in order to fail safe. (i.e. if your if's aren't
> exhaustive, you want this to block connecting.)
> 
> Or extract the decision logic into a (static?) function that can return its
> decision.

Default to false, static function looks a bit contrived.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.cc:220: // Legacy
behaviour. Any extension, no webpages.
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> What makes this legacy? Is there a plan to require the externally_connectable
> key, say in manifest v3? If users want to opt out of the legacy behavior, how
do
> they do it? (This needs to make its way to the documentation (i.e. add a
TODO),
> but you can just comment it for now.)

Fair enough, legacy is the wrong word. I don't see any reason to deprecate the
default behaviour.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.cc:226: source,
receiver_port_id, kReceivingEndDoesntExistError);
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> This message is, of course, a lie. Please comment why we need to lie so
someone
> doesn't "correct" it.

Done.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.h:215: // Immediate
dispatches a disconnect to |source| for |port_id| given |reason|.
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> Is "reason" a human-readable string or something else? What does extension_id
> mean about the behavior?

Done.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
File chrome/browser/extensions/extension_messages_apitest.cc (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/extension_messages_apitest.cc:126: // TODO(kalman):
test with host permissions.
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> nit: Capitalize comment sentences.

Done.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/extension_messages_apitest.cc:129: // Result codes
from the test. These must match up |results| in
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> s/match up |results|//match up with |results|/ ?

Done.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/extension_messages_apitest.cc:158: "sendMessage"
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> It's odd to see "sendMessage" in a list of non-messaging APIs.

Oops. this test should not pass.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/extension_messages_apitest.cc:227: GURL path_url =
embedded_test_server()->GetURL(replacements[i].path);
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> Do this as a helper function (returning the GURL) instead of a loop. That'll
> also help you avoid the puzzle of a non-const variable starting with 'k'.

Done.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/extension_messages_apitest.cc:235: // When the
extension isn't installed either should fail.
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> "either"? Maybe "neither domain should be able to connect to the extension."

Done.

https://codereview.chromium.org/16174005/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/extension_messages_apitest.cc:235: // When the
extension isn't installed either should fail.
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> "either"? Maybe "neither domain should be able to connect to the extension."

Done.

https://codereview.chromium.org/16174005/diff/34002/chrome/common/extensions/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:74: "contexts": "all",
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> Can you add a comment saying where the schema for this file is defined?
Probably
> at the top of the file.

Done.

https://codereview.chromium.org/16174005/diff/34002/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:998: RegisterBinding("app", context);
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> This change is because the lazy binding loader works now?

"runtime" needs to be used via RegisterBinding, InstallBindings would be wrong
because (as I think you're saying) it would break laziness.

I changed the others to RegisterBinding to make it consistent, but the code path
is identical for these (until we finally convert app and webstore to use
bindings system).

https://codereview.chromium.org/16174005/diff/34002/chrome/renderer/extension...
File chrome/renderer/extensions/runtime_custom_bindings.cc (right):

https://codereview.chromium.org/16174005/diff/34002/chrome/renderer/extension...
chrome/renderer/extensions/runtime_custom_bindings.cc:53:
ExtensionMsg_ExternalConnectionInfo info;
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> This is where you'd need to record whether the event was user-initiated,
right?
> https://code.google.com/p/chromium/issues/detail?id=55316#c66

Yeah, but it's something for developers not something we're gating this API call
on. I'll do in a follow up.

https://codereview.chromium.org/16174005/diff/34002/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js
(right):

https://codereview.chromium.org/16174005/diff/34002/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js:71:
port.onMessage.addListener(function(response) {
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> Do you need to add this listener before posting your messages, or is JS
> synchronous enough that it doesn't matter?

JS is defined by its single threadedness.

https://codereview.chromium.org/16174005/diff/34002/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js:73:
if (checkLastError(reply) &&
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> If the first response produces a lastError, but the second doesn't, I suspect
> this will call reply(results.OK) on the second one, producing a duplicate
reply.

Done.

https://codereview.chromium.org/16174005/diff/34002/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/messaging/externally_connectable/web_connectable/background.js
(right):

https://codereview.chromium.org/16174005/diff/34002/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/messaging/externally_connectable/web_connectable/background.js:12:
chrome.runtime.onConnectExternal.addListener(function(port) {
On 2013/06/08 00:28:09, Jeffrey Yasskin wrote:
> Both of these tests should include some mention of the sender, since that's
> going to have a different format for web senders than extension senders.
> 
> Or extend messaging/connect_external to check this.

Ah. Very good point. WDYT now.

[Jeffrey Yasskin, 2013-06-08 02:27:47]

lgtm, after the comments below

https://codereview.chromium.org/16174005/diff/86001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/16174005/diff/86001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.h:216: //
|error_message| for source if non-empty.
If the source is non-empty? What's it mean to set an error message? Is that
"sets runtime.lastError=|error_message|"?

https://codereview.chromium.org/16174005/diff/86001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.h:219: const
std::string& reason);
Remember to rename this argument.

https://codereview.chromium.org/16174005/diff/86001/chrome/common/extensions/...
File chrome/common/extensions/manifest_handlers/externally_connectable.h
(right):

https://codereview.chromium.org/16174005/diff/86001/chrome/common/extensions/...
chrome/common/extensions/manifest_handlers/externally_connectable.h:63: const
URLPatternSet matches;
"url_matches"?

https://codereview.chromium.org/16174005/diff/86001/chrome/common/extensions/...
chrome/common/extensions/manifest_handlers/externally_connectable.h:76: bool
MatchesIdOrAllIds(const std::string& id);
I might just call this "MatchesId" since if it matches all ids it definitely
matches this one.

[not at google - send to devlin, 2013-06-08 02:36:50]

https://codereview.chromium.org/16174005/diff/86001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/16174005/diff/86001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.h:216: //
|error_message| for source if non-empty.
On 2013/06/08 02:27:47, Jeffrey Yasskin wrote:
> If the source is non-empty? What's it mean to set an error message? Is that
> "sets runtime.lastError=|error_message|"?

Done.

https://codereview.chromium.org/16174005/diff/86001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.h:219: const
std::string& reason);
On 2013/06/08 02:27:47, Jeffrey Yasskin wrote:
> Remember to rename this argument.

Done.

https://codereview.chromium.org/16174005/diff/86001/chrome/common/extensions/...
File chrome/common/extensions/manifest_handlers/externally_connectable.h
(right):

https://codereview.chromium.org/16174005/diff/86001/chrome/common/extensions/...
chrome/common/extensions/manifest_handlers/externally_connectable.h:63: const
URLPatternSet matches;
On 2013/06/08 02:27:47, Jeffrey Yasskin wrote:
> "url_matches"?

we use matches elsewhere to mean URLs (notably content scripts, ideally want to
be consistent here.

https://codereview.chromium.org/16174005/diff/86001/chrome/common/extensions/...
chrome/common/extensions/manifest_handlers/externally_connectable.h:76: bool
MatchesIdOrAllIds(const std::string& id);
On 2013/06/08 02:27:47, Jeffrey Yasskin wrote:
> I might just call this "MatchesId" since if it matches all ids it definitely
> matches this one.

now you've inspired me to not use the term "matches" anywhere else but with the
URLs. I see your point.

[commit-bot: I haz the power, 2013-06-08 02:37:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/16174005/112001

[commit-bot: I haz the power, 2013-06-08 02:37:46]

Failed to apply patch for
chrome/common/extensions/manifest_handlers/externally_connectable.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/common/extensions/manifest_handlers/externally_connectable.cc
  Hunk #1 FAILED at 4.
  1 out of 5 hunks FAILED -- saving rejects to file
chrome/common/extensions/manifest_handlers/externally_connectable.cc.rej

Patch:      
chrome/common/extensions/manifest_handlers/externally_connectable.cc
Index: chrome/common/extensions/manifest_handlers/externally_connectable.cc
diff --git
a/chrome/common/extensions/manifest_handlers/externally_connectable.cc
b/chrome/common/extensions/manifest_handlers/externally_connectable.cc
index
2566b72b1cdd3a6aa8c1d4114e9eb7cef9307386..ea0670d5c911a49471645347cbcf9c770b12fcd1
100644
--- a/chrome/common/extensions/manifest_handlers/externally_connectable.cc
+++ b/chrome/common/extensions/manifest_handlers/externally_connectable.cc
@@ -4,6 +4,9 @@
 
 #include "chrome/common/extensions/manifest_handlers/externally_connectable.h"
 
+#include <algorithm>
+
+#include "base/stl_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/common/extensions/api/manifest_types.h"
 #include "chrome/common/extensions/extension_manifest_constants.h"
@@ -32,7 +35,16 @@ namespace errors = externally_connectable_errors;
 using api::manifest_types::ExternallyConnectable;
 
 namespace {
+
 const char kAllIds[] = "*";
+
+template <typename T>
+std::vector<T> Sorted(const std::vector<T>& in) {
+  std::vector<T> out = in;
+  std::sort(out.begin(), out.end());
+  return out;
+}
+
 }
 
 ExternallyConnectableHandler::ExternallyConnectableHandler() {}
@@ -129,14 +141,14 @@ scoped_ptr<ExternallyConnectableInfo>
ExternallyConnectableInfo::FromValue(
   }
 
   std::vector<std::string> ids;
-  bool matches_all_ids = false;
+  bool all_ids = false;
 
   if (externally_connectable->ids) {
     for (std::vector<std::string>::iterator it =
              externally_connectable->ids->begin();
          it != externally_connectable->ids->end(); ++it) {
       if (*it == kAllIds) {
-        matches_all_ids = true;
+        all_ids = true;
       } else if (Extension::IdIsValid(*it)) {
         ids.push_back(*it);
       } else {
@@ -148,7 +160,7 @@ scoped_ptr<ExternallyConnectableInfo>
ExternallyConnectableInfo::FromValue(
   }
 
   return make_scoped_ptr(
-      new ExternallyConnectableInfo(matches, ids, matches_all_ids));
+      new ExternallyConnectableInfo(matches, ids, all_ids));
 }
 
 ExternallyConnectableInfo::~ExternallyConnectableInfo() {}
@@ -156,7 +168,14 @@ ExternallyConnectableInfo::~ExternallyConnectableInfo() {}
 ExternallyConnectableInfo::ExternallyConnectableInfo(
     const URLPatternSet& matches,
     const std::vector<std::string>& ids,
-    bool matches_all_ids)
-    : matches(matches), ids(ids), matches_all_ids(matches_all_ids) {}
+    bool all_ids)
+    : matches(matches), ids(Sorted(ids)), all_ids(all_ids) {}
+
+bool ExternallyConnectableInfo::IdCanConnect(const std::string& id) {
+  if (all_ids)
+    return true;
+  DCHECK(base::STLIsSorted(ids));
+  return std::binary_search(ids.begin(), ids.end(), id);
+}
 
 }   // namespace extensions

[commit-bot: I haz the power, 2013-06-08 02:43:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/16174005/88006

[commit-bot: I haz the power, 2013-06-08 04:17:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-08 04:36:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/16174005/134002

[commit-bot: I haz the power, 2013-06-08 07:05:23]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-06-08 15:44:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/16174005/152001

[commit-bot: I haz the power, 2013-06-08 17:30:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/16174005/157005

[commit-bot: I haz the power, 2013-06-08 18:36:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/16174005/156003

[commit-bot: I haz the power, 2013-06-08 19:03:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/16174005/152011

[commit-bot: I haz the power, 2013-06-08 20:31:13]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[not at google - send to devlin, 2013-06-08 20:36:20]

Hey Pawel, I could really use your help here.

I just can't get the critical test,
ExternallyConnectableMessagingTest.ExternallyConnectableMessaging
(browser_tests, defined in
chrome/browser/extensions/extension_messages_apitest.cc), to pass on the bots.
It fails 100% of the time irrespective of whether I use test_server or
embedded_test server - maybe something to do with the host resolver?

The test is supposed to set up two pages on different URLs, chromium.org and
google.com, and tests that a sample extension can receive messages from one not
the other. Works fine locally every time (gtest_repetitions=100 etc).

But - it appears to fail to load the page on the bots (404 for the embedded test
server). Example failure:

http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/136386...

Given the predictability of it, it's not a flake. Presumably just a difference
in environment - but why would this be affected by the embedded test server? And
what is the difference anyway?

I'd be great if you could have a look. I need to land this before Monday so
worst case I disable it for now.

[方觉(Fang Jue), 2013-06-09 01:43:41]

On 2013/06/08 20:36:20, kalman wrote:
> Hey Pawel, I could really use your help here.
> 
> I just can't get the critical test,
> ExternallyConnectableMessagingTest.ExternallyConnectableMessaging
> (browser_tests, defined in
> chrome/browser/extensions/extension_messages_apitest.cc), to pass on the bots.
> It fails 100% of the time irrespective of whether I use test_server or
> embedded_test server - maybe something to do with the host resolver?
> 
> The test is supposed to set up two pages on different URLs,
http://chromium.org and
> http://google.com, and tests that a sample extension can receive messages from
one not
> the other. Works fine locally every time (gtest_repetitions=100 etc).
> 
> But - it appears to fail to load the page on the bots (404 for the embedded
test
> server). Example failure:
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/136386...
> 
> Given the predictability of it, it's not a flake. Presumably just a difference
> in environment - but why would this be affected by the embedded test server?
And
> what is the difference anyway?
> 
> I'd be great if you could have a look. I need to land this before Monday so
> worst case I disable it for now.

What's the 'current directory' when you run the test locally and when it's ran
on the trybot?
It seems that embedded test server reads files from
<current
directory>/chrome/test/data/extensions/api_test/messaging/externally_connectable
and if <current directory> isn't src/, files are not found and the result will
be a 404.

Looking at calls of ServeFilesFromDirectory, two of them use absolute path. The
other one
(ExtensionLoadingTest.UpgradeAfterNavigatingFromOverriddenNewTabPage)'s path is
"chrome/test/data" but from the code it seems that this particular test will run
as expected
even if the url request returns 404.

[not at google - send to devlin, 2013-06-09 02:58:53]

Yes I bet it's that, thanks! Changing to PathService like the other tests makes
it pass from any directory. Chose the wrong usage to cargo cult it seems.
Something for that method to assert, perhaps, but another day.

[commit-bot: I haz the power, 2013-06-09 02:59:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/16174005/170001

[commit-bot: I haz the power, 2013-06-09 12:58:06]

Change committed as 205116

[Paweł, 2013-07-24 01:13:45]

Yes, I think it's the issue is with absolute vs. relative paths. Make sure
to base the path off source root obtain from PathService.

Please let me know if you have more questions.

Paweł

On Sat, Jun 8, 2013 at 6:43 PM, <fangjue23303@gmail.com> wrote:

> On 2013/06/08 20:36:20, kalman wrote:
>
>> Hey Pawel, I could really use your help here.
>>
>
>  I just can't get the critical test,
>> ExternallyConnectableMessaging**Test.**ExternallyConnectableMessaging
>> (browser_tests, defined in
>> chrome/browser/extensions/**extension_messages_apitest.cc)**, to pass on
>> the bots.
>> It fails 100% of the time irrespective of whether I use test_server or
>> embedded_test server - maybe something to do with the host resolver?
>>
>
>  The test is supposed to set up two pages on different URLs,
>>
> http://chromium.org and
>
>> http://google.com, and tests that a sample extension can receive
>> messages from
>>
> one not
>
>> the other. Works fine locally every time (gtest_repetitions=100 etc).
>>
>
>  But - it appears to fail to load the page on the bots (404 for the
>> embedded
>>
> test
>
>> server). Example failure:
>>
>
>
> http://build.chromium.org/p/**tryserver.chromium/builders/**
> mac_rel/builds/136386/steps/**browser_tests/logs/**
>
ExternallyConnectableMessaging<http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/136386/steps/browser_tests/logs/ExternallyConnectableMessaging>
>
>  Given the predictability of it, it's not a flake. Presumably just a
>> difference
>> in environment - but why would this be affected by the embedded test
>> server?
>>
> And
>
>> what is the difference anyway?
>>
>
>  I'd be great if you could have a look. I need to land this before Monday
>> so
>> worst case I disable it for now.
>>
>
> What's the 'current directory' when you run the test locally and when it's
> ran
> on the trybot?
> It seems that embedded test server reads files from
> <current
> directory>/chrome/test/data/**extensions/api_test/messaging/**
> externally_connectable
> and if <current directory> isn't src/, files are not found and the result
> will
> be a 404.
>
> Looking at calls of ServeFilesFromDirectory, two of them use absolute
> path. The
> other one
> (ExtensionLoadingTest.**UpgradeAfterNavigatingFromOver**riddenNewTabPage)'s
> path is
> "chrome/test/data" but from the code it seems that this particular test
> will run
> as expected
> even if the url request returns 404.
>
>
https://chromiumcodereview.**appspot.com/16174005/<https://chromiumcodereview...
>