Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11359143:
Make sure Java Bridge cleans up all stub object it creates. (Closed)
|
Created:
8 years, 1 month ago by acleung Modified:
8 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake sure Java Bridge cleans up all stub object it creates.
I have been trying to see why the JavaBridgeChannelHost, which is reference counted, sticks around forever.
Here is what I found:
When talk back is enabled. Each JavaBridgeChannelHost typically have 5 references to it. 3 of them should be cleaned up by this patch. (see comments in the .cc file)
BUG=157699
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170694
Patch Set 1 #
Total comments: 6
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : Don't pass by copy to bind() #
Total comments: 4
Patch Set 4 : Fix crash from bad rebase #
Messages
Total messages: 20 (0 generated)
+jam as I think he did most the previous JavaBridge reviews. This change seems to tally with how WebPluginDelegateStub uses NPObjectStub, so lgtm. https://codereview.chromium.org/11359143/diff/1/content/browser/renderer_host... File content/browser/renderer_host/java/java_bridge_dispatcher_host.h (right): https://codereview.chromium.org/11359143/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bridge_dispatcher_host.h:66: static void CleanUpStubs(std::vector<base::WeakPtr<NPObjectStub> > stubs_); as this is a helper for the d'tor, no need for it to live on the class -- I'd just put this into the .cc in an anonymous namespace. Also I think |stubs| can be a const reference? (Note - no underscore in param name)
I'm not familiar with the java version, so hard for me to comment in detail. seems reasonable
https://codereview.chromium.org/11359143/diff/1/content/browser/renderer_host... File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/11359143/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:159: std::vector<base::WeakPtr<NPObjectStub> > stubs) { are you sure you want to pass by value here? https://codereview.chromium.org/11359143/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:161: if (stubs[i]) { nit: indentation
https://codereview.chromium.org/11359143/diff/1/content/browser/renderer_host... File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/11359143/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:159: std::vector<base::WeakPtr<NPObjectStub> > stubs) { On 2012/11/15 21:08:59, darin wrote: > are you sure you want to pass by value here? I thought about that a bit. If I pass by reference, CleanUpStubs can be called when this host is gone. Am I correct? https://codereview.chromium.org/11359143/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:161: if (stubs[i]) { On 2012/11/15 21:08:59, darin wrote: > nit: indentation Done. https://codereview.chromium.org/11359143/diff/1/content/browser/renderer_host... File content/browser/renderer_host/java/java_bridge_dispatcher_host.h (right): https://codereview.chromium.org/11359143/diff/1/content/browser/renderer_host... content/browser/renderer_host/java/java_bridge_dispatcher_host.h:66: static void CleanUpStubs(std::vector<base::WeakPtr<NPObjectStub> > stubs_); On 2012/11/14 00:51:27, joth wrote: > as this is a helper for the d'tor, no need for it to live on the class -- I'd > just put this into the .cc in an anonymous namespace. > Also I think |stubs| can be a const reference? > (Note - no underscore in param name) Great idea.
https://codereview.chromium.org/11359143/diff/5001/content/browser/renderer_h... File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/11359143/diff/5001/content/browser/renderer_h... content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:54: base::Bind(&CleanUpStubs, stubs_)); under the covers, Bind copy-constructs a copy of all params passed and keeps them around until the bound method was run. So it's that copy you'll be taking a reference to in the function param. You can convince yourself of this either by logging the address of stubs_ and seeing they're different instances, or by digging through the docs on Bind. :)
https://codereview.chromium.org/11359143/diff/5001/content/browser/renderer_h... File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/11359143/diff/5001/content/browser/renderer_h... content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:54: base::Bind(&CleanUpStubs, stubs_)); On 2012/11/15 23:29:07, joth wrote: > under the covers, Bind copy-constructs a copy of all params passed and keeps > them around until the bound method was run. So it's that copy you'll be taking a > reference to in the function param. > You can convince yourself of this either by logging the address of stubs_ and > seeing they're different instances, or by digging through the docs on Bind. :) Checked the docs. You are absolutely right.
On 2012/11/16 02:07:18, acleung wrote: > https://codereview.chromium.org/11359143/diff/5001/content/browser/renderer_h... > File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right): > > https://codereview.chromium.org/11359143/diff/5001/content/browser/renderer_h... > content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:54: > base::Bind(&CleanUpStubs, stubs_)); > On 2012/11/15 23:29:07, joth wrote: > > under the covers, Bind copy-constructs a copy of all params passed and keeps > > them around until the bound method was run. So it's that copy you'll be taking > a > > reference to in the function param. > > You can convince yourself of this either by logging the address of stubs_ and > > seeing they're different instances, or by digging through the docs on Bind. :) > > Checked the docs. You are absolutely right. Could you guys take another look? Anything else?
adding steveblock as he's back online now and may still have some memory of this. But, still, lgtm
Perhaps I'm imagining this, but I thought there was logic in the channel whereby if it detects that a renderer is dead (no response to pings?) it tears itself down gracefully. If not, how do other users of channel get around this problem? https://codereview.chromium.org/11359143/diff/10001/content/browser/renderer_... File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/11359143/diff/10001/content/browser/renderer_... content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:36: stubs[i]->DeleteSoon(); Is there any possibility that this deletion will run before the NPObjectProxy tries to delete these objects (via the channel)? If so, will this cause problems with double-deletion? https://codereview.chromium.org/11359143/diff/10001/content/browser/renderer_... content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:150: // we cannot guaranteed that the renderer always terminates cleanly s/guaranteed/guarantee
It doesn't seem to be that way. Multiple renderer share the same channel and they come and go. As far as I know the channel doesn't know there is no one waiting anymore. Other clients use AddRoute and RemoveRoute to reference count the channel. According to the documentation, NPObjects stubs and proxy are explicitly not taken into account for reference counting because are they prone to leaks (may be renderer crashes?) https://codereview.chromium.org/11359143/diff/10001/content/browser/renderer_... File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/11359143/diff/10001/content/browser/renderer_... content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:36: stubs[i]->DeleteSoon(); On 2012/11/27 05:04:33, Steve Block wrote: > Is there any possibility that this deletion will run before the NPObjectProxy > tries to delete these objects (via the channel)? If so, will this cause problems > with double-deletion? Hmm. The dispatcher gets deleted when the renderer is gone. I am guessing that everything neither cleanly existed (which the stubs are deleted already) or we have a unclean exist. Which requires this step. https://codereview.chromium.org/11359143/diff/10001/content/browser/renderer_... content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:150: // we cannot guaranteed that the renderer always terminates cleanly On 2012/11/27 05:04:33, Steve Block wrote: > s/guaranteed/guarantee Done.
LGTM Thanks for double-checking
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@google.com/11359143/10001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@google.com/11359143/10001
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is android_dbg, revision is HEAD
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@google.com/11359143/10001
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is android_dbg, revision is HEAD
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@google.com/11359143/10001
Commit queue patch verification failed without an error message. Something went wrong, probably a crash, a hickup or simply the monkeys went out for dinner. Please email commit-bot@chromium.org with the CL url.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@google.com/11359143/7010
Message was sent while issue was closed.
Change committed as 170694 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
