Memory errors when releasing JSAPI object with registered event handlers

[GoogleCodeExporter]

This problem was reported to me by a MacOSX developer.
It produces a crash report on and Safari.
Since I do not have Mac at hand I was able to reproduce it with valgrind 
and firefox.

How to reproduce:
1. Build FBTestPlugin from examples and put it to firefox plugins directory
2. Create an HTML page that loads the plugin:
   <object id="plugin" type="application/x-fbtestplugin" width="300" 
height="300"></object>
3. Register a dummy event handler:
   document.getElementById('plugin').addEventListener('load', 
function(test) { alert(test); });
3. Run firefox in valgrind:
   valgrind --trace-children=yes firefox test.html
4. Hit reload a couple of times.

The following error appears:

==11918== Invalid read of size 4
==11918==    at 0x3D7102C3A9: ??? (in /lib64/libnspr4.so)
==11918==    by 0x1F5714AD: 
FB::Npapi::NpapiBrowserHost::ReleaseObject(NPObject*) (in 
/home/anttix/.mozilla/plugins/npFBTestPlugin.so)
==11918==    by 0x1F57535B: FB::Npapi::NPObjectAPI::~NPObjectAPI() (in 
/home/anttix/.mozilla/plugins/npFBTestPlugin.so)
==11918==    by 0x1F5753F1: FB::Npapi::NPObjectAPI::~NPObjectAPI() (in 
/home/anttix/.mozilla/plugins/npFBTestPlugin.so)
==11918==    by 0x1F57F952: FB::JSAPI::Release() (in 
/home/anttix/.mozilla/plugins/npFBTestPlugin.so)
==11918==    by 0x1F560617: 
FB::AutoPtr<FB::BrowserObjectAPI>::Assign(FB::BrowserObjectAPI*) (in 
/home/anttix/.mozilla/plugins/npFBTestPlugin.so)
==11918==    by 0x1F55EF54: FB::AutoPtr<FB::BrowserObjectAPI>::~AutoPtr() 
(in /home/anttix/.mozilla/plugins/npFBTestPlugin.so)
==11918==    by 0x1F580350: std::pair<std::string const, 
FB::AutoPtr<FB::BrowserObjectAPI> >::~pair() (in 
/home/anttix/.mozilla/plugins/npFBTestPlugin.so)
==11918==    by 0x1F582467: __gnu_cxx::new_allocator<std::pair<std::string 
const, FB::AutoPtr<FB::BrowserObjectAPI> > >::destroy(std::pair<std::string 
const, FB::AutoPtr<FB::BrowserObjectAPI> >*) (in 
/home/anttix/.mozilla/plugins/npFBTestPlugin.so)
==11918==    by 0x1F581404: std::_Rb_tree<std::string, 
std::pair<std::string const, FB::AutoPtr<FB::BrowserObjectAPI> >, 
std::_Select1st<std::pair<std::string const, 
FB::AutoPtr<FB::BrowserObjectAPI> > >, std::less<std::string>, 
std::allocator<std::pair<std::string const, 
FB::AutoPtr<FB::BrowserObjectAPI> > > 
>::_M_destroy_node(std::_Rb_tree_node<std::pair<std::string const, 
FB::AutoPtr<FB::BrowserObjectAPI> > >*) (in 
/home/anttix/.mozilla/plugins/npFBTestPlugin.so)
==11918==    by 0x1F580AD0: std::_Rb_tree<std::string, 
std::pair<std::string const, FB::AutoPtr<FB::BrowserObjectAPI> >, 
std::_Select1st<std::pair<std::string const, 
FB::AutoPtr<FB::BrowserObjectAPI> > >, std::less<std::string>, 
std::allocator<std::pair<std::string const, 
FB::AutoPtr<FB::BrowserObjectAPI> > > 
>::_M_erase(std::_Rb_tree_node<std::pair<std::string const, 
FB::AutoPtr<FB::BrowserObjectAPI> > >*) (in 
/home/anttix/.mozilla/plugins/npFBTestPlugin.so)
==11918==    by 0x1F5803E6: std::_Rb_tree<std::string, 
std::pair<std::string const, FB::AutoPtr<FB::BrowserObjectAPI> >, 
std::_Select1st<std::pair<std::string const, 
FB::AutoPtr<FB::BrowserObjectAPI> > >, std::less<std::string>, 
std::allocator<std::pair<std::string const, 
FB::AutoPtr<FB::BrowserObjectAPI> > > >::~_Rb_tree() (in 
/home/anttix/.mozilla/plugins/npFBTestPlugin.so)
==11918==  Address 0x1664c8f8 is 8 bytes inside a block of size 32 free'd
==11918==    at 0x4A04A84: operator delete(void*) (vg_replace_malloc.c:346)
==11918==    by 0x3F16B4D332: ??? (in /usr/lib64/xulrunner-1.9.1/libxul.so)
==11918==    by 0x3F16C8F8D6: ??? (in /usr/lib64/xulrunner-1.9.1/libxul.so)
==11918==    by 0x3F16B4D8CF: ??? (in /usr/lib64/xulrunner-1.9.1/libxul.so)
==11918==    by 0x3F16B3B684: ??? (in /usr/lib64/xulrunner-1.9.1/libxul.so)
==11918==    by 0x3F1665AC93: ??? (in /usr/lib64/xulrunner-1.9.1/libxul.so)
==11918==    by 0x3F1665B03C: ??? (in /usr/lib64/xulrunner-1.9.1/libxul.so)
==11918==    by 0x3F16CC1676: ??? (in /usr/lib64/xulrunner-1.9.1/libxul.so)
==11918==    by 0x3F16C9501C: ??? (in /usr/lib64/xulrunner-1.9.1/libxul.so)
==11918==    by 0x3F16C13F80: ??? (in /usr/lib64/xulrunner-1.9.1/libxul.so)
==11918==    by 0x3F16AD794F: ??? (in /usr/lib64/xulrunner-1.9.1/libxul.so)
==11918==    by 0x3F16471651: XRE_main (in /usr/lib64/xulrunner-
1.9.1/libxul.so)
==11918== 

I think the problem is that neither NPJavascriptObject::callSetEventListener
nor JSAPI::registerEventMethod will call RetainObject before pushing
event handler JSObject to eventMap for storage.

Original issue reported on code.google.com by ant...@gmail.com on 11 Feb 2010 at 10:29

[GoogleCodeExporter]

Having debugged it for half a day I can now say that Firefox 3.5 is buggy: 
reference 
counting is off by one.

Here are the results of a small debugging patch and runs with different 
browsers on 
my Fedora 12 machine. During a test run, the browser was launched and then 
immediately closed with CTRL+W (close tab) command.

I'll contact my Mac deveoper friend to verify if this bug is present in Safari 
as 
well or if it's a firefox specific quirk.

Anyway, I think we should look deeper into those Retain/Release logs to catch a 
number of possible memory leaks.

Original comment by ant...@gmail.com on 11 Feb 2010 at 3:34

Attachments:

• debug-retainobject.patch
• test.html
• firefox.txt
• chrome.txt
• arora.txt

[GoogleCodeExporter]

I assume by now that you have found that the NPObjectAPI object does a Retain 
and a
Release on shutdown.

I have heard  report that the same thing was happening on Safari 64 bit;  The 
thing
is, if it happens on both Safari and Firefox, that makes it sound like there 
could
actually be a problem with what we're doing, but I can't figure out what, since 
it
does indeed seem like we're doing what we should.

Good work on this; let me know how I can help.

Original comment by taxilian on 11 Feb 2010 at 4:34

[GoogleCodeExporter]

See also issue 26; I think these are related.  If it's a browser bug, though, 
I'm not
sure how to fix it except to do a second retain on each one.

Original comment by taxilian on 11 Feb 2010 at 8:02

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Well it seems that issue 26 is not the same, at least double retain didn't make 
the
crashes go away. I'll assume for now that the current issue is firefox specific 
and
firefox code has a habit of checking if the reference counts is greater than 
zero
before actually freeing the object so normal malloc sanity checks (built into 
glibc
for instance) do not catch those reference counting bugs. I'll post a bug 
report to
mozilla's bugtracker when I can allocate some free time for this :P

Original comment by ant...@gmail.com on 15 Feb 2010 at 3:00

[GoogleCodeExporter]

Can anyone reproduce this with the latest version?  If someone can give me 
instructions to reproduce it, I'll look into fixing it.

Original comment by taxilian on 9 Sep 2010 at 2:06

[GoogleCodeExporter]

I think this issue has probably been fixed, but I don't want to mark it so 
until someone who has seen it can confirm that it is gone.

Original comment by taxilian on 15 Sep 2010 at 7:22

• Changed state: Recheck

[GoogleCodeExporter]

Making a little headway; this issue, 22, and 69 are the same problem.

Original comment by taxilian on 20 Sep 2010 at 3:57

• Changed state: Duplicate