Closed Bug 609803 Opened 14 years ago Closed 13 years ago

Intermittent timeout in browser/base/content/test/tabview/browser_tabview_launch.js followed by "browser_tabview_privatebrowsing.js | Tab View is visible again" and finally a suite timeout in browser_tabview_undo_group.js

Categories

(Firefox Graveyard :: Panorama, defect, P3)

Product:
Firefox Graveyard
Component:
Panorama
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 5

People

(Reporter: ehsan.akhgari, Assigned: raymondlee)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 3 obsolete files)

patch for check-in
3.13 KB, patch
Details | Diff | Splinter Review 
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288912309.1288913819.3803.gz&fulltext=1
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2010/11/04 16:11:49
Priority: -- → P3
Blocks: 585689
Re-open if this happens again.
Status: NEW → RESOLVED
Closed: 13 years ago
OS: Mac OS X → Windows 7
Resolution: --- → WORKSFORME
OS: Windows 7 → All
Hardware: x86 → All
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Attached patch v1 (obsolete) — — Splinter Review 
I've checked the launch test and the group test which runs before the launch test but couldn't find any potential issues.  

I've updated the test so it runs in a new window so the test wouldn't be affected by other tests run before it.
Attachment #516182 - Flags: review?(ian)
I unfortunately don't think that this will solve it. As you see in the log above browser_tabview_launch.js keeps running in the background while "_multiwindow_search.js" and "_undo_group.js" and "_privatebrowsing.js" are firing events.

1.) I think we should definitely use registerCleanupFunction() to remove our listeners and hide the tabview (etc.) when our test times out.

2.) The log shows that the "tabviewshown" event is fired but the condition "deck.selectedIndex == 1" never becomes true. That seems to become true when the tabview is shown in "_multiwindow_search.js".

I can't really explain why this isn't working but is .doCommand() really reliable? Though that seems to work because of the "tabviewshown" event. But then really selectedIndex should become "1"?!
Comment on attachment 516182 [details] [diff] [review]
v1

Please address Tim's comments.
Attachment #516182 - Flags: review?(ian)
Attached patch v2 (obsolete) — — Splinter Review 
(In reply to comment #26)
> I unfortunately don't think that this will solve it. As you see in the log
> above browser_tabview_launch.js keeps running in the background while
> "_multiwindow_search.js" and "_undo_group.js" and "_privatebrowsing.js" are
> firing events.
> 
> 1.) I think we should definitely use registerCleanupFunction() to remove our
> listeners and hide the tabview (etc.) when our test times out.

Added registerCleanupFunction()

> 
> 2.) The log shows that the "tabviewshown" event is fired but the condition
> "deck.selectedIndex == 1" never becomes true. That seems to become true when
> the tabview is shown in "_multiwindow_search.js".

I read the log but I can't see that the "tabviewshown" event is fired (onTabViewLoadedAndShown is called) and the condition "deck.selectedIndex == 1" never becomes true. May be I am missing something?

> 
> I can't really explain why this isn't working but is .doCommand() really
> reliable? Though that seems to work because of the "tabviewshown" event. But
> then really selectedIndex should become "1"?!

As mentioned above, I can't see the "tabviewshown" event is fired after .doCommand().  I've switched the selectedIndex to selectedPanel and hope it would make a difference.  

If doCommand is not reliable, I am not sure what we can do to fix it.  The "Browser:ToggleTabView" command element is simply called Tabview.toggle() and we are pretty sure that TabView.toggle() works fine because we use it in most of our tests.
Attachment #516182 - Attachment is obsolete: true
Attachment #516848 - Flags: review?(ian)
Attachment #516848 - Flags: feedback?(tim.taubert)
Attachment #516848 - Attachment is patch: true
Attachment #516848 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 516848 [details] [diff] [review]
v2

(In reply to comment #28)
> Added registerCleanupFunction()

Please use that to cancel the poll timeout, too. Otherwise the tests are run concurrently as shown below.

> > 2.) The log shows that the "tabviewshown" event is fired but the condition
> > "deck.selectedIndex == 1" never becomes true. That seems to become true when
> > the tabview is shown in "_multiwindow_search.js".
> 
> I read the log but I can't see that the "tabviewshown" event is fired
> (onTabViewLoadedAndShown is called) and the condition "deck.selectedIndex == 1"
> never becomes true. May be I am missing something?

Have a look at http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1299371110.1299372254.10283.gz#err2

The test keeps polling here: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/tabview/browser_tabview_launch.js#40

browser_tabview_launch.js times out and continues to poll. deck.selectedIndex becomes "1" when the tabview gets shown in browser_tabview_multiwindow_search.js:

TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_multiwindow_search.js | Tab View is hidden
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is visible. Count: 0

> If doCommand is not reliable, I am not sure what we can do to fix it.  The
> "Browser:ToggleTabView" command element is simply called Tabview.toggle() and
> we are pretty sure that TabView.toggle() works fine because we use it in most
> of our tests.

You're right. IMHO we could safely use TabView.toggle() instead of doCommand(). I think that could solve the problem - I have no idea what else could cause it.
Attachment #516848 - Flags: feedback?(tim.taubert) → feedback-
Comment on attachment 516848 [details] [diff] [review]
v2

Unflagging myself until Tim's new comments are addressed.
Attachment #516848 - Flags: review?(ian)
(In reply to comment #30)
> Have a look at
> http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1299371110.1299372254.10283.gz#err2
> 
> The test keeps polling here:
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/tabview/browser_tabview_launch.js#40
> 
> browser_tabview_launch.js times out and continues to poll. deck.selectedIndex
> becomes "1" when the tabview gets shown in
> browser_tabview_multiwindow_search.js:
> 
> TEST-PASS |
> chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_multiwindow_search.js
> | Tab View is hidden
> TEST-PASS |
> chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js
> | Tab View is visible. Count: 0
> 

Yes but we can't tell whether the poll has started in the _launch.js or not.  In the _launch.js, a "tabviewshown" event listener is added to call onTabViewLoadedAndShown().  In the _multiwindow_search.js, TabView.toggle() is called which can either trigger the "tabviewshown" code in the _launch.js or as you said the polling code which is already running.  But there is not way we can tell whether the polling code has started or not.
Attached patch v3 (obsolete) — — Splinter Review 
Added code to clear the timer and use TabView.toggle() instead.
Assignee: nobody → raymond
Attachment #516848 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #517676 - Flags: review?(ian)
(In reply to comment #32)
> Yes but we can't tell whether the poll has started in the _launch.js or not. 
> In the _launch.js, a "tabviewshown" event listener is added to call
> onTabViewLoadedAndShown().  In the _multiwindow_search.js, TabView.toggle() is
> called which can either trigger the "tabviewshown" code in the _launch.js or as
> you said the polling code which is already running.  But there is not way we
> can tell whether the polling code has started or not.

Sorry, you're right. But thanks for making sure both ways don't lead to cascading failures :)
Comment on attachment 517676 [details] [diff] [review]
v3

Looks good, thanks!
Attachment #517676 - Flags: review?(ian) → review+
Keywords: checkin-needed
No longer blocks: 585689
bugspam
http://hg.mozilla.org/mozilla-central/rev/86aa49121031
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox4.2
Target Milestone: Firefox5 → Firefox 5
Filed bug 657219 to investigate timeout of browser_tabview_undo_group.js (this one's for browser_tabview_launch.js).
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [orange]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: