Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
449753002:
Improve error reporting when indexing tab. (Closed)
DescriptionImprove error reporting when indexing tab.
If we created 3 new tabs, and 1 of them crashed, then there
is no guaranteed that browser.tabs[1] return the correct tab
object. The tabs list can no longer be trusted in this case. Hence if
the number of inspectable contexts doesn't match the number
of expected tabs (based on the number of
tab_list_backend.New & tab_list_backend.Close calls),
tab_list_backend's __getitem__ throws a TabCrashException.
BUG=398467
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290974
Patch Set 1 #
Total comments: 3
Patch Set 2 : Update tab_list_backend to keep track of openning tabs number. #Patch Set 3 : Tab test that crashes any tab must restart the browser. #
Total comments: 6
Patch Set 4 : Address Ken's comments #
Total comments: 2
Patch Set 5 : Address Tony's comments #Patch Set 6 : Remove setting num expected backends upon first usage logic. #
Total comments: 1
Patch Set 7 : Add comments explain what to do with exceptions #
Messages
Total messages: 39 (0 generated)
Thanks for cleaning this up. A couple of questions/suggestions to consider. https://codereview.chromium.org/449753002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/inspector_backend_list.py (right): https://codereview.chromium.org/449753002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/inspector_backend_list.py:49: raise exceptions.TabIndexOutOfBoundError( Should we just raise a built-in IndexError? If not, I recommend we rename this TabIndexError and make it subclass IndexError. https://codereview.chromium.org/449753002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/exceptions.py (right): https://codereview.chromium.org/449753002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/exceptions.py:29: class TabIndexOutOfBoundError(TabCrashException): I don't think this should be a TabCrashException. It usually doesn't directly mean that. If I have one tab and I try to access browser.tabs[1], then I'll get this, but nothing crashed. Handling it like a TabCrashException means it goes down all kinds of unexpected code paths having to do w/ retries, etc. Also, we probably want a unittest that simply accesses an index out of range and does an assertRaises().
https://codereview.chromium.org/449753002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/exceptions.py (right): https://codereview.chromium.org/449753002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/exceptions.py:29: class TabIndexOutOfBoundError(TabCrashException): On 2014/08/07 13:54:45, tonyg wrote: > I don't think this should be a TabCrashException. It usually doesn't directly > mean that. > > If I have one tab and I try to access browser.tabs[1], then I'll get this, but > nothing crashed. Handling it like a TabCrashException means it goes down all > kinds of unexpected code paths having to do w/ retries, etc. > > Also, we probably want a unittest that simply accesses an index out of range and > does an assertRaises(). This is true. However, I suspect in Ken's case, the tab crashed, then the inspectable context corresponds to it goes away, rather than an indexing error from the callsite. I think I can update the code in the inspectable context to distinguish between the two scenario.
On 2014/08/07 14:50:49, nednguyen wrote: > https://codereview.chromium.org/449753002/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/exceptions.py (right): > > https://codereview.chromium.org/449753002/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/exceptions.py:29: class > TabIndexOutOfBoundError(TabCrashException): > On 2014/08/07 13:54:45, tonyg wrote: > > I don't think this should be a TabCrashException. It usually doesn't directly > > mean that. > > > > If I have one tab and I try to access browser.tabs[1], then I'll get this, but > > nothing crashed. Handling it like a TabCrashException means it goes down all > > kinds of unexpected code paths having to do w/ retries, etc. > > > > Also, we probably want a unittest that simply accesses an index out of range > and > > does an assertRaises(). > > This is true. However, I suspect in Ken's case, the tab crashed, then the > inspectable context corresponds to it goes away, rather than an indexing error > from the callsite. I think I can update the code in the inspectable context to > distinguish between the two scenario. That would be ideal. Getting actionable information in these cases is really important.
On 2014/08/07 15:12:31, tonyg wrote: > On 2014/08/07 14:50:49, nednguyen wrote: > > > https://codereview.chromium.org/449753002/diff/1/tools/telemetry/telemetry/co... > > File tools/telemetry/telemetry/core/exceptions.py (right): > > > > > https://codereview.chromium.org/449753002/diff/1/tools/telemetry/telemetry/co... > > tools/telemetry/telemetry/core/exceptions.py:29: class > > TabIndexOutOfBoundError(TabCrashException): > > On 2014/08/07 13:54:45, tonyg wrote: > > > I don't think this should be a TabCrashException. It usually doesn't > directly > > > mean that. > > > > > > If I have one tab and I try to access browser.tabs[1], then I'll get this, > but > > > nothing crashed. Handling it like a TabCrashException means it goes down all > > > kinds of unexpected code paths having to do w/ retries, etc. > > > > > > Also, we probably want a unittest that simply accesses an index out of range > > and > > > does an assertRaises(). > > > > This is true. However, I suspect in Ken's case, the tab crashed, then the > > inspectable context corresponds to it goes away, rather than an indexing error > > from the callsite. I think I can update the code in the inspectable context to > > distinguish between the two scenario. > > That would be ideal. Getting actionable information in these cases is really > important. Updated. ptal
LGTM from my standpoint. Couple of comments. https://codereview.chromium.org/449753002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py (right): https://codereview.chromium.org/449753002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py:19: # This variable keeps track of the number of openning tabs based on the typo: open or opening, not openning. _num_open_tabs or _num_opening_tabs throughout. https://codereview.chromium.org/449753002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py:64: 'Number of openning tabs is %i, whereas number of alive tabs is %i, ' openning -> opening, alive -> live https://codereview.chromium.org/449753002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py:65: 'Try to get tab at index %i but this may not return the right tab.' % Try -> Tried
Tony, can you take a look again? Thanks. https://codereview.chromium.org/449753002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py (right): https://codereview.chromium.org/449753002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py:19: # This variable keeps track of the number of openning tabs based on the On 2014/08/08 18:34:12, Ken Russell wrote: > typo: open or opening, not openning. _num_open_tabs or _num_opening_tabs > throughout. Done. https://codereview.chromium.org/449753002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py:64: 'Number of openning tabs is %i, whereas number of alive tabs is %i, ' On 2014/08/08 18:34:12, Ken Russell wrote: > openning -> opening, alive -> live Done. https://codereview.chromium.org/449753002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py:65: 'Try to get tab at index %i but this may not return the right tab.' % On 2014/08/08 18:34:12, Ken Russell wrote: > Try -> Tried Done.
lgtm https://codereview.chromium.org/449753002/diff/160001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py (right): https://codereview.chromium.org/449753002/diff/160001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py:24: self._num_opening_tabs = None Would this be more readable as self._num_expected_tabs?
https://codereview.chromium.org/449753002/diff/160001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py (right): https://codereview.chromium.org/449753002/diff/160001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py:24: self._num_opening_tabs = None On 2014/08/09 17:37:24, tonyg wrote: > Would this be more readable as self._num_expected_tabs? Done.
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nednguyen@google.com/449753002/180001
The CQ bit was unchecked by nednguyen@google.com
On 2014/08/11 16:13:10, nednguyen wrote: > The CQ bit was unchecked by mailto:nednguyen@google.com Sorry for the back and forth on call review, I think initializing the num_expected_tabs to None and set it to the right number is a brittle hack :-(. So I add a DidStartBrowser() method to tab_list_backend that set num_expted_tabs after browser is started. PTAL again. thanks
lgtm https://codereview.chromium.org/449753002/diff/200001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py (right): https://codereview.chromium.org/449753002/diff/200001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py:50: self._num_expected_tabs -= 1 Thinking about this some more. I'm wondering whether this variable could get into a permanently bad state after an exception in this method. It's probably worth thinking through whether this should be in a finally-block for anything (perhaps maybe just the line immediately above it)?
On 2014/08/11 16:37:22, tonyg wrote: > lgtm > > https://codereview.chromium.org/449753002/diff/200001/tools/telemetry/telemet... > File tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py (right): > > https://codereview.chromium.org/449753002/diff/200001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/backends/chrome/tab_list_backend.py:50: > self._num_expected_tabs -= 1 > Thinking about this some more. I'm wondering whether this variable could get > into a permanently bad state after an exception in this method. It's probably > worth thinking through whether this should be in a finally-block for anything > (perhaps maybe just the line immediately above it)? That's right. We chatted offline, and callsite probably should handle the exception by restarting the browser. I added comments explaining the situation.
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nednguyen@google.com/449753002/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nednguyen@google.com/449753002/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2014/08/11 22:22:24, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_chromium_rel_swarming on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) This patch still cannot be landed since invoking tab_list_backend.DidStartBrowser() (which initialize the number of expected tabs) somehow increase executionContextCreated by 1. I am not sure whether this is expected or not.
+nduca
+achuith
Do you remember which test failed? If it was the iframe one, it might be fixed now. If it was something else, can you paste in the failure?
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nednguyen@google.com/449753002/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nednguyen@google.com/449753002/220001
Message was sent while issue was closed.
Committed patchset #7 (220001) as 290974
Message was sent while issue was closed.
A revert of this CL (patchset #7) has been created in https://codereview.chromium.org/474983006/ by jennyz@chromium.org. The reason for reverting is: This cl broke the some VMTest.SimpleVerifyTest on both amd64 and x86 chromium os bots. https://build.chromium.org/p/chromiumos.chromium/builders/AMD64%20%28chromium... https://build.chromium.org/p/chromiumos.chromium/builders/X86%20%28chromium%2... The detailed log shows it is related to this cl: https://00e9e64bac2888f7376cb33c571e0e5498754fbdb7e8acee1a-apidata.googleuser....
Message was sent while issue was closed.
A revert of this CL (patchset #7) has been created in https://codereview.chromium.org/494753003/ by nednguyen@google.com. The reason for reverting is: This patch broke chromeos bot in https://code.google.com/p/chromium/issues/detail?id=406010 A reland of this must include fixes in: https://codereview.chromium.org/484353003/ https://codereview.chromium.org/499453002/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
