Telemetry: Update exception handling in FastNavigationProfileExtender.

tools/perf/profile_creators/fast_navigation_profile_extender.py

Index: tools/perf/profile_creators/fast_navigation_profile_extender.py
diff --git a/tools/perf/profile_creators/fast_navigation_profile_extender.py b/tools/perf/profile_creators/fast_navigation_profile_extender.py
index d5b3da74caea6f517258f50e6b0c5dd16037b862..164b30aaf955b6f146992eff79f1848ebcfb0fa9 100644
--- a/tools/perf/profile_creators/fast_navigation_profile_extender.py
+++ b/tools/perf/profile_creators/fast_navigation_profile_extender.py
@@ -5,6 +5,7 @@ import time
 
 from profile_creators import profile_extender
 from telemetry.core import exceptions
+from telemetry.core import util
 
 
 class FastNavigationProfileExtender(profile_extender.ProfileExtender):
@@ -36,11 +37,13 @@ class FastNavigationProfileExtender(profile_extender.ProfileExtender):
     # The number of tabs to use.
     self._NUM_TABS = maximum_batch_size
 
-    # The amount of time to wait for a batch of pages to finish loading.
-    self._BATCH_PAGE_LOAD_TIMEOUT_IN_SECONDS = 10
+    # The amount of additional time to wait for a batch of pages to finish
+    # loading for each page in the batch.
+    self._BATCH_TIMEOUT_PER_PAGE_IN_SECONDS = 20
 
-    # The default amount of time to wait for the retrieval of the URL of a tab.
-    self._TAB_URL_RETRIEVAL_TIMEOUT_IN_SECONDS = 1
+    # The amount of time to wait for a page to quiesce. Some pages will never
+    # quiesce.
+    self._TIME_TO_WAIT_FOR_PAGE_TO_QUIESCE_IN_SECONDS = 10
 
   def Run(self):
     """Superclass override."""
@@ -49,6 +52,7 @@ class FastNavigationProfileExtender(profile_extender.ProfileExtender):
       self._PerformNavigations()
     finally:
       self.TearDownBrowser()
+      self.VerifyProfileWasExtended()

[erikchen, 2015/05/05 22:16:45]

I'm intentionally omitting my implementation of VerifyProfileWasExtended in
small_profile_creator, since the profile is not correctly extended on both mac
and windows.

[nednguyen, 2015/05/07 22:39:20]

These notes to the code reviewers should also be put as comment in the code :-)

[erikchen, 2015/05/08 19:47:58]

Done.

 
   def GetUrlIterator(self):
     """Gets URLs for the browser to navigate to.
@@ -77,18 +81,7 @@ class FastNavigationProfileExtender(profile_extender.ProfileExtender):
 
   def _AddNewTab(self):
     """Adds a new tab to the browser."""
-
-    # Adding a new tab requires making a request over devtools. This can fail
-    # for a variety of reasons. Retry 3 times.
-    retry_count = 3
-    for i in range(retry_count):
-      try:
-        self._navigation_tabs.append(self._browser.tabs.New())
-      except exceptions.Error:
-        if i == retry_count - 1:
-          raise
-      else:
-        break
+    self._navigation_tabs.append(self._browser.tabs.New())
 
   def _RefreshNavigationTabs(self):
     """Updates the member self._navigation_tabs to contain self._NUM_TABS
@@ -114,14 +107,14 @@ class FastNavigationProfileExtender(profile_extender.ProfileExtender):
 
   def _RetrieveTabUrl(self, tab, timeout):
     """Retrives the URL of the tab."""
-    try:
-      return tab.EvaluateJavaScript('document.URL', timeout)
-    except exceptions.Error:
-      return None
+    return tab.EvaluateJavaScript('document.URL', timeout)

[dtu, 2015/05/07 21:50:36]

Does tab.url not work in this case? (tab.url uses DevTools JSON to talk with the
browser process, not the renderer)

[erikchen, 2015/05/08 19:47:58]

That's a good suggestion - I've added a comment here and filed a bug against
myself to investigate further.

+
+  def _WaitForUrlToChange(self, tab, initial_url, end_time):
+    """Waits for the tab to navigate away from its initial url.
 
-  def _WaitForUrlToChange(self, tab, initial_url, timeout):
-    """Waits for the tab to navigate away from its initial url."""
-    end_time = time.time() + timeout
+    If time.time() is larger than end_time, the function does nothing.
+    Otherwise, the function tries to return no later than end_time.
+    """
     while True:
       seconds_to_wait = end_time - time.time()
       seconds_to_wait = max(0, seconds_to_wait)
