Last Comment Bug 790198 - Android NPAPI extensions leak Java references
: Android NPAPI extensions leak Java references
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Plugins (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 18
Assigned To: Santtu Lakkala
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-11 05:15 PDT by Santtu Lakkala
Modified: 2012-09-26 17:13 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch that deletes the local references (1.13 KB, patch)
2012-09-11 05:19 PDT, Santtu Lakkala
snorp: review+
bugmail.mozilla: feedback+
Details | Diff | Review

Description Santtu Lakkala 2012-09-11 05:15:42 PDT
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120825192204

Steps to reproduce:

Created an android NPAPI plugin and a continuous stress tester page that causes kANPGetValue and ANPSystemInterface.loadJavaClass to be called multiple times.


Actual results:

Eventually Fennec died with a message in system log about Java Local Reference table running out, the table is filled with java.lang.Class and java.lang.String objects.


Expected results:

The test page should run ad infinitum.
Comment 1 Santtu Lakkala 2012-09-11 05:19:04 PDT
Created attachment 660041 [details] [diff] [review]
Patch that deletes the local references

Normally Java Local references are cleaned up automatically, but it seems that it is not happening here (probably due to Java VM being constantly being called "from the outside", instead of just implementing Java native methods.
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-09-24 14:57:37 PDT
James, can you take a look at this patch?

Santtu, here is a guid on how to submit a patch: https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2012-09-24 15:06:01 PDT
Comment on attachment 660041 [details] [diff] [review]
Patch that deletes the local references

Review of attachment 660041 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch! This patch looks acceptable to me but I'd prefer if we updated this code to use mozilla::AutoLocalJNIFrame instead. It looks like some of the other plugin code (e.g. dom/plugins/base/android/ANPAudio.cpp) uses that already.
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-09-24 15:12:07 PDT
Santtu, can you do that?
Comment 5 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-09-24 17:56:23 PDT
Comment on attachment 660041 [details] [diff] [review]
Patch that deletes the local references

Review of attachment 660041 [details] [diff] [review]:
-----------------------------------------------------------------

Kats is right, this is better solved with AutoLocalJNIFrame. If you look around in AndroidBridge.cpp you can find examples of how it gets used. The main reason this is better is because we don't have to worry about deleting every local reference individually.

Great job on discovering the issue and coming up with a good patch! Looking forward to r+ing the next one.
Comment 6 Santtu Lakkala 2012-09-24 23:51:16 PDT
Comment on attachment 660041 [details] [diff] [review]
Patch that deletes the local references

Review of attachment 660041 [details] [diff] [review]:
-----------------------------------------------------------------

If I understand AutoLocalJNIFrame (and JNI Push/PopLocalFrame) correctly, there is no way to "leak" one local ref (unless it is created before or independently after all the ones to be cleaned), which is something both of these methods need to do. And as android WebKit does not use global references here, I guess fennec can't either.

IMO this is still prettier than AutoFrame - NewGlobalRef - PopFrame - NewLocalRef - DeleteGlobalRef (granted this dance would add certain robustness, but especially in kJavaContext it would be 5 lines of code vs 1.
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2012-09-25 05:50:01 PDT
I haven't looked into the full context of how these functions are used but the "leaked" local ref (by which I assume you mmean the function return value) still does need to be freed at some point. In general, for situations like this, it might be better to create the AutoLocalJNIFrame in the caller code, and pass it in to this function. As long as the scope of the AutoLocalJNIFrame encompasses the scope of that returned java object there should be no need to do a NewGlobalRef. If the function is called in such a way that the return value is propagated far and wide then perhaps a NewGlobalRef is a good idea anyway for robustness.
Comment 8 Santtu Lakkala 2012-09-25 06:13:04 PDT
Yes, the return value was what I meant.

The caller code would be an NPAPI plugin, so there's not much possibility of changing things (or enforcing either) there. Also as this is android-specific code, I believe that fennec should not diverse from android "reference implementation" for these API extensions.
Comment 9 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-09-25 06:26:29 PDT
Yeah, I wanted to see a NewGlobalRef there for the return value. Santtu is right, though, Android WebKit doesn't return a global ref so I'd assume the plugin expects a local one (ugh). In that case the original patch looks fine.
Comment 10 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-09-26 07:24:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/490abcc57fec
Comment 11 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-09-26 07:27:21 PDT
Congratulations on your first patch, Santtu. It has been checked into the 'inbound' branch, and will be merged to mozilla-central later today most likely. The fix will then appear in the 18.0 release. Thanks a lot!
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-09-26 17:13:59 PDT
https://hg.mozilla.org/mozilla-central/rev/490abcc57fec

Note You need to log in before you can comment on or make changes to this bug.