[Extensions] Handle some funny cases in script injection

chrome/test/data/extensions/api_test/executescript/removed_frames/test.js

Index: chrome/test/data/extensions/api_test/executescript/removed_frames/test.js
diff --git a/chrome/test/data/extensions/api_test/executescript/removed_frames/test.js b/chrome/test/data/extensions/api_test/executescript/removed_frames/test.js
new file mode 100644
index 0000000000000000000000000000000000000000..364ff6c5921698f89cae6bea4ac7dc953a258bb2
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/executescript/removed_frames/test.js
@@ -0,0 +1,100 @@
+// Copyright 2015 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.
+
+var responsesReceived = 0;

[not at google - send to devlin, 2015/06/29 18:28:11]

Can we avoid global variables? Worst case it'll be buggy when you enable that
other test, best case it's harder to follow.

Instead, use chrome.test.listenForever inside injectAndDeleteIframeFromMainWorld
to track this count locally.

[Devlin, 2015/06/29 19:41:44]

Not sure I follow.  This count is used in both tests, so how does localizing the
variable help it?

[not at google - send to devlin, 2015/06/29 20:09:07]

It seems like it's reset to 0 in each test. Why do you need it to be global? It
just makes the risk of the tests interfering with each other, and/or me being
forced to spend time reasoning about why it needs to be global.

[Devlin, 2015/06/29 20:43:13]

chrome.runtime.onMessage listener is global to avoid writing the same exact code
in both test functions.  The listener function also uses responsesReceived.  So
responsesReceived needs to be global, or we can duplicate the code in each test,
or we can do something fancy (like having a function and binding
responsesReceived, but that seems kinda excessive).

[not at google - send to devlin, 2015/06/29 21:24:35]

I don't think it's excessive. You can listen to the same event more than once,
and there are test helpers to listen for single and multiple events. Tests
should be self-contained. There are a number of ways you could factor this.

[Devlin, 2015/06/29 21:51:20]

Done.

+chrome.runtime.onMessage.addListener(function(request, sender, sendResponse) {
+  if (request == 'fail') {
+    chrome.test.fail();
+  } else {
+    chrome.test.assertEq('complete', request);
+    ++responsesReceived;
+  }
+});
+
+var waitForCommittedAndRun = function(functionToRun, numCommits, url) {
+  responsesReceived = 0;
+  var committedCount = 0;
+  var onCommitted = function(details) {
+    if (++committedCount == numCommits) {
+      functionToRun(details.tabId);
+      chrome.webNavigation.onCommitted.removeListener(onCommitted);
+    }
+  };
+  chrome.webNavigation.onCommitted.addListener(onCommitted);
+  chrome.tabs.create({url: url});
+};
+
+chrome.test.getConfig(function(config) {
+  var url = ('http://a.com:PORT/extensions/api_test/executescript/' +
+             'removed_frames/outer.html').replace(/PORT/,
+                                                  config.testServer.port);

[not at google - send to devlin, 2015/06/29 18:28:11]

I find this pattern pretty weird, and I've seen it used elsewhere, but I don't
know why it's not just:

var url = 'http://a.com:' + config.testServer.port +
          '/extensions/...';

[Devlin, 2015/06/29 19:41:44]

I don't know either (copied).  Changed.

+  // Regression tests for crbug.com/500574.
+  chrome.test.runTests([
+   function() {
+      waitForCommittedAndRun(injectAndDeleteIframeFromMainFrame, 2, url);

[not at google - send to devlin, 2015/06/29 21:24:35]

Err, why is the tab ID 2? Can you make it not depend on the order of tab
creation, and the whim of prerender, and so forth?

[Devlin, 2015/06/29 21:51:20]

Tab id is not 2.  The number of commits to wait for is two - one for the main
frame and one for the iframe.

+    }
+    // This is another great test to have, but currently it crashes in blink.
+    // TODO(devlin): Fix the crash in blink and enable this!

[Devlin, 2015/06/25 21:12:27]

This will be done soon.

+    // function() {
+    //   waitForCommittedAndRun(injectAndDeleteIframeFromIframe, 2, url);
+    // }
+  ]);
+});
+
+function injectAndDeleteIframeFromMainFrame(tabId) {
+  // Inject code into each frame. If it's the parent frame, it removes the child
+  // frame from the DOM (invalidating it). The child frame's code shouldn't
+  // finish executing, since it's been removed.
+  var injectFrameCode =
+      'if (window !== window.top) {' +

[not at google - send to devlin, 2015/06/29 18:28:11]

nit (sorta): the problem with this is that the line numbers will be messed up if
there's a problem (syntax error or crash). It's better to do:

var injectFrameCode = [
  'if (window !== window.top) {',
  '  // Etc.',
  '  ...',
  '}',
].join('\n');

[Devlin, 2015/06/29 19:41:44]

Done.

+      // There's a (good) chance the child frame reaches "idle" first, since it
+      // is a subresource of the main frame. But it shouldn't stick around for
+      // more than 200ms or so, since that's the max delay for idle. If the
+      // script still tries to execute, it's probably bad.
+      '  window.setTimeout(function() {' +
+      '    chrome.runtime.sendMessage("fail");' +

[not at google - send to devlin, 2015/06/29 18:28:11]

Why does it matter? The way I read this (?) without sending a "fail" message the
test will simply time out if it has a bug? I think that's fine vs a 250ms delay
which has the danger of being flaky?

[Devlin, 2015/06/29 19:41:44]

Well, even if that were the case, failing is a lot better than timing out.  But
that's beside the point.

This check ensures that the code should *not* execute.  If we remove it, and the
code executes, we don't get a timeout, we just, well, mess up, and don't catch
it.

[not at google - send to devlin, 2015/06/29 20:09:07]

That is beside the point, but I disagree, and will instead claim that
"predictable failure is a lot better than flakiness". Followed by "failing is a
lot better than timing out" with the caveat that only if it's predictable.
I see. Well either way a 250ms timeout will absolutely flake, and also, it
forces the test to be 250ms. Can you make this event based somehow? E.g. It
seems like you could do something like have 2 frames, 1st of which is deleted,
the 2nd of which watches for the DOM element to be removed (with a much shorter
poll time, say 10ms) at which point passes the test (via posting back to the
main frame). Still a potential for flake but it should be much smaller, and the
test will run faster.

[Devlin, 2015/06/29 20:43:13]

But the point is that this code will *not* run.  It shouldn't flake, and won't
make the test take 250ms, because the frame should be deleted by then, and this
code won't do anything.  And, if it does do something, it's a failure.

+      '  }, 250);' +
+      '} else {' +
+      '  iframe = document.getElementsByTagName("iframe")[0];' +
+      '  iframe.parentElement.removeChild(iframe);' +
+      '  chrome.runtime.sendMessage("complete");' +
+      '}';
+  chrome.tabs.executeScript(
+      tabId,
+      {code: injectFrameCode, allFrames: true, runAt: 'document_idle'},
+      function() {
+    chrome.test.assertEq(1, responsesReceived);
+    chrome.test.succeed();
+  });
+};
+
+function injectAndDeleteIframeFromIframe(tabId) {
+  responsesReceived = 0;
+  // Inject code into each frame. Have the child frame remove itself, deleting
+  // the frame while it's still executing.
+  var injectFrameCode =
+      'if (window.self !== window.top) {' +
+      '  var iframe = window.top.document.getElementsByTagName("iframe")[0];' +
+      '  if (!iframe || iframe.contentWindow !== window)' +
+      '    chrome.runtime.sendMessage("fail");' +
+      '  else' +
+      '    window.top.document.body.removeChild(iframe);' +
+      '} else {' +
+      '  chrome.runtime.sendMessage("complete");' +
+      '}';
+  // We also use two "executeScript" calls here so that we have a pending script
+  // execution on a frame that gets deleted.
+  chrome.tabs.executeScript(
+      tabId,
+      {code: injectFrameCode, allFrames: true, runAt: 'document_idle'});
+  chrome.tabs.executeScript(
+      tabId,
+      {code: injectFrameCode, allFrames: true, runAt: 'document_idle'},
+      function() {
+    // Script execution, all other things equal, should happen in the order it
+    // was received, so we only need a check in the second callback.
+    chrome.test.assertEq(2, responsesReceived);
+    chrome.test.succeed();
+  });
+}