790198 - Android NPAPI extensions leak Java references

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Plugins, defect)

Description

[Santtu Lakkala]

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]

Attachment: 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 (dead)]

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:[e-mail])]

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 (dead)]

Santtu, can you do that?

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

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]

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:[e-mail])]

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]

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) ([e-mail]) (he/him)]

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) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/490abcc57fec

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

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]]

https://hg.mozilla.org/mozilla-central/rev/490abcc57fec