Skip to content

Conversation

@Teun
Copy link
Contributor

@Teun Teun commented Jan 14, 2015

@kevingy ,
Please review. If it is OK, you can pull and I will create a new version on nuget later today.

Reintroduced static ClearWindows
Had to rename instance scoped ClearWindows to ClearWindowsInContext
Added unit tests

Had to rename instance scoped ClearWindows to ClearWindowsInContext
Added unit tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add the removal of the local copies
Maybe ClearWindowsInContext() or similar

@iEmiya
Copy link
Contributor

iEmiya commented Jan 14, 2015

Added fixes to correct (# 120 fix)
I think this will be enough to put the API intact and prevent memory leaks.

I need about two days to test solutions to combat servers and validation.

@iEmiya
Copy link
Contributor

iEmiya commented Jan 20, 2015

A simple test for memory leaks.

        [Test]
        public void MemoryLeak()
        {
            // look : exception
            long? useTotalMemory = null;
            var task = System.Threading.Tasks.Task.Factory.StartNew(() =>
            {
                long threadId = 1;
                while (true)
                {
                    long totalMemory = GC.GetTotalMemory(false);
                    Trace.WriteLine(string.Format("Use total memory: {0:#,##0} byte(s)", totalMemory));
                    if (threadId % 20 == 0)
                    {
                        if (!useTotalMemory.HasValue) useTotalMemory = totalMemory;
                        else
                        {
                            if (totalMemory > useTotalMemory) throw new ApplicationException("Memory leak in 'SimpleBrowser'");
                            break; // Ok
                        }
                    }


                    try
                    {
                        threadId++;
                        Browser b1 = new Browser(Helper.GetFramesMock());
                        b1.Navigate("http://localhost/");
                        if (threadId % 2 == 0) throw new ApplicationException();
                        b1.Close();

                        GC.WaitForPendingFinalizers();
                        GC.Collect();
                    }
                    catch (Exception e)
                    {
                        Trace.Fail("Failed to navigate");
                    }
                }
            });

            task.Wait();
        }

@kevingy
Copy link
Contributor

kevingy commented Jan 21, 2015

I suggest checking in the change that brings back the static CloseWindows then open a separate issue for the memory leak clean-up. It will be at least two weeks before I can concentrate on looking for the memory leak. At least with the static CloseWindows back in place, SimpleBrowser users won't have to change their API usage.

@Teun
Copy link
Contributor Author

Teun commented Jan 21, 2015

I agree. Pull the request. I will push the build to nuget once it builds.

Teun
Op 21 jan. 2015 04:21 schreef "Kevin Yochum" [email protected]:

I suggest checking in the change that brings back the static CloseWindows
then open a separate issue for the memory leak clean-up. It will be at
least two weeks before I can concentrate on looking for the memory leak. At
least with the static CloseWindows back in place, SimpleBrowser users won't
have to change their API usage.


Reply to this email directly or view it on GitHub
#120 (comment).

kevingy added a commit that referenced this pull request Jan 29, 2015
Restore backward compatibility on ClearWindows
@kevingy kevingy merged commit a04043e into master Jan 29, 2015
@kevingy kevingy mentioned this pull request Jan 29, 2015
@kevingy kevingy deleted the backward-compatibility-restore branch August 29, 2019 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants