Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11802002:
[Android] Fix leak in Java Bridge. (Closed)
|
Created:
7 years, 11 months ago by benm (inactive) Modified:
7 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, sgurun-gerrit only Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Android] Fix leak in Java Bridge.
First draft at moving the strong reference native side into a
weak reference and maintaining the strong reference on the
java side.
Android only change, android bots green.
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176079
Patch Set 1 #
Total comments: 16
Patch Set 2 : Comments. #
Total comments: 4
Patch Set 3 : Add set to keep objects alive until page navigation. #
Total comments: 7
Patch Set 4 : joth comments #
Messages
Total messages: 11 (0 generated)
first shot ... ptal
+alan ptal
seems reasonable. Main thing this will need is plenty of commenting :-) https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/java/java_bound_object.cc (right): https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.cc:591: result.l = JavaBoundObject::GetJavaObject(object); this may now be NULL. Any idea what that might break? https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.cc:826: DCHECK(!obj.is_null()); this could happen though. If the java side ophen's its WebView and associated tree of java objects, they will get GC'ed but the native side won't get told until some time after. (CleanupReference is async in nature) Ah... the assumption is EnsureMethodsAreSetUp() is done synchronously in a setup flow, where it would be impossible for for a java-side GC to have already released these guys? Hmmm guess so but definitely add a comment here explaining it, to discourage anyone from copying this DCHECK(!null) pattern anywhere else. https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/java/java_bound_object.h (right): https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.h:41: // Gets a local ref to the underlying JavaObject from a JavaBoundObject actually, whether it's local or global is not so important as whether the caller should take ownership for releasing it. I guess returning ScopedJavaLocalRef would make it clearer. https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.h:42: // wrapped as an NPObject. Mention it may return NULL too, if the native side was GCed. https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.h:55: \n nit https://codereview.chromium.org/11802002/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/11802002/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:78: private Map<String, Object> mJavaScriptObjects = new HashMap<String, Object>(); comment what this is for / why the native side only holds weak refs etc. https://codereview.chromium.org/11802002/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2213: nativeRemoveJavascriptInterface(mNativeContentViewCore, name); I'd rather nativeRemoveJavascriptInterface returned the jobject that corresponded to |object| originally passed into nativeAddJavascriptInterface so then we could just remove that from a 'set' here (to avoid the duplicate name->object maps on both sides). But a cursory read of the native-side code, I can't easily see how it could do that: I think it just sends IPC to renderer, and only when that is responded to does it remove the browser side object containing the weak ref. we definitely should remove the java-side object synchronously like you are here (and be aware that this means we may open a window when the weak ref will be referring to NULL) so using the name->object map here as you have it is probably best choice.
thanks joth, fixed all but one comment, that one's a bit hairy I think :/ https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/java/java_bound_object.cc (right): https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.cc:591: result.l = JavaBoundObject::GetJavaObject(object); On 2013/01/07 19:32:41, joth wrote: > this may now be NULL. Any idea what that might break? I think this is for the case that during a JS -> java method call, you pass a JS object that's really a Java object as a parameter. So now that parameter could be passed as null. Hrm, maybe this CL isn't right then. If we had the following JavaScript: window.jsbridge.foo(window.jsbridge.getBar()); with jsbridge defined in java: class jsbridge { public void foo(Object o) { //assume o is not null... } public Object getBar() { return new Object(); } } Then we'll only have a weakref native side to the getBar() object and so it could get GC'd before we receive it into the NewLocalRef here in java_bound_object.cc for the call to foo(), right? And in the case that that happens, the foo function will now start crashing (or not, depending on GC). Does that make sense? https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.cc:826: DCHECK(!obj.is_null()); No, I think your first comment is correct. EnsureMethodsAreSetup is performed lazily so I think that we could end up in the situation you describe - a null obj here. I think in that case we just want to return without adding anything into methods_. https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/java/java_bound_object.h (right): https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.h:41: // Gets a local ref to the underlying JavaObject from a JavaBoundObject Will change to SJLR, and add back the NewLocalRef in the caller. https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.h:42: // wrapped as an NPObject. On 2013/01/07 19:32:41, joth wrote: > Mention it may return NULL too, if the native side was GCed. Done. https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.h:55: On 2013/01/07 19:32:41, joth wrote: > \n nit Done. https://codereview.chromium.org/11802002/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/11802002/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:78: private Map<String, Object> mJavaScriptObjects = new HashMap<String, Object>(); On 2013/01/07 19:32:41, joth wrote: > comment what this is for / why the native side only holds weak refs etc. Done.
lgtm - we should probably add a Set<Object> as a followup. (if not immediately, open a bug for it.) https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/java/java_bound_object.cc (right): https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.cc:591: result.l = JavaBoundObject::GetJavaObject(object); On 2013/01/08 14:43:05, benm wrote: > On 2013/01/07 19:32:41, joth wrote: > > this may now be NULL. Any idea what that might break? > > I think this is for the case that during a JS -> java method call, you pass a JS > object that's really a Java object as a parameter. So now that parameter could > be passed as null. > > Hrm, maybe this CL isn't right then. If we had the following JavaScript: > > window.jsbridge.foo(window.jsbridge.getBar()); > > with jsbridge defined in java: > > class jsbridge { > public void foo(Object o) { > //assume o is not null... > } > > public Object getBar() { > return new Object(); > } > } > > Then we'll only have a weakref native side to the getBar() object and so it > could get GC'd before we receive it into the NewLocalRef here in > java_bound_object.cc for the call to foo(), right? And in the case that that > happens, the foo function will now start crashing (or not, depending on GC). > > Does that make sense? Yes. I chatted with sgurun and looks like classic didn't cope with that case ether. However in https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... you'll see there's both mJavaScriptObjects (serving same purpose as your field of same name) but also a 'set' of objects bound to the current page (including those that have been removed from the JS API). Adding this set would be the logical place to stash a hard ref to any object we received here (as unlike the primary API objects, these should not persist across navigations). we pass a jweak that refers to the Set<Object> which the implementation can just call add() on here as needed. Hmmmm that might be sweet. Call it the "bound java object retaining set" or something. JavaBridgeDispatcherHostManager is a WebContents observer, so it knows when page navigations occur and could automatically clear() the set as needed. Suggest opening a new crbug to track that.... https://codereview.chromium.org/11802002/diff/10001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/11802002/diff/10001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2148: public void addJavascriptInterface(Object object, String name, boolean requireAnnotation) { you can delete this now. https://codereview.chromium.org/11802002/diff/10001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2220: public void removeJavascriptInterface(String name) { I think we might as well remove it here even if the native pointer is 0. (or, even better, we could do mJavaScriptObjects.clear() in CVC.destroy())
thanks joth! https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/java/java_bound_object.cc (right): https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.cc:591: result.l = JavaBoundObject::GetJavaObject(object); Happy to add this in a follow up change but just want to make sure I understand fully... 1. Add a Set<Object> in CVC that keeps tack of removed JS interfaces and is cleared on page navigation (actually that feels like it should be part of this patch as I think as it stands this change will potentially break the case that you remove a JS interface but expect to still be able to call methods on it until the page is navigated) 2. Pass a weak ref to that set to native (somehow, via addJavascriptInterface perhaps?) so that in java_bound_object.cc we can add any object returned from java to that set, thus keeping it alive and fixing the issue described in my comment above. I guess we'd do that add() in JavaBoundObject::CallJNIMethod() when the return type is a JavaType::Object? (line 211 in this file?) 3. I can imagine this set might become quite large for apps that make heavy use of the java bridge, potentially unnecessarily keeping objects returned from java alive until the next page navigation. Should we be more proactive about clearing it up? Can we be? I think I'll try to add 1. in this patch, and if I've understood right file a bug and do 2/3 in the follow up patch. https://codereview.chromium.org/11802002/diff/10001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/11802002/diff/10001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2148: public void addJavascriptInterface(Object object, String name, boolean requireAnnotation) { On 2013/01/09 00:28:22, joth wrote: > you can delete this now. Done. https://codereview.chromium.org/11802002/diff/10001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2220: public void removeJavascriptInterface(String name) { On 2013/01/09 00:28:22, joth wrote: > I think we might as well remove it here even if the native pointer is 0. > > (or, even better, we could do mJavaScriptObjects.clear() in CVC.destroy()) Done.
https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/java/java_bound_object.cc (right): https://codereview.chromium.org/11802002/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bound_object.cc:591: result.l = JavaBoundObject::GetJavaObject(object); For 3, I guess we could remove from the set in JavaNPObject::Deallocate. (line 86)
ace. lgtm. I think my remaining comments amount to TODO/doc/bug updates rather than code changes. https://codereview.chromium.org/11802002/diff/18001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/11802002/diff/18001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:90: private Map<String, Object> mJavaScriptObjects = new HashMap<String, Object>(); naming nits: while it's slightly handy now having the same name here as in classic, I think long term having clearer names will be better. (and, especially if we slightly change purposes of the set as discussed below). Suggest mJavaScriptInterfaces[Map] for this, as this it the map from named API (interface) to the object that implements it. Something like mRetainedJavaScript[Objects|InterfaceInstances] or mLiveJavaScriptObjectRetainingSet for the set. https://codereview.chromium.org/11802002/diff/18001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:94: // they aren't garbage collected before the page is navigated. aside - with the future changes discussed this comment will need update, I was going to suggest making it more general now but I think it would be too vague? ("Additionally, we keep track of all JS interface objects in use on the current page, to ensure they remain live so long as JavaScript maybe holding a reference to them. This covers the case of the interface being removed while in use on the page, or transient Java objects returned from methods on a primary interface object. (TODO: implement transient object retention; move the management of this set into the JavaScript bridge)." https://codereview.chromium.org/11802002/diff/18001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:516: mRemovedJavaScriptObjects.clear(); this definitely seems suspect - if the load fails then the page remains un-navigated, yet the JSI will be half dead. in classic they used "frame cleared" event which makes sense, I can't easily see what that maps to web contents observer as. Maybe see if mksoiba knows, or trace it through. my guess would be DidNavigateMainFrame (conversely that maybe too late...) BUT see also my comment below -- if we can move all management of this Set into the java_bound_object.cc and friends, then I think this clear() should become redundent. The NPObject should always naturally get cleared up at the correct point on navigation (c.f. flash plugin is released when you navigate off youtube) so this shotgun clearing of objects should then be redundant. a TODO here and/or below (and/or details on the new bug) should be fine to capture this though. https://codereview.chromium.org/11802002/diff/18001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2224: mRemovedJavaScriptObjects.add(mJavaScriptObjects.get(name)); my preference would be to eventually move this into java_bound_object.cc. (and the clear() call too, perhaps). i.e. java_bound_object.cc would be responsible to put any object into the Set whenever it is accessed by JS (regardless of whether it's a primary AJI api object, or if its a object returned from an object), and remove it from the set in JavaNPObject::Deallocate or when the frame is cleared (navigated).
thanks joth! I filed https://code.google.com/p/chromium/issues/detail?id=169228 to track moveing the set native side and no functional change in this patch. So if the bots are happy I will land. https://codereview.chromium.org/11802002/diff/18001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/11802002/diff/18001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:90: private Map<String, Object> mJavaScriptObjects = new HashMap<String, Object>(); On 2013/01/09 20:53:22, joth wrote: > naming nits: while it's slightly handy now having the same name here as in > classic, I think long term having clearer names will be better. (and, especially > if we slightly change purposes of the set as discussed below). > Suggest > mJavaScriptInterfaces[Map] for this, as this it the map from named API > (interface) to the object that implements it. > Something like mRetainedJavaScript[Objects|InterfaceInstances] or > mLiveJavaScriptObjectRetainingSet for the set. Done. https://codereview.chromium.org/11802002/diff/18001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:94: // they aren't garbage collected before the page is navigated. On 2013/01/09 20:53:22, joth wrote: > aside - with the future changes discussed this comment will need update, I was > going to suggest making it more general now but I think it would be too vague? > ("Additionally, we keep track of all JS interface objects in use on the current > page, to ensure they remain live so long as JavaScript maybe holding a reference > to them. This covers the case of the interface being removed while in use on the > page, or transient Java objects returned from methods on a primary interface > object. (TODO: implement transient object retention; move the management of > this set into the JavaScript bridge)." Done. https://codereview.chromium.org/11802002/diff/18001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2224: mRemovedJavaScriptObjects.add(mJavaScriptObjects.get(name)); Agreed. Filed crbug/169228
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benm@chromium.org/11802002/19004
Message was sent while issue was closed.
Change committed as 176079 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
