From 0b384f1f3709333a2a4baf9de5661e1dbda40fa1 Mon Sep 17 00:00:00 2001 From: erikchen Date: Mon, 23 Feb 2015 11:40:57 -0800 Subject: [PATCH] Telemetry: Fix several small problems in the profile creator. - The profile creator was using integer indexing into the tab list, which non-deterministically fails. The new code uses dictionary lookup. - The profile creator was not robust against several types of exceptions that occur more frequently with developer builds of Chrome. BUG=442546 Review URL: https://codereview.chromium.org/939143004 Cr-Commit-Position: refs/heads/master@{#317624} --- .../fast_navigation_profile_extender.py | 57 ++++++++++++++++++---- .../fast_navigation_profile_extender_unittest.py | 23 ++++++--- 2 files changed, 64 insertions(+), 16 deletions(-) diff --git a/tools/perf/profile_creators/fast_navigation_profile_extender.py b/tools/perf/profile_creators/fast_navigation_profile_extender.py index ec2cb7ce7f13..d010cc7bdfd2 100644 --- a/tools/perf/profile_creators/fast_navigation_profile_extender.py +++ b/tools/perf/profile_creators/fast_navigation_profile_extender.py @@ -8,6 +8,7 @@ from telemetry.core import browser_finder_exceptions from telemetry.core import exceptions from telemetry.core import platform from telemetry.core import util +from telemetry.core.backends.chrome_inspector import devtools_http class FastNavigationProfileExtender(object): @@ -40,6 +41,11 @@ class FastNavigationProfileExtender(object): # This member is initialized during SetUp(). self._browser = None + # The instance keeps a list of Tabs that can be navigated successfully. + # This means that the Tab is not crashed, and is processing JavaScript in a + # timely fashion. + self._navigation_tabs = [] + # The number of tabs to use. self._NUM_TABS = maximum_batch_size @@ -95,9 +101,6 @@ class FastNavigationProfileExtender(object): ["win", "mac", "linux"]) self._browser = possible_browser.Create(finder_options) - while(len(self._browser.tabs) < self._NUM_TABS): - self._browser.tabs.New() - def TearDown(self): """Teardown that is guaranteed to be executed before the instance is destroyed. @@ -121,6 +124,28 @@ class FastNavigationProfileExtender(object): def profile_path(self): return self._profile_path + def _RefreshNavigationTabs(self): + """Updates the member self._navigation_tabs to contain self._NUM_TABS + elements, each of which is not crashed. The crashed tabs are intentionally + leaked, since Telemetry doesn't have a good way of killing crashed tabs. + + It is also possible for a tab to be stalled in an infinite JavaScript loop. + These tabs will be in self._browser.tabs, but not in self._navigation_tabs. + There is no way to kill these tabs, so they are also leaked. This method is + careful to only use tabs in self._navigation_tabs, or newly created tabs. + """ + live_tabs = [tab for tab in self._navigation_tabs if tab.IsAlive()] + self._navigation_tabs = live_tabs + + while len(self._navigation_tabs) < self._NUM_TABS: + self._navigation_tabs.append(self._browser.tabs.New()) + + def _RemoveNavigationTab(self, tab): + """Removes a tab which is no longer in a useable state from + self._navigation_tabs. The tab is not removed from self._browser.tabs, + since there is no guarantee that the tab can be safely removed.""" + self._navigation_tabs.remove(tab) + def _GetPossibleBrowser(self, finder_options): """Return a possible_browser with the given options.""" possible_browser = browser_finder.FindBrowser(finder_options) @@ -137,7 +162,9 @@ class FastNavigationProfileExtender(object): """Retrives the URL of the tab.""" try: return tab.EvaluateJavaScript('document.URL', timeout) - except exceptions.DevtoolsTargetCrashException: + except (exceptions.DevtoolsTargetCrashException, + devtools_http.DevToolsClientConnectionError, + devtools_http.DevToolsClientUrlError): return None def _WaitForUrlToChange(self, tab, initial_url, timeout): @@ -178,9 +205,12 @@ class FastNavigationProfileExtender(object): try: tab.Navigate(url, None, timeout_in_seconds) - except exceptions.DevtoolsTargetCrashException: - # We expect a time out, and don't mind if the webpage crashes. Ignore - # both exceptions. + except (exceptions.DevtoolsTargetCrashException, + devtools_http.DevToolsClientConnectionError, + devtools_http.DevToolsClientUrlError): + # We expect a time out. It's possible for other problems to arise, but + # this method is not responsible for dealing with them. Ignore all + # exceptions. pass queued_tabs.append((tab, initial_url)) @@ -210,9 +240,15 @@ class FastNavigationProfileExtender(object): try: tab.WaitForDocumentReadyStateToBeComplete(seconds_to_wait) - except (util.TimeoutException, exceptions.DevtoolsTargetCrashException): - # Ignore time outs and web page crashes. + except util.TimeoutException: + # Ignore time outs. pass + except (exceptions.DevtoolsTargetCrashException, + devtools_http.DevToolsClientConnectionError, + devtools_http.DevToolsClientUrlError): + # If any error occurs, remove the tab. it's probably in an + # unrecoverable state. + self._RemoveNavigationTab(tab) def _GetUrlsToNavigate(self, url_iterator): """Returns an array of urls to navigate to, given a url_iterator.""" @@ -231,6 +267,7 @@ class FastNavigationProfileExtender(object): """ url_iterator = self.GetUrlIterator() while True: + self._RefreshNavigationTabs() urls = self._GetUrlsToNavigate(url_iterator) if len(urls) == 0: @@ -239,7 +276,7 @@ class FastNavigationProfileExtender(object): batch = [] for i in range(len(urls)): url = urls[i] - tab = self._browser.tabs[i] + tab = self._navigation_tabs[i] batch.append((tab, url)) queued_tabs = self._BatchNavigateTabs(batch) diff --git a/tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py b/tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py index 2a2af9d779e7..05cf334e546f 100644 --- a/tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py +++ b/tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py @@ -15,11 +15,22 @@ class FakeTab(object): pass +class FakeTabList(object): + def __init__(self): + self._tabs = [] + + def New(self): + tab = FakeTab() + self._tabs.append(tab) + return tab + + def __len__(self): + return len(self._tabs) + + class FakeBrowser(object): - def __init__(self, tab_count): - self.tabs = [] - for _ in range(tab_count): - self.tabs.append(FakeTab()) + def __init__(self): + self.tabs = FakeTabList() # Testing private method. @@ -40,7 +51,7 @@ class FastNavigationProfileExtenderTest(unittest.TestCase): extender.ShouldExitAfterBatchNavigation = mock.MagicMock(return_value=True) extender._WaitForQueuedTabsToLoad = mock.MagicMock() - extender._browser = FakeBrowser(extender._NUM_TABS) + extender._browser = FakeBrowser() extender._BatchNavigateTabs = mock.MagicMock() # Set up a callback to record the tabs and urls in each navigation. @@ -67,7 +78,7 @@ class FastNavigationProfileExtenderTest(unittest.TestCase): # The first couple of tabs should have been navigated once. The remaining # tabs should not have been navigated. for i in range(len(extender._browser.tabs)): - tab = extender._browser.tabs[i] + tab = extender._browser.tabs._tabs[i] if i < batch_size: expected_tab_navigation_count = 1 -- 2.11.4.GIT