[#SRC-371] reuseBrowserSession broken during deadlock patch

Selenium Remote Control

reuseBrowserSession broken during deadlock patch

Created: 19/Oct/07 06:47 PM Updated: 05/Mar/08 01:49 AM

Component/s:

None

Affects Version/s:

1.0

Fix Version/s:

1.0

Original Estimate:	Unknown	Remaining Estimate:	Unknown	Time Spent:	Unknown
File Attachments:	1. jbevan-src371.patch (49 kb) 2. jbevan-src371.patch (44 kb)

Description

« Hide

endSession is sort-of doing the right thing by resetting the session instead of closing it, but getNewBrowserSession no longer attempts to get sessions from the cache of unused sessions, so this will just leave lots of open sessions lying around.

[The old unused browser session scheme wasn't very good anyway, because you couldn't use it with non-PI sessions. This is ~~SRC-341~~; to fix it, we'd need to track which sessions have been used with which domains and only reuse a session if the starting domain matches (or if the launcher is multi-domain compatible, e.g. *chrome or *iehta).]

All

Comments

Work Log

Change History

Sort Order:

[ Permlink | « Hide ]

Jennifer Bevan - 06/Dec/07 10:26 AM

Hey -- I'll be taking a look at this soon. Just to clarify, you don't want the sessions to just be dereferenced for the garbage collector, but instead to be kept and reused like a ThreadPool?

[ Permlink | « Hide ]

Dan Fabulich - 06/Dec/07 10:30 AM

Not just the session objects, but the browser sessions themselves. If reuseBrowserSession is enabled, the browser should never close until the server shuts down.

[ Permlink | « Hide ]

Jennifer Bevan - 14/Jan/08 12:55 PM

I have a working patch for this -- care to review it? I extracted the session handling code out of SeleniumDriverResourceHandler and into "BrowserSessionFactory".

I don't think that any of the functional test suite (which no longer runs by default between rev 2102 and 2116?) had any reuseBrowserSessions, but the unit tests work and I'm looking into adding more functional tests.

Attaching the patch to the issue...
-Jen

[ Permlink | « Hide ]

Dan Fabulich - 04/Mar/08 03:59 AM

we've got to fix this for 1.0

[ Permlink | « Hide ]

Jennifer Bevan - 04/Mar/08 11:14 PM

This patch handles AFAIK all of the issues for ~~SRC-371~~, and is 99% done for ~~SRC-341~~. I'll begin testing on my various configurations tonight, because previously this has worked for firefox but not so much for IE.

There is an outstanding issue around line 818 of FrameGroupCommandQueueSet about what URL to open when resetting (and reusing) a non-PI mode browser. This (last I checked) worked for non-PI *chrome but not for non-PI *iexplore.

There is also an outstanding issue on the number/type of tests. I (or Dan?) need to add some higher-level tests. The unit tests are okay.

[ Permlink | « Hide ]

Dan Fabulich - 05/Mar/08 01:49 AM

Fixed in revision 2200