31824 2009-11-24 03:24:59 -0800 [Android] Upstream Android changes to WebCore/bridge/jni 2009-11-25 03:25:25 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Other Other RESOLVED FIXED P2 Normal --- 1 benm benm andersca android-webkit-unforking darin eric jens kdecker oldest_to_newest 166027 0 benm 2009-11-24 03:24:59 -0800 The Android team has made some edits in the WebCore/bridge/jni directory and we would like to contribute them back upstream. 166037 1 43759 benm 2009-11-24 05:00:21 -0800 Created attachment 43759 Patch 166044 2 benm 2009-11-24 05:41:51 -0800 > Index: WebCore/bridge/jni/jni_instance.h > =================================================================== > --- WebCore/bridge/jni/jni_instance.h (revision 51337) > +++ WebCore/bridge/jni/jni_instance.h (working copy) > @@ -47,6 +47,10 @@ friend class JavaField; > friend class JavaInstance; > friend class JavaMethod; > > +public: > + jobject instance() const { return _instance; } > + void setInstance(jobject instance) { _instance = instance; } > + > protected: > JObjectWrapper(jobject instance); > ~JObjectWrapper(); > @@ -89,12 +93,10 @@ public: > JSValue booleanValue() const; > > protected: > + JavaInstance(jobject instance, PassRefPtr<RootObject>); > virtual void virtualBegin(); > virtual void virtualEnd(); > > -private: > - JavaInstance(jobject instance, PassRefPtr<RootObject>); > - > RefPtr<JObjectWrapper> _instance; > mutable JavaClass *_class; > }; FYI, a little background for this change that I can add to the ChangeLog on landing: In WebKit/android (to be upstreamed later) we have a class that inherits from JavaInstance and calls it's parent class constructor. That's why I have moved the constructor from private to protected. Also within that class, we access the internals of the _instance member which is why I added a getter/setter to JObjectWrapper. 166133 3 43759 eric 2009-11-24 10:07:33 -0800 Comment on attachment 43759 Patch Looks sane enough, but as you note in the bug, the ChangeLog could be improved. Going to CC some folks who may have worked on this code. 166134 4 43759 eric 2009-11-24 10:09:27 -0800 Comment on attachment 43759 Patch So were these leaks before? Should the leak bot have been able to detect these? Or does this just make GC easier for the Java VM? 166195 5 benm 2009-11-24 11:26:06 -0800 (In reply to comment #4) > (From update of attachment 43759 [details]) > So were these leaks before? Should the leak bot have been able to detect > these? Or does this just make GC easier for the Java VM? DeleteLocalRef is a hint to the garbage collector (like setting a local variable to null in java), and these were potential leaks before. Normally local references are automatically collected when they go out of scope (i.e. when we return to Java) and this is likely the case now and is why the leak bots detect nothing. But as this is inside a constructor, we have no guarantees where it is being called from. Consider the case that the objects were allocated in a loop - without a call to DeleteLocalRef the references would be leaked until we return to Java, which is a concern on devices with limited memory. 166198 6 eric 2009-11-24 11:32:01 -0800 So these were found by inspection? or due to OOM crashes on android in certain uses? I'm ready to r+ this patch, although it would be best if we had a way to test it. Sadly Java is disabled in DumpRenderTree and although we could turn it on on a per-test basis with layoutTestController.overridePreference(), there may be a reason why Java is off that I don't know of. 166199 7 43759 eric 2009-11-24 11:33:49 -0800 Comment on attachment 43759 Patch These could have been made separate changes. But overall this change looks fine, and there are certainly enough Java experts CC'd on this bug to correct me if I'm wrong. :) I see you listed in committers.py, so you can either commit this yourself or cq+ it yourself. Thanks for the patch! 166206 8 benm 2009-11-24 11:46:36 -0800 (In reply to comment #6) > So these were found by inspection? or due to OOM crashes on android in certain > uses? By inspection, it was added as a safety mechanism. I will land this manually in the morning (night time here :)) so I can update the changelog comments a little and watch the build bots properly. Thanks! 166207 9 43759 benm 2009-11-24 11:47:07 -0800 Comment on attachment 43759 Patch cq- for manual landing. 166209 10 eric 2009-11-24 11:47:55 -0800 Thanks! 166401 11 benm 2009-11-25 03:25:25 -0800 Landed as r51379. 43759 2009-11-24 05:00:21 -0800 2009-11-24 11:47:07 -0800 Patch bridge-jni-patch.txt text/plain 4145 benm SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1MTMzNykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjMgQEAKKzIwMDktMTEtMjQgIEJlbiBNdXJkb2NoICA8YmVubUBnb29nbGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtBbmRy b2lkXSBVcHN0cmVhbSBBbmRyb2lkIGNoYW5nZXMgdG8gV2ViQ29yZS9icmlkZ2Uvam5pCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMTgyNAorCisgICAg ICAgIE5vIG5ldyB0ZXN0cyByZXF1aXJlZCBhcyBubyBuZXcgZnVuY3Rpb25hbGl0eS4KKworICAg ICAgICAqIGJyaWRnZS9qbmkvam5pX2NsYXNzLmNwcDoKKyAgICAgICAgKEphdmFDbGFzczo6SmF2 YUNsYXNzKTogQWRkIGNhbGxzIHRvIGRlbGV0ZSBhbGxvY2F0ZWQgcmVmZXJlbmNlcy4KKyAgICAg ICAgKiBicmlkZ2Uvam5pL2puaV9pbnN0YW5jZS5jcHA6CisgICAgICAgICogYnJpZGdlL2puaS9q bmlfaW5zdGFuY2UuaDogQWRkIGdldHRlci9zZXR0ZXIgZm9yIEpPYmplY3RXcmFwcGVyOjpfaW5z dGFuY2UKKyAgICAgICAgKEpTQzo6QmluZGluZ3M6OkpPYmplY3RXcmFwcGVyOjppbnN0YW5jZSk6 IEFkZGVkLgorICAgICAgICAoSlNDOjpCaW5kaW5nczo6Sk9iamVjdFdyYXBwZXI6OnNldEluc3Rh bmNlKTogQWRkZWQuCisgICAgICAgICogYnJpZGdlL2puaS9qbmlfcnVudGltZS5jcHA6CisgICAg ICAgIChKYXZhTWV0aG9kOjpKYXZhTWV0aG9kKTogRGVsZXRlIGFuIGFsbG9jYXRlZCByZWZlcmVu Y2UuCisgICAgICAgICogYnJpZGdlL2puaS9qbmlfdXRpbGl0eS5oOgorICAgICAgICAoSlNDOjpC aW5kaW5nczo6Y2FsbEpOSU1ldGhvZFYpOiBEZWxldGUgYW4gYWxsb2NhdGVkIHJlZmVyZW5jZS4K KwogMjAwOS0xMS0yNCAgTWFyayBSb3dlICA8bXJvd2VAYXBwbGUuY29tPgogCiAgICAgICAgIEZp eCBwcm9kdWN0aW9uIGJ1aWxkcyB3aGVyZSB0aGUgc291cmNlIHRyZWUgbWF5IGJlIHJlYWQtb25s eS4KSW5kZXg6IFdlYkNvcmUvYnJpZGdlL2puaS9qbmlfY2xhc3MuY3BwCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFdlYkNvcmUvYnJpZGdlL2puaS9qbmlfY2xhc3MuY3BwCShyZXZpc2lvbiA1MTMzNykKKysrIFdl YkNvcmUvYnJpZGdlL2puaS9qbmlfY2xhc3MuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC04NSw2ICs4 NSwxMCBAQCBKYXZhQ2xhc3M6OkphdmFDbGFzcyhqb2JqZWN0IGFuSW5zdGFuY2UpCiAgICAgICAg IG1ldGhvZExpc3QtPmFwcGVuZChhTWV0aG9kKTsKICAgICAgICAgZW52LT5EZWxldGVMb2NhbFJl ZihhSk1ldGhvZCk7CiAgICAgfSAgICAKKworICAgIGVudi0+RGVsZXRlTG9jYWxSZWYoZmllbGRz KTsKKyAgICBlbnYtPkRlbGV0ZUxvY2FsUmVmKG1ldGhvZHMpOworICAgIGVudi0+RGVsZXRlTG9j YWxSZWYoYUNsYXNzKTsKIH0KIAogSmF2YUNsYXNzOjp+SmF2YUNsYXNzKCkgewpJbmRleDogV2Vi Q29yZS9icmlkZ2Uvam5pL2puaV9pbnN0YW5jZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9i cmlkZ2Uvam5pL2puaV9pbnN0YW5jZS5jcHAJKHJldmlzaW9uIDUxMzM3KQorKysgV2ViQ29yZS9i cmlkZ2Uvam5pL2puaV9pbnN0YW5jZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTM3LDYgKzM3LDEw IEBACiAjaW5jbHVkZSA8cnVudGltZS9FcnJvci5oPgogI2luY2x1ZGUgPHJ1bnRpbWUvSlNMb2Nr Lmg+CiAKKyNpZiBQTEFURk9STShBTkRST0lEKQorI2luY2x1ZGUgPGFzc2VydC5oPgorI2VuZGlm CisKICNpZmRlZiBOREVCVUcKICNkZWZpbmUgSlNfTE9HKGZvcm1hdEFuZEFyZ3MuLi4pICgodm9p ZCkwKQogI2Vsc2UKSW5kZXg6IFdlYkNvcmUvYnJpZGdlL2puaS9qbmlfaW5zdGFuY2UuaAo9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Ci0tLSBXZWJDb3JlL2JyaWRnZS9qbmkvam5pX2luc3RhbmNlLmgJKHJldmlzaW9uIDUx MzM3KQorKysgV2ViQ29yZS9icmlkZ2Uvam5pL2puaV9pbnN0YW5jZS5oCSh3b3JraW5nIGNvcHkp CkBAIC00Nyw2ICs0NywxMCBAQCBmcmllbmQgY2xhc3MgSmF2YUZpZWxkOwogZnJpZW5kIGNsYXNz IEphdmFJbnN0YW5jZTsKIGZyaWVuZCBjbGFzcyBKYXZhTWV0aG9kOwogCitwdWJsaWM6CisgICAg am9iamVjdCBpbnN0YW5jZSgpIGNvbnN0IHsgcmV0dXJuIF9pbnN0YW5jZTsgfQorICAgIHZvaWQg c2V0SW5zdGFuY2Uoam9iamVjdCBpbnN0YW5jZSkgeyBfaW5zdGFuY2UgPSBpbnN0YW5jZTsgfQor CiBwcm90ZWN0ZWQ6CiAgICAgSk9iamVjdFdyYXBwZXIoam9iamVjdCBpbnN0YW5jZSk7ICAgIAog ICAgIH5KT2JqZWN0V3JhcHBlcigpOwpAQCAtODksMTIgKzkzLDEwIEBAIHB1YmxpYzoKICAgICBK U1ZhbHVlIGJvb2xlYW5WYWx1ZSgpIGNvbnN0OwogCiBwcm90ZWN0ZWQ6CisgICAgSmF2YUluc3Rh bmNlKGpvYmplY3QgaW5zdGFuY2UsIFBhc3NSZWZQdHI8Um9vdE9iamVjdD4pOwogICAgIHZpcnR1 YWwgdm9pZCB2aXJ0dWFsQmVnaW4oKTsKICAgICB2aXJ0dWFsIHZvaWQgdmlydHVhbEVuZCgpOwog Ci1wcml2YXRlOgotICAgIEphdmFJbnN0YW5jZShqb2JqZWN0IGluc3RhbmNlLCBQYXNzUmVmUHRy PFJvb3RPYmplY3Q+KTsKLQogICAgIFJlZlB0cjxKT2JqZWN0V3JhcHBlcj4gX2luc3RhbmNlOwog ICAgIG11dGFibGUgSmF2YUNsYXNzICpfY2xhc3M7CiB9OwpJbmRleDogV2ViQ29yZS9icmlkZ2Uv am5pL2puaV9ydW50aW1lLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2JyaWRnZS9qbmkvam5p X3J1bnRpbWUuY3BwCShyZXZpc2lvbiA1MTMzNykKKysrIFdlYkNvcmUvYnJpZGdlL2puaS9qbmlf cnVudGltZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTI4NCw2ICsyODQsNyBAQCBKYXZhTWV0aG9k OjpKYXZhTWV0aG9kIChKTklFbnYgKmVudiwgam9iCiAgICAgamNsYXNzIG1vZGlmaWVyQ2xhc3Mg PSBlbnYtPkZpbmRDbGFzcygiamF2YS9sYW5nL3JlZmxlY3QvTW9kaWZpZXIiKTsKICAgICBpbnQg bW9kaWZpZXJzID0gY2FsbEpOSU1ldGhvZDxqaW50PihhTWV0aG9kLCAiZ2V0TW9kaWZpZXJzIiwg IigpSSIpOwogICAgIF9pc1N0YXRpYyA9IChib29sKWNhbGxKTklTdGF0aWNNZXRob2Q8amJvb2xl YW4+KG1vZGlmaWVyQ2xhc3MsICJpc1N0YXRpYyIsICIoSSlaIiwgbW9kaWZpZXJzKTsKKyAgICBl bnYtPkRlbGV0ZUxvY2FsUmVmKG1vZGlmaWVyQ2xhc3MpOwogfQogCiBKYXZhTWV0aG9kOjp+SmF2 YU1ldGhvZCgpIApJbmRleDogV2ViQ29yZS9icmlkZ2Uvam5pL2puaV91dGlsaXR5LmgKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gV2ViQ29yZS9icmlkZ2Uvam5pL2puaV91dGlsaXR5LmgJKHJldmlzaW9uIDUxMzM3 KQorKysgV2ViQ29yZS9icmlkZ2Uvam5pL2puaV91dGlsaXR5LmgJKHdvcmtpbmcgY29weSkKQEAg LTIxNiw2ICsyMTYsOCBAQCBzdGF0aWMgVCBjYWxsSk5JTWV0aG9kVihqb2JqZWN0IG9iaiwgY29u CiAgICAgICAgICAgICBqbWV0aG9kSUQgbWlkID0gZW52LT5HZXRNZXRob2RJRChjbHMsIG5hbWUs IHNpZyk7CiAgICAgICAgICAgICBpZiAoIG1pZCAhPSBOVUxMICkKICAgICAgICAgICAgIHsKKyAg ICAgICAgICAgICAgICAvLyBBdm9pZHMgcmVmZXJlbmNlcyB0byBjbHMgd2l0aG91dCBwb3BwaW5n IHRoZSBsb2NhbCBmcmFtZS4KKyAgICAgICAgICAgICAgICBlbnYtPkRlbGV0ZUxvY2FsUmVmKGNs cyk7CiAgICAgICAgICAgICAgICAgcmV0dXJuIEpOSUNhbGxlcjxUPjo6Y2FsbFYob2JqLCBtaWQs IGFyZ3MpOwogICAgICAgICAgICAgfQogICAgICAgICAgICAgZWxzZQo=