Last Comment Bug 790198 - Android NPAPI extensions leak Java references
: Android NPAPI extensions leak Java references
Product: Firefox for Android
Classification: Client Software
Component: Plugins (show other bugs)
: Trunk
: ARM Android
-- normal (vote)
: Firefox 18
Assigned To: Santtu Lakkala
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image 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 User image 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 User image Martijn Wargers [:mwargers] 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:
Comment 3 User image Kartikaya Gupta ( 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 User image Martijn Wargers [:mwargers] 2012-09-24 15:12:07 PDT
Santtu, can you do that?
Comment 5 User image James Willcox (:snorp) ( 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 User image 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 User image Kartikaya Gupta ( 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 User image 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 User image James Willcox (:snorp) ( 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 User image James Willcox (:snorp) ( 2012-09-26 07:24:16 PDT
Comment 11 User image James Willcox (:snorp) ( 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 User image Ryan VanderMeulen [:RyanVM] 2012-09-26 17:13:59 PDT

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