@@ -130,7 +123,7 @@ class FastNavigationProfileExtender(profile_extender.ProfileExtender):
         break
 
       current_url = self._RetrieveTabUrl(tab, seconds_to_wait)
-      if current_url != initial_url:
+      if current_url != initial_url and current_url != "":
         break
 
       # Retrieving the current url is a non-trivial operation. Add a small
@@ -138,6 +131,18 @@ class FastNavigationProfileExtender(profile_extender.ProfileExtender):
       # navigation.
       time.sleep(0.01)
 
+  def _WaitForTabToBeReady(self, tab, end_time):
+    """Waits for the tab to be ready.
+
+    If time.time() is larger than end_time, the function does nothing.
+    Otherwise, the function tries to return no later than end_time.
+    """
+    seconds_to_wait = end_time - time.time()
+    seconds_to_wait = max(0, seconds_to_wait)
+    if seconds_to_wait == 0:

[dtu, 2015/05/07 21:50:36]

Instead of doing max(), why not just check seconds_to_wait <= 0?

[erikchen, 2015/05/08 19:47:58]

Done.

+      return
+    tab.WaitForDocumentReadyStateToBeComplete(seconds_to_wait)
+
   def _BatchNavigateTabs(self, batch):
     """Performs a batch of tab navigations with minimal delay.
 
@@ -148,21 +153,22 @@ class FastNavigationProfileExtender(profile_extender.ProfileExtender):
       A list of tuples (tab, initial_url). |initial_url| is the url of the
       |tab| prior to a navigation command being sent to it.
     """
-    timeout_in_seconds = 0
+    # Attempting to pass in a timeout of 0 seconds results in a synchronous
+    # socket error from the websocket library. Pass in a very small timeout
+    # instead so that the websocket library raises a Timeout exception. This
+    # prevents the logic from accidentally catching different socket
+    # exceptions.
+    timeout_in_seconds = 0.01
 
     queued_tabs = []
     for tab, url in batch:
-      initial_url = self._RetrieveTabUrl(tab,
-          self._TAB_URL_RETRIEVAL_TIMEOUT_IN_SECONDS)
-
+      initial_url = self._RetrieveTabUrl(tab, 20)
       try:
         tab.Navigate(url, None, timeout_in_seconds)
-      except exceptions.Error:
-        # 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.
+      except exceptions.TimeoutException:
+        # We expect to receive a timeout exception, since we're not waiting for
+        # the navigation to complete.
         pass
-
       queued_tabs.append((tab, initial_url))
     return queued_tabs
 
@@ -173,30 +179,26 @@ class FastNavigationProfileExtender(profile_extender.ProfileExtender):
       queued_tabs: A list of tuples (tab, initial_url). Each tab is guaranteed
       to have already been sent a navigation command.
     """
-    end_time = time.time() + self._BATCH_PAGE_LOAD_TIMEOUT_IN_SECONDS
+    total_batch_timeout = (len(queued_tabs) *
+        self._BATCH_TIMEOUT_PER_PAGE_IN_SECONDS)
+    end_time = time.time() + total_batch_timeout
     for tab, initial_url in queued_tabs:
-      seconds_to_wait = end_time - time.time()
-      seconds_to_wait = max(0, seconds_to_wait)
-
-      if seconds_to_wait == 0:
-        break
-
-      # Since we don't wait any time for the tab url navigation to commit, it's
+      # Since we didn't wait any time for the tab url navigation to commit, it's
       # possible that the tab hasn't started navigating yet.
-      self._WaitForUrlToChange(tab, initial_url, seconds_to_wait)
+      self._WaitForUrlToChange(tab, initial_url, end_time)
+      self._WaitForTabToBeReady(tab, end_time)
 
+      # Wait up to 10 seconds for the page to quiesce. If the page hasn't
+      # quiesced in 10 seconds, it will probably never quiesce.
       seconds_to_wait = end_time - time.time()
       seconds_to_wait = max(0, seconds_to_wait)
-
+      seconds_to_wait = min(self._TIME_TO_WAIT_FOR_PAGE_TO_QUIESCE_IN_SECONDS,
+                            seconds_to_wait)
       try:
-        tab.WaitForDocumentReadyStateToBeComplete(seconds_to_wait)
+        util.WaitFor(lambda tab=tab: tab.HasReachedQuiescence(),

[dtu, 2015/05/07 21:50:36]

Just lambda: tab.HasReachedQuiescence()

[erikchen, 2015/05/08 19:47:58]

Actually, there's no need for the lambda either.

+                     seconds_to_wait)
       except exceptions.TimeoutException:
-        # Ignore time outs.
         pass
-      except exceptions.Error:
-        # 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."""