Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1124543006:
Telemetry: Update exception handling in FastNavigationProfileExtender. (Closed)
DescriptionTelemetry: Update exception handling in FastNavigationProfileExtender.
When FastNavigationProfileExtender was first written, it was intended to
navigate to the top 10,000 sites in Alexa. Many of these sites would fail to
load for unexpected reasons. Furthermore, the exceptions raised by Telemetry
when these errors occured were frequently less than useful. As a result,
FastNavigationProfileExtender was catching many fatal exceptions but treating
them as non-fatal exceptions.
When FastNavigationProfileExtender is used on Windows, and Chrome crashes in a
way that invokes the Windows Error dialog, attempts by Telemetry to communicate
with the crashed instance of Chrome can cause Python to stall. I did not
investigate the details, but I suspect this is related to the implementation of
the thirdparty websocket library that Telemetry uses.
This CL makes the FastNavigationProfileExtender treat fatal exceptions
appropriately, instead of ignoring them.
BUG=477375
Committed: https://crrev.com/b07222926f5b8cb3f43cd57ff22c87772a6cac64
Cr-Commit-Position: refs/heads/master@{#329488}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 11
Patch Set 4 : Comments from nednguyen, dtu. #
Total comments: 11
Patch Set 5 : Comments from nednguyen. #Patch Set 6 : Comments from nednguyen. #Messages
Total messages: 18 (5 generated)
erikchen@chromium.org changed reviewers: + dtu@chromium.org
dtu: Please review. https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:55: self.VerifyProfileWasExtended() I'm intentionally omitting my implementation of VerifyProfileWasExtended in small_profile_creator, since the profile is not correctly extended on both mac and windows.
On 2015/05/05 22:16:46, erikchen wrote: > dtu: Please review. > > https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... > File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): > > https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... > tools/perf/profile_creators/fast_navigation_profile_extender.py:55: > self.VerifyProfileWasExtended() > I'm intentionally omitting my implementation of VerifyProfileWasExtended in > small_profile_creator, since the profile is not correctly extended on both mac > and windows. dtu: ping
In the past, when we've had stalling behavior, it was generally because a timeout was missing somewhere. Those can be anywhere, so they've been tricky to catch. https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:89: leaked, since Telemetry doesn't have a good way of killing crashed tabs. I have a question. What happens if you call tab.Close() on a crashed tab? https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:110: return tab.EvaluateJavaScript('document.URL', timeout) Does tab.url not work in this case? (tab.url uses DevTools JSON to talk with the browser process, not the renderer) https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:142: if seconds_to_wait == 0: Instead of doing max(), why not just check seconds_to_wait <= 0? https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:198: util.WaitFor(lambda tab=tab: tab.HasReachedQuiescence(), Just lambda: tab.HasReachedQuiescence()
nednguyen@google.com changed reviewers: + nednguyen@google.com
https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:55: self.VerifyProfileWasExtended() On 2015/05/05 22:16:45, erikchen wrote: > I'm intentionally omitting my implementation of VerifyProfileWasExtended in > small_profile_creator, since the profile is not correctly extended on both mac > and windows. These notes to the code reviewers should also be put as comment in the code :-)
Patchset #4 (id:60001) has been deleted
dtu, nednguyen: PTAL https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:55: self.VerifyProfileWasExtended() On 2015/05/07 22:39:20, nednguyen wrote: > On 2015/05/05 22:16:45, erikchen wrote: > > I'm intentionally omitting my implementation of VerifyProfileWasExtended in > > small_profile_creator, since the profile is not correctly extended on both mac > > and windows. > > These notes to the code reviewers should also be put as comment in the code :-) Done. https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:89: leaked, since Telemetry doesn't have a good way of killing crashed tabs. On 2015/05/07 21:50:36, dtu wrote: > I have a question. What happens if you call tab.Close() on a crashed tab? The exception: "DevtoolsTargetCrashException: Devtools target crashed" is raised. https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:110: return tab.EvaluateJavaScript('document.URL', timeout) On 2015/05/07 21:50:36, dtu wrote: > Does tab.url not work in this case? (tab.url uses DevTools JSON to talk with the > browser process, not the renderer) That's a good suggestion - I've added a comment here and filed a bug against myself to investigate further. https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:142: if seconds_to_wait == 0: On 2015/05/07 21:50:36, dtu wrote: > Instead of doing max(), why not just check seconds_to_wait <= 0? Done. https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:198: util.WaitFor(lambda tab=tab: tab.HasReachedQuiescence(), On 2015/05/07 21:50:36, dtu wrote: > Just lambda: tab.HasReachedQuiescence() Actually, there's no need for the lambda either.
On 2015/05/08 19:47:58, erikchen wrote: > dtu, nednguyen: PTAL > > https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... > File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): > > https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... > tools/perf/profile_creators/fast_navigation_profile_extender.py:55: > self.VerifyProfileWasExtended() > On 2015/05/07 22:39:20, nednguyen wrote: > > On 2015/05/05 22:16:45, erikchen wrote: > > > I'm intentionally omitting my implementation of VerifyProfileWasExtended in > > > small_profile_creator, since the profile is not correctly extended on both > mac > > > and windows. > > > > These notes to the code reviewers should also be put as comment in the code > :-) > > Done. > > https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... > tools/perf/profile_creators/fast_navigation_profile_extender.py:89: leaked, > since Telemetry doesn't have a good way of killing crashed tabs. > On 2015/05/07 21:50:36, dtu wrote: > > I have a question. What happens if you call tab.Close() on a crashed tab? > > The exception: "DevtoolsTargetCrashException: Devtools target crashed" is > raised. > > https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... > tools/perf/profile_creators/fast_navigation_profile_extender.py:110: return > tab.EvaluateJavaScript('document.URL', timeout) > On 2015/05/07 21:50:36, dtu wrote: > > Does tab.url not work in this case? (tab.url uses DevTools JSON to talk with > the > > browser process, not the renderer) > > That's a good suggestion - I've added a comment here and filed a bug against > myself to investigate further. > > https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... > tools/perf/profile_creators/fast_navigation_profile_extender.py:142: if > seconds_to_wait == 0: > On 2015/05/07 21:50:36, dtu wrote: > > Instead of doing max(), why not just check seconds_to_wait <= 0? > > Done. > > https://codereview.chromium.org/1124543006/diff/40001/tools/perf/profile_crea... > tools/perf/profile_creators/fast_navigation_profile_extender.py:198: > util.WaitFor(lambda tab=tab: tab.HasReachedQuiescence(), > On 2015/05/07 21:50:36, dtu wrote: > > Just lambda: tab.HasReachedQuiescence() > > Actually, there's no need for the lambda either. nednguyen: Ping?
https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:89: def _AddNewTab(self): The content of this method can just be moved to _RefreshNavigationTab https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:129: if seconds_to_wait <= 0: what about: if time.time() >= end_time: break https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:141: def _WaitForTabToBeReady(self, tab, end_time): Why do we need this? Ain't HasReachedQuiescence check below is good enough? https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/profile_extender.py (right): https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/profile_extender.py:90: def VerifyProfileWasExtended(self): Putting this API here is strange since there is no usage of it in ProfileExtender. Move this to FastNavigationProfileExtender ?
nednguyen: PTAL https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:89: def _AddNewTab(self): On 2015/05/12 16:07:43, nednguyen (slow review) wrote: > The content of this method can just be moved to _RefreshNavigationTab Done. https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:129: if seconds_to_wait <= 0: On 2015/05/12 16:07:43, nednguyen (slow review) wrote: > what about: > if time.time() >= end_time: > break Each iteration of the loop should only call time.time() once. In your suggestion, a second call of time.time() is required to generate seconds_to_wait. https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:141: def _WaitForTabToBeReady(self, tab, end_time): On 2015/05/12 16:07:43, nednguyen (slow review) wrote: > Why do we need this? Ain't HasReachedQuiescence check below is good enough? Through experimental testing, it isn't sufficient. Just because the network has quiesced does not necessarily mean that the page has finished loading, for example. https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/profile_extender.py (right): https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/profile_extender.py:90: def VerifyProfileWasExtended(self): On 2015/05/12 16:07:43, nednguyen (slow review) wrote: > Putting this API here is strange since there is no usage of it in > ProfileExtender. > > Move this to FastNavigationProfileExtender ? Sure.
lgtm https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:129: if seconds_to_wait <= 0: On 2015/05/12 18:43:48, erikchen wrote: > On 2015/05/12 16:07:43, nednguyen (slow review) wrote: > > what about: > > if time.time() >= end_time: > > break > > Each iteration of the loop should only call time.time() once. In your > suggestion, a second call of time.time() is required to generate > seconds_to_wait. ah, I didn't show the seconds_to_wait below. https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:141: def _WaitForTabToBeReady(self, tab, end_time): On 2015/05/12 18:43:48, erikchen wrote: > On 2015/05/12 16:07:43, nednguyen (slow review) wrote: > > Why do we need this? Ain't HasReachedQuiescence check below is good enough? > > Through experimental testing, it isn't sufficient. Just because the network has > quiesced does not necessarily mean that the page has finished loading, for > example. Ok, this makes sense. I think we should also move the tab.HasReachedQuiescence here (tab ready == page finished loading + network quiescen)
https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/1124543006/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:141: def _WaitForTabToBeReady(self, tab, end_time): On 2015/05/12 19:37:57, nednguyen (slow review) wrote: > On 2015/05/12 18:43:48, erikchen wrote: > > On 2015/05/12 16:07:43, nednguyen (slow review) wrote: > > > Why do we need this? Ain't HasReachedQuiescence check below is good enough? > > > > Through experimental testing, it isn't sufficient. Just because the network > has > > quiesced does not necessarily mean that the page has finished loading, for > > example. > > Ok, this makes sense. I think we should also move the tab.HasReachedQuiescence > here (tab ready == page finished loading + network quiescen) Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1124543006/#ps120001 (title: "Comments from nednguyen.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124543006/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b07222926f5b8cb3f43cd57ff22c87772a6cac64 Cr-Commit-Position: refs/heads/master@{#329488} |
