Android NPAPI extensions leak Java references

RESOLVED FIXED in Firefox 18



Firefox for Android
5 years ago
5 years ago


(Reporter: Santtu Lakkala, Assigned: Santtu Lakkala)


Firefox 18
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(1 attachment)



5 years ago
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

5 years ago
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.


5 years ago
Component: General → Plugins
OS: Linux → Android
Product: Fennec → Firefox for Android
Hardware: x86_64 → ARM
James, can you take a look at this patch?

Santtu, here is a guid on how to submit a patch:
Ever confirmed: true


5 years ago
Attachment #660041 - Flags: review?(snorp)
Attachment #660041 - Flags: feedback?(bugmail.mozilla)
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.
Attachment #660041 - Flags: feedback?(bugmail.mozilla) → feedback+
Santtu, can you do that?
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.
Attachment #660041 - Flags: review?(snorp) → review-

Comment 6

5 years ago
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.
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

5 years ago
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.
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.
Attachment #660041 - Flags: review- → review+
Keywords: checkin-needed
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!
Assignee: nobody → inz
Keywords: checkin-needed
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
You need to log in before you can comment on or make changes to this bug.