Reference count issues and memory leaks closing browser

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Generate a basic plugin and do not add custom code (named ie: DummyTest or 
use provided FBTestPlugin example) 
2. Open at least two tabbed web pages and load FBControl.html (or sample web 
page) in one of them=> All works 
fine.
3. Closes browser, two scenarios but the same issue:
    3.1. Closes tabbed window where FBControl.htm (or sample web page used) is loaded.
    3.2. Or do not close tabbed window and close browser clicking on application close button.

What is the expected output? What do you see instead?

At this point I expect that closing tabbed window or closing application all 
destructors (~DummyTest()  and ~DummyTest2API()) will be called, but they 
aren't. 

What version of FireBreath are you using? On what operating system and
browsers?

I'm using FB v1.3.0 on Windows 7 and FireFox v4.0b6, Chrome v7.0.517.14 and IE 
v8.0.7600.16385. I guess that this issue exists in all 1.3.0 and 1.4.0 versions 
(RC or not) 
and probably in all 1.2.x versions too.

Notes:
In scenario 3.1 the close process calls to ~PluginCore() and ~DummyTest() but 
not to the rest of class destructors (API classes derived from FB::JSAPIAuto). 
Investigating about it, I can see that there is an 
extra reference (use_count_= 2 and weak_count_=2) to object m_api at 
PluginCore::~PluginCore() and the object is not deleted. In contrast, the 
example BasicMediaPlayer has not this problem. At PluginCore::~PluginCore() 
there are a reference count with value 
equals to 1 (use_count_= 1 and weak_count_=2) and everything works fine. 

As Richard comments in firebreath-dev google group, this means that we have a 
circular reference somewhere. By now I cannot locate it!

In scenario 3.2 we have the same behavior but using IE we have got a worst 
result, because no destructosr are called. 

For more information you can review discussion on 
http://groups.google.com/group/firebreath-dev/browse_thread/thread/368ee1cbe8adc
74d

Best regards.

Original issue reported on code.google.com by noant...@gmail.com on 8 Nov 2010 at 10:47

[GoogleCodeExporter]

Ahh; I hadn't caught that ~DummyTest wasn't being called either.  That should 
be easier to track down, since there are *very* few places that need to hold a 
reference to the plugin object (it hasn't been a shared_ptr for very long).  
DummyTest is most likely holding the reference to DummyTestAPI, but 
DummyTestAPI should only have a weak_ptr to DummyTest, so that shouldn't be 
keeping it open.

Original comment by taxilian on 8 Nov 2010 at 3:47

• Changed state: Accepted

[GoogleCodeExporter]

Hi taxilian,

I suspected the same and I tried it the DummtTest using a weak pointers, but 
the bug remains.

[code]
class DummyTestAPI : public FB::JSAPIAuto
{
public:
    DummyTestAPI(DummyTestWeakPtr plugin, FB::BrowserHostPtr host);
    virtual ~DummyTestAPI();

    DummyTestPtr getPlugin();

    // ....

private:
    DummyTestWeakPtr m_plugin;
    FB::BrowserHostPtr m_host;

    std::string m_testString;
};
[code]

Best regards.

Original comment by noant...@gmail.com on 9 Nov 2010 at 10:49

[GoogleCodeExporter]

That wouldn't make a difference, since it's storing the reference to the plugin 
in a weak_ptr in the class, so at the end of the constructor the shared_ptr 
releases it.

My current theory is that it has to do with the onload event; still working 
through things.  The async stuff is a little tricky to track what is going on, 
there could be a leak in there that holds a copy of the shared_ptr, since the 
onload event passes a reference to the root JSAPI object.

In my tests, btw, the FBTestPlugin object is getting destroyed; only the 
FBTestPluginAPI is not.  Keep in mind that the lifetime of the two are not 
directly related, they just usually end up going together more or less.

Original comment by taxilian on 9 Nov 2010 at 4:13

[GoogleCodeExporter]

ok, I believe this is fixed in the trunk by: 
http://code.google.com/p/firebreath/source/detail?r=ec716d4e567fc4bc0b6197dbd02b
befb81e302b0

It appears to have been caused by a missing destructor in one of the utility 
classes used by the cross-thread calls.  It was actually a memory leak 
completely unrelated to the API class, but since we pass a reference to the API 
in the onload callback that caused it to leak a reference.  This is actually 
good, because I don't know how else we would ever have caught the leak =]

Original comment by taxilian on 9 Nov 2010 at 5:32

• Changed state: Fixed

[GoogleCodeExporter]

Hmm.  Well, that definitely fixed part of the problem.  Further testing shows 
that IE is still experiencing difficulties.

I'm still looking...

Original comment by taxilian on 9 Nov 2010 at 6:19

• Changed state: Accepted

[GoogleCodeExporter]

The internet explorer issue should be fixed here:

http://code.google.com/p/firebreath/source/detail?r=f62a2df7d6a58efc1e1648501ffd
a38887c829cd

There were several things working into this on IE.  The first of course was the 
leak in the cross-thread stuff. The second is that there was an off-by-1 error 
in the reference counting of the ActiveX object used for passing a JSAPI object 
into the page.

The reason that the destructors weren't being called when the browser quits is 
that the browser aparently isn't properly releasing the activex control in some 
cases.  We "solve" this by simply releasing the plugincore and jsapi objects 
when the browser tells us it will be shutting down so that deinitialization 
stuff can be run.  There is the possibility of a memory leak with the activex 
control itself not getting released, but that will only happen on browser 
shutdown so shouldn't be a big deal as the OS will clean it all up when the 
browser quits.

Give it a test (it's in trunk in source control).

Original comment by taxilian on 9 Nov 2010 at 7:59

• Changed state: Fixed

[GoogleCodeExporter]

Hi, 

Good job, Richard! It seems that works fine.

Best regards!

Original comment by noant...@gmail.com on 11 Nov 2010 at 10:41

[GoogleCodeExporter]

Excelent

Original comment by taxilian on 11 Nov 2010 at 3:58

• Changed state: Verified