Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1216453002:
[Extensions] Handle some funny cases in script injection (Closed)
|
Created:
5 years, 6 months ago by Devlin Modified:
5 years, 5 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Extensions] Handle some funny cases in script injection
Script injection is full of corner cases, one of which is subframes going and
getting deleted. Fix some of these.
BUG=504541
Committed: https://crrev.com/3ae4a3201adf3a9f9920724c30d6222ac49bc470
Cr-Commit-Position: refs/heads/master@{#336811}
Patch Set 1 #
Total comments: 33
Patch Set 2 : Ben's #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Total comments: 8
Patch Set 5 : #Messages
Total messages: 18 (4 generated)
rdevlin.cronin@chromium.org changed reviewers: + kalman@chromium.org
https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/executescript/removed_frames/test.js (right): https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:38: // TODO(devlin): Fix the crash in blink and enable this! This will be done soon.
https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/executescript/removed_frames/test.js (right): https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:5: var responsesReceived = 0; 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. https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:31: config.testServer.port); 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/...'; https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:50: 'if (window !== window.top) {' + 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'); https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:56: ' chrome.runtime.sendMessage("fail");' + 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? https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... extensions/browser/script_executor.cc:107: DCHECK(erased); I prefer the pattern of not having a variable which is only used for a DCHECK, but rather: if (!pending_render_frames_.erase(...)) NOTREACHED(); and speaking of which, what happens if that actually does fail? Will bad/crazy things happen, in which case, this should actually be a CHECK. Or if you're feeling conservative, an early-return. https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... extensions/browser/script_executor.cc:140: id_map[host_id_.id()] = std::set<std::string>(); In tracking down this slightly odd-looking line of code, I came across the comment in extensions/browser/script_execution_observer.h: // |executing_scripts_map| contains all extensions that are executing // scripts, mapped to the paths for those scripts. This may be an empty set // if the script has no path associated with it (e.g. in the case of // tabs.executeScript). Could you change that slightly to imply that there will still be *entries* for those extensions (i.e. the extension will exist in the map), just that the set of scripts they reference may be empty? The way it's worded is ambiguous, it may imply that the set of *map keys* is empty, which isn't true. https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... extensions/browser/script_executor.cc:168: GURL url_; Could you rename to main_frame_url_ and main_frame_error_? https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/program... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/program... extensions/renderer/programmatic_script_injector.cc:26: class ProgrammaticScriptInjector::FrameWatcher // Watches for the deletion of a RenderFrame, after which is_valid will return false. https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/script_... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/script_... extensions/renderer/script_injection_manager.cc:209: should_run_idle_ = true; = false? https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/script_... extensions/renderer/script_injection_manager.cc:280: // note it. This looks a little fishy to me. Is it really not possible, under any crazy circumstance, to have a nested script injection running? Almost certainly not (at least with the APIs we have), because that would be a bit crazy... but ideally I wouldn't even have needed to think about it, and this could be a set of "active frames" or something. https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/scripts... File extensions/renderer/scripts_run_info.cc (right): https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/scripts... extensions/renderer/scripts_run_info.cc:12: #include "third_party/WebKit/public/web/WebLocalFrame.h" Remove this include?
https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/executescript/removed_frames/test.js (right): https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:5: var responsesReceived = 0; On 2015/06/29 18:28:11, kalman wrote: > 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. Not sure I follow. This count is used in both tests, so how does localizing the variable help it? https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:31: config.testServer.port); On 2015/06/29 18:28:11, kalman wrote: > 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/...'; I don't know either (copied). Changed. https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:50: 'if (window !== window.top) {' + On 2015/06/29 18:28:11, kalman wrote: > 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'); Done. https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:56: ' chrome.runtime.sendMessage("fail");' + On 2015/06/29 18:28:11, kalman wrote: > 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? 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. https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... extensions/browser/script_executor.cc:107: DCHECK(erased); On 2015/06/29 18:28:11, kalman wrote: > I prefer the pattern of not having a variable which is only used for a DCHECK, > but rather: > > if (!pending_render_frames_.erase(...)) > NOTREACHED(); > > and speaking of which, what happens if that actually does fail? Will bad/crazy > things happen, in which case, this should actually be a CHECK. Or if you're > feeling conservative, an early-return. Not to be difficult, but why is an if-statement for the sole purpose of a DCHECK any better than a variable for the sole purpose of the DCHECK? In my mind, the former is more confusing, because it makes it look like the code path can change, whereas the latter makes it clear it won't. Re what actually happens, nothing bad will really happen, and it should never happen, so I don't really wanna slow down (even microscopically) a production build for this sanity check. https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... extensions/browser/script_executor.cc:140: id_map[host_id_.id()] = std::set<std::string>(); On 2015/06/29 18:28:11, kalman wrote: > In tracking down this slightly odd-looking line of code, I came across the > comment in extensions/browser/script_execution_observer.h: > > // |executing_scripts_map| contains all extensions that are executing > // scripts, mapped to the paths for those scripts. This may be an empty set > // if the script has no path associated with it (e.g. in the case of > // tabs.executeScript). > > Could you change that slightly to imply that there will still be *entries* for > those extensions (i.e. the extension will exist in the map), just that the set > of scripts they reference may be empty? The way it's worded is ambiguous, it may > imply that the set of *map keys* is empty, which isn't true. Done. https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... extensions/browser/script_executor.cc:168: GURL url_; On 2015/06/29 18:28:11, kalman wrote: > Could you rename to main_frame_url_ and main_frame_error_? Good call; done. https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/program... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/program... extensions/renderer/programmatic_script_injector.cc:26: class ProgrammaticScriptInjector::FrameWatcher On 2015/06/29 18:28:11, kalman wrote: > // Watches for the deletion of a RenderFrame, after which is_valid will return > false. Done. https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/script_... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/script_... extensions/renderer/script_injection_manager.cc:209: should_run_idle_ = true; On 2015/06/29 18:28:11, kalman wrote: > = false? = false = false; = true. https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/script_... extensions/renderer/script_injection_manager.cc:280: // note it. On 2015/06/29 18:28:11, kalman wrote: > This looks a little fishy to me. Is it really not possible, under any crazy > circumstance, to have a nested script injection running? Almost certainly not > (at least with the APIs we have), because that would be a bit crazy... but > ideally I wouldn't even have needed to think about it, and this could be a set > of "active frames" or something. DCHECK'd to account for potential craziness. I believe we can *not* be doing this anymore, since we now have possibly-asynchronous script execution via the suspendable script executor. (That's its own bag of worms, but not the issue at hand.) https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/scripts... File extensions/renderer/scripts_run_info.cc (right): https://codereview.chromium.org/1216453002/diff/1/extensions/renderer/scripts... extensions/renderer/scripts_run_info.cc:12: #include "third_party/WebKit/public/web/WebLocalFrame.h" On 2015/06/29 18:28:11, kalman wrote: > Remove this include? We need it to know that a WebLocalFrame is a WebFrame on line 23-24. Ideally, GetDataSourceUrlForFrame would just take a WebLocalFrame, but that's a job for another patch.
https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/executescript/removed_frames/test.js (right): https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:5: var responsesReceived = 0; On 2015/06/29 19:41:44, Devlin wrote: > On 2015/06/29 18:28:11, kalman wrote: > > 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. > > Not sure I follow. This count is used in both tests, so how does localizing the > variable help it? 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. https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:56: ' chrome.runtime.sendMessage("fail");' + On 2015/06/29 19:41:44, Devlin wrote: > On 2015/06/29 18:28:11, kalman wrote: > > 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? > > Well, even if that were the case, failing is a lot better than timing out. But > that's beside the point. 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. > > 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. 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. https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... extensions/browser/script_executor.cc:107: DCHECK(erased); On 2015/06/29 19:41:44, Devlin wrote: > On 2015/06/29 18:28:11, kalman wrote: > > I prefer the pattern of not having a variable which is only used for a DCHECK, > > but rather: > > > > if (!pending_render_frames_.erase(...)) > > NOTREACHED(); > > > > and speaking of which, what happens if that actually does fail? Will bad/crazy > > things happen, in which case, this should actually be a CHECK. Or if you're > > feeling conservative, an early-return. > > Not to be difficult, but why is an if-statement for the sole purpose of a DCHECK > any better than a variable for the sole purpose of the DCHECK? In my mind, the > former is more confusing, because it makes it look like the code path can > change, whereas the latter makes it clear it won't. > > Re what actually happens, nothing bad will really happen, and it should never > happen, so I don't really wanna slow down (even microscopically) a production > build for this sanity check. If nothing bad happens if it fails, and it should never happen anyway, why bother DCHECKing at all? https://codereview.chromium.org/1216453002/diff/20001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/1216453002/diff/20001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:206: void ScriptInjectionManager::RFOHelper::InvalidateFrame() { Could you rename this "ResetFrame"? Then you wouldn't even really need these comments.
https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/executescript/removed_frames/test.js (right): https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:5: var responsesReceived = 0; On 2015/06/29 20:09:07, kalman wrote: > On 2015/06/29 19:41:44, Devlin wrote: > > On 2015/06/29 18:28:11, kalman wrote: > > > 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. > > > > Not sure I follow. This count is used in both tests, so how does localizing > the > > variable help it? > > 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. 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). https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:56: ' chrome.runtime.sendMessage("fail");' + On 2015/06/29 20:09:07, kalman wrote: > On 2015/06/29 19:41:44, Devlin wrote: > > On 2015/06/29 18:28:11, kalman wrote: > > > 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? > > > > Well, even if that were the case, failing is a lot better than timing out. > But > > that's beside the point. > > 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. > > > > > 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. > > 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. 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. https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/1216453002/diff/1/extensions/browser/script_e... extensions/browser/script_executor.cc:107: DCHECK(erased); On 2015/06/29 20:09:07, kalman wrote: > On 2015/06/29 19:41:44, Devlin wrote: > > On 2015/06/29 18:28:11, kalman wrote: > > > I prefer the pattern of not having a variable which is only used for a > DCHECK, > > > but rather: > > > > > > if (!pending_render_frames_.erase(...)) > > > NOTREACHED(); > > > > > > and speaking of which, what happens if that actually does fail? Will > bad/crazy > > > things happen, in which case, this should actually be a CHECK. Or if you're > > > feeling conservative, an early-return. > > > > Not to be difficult, but why is an if-statement for the sole purpose of a > DCHECK > > any better than a variable for the sole purpose of the DCHECK? In my mind, > the > > former is more confusing, because it makes it look like the code path can > > change, whereas the latter makes it clear it won't. > > > > Re what actually happens, nothing bad will really happen, and it should never > > happen, so I don't really wanna slow down (even microscopically) a production > > build for this sanity check. > > If nothing bad happens if it fails, and it should never happen anyway, why > bother DCHECKing at all? Sanity-checking purposes. So that future Ben and Devlin get a nice stack trace when they make a silly patch. ;) https://codereview.chromium.org/1216453002/diff/20001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/1216453002/diff/20001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:206: void ScriptInjectionManager::RFOHelper::InvalidateFrame() { On 2015/06/29 20:09:07, kalman wrote: > Could you rename this "ResetFrame"? Then you wouldn't even really need these > comments. Made it InvalidateAndResetFrame() (I like the invalidate, since it hints at invalidating it in the ScriptInjectionManager).
https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/executescript/removed_frames/test.js (right): https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:5: var responsesReceived = 0; On 2015/06/29 20:43:13, Devlin wrote: > On 2015/06/29 20:09:07, kalman wrote: > > On 2015/06/29 19:41:44, Devlin wrote: > > > On 2015/06/29 18:28:11, kalman wrote: > > > > 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. > > > > > > Not sure I follow. This count is used in both tests, so how does localizing > > the > > > variable help it? > > > > 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. > > 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). 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. https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:35: waitForCommittedAndRun(injectAndDeleteIframeFromMainFrame, 2, url); 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?
https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/executescript/removed_frames/test.js (right): https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:5: var responsesReceived = 0; On 2015/06/29 21:24:35, kalman wrote: > On 2015/06/29 20:43:13, Devlin wrote: > > On 2015/06/29 20:09:07, kalman wrote: > > > On 2015/06/29 19:41:44, Devlin wrote: > > > > On 2015/06/29 18:28:11, kalman wrote: > > > > > 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. > > > > > > > > Not sure I follow. This count is used in both tests, so how does > localizing > > > the > > > > variable help it? > > > > > > 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. > > > > 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). > > 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. Done. https://codereview.chromium.org/1216453002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:35: waitForCommittedAndRun(injectAndDeleteIframeFromMainFrame, 2, url); On 2015/06/29 21:24:35, kalman wrote: > 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? Tab id is not 2. The number of commits to wait for is two - one for the main frame and one for the iframe.
lgtm https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/removed_frames/test.js (right): https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:8: function MessageListener() { ResponseCounter (and replace instances of "listener" with "counter"). Semantically correct, whereas "listener" could be anything. https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:22: var listener = new MessageListener(); Constructing the listener here is a bit weird, it would be better if the tests constructed these themselves i.e. waitForCommittedAndRun.bind(null, new MessageListener()) but constructing it here is ok, it makes sense to say that every test just gets a counter. https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:29: chrome.webNavigation.onCommitted.addListener(onCommitted); Chrome test way of doing this (modulo your favourite indentation): var done = chrome.test.listenForever( chrome.webNavigation.onCommitted, function(details) { ... done(); }); also: rather than basing this on "num commits" you could get the count of the # of iframes on the page, then +1 for the main frame. Call it "runWhenAllFramesCommitted".
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/removed_frames/test.js (right): https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:8: function MessageListener() { On 2015/06/29 22:33:29, kalman wrote: > ResponseCounter (and replace instances of "listener" with "counter"). > Semantically correct, whereas "listener" could be anything. To me, ResponseCounter doesn't imply at all that it listens for multiple messages and handles them differently (i.e., fails in the case of a failure), but I'm too tired to argue. ;) https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:22: var listener = new MessageListener(); On 2015/06/29 22:33:29, kalman wrote: > Constructing the listener here is a bit weird, it would be better if the tests > constructed these themselves i.e. > > waitForCommittedAndRun.bind(null, new MessageListener()) > > but constructing it here is ok, it makes sense to say that every test just gets > a counter. Leaving here for now; added a comment saying each test gets one. https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:29: chrome.webNavigation.onCommitted.addListener(onCommitted); On 2015/06/29 22:33:29, kalman wrote: > Chrome test way of doing this (modulo your favourite indentation): > > var done = chrome.test.listenForever( > chrome.webNavigation.onCommitted, function(details) { > ... > done(); > }); Unfortunately, this won't work because once the listener is removed, it causes the test to pass. Even though the safe apply is kind of nice, it's not worth the confusion it would add to have a random callback added here. :/ > also: rather than basing this on "num commits" you could get the count of the # > of iframes on the page, then +1 for the main frame. Call it > "runWhenAllFramesCommitted". Getting the count of iframes on the page (programmatically) risks creating a race condition in which we don't queue up the script injection prior to the load finishing, which can be necessary for this test, because otherwise the script injection happens more synchronously, and the sub frame injection doesn't try to inject.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1216453002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216453002/100001
https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/removed_frames/test.js (right): https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:29: chrome.webNavigation.onCommitted.addListener(onCommitted); > Unfortunately, this won't work because once the listener is removed, it causes > the test to pass. Even though the safe apply is kind of nice, it's not worth > the confusion it would add to have a random callback added here. :/ Ok. My next suggestion was going to be to cut this down by 1 line: chrome.webNavigation.onCommitted.addListener(function self(details) { ...; chrome.webNavigation.onCommitted.removeListener(self); }); > Getting the count of iframes on the page (programmatically) risks creating a > race condition in which we don't queue up the script injection prior to the load > finishing, which can be necessary for this test, because otherwise the script > injection happens more synchronously, and the sub frame injection doesn't try to > inject. Doesn't the script injection always happen on idle? In which case the document must be ready and at the very least the iframe element added (whether or not it's loaded). Anyway, whatever. "2" just looks weird to me but obviously if it's not trivial to see why counting iframes would be correct, might as well leave it.
https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/removed_frames/test.js (right): https://codereview.chromium.org/1216453002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/removed_frames/test.js:29: chrome.webNavigation.onCommitted.addListener(onCommitted); On 2015/06/30 16:53:34, kalman wrote: > > > Unfortunately, this won't work because once the listener is removed, it causes > > the test to pass. Even though the safe apply is kind of nice, it's not worth > > the confusion it would add to have a random callback added here. :/ > > Ok. My next suggestion was going to be to cut this down by 1 line: > > chrome.webNavigation.onCommitted.addListener(function self(details) { > ...; > chrome.webNavigation.onCommitted.removeListener(self); > }); > That's a good thought. Since this is already in the CQ, I'll do it when I come back in to re-enable the second test. > > Getting the count of iframes on the page (programmatically) risks creating a > > race condition in which we don't queue up the script injection prior to the > load > > finishing, which can be necessary for this test, because otherwise the script > > injection happens more synchronously, and the sub frame injection doesn't try > to > > inject. > > Doesn't the script injection always happen on idle? In which case the document > must be ready and at the very least the iframe element added (whether or not > it's loaded). Anyway, whatever. "2" just looks weird to me but obviously if it's > not trivial to see why counting iframes would be correct, might as well leave > it. It's interesting. Script injection happens on idle, but if we wait until idle has already happened, when we try to inject, the flow becomes: - Try to inject in frame 1, send IPC 1. - Frame receives IPC 1, already at idle, injects, removes subframe. - Somewhere in there, IPC 2 was sent, but subframe got blown up and IPC lands on the floor. This is probably also a good test to throw in (along with a few other edge cases), but doesn't test the case of the crash itself, when we have multiple pending injections.
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3ae4a3201adf3a9f9920724c30d6222ac49bc470 Cr-Commit-Position: refs/heads/master@{#336811} |
