Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
498633003:
Cache pending JS bridge sync IPC replies, and send in case of RenderFrame deletion. (Closed)
|
Created:
6 years, 4 months ago by benm (inactive) Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mnaganov (inactive) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionCache pending JS bridge sync IPC replies, and send in case of RenderFrame deletion.
When the WebView app makes a call to java over the JavaScriptBridge,
we leave the renderer hanging on a synchronous IPC. Once control
is passed into Java, it's possible that the WebView may get destroyed
(and thus the IPC channel back to the renderer closed) which means we
can't unblock the renderer waiting on the IPC response.
Instead we cache the IPC reply message and while waiting on Java to come
back to us, if we detect that the RenderFrame has been deleted, send a
reponse back before the IPC channel is closed.
BUG=408188
Committed: https://crrev.com/48334b78f249e571cba998184146045114d46ce0
Cr-Commit-Position: refs/heads/master@{#292631}
Patch Set 1 #Patch Set 2 : remove logging #
Total comments: 13
Patch Set 3 : comments and a test #
Total comments: 18
Patch Set 4 : Factor out reply_msg param where possible #
Total comments: 2
Patch Set 5 : remove sendreply #
Total comments: 1
Patch Set 6 : nits #
Total comments: 1
Patch Set 7 : spelling #
Messages
Total messages: 26 (0 generated)
ptal
today I learned all javascript bridge methods are synchronous ipc to UI thread :( :( :( https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:58: GinJavaBridgeDispatcherHost::~GinJavaBridgeDispatcherHost() { Is pending_replies_ guaranteed to be empty here? If so DCHECK, if not, should wake up those render frames like in RenderFrameDeleted https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:401: pending_replies_[render_frame_host] = reply_msg; DCHECK(pending_replies_.find(render_frame_host) == end()) and below https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:510: DCHECK_CURRENTLY_ON(BrowserThread::UI); nit: general chrome style is do the shorter branch first as an early out, then no need to add indent, so... if (!IsValidRenderFrameHost(render_frame_host)) { DCHECK(...); SendReply(...); return; } // RFH valid here. https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:531: IPC::WriteParam(reply_msg, result->GetInvocationError()); Erase rfh from pending_replies_ in this branch, and also DCHECK it's there in the map https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:537: SendReply(render_frame_host, reply_msg); SendReply will just delete the message if rfh is invalid, so that's not different. I guess that's intended and you just wanted to merge the code paths? I say don't bother, just add the DCHECK :D https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.h (right): https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.h:121: std::map<RenderFrameHost*, IPC::Message*> pending_replies_; nit, typedef the type to save typing
Looks good, thanks for looking into this! (although, please don't wait for a formal approval, as I don't check mail often here) Sorry for missing this use case in the first place. I think it's worth adding a test for it, maybe as a separate patch. Bo, we have to use synchronous requests for cases when we need to "pause" JS execution before getting a reply from the browser side. https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:81: IPC::WriteParam(reply_msg, kGinJavaBridgeObjectIsGone); Maybe, add a specific error code / message for this case?
On 2014/08/24 13:31:50, mnaganov_OOO_until_Sep_1st wrote: > Looks good, thanks for looking into this! (although, please don't wait for a > formal approval, as I don't check mail often here) > > Sorry for missing this use case in the first place. I think it's worth adding a > test for it, maybe as a separate patch. > > Bo, we have to use synchronous requests for cases when we need to "pause" JS > execution before getting a reply from the browser side. Yeah I understand it's required to be synchronous by the API. That just means it's a bad API. It's not the first or the worst bad API we inherited from classic webview. Now get back to your vacation and stop reading work email!! :p > > https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... > File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): > > https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... > content/browser/android/java/gin_java_bridge_dispatcher_host.cc:81: > IPC::WriteParam(reply_msg, kGinJavaBridgeObjectIsGone); > Maybe, add a specific error code / message for this case?
thanks all, new ps coming but trying to write a test too. https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:81: IPC::WriteParam(reply_msg, kGinJavaBridgeObjectIsGone); On 2014/08/24 13:31:49, mnaganov_OOO_until_Sep_1st wrote: > Maybe, add a specific error code / message for this case? Done. https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:401: pending_replies_[render_frame_host] = reply_msg; On 2014/08/22 21:05:17, boliu wrote: > DCHECK(pending_replies_.find(render_frame_host) == end()) > > and below good call. https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:510: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2014/08/22 21:05:17, boliu wrote: > nit: general chrome style is do the shorter branch first as an early out, then > no need to add indent, so... > > if (!IsValidRenderFrameHost(render_frame_host)) { > DCHECK(...); > SendReply(...); > return; > } > > // RFH valid here. > Done. https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:531: IPC::WriteParam(reply_msg, result->GetInvocationError()); On 2014/08/22 21:05:17, boliu wrote: > Erase rfh from pending_replies_ in this branch, and also DCHECK it's there in > the map We do that in SendReply already. https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:537: SendReply(render_frame_host, reply_msg); On 2014/08/22 21:05:17, boliu wrote: > SendReply will just delete the message if rfh is invalid, so that's not > different. I guess that's intended and you just wanted to merge the code paths? > I say don't bother, just add the DCHECK :D Hold up; we shouldn't get into the case now where deleting the message is required: we should send it in all cases (either as a consequence of RFDeleted, or the normal SendReply flow. But yeah, you're right we can just remove the SendReply now I refactored the short path up tho the top. https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.h (right): https://codereview.chromium.org/498633003/diff/20001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.h:121: std::map<RenderFrameHost*, IPC::Message*> pending_replies_; On 2014/08/22 21:05:17, boliu wrote: > nit, typedef the type to save typing Done.
ptal
only looked at host https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:376: DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end()); delete reply_msg here I think? https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:520: DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end()); delete reply_msg?
https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:376: DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end()); On 2014/08/27 17:47:34, boliu wrote: > delete reply_msg here I think? I don't think so, as in this case we should have sent the reply already in RenderFrameDeleted. https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:520: DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end()); On 2014/08/27 17:47:34, boliu wrote: > delete reply_msg? same reply as above.
https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java (right): https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java:28: @Feature({"AndroidWebView", "Android-JavaBridge"}) I think features are supposed to match crbug labels, but I don't know if anyone actually cares that much https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:376: DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end()); On 2014/08/27 17:53:34, benm wrote: > On 2014/08/27 17:47:34, boliu wrote: > > delete reply_msg here I think? > > I don't think so, as in this case we should have sent the reply already in > RenderFrameDeleted. Hmm? I mean |reply_msg| argument to this function, not what's in |pending_replies_|.
https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:376: DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end()); On 2014/08/27 17:55:56, boliu wrote: > On 2014/08/27 17:53:34, benm wrote: > > On 2014/08/27 17:47:34, boliu wrote: > > > delete reply_msg here I think? > > > > I don't think so, as in this case we should have sent the reply already in > > RenderFrameDeleted. > > Hmm? I mean |reply_msg| argument to this function, not what's in > |pending_replies_|. They are one and the same I believe. the param here is put into the callback (e.g. see line 412)
https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java (right): https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java:32: final AwTestContainerView view2 = createAwTestContainerViewOnMainSync(client2); what's with 2s everywhere? https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java:48: HTML, "text/html", false); I think you can load a new page with a different title, then check the title at the end. https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java:50: throw new RuntimeException(t); Wot? https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:376: DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end()); On 2014/08/27 17:57:50, benm wrote: > On 2014/08/27 17:55:56, boliu wrote: > > On 2014/08/27 17:53:34, benm wrote: > > > On 2014/08/27 17:47:34, boliu wrote: > > > > delete reply_msg here I think? > > > > > > I don't think so, as in this case we should have sent the reply already in > > > RenderFrameDeleted. > > > > Hmm? I mean |reply_msg| argument to this function, not what's in > > |pending_replies_|. > > They are one and the same I believe. the param here is put into the callback > (e.g. see line 412) Oh god. That's just freaking confusing. That means SendReply, and probably a bunch of other places no longer needs to take the reply_msg arg.
https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java (right): https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java:28: @Feature({"AndroidWebView", "Android-JavaBridge"}) On 2014/08/27 17:55:56, boliu wrote: > I think features are supposed to match crbug labels, but I don't know if anyone > actually cares that much Oh, hm. I just used the ones we've added for all the other java bridge tests... https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java:32: final AwTestContainerView view2 = createAwTestContainerViewOnMainSync(client2); On 2014/08/27 18:23:48, boliu wrote: > what's with 2s everywhere? Well, I want another one ;-) https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java:48: HTML, "text/html", false); On 2014/08/27 18:23:48, boliu wrote: > I think you can load a new page with a different title, then check the title at > the end. I thought that was discouraged and that callback helper was the right way to do it? I just want to show that the second webview is alive even though we killed the first one from the callback. https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java:50: throw new RuntimeException(t); On 2014/08/27 18:23:48, boliu wrote: > Wot? Gotta catch something, and in that case I want to fail the test, rethrowing as an unchecked exception makes that work ... https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/498633003/diff/40001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:376: DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end()); Mmm, yes that's true. We can always get it from the map when we want it.
https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java (right): https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java:28: @Feature({"AndroidWebView", "Android-JavaBridge"}) On 2014/08/27 18:30:02, benm wrote: > On 2014/08/27 17:55:56, boliu wrote: > > I think features are supposed to match crbug labels, but I don't know if > anyone > > actually cares that much > > Oh, hm. I just used the ones we've added for all the other java bridge tests... Firmly in the nobody cares camp then. Consistency always trumps everything else. https://codereview.chromium.org/498633003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java:32: final AwTestContainerView view2 = createAwTestContainerViewOnMainSync(client2); On 2014/08/27 18:30:02, benm wrote: > On 2014/08/27 18:23:48, boliu wrote: > > what's with 2s everywhere? > > Well, I want another one ;-) Ohh...the first one is from the base class. Ok, test looks good then
ptal!
https://codereview.chromium.org/498633003/diff/60001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/498633003/diff/60001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:370: IPC::Message* reply_msg) { Still don't like this one. I think the erase should be moved to callers. Also erase should come before Send which deletes the message, so |pending_replies_| holds pointers to real objects at all times. https://codereview.chromium.org/498633003/diff/60001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:562: IPC::Message* GinJavaBridgeDispatcherHost::GetPendingReply( I think this should be split. Have a bool HasPendingReply(...) const that can be used in DCHECKs etc. Then also have a IPC::Message* TakePendingReply which DCHECKs it's in the map, and also does the erase before it returns. Works better with the above suggestion as well.
thanks Bo! On 2014/08/27 19:07:42, boliu wrote: > https://codereview.chromium.org/498633003/diff/60001/content/browser/android/... > File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): > > https://codereview.chromium.org/498633003/diff/60001/content/browser/android/... > content/browser/android/java/gin_java_bridge_dispatcher_host.cc:370: > IPC::Message* reply_msg) { > Still don't like this one. Why? :-/ Each of the callers of SendReply manipulates the message that's going to be sent in some unique way so I'm not sure how we can have a general SendReply method without passing in the parameter. Are you proposing removing SendReply entirely and having each of the callers deal with the message themselves? > > I think the erase should be moved to callers. > > Also erase should come before Send which deletes the message, so > |pending_replies_| holds pointers to real objects at all times. That's a fair comment; and I like the Take.. method you propose dealing with it. > > https://codereview.chromium.org/498633003/diff/60001/content/browser/android/... > content/browser/android/java/gin_java_bridge_dispatcher_host.cc:562: > IPC::Message* GinJavaBridgeDispatcherHost::GetPendingReply( > I think this should be split. > > Have a bool HasPendingReply(...) const that can be used in DCHECKs etc. > > Then also have a IPC::Message* TakePendingReply which DCHECKs it's in the map, > and also does the erase before it returns. > > Works better with the above suggestion as well.
On 2014/08/27 21:25:46, benm wrote: > thanks Bo! > > On 2014/08/27 19:07:42, boliu wrote: > > > https://codereview.chromium.org/498633003/diff/60001/content/browser/android/... > > File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): > > > > > https://codereview.chromium.org/498633003/diff/60001/content/browser/android/... > > content/browser/android/java/gin_java_bridge_dispatcher_host.cc:370: > > IPC::Message* reply_msg) { > > Still don't like this one. > > Why? :-/ Each of the callers of SendReply manipulates the message that's going > to be sent in some unique way so I'm not sure how we can have a general > SendReply method without passing in the parameter. Are you proposing removing > SendReply entirely and having each of the callers deal with the message > themselves? Exactly, there is nothing to common to factor out for SendReply imo, except the DCHECKs. > > > > > I think the erase should be moved to callers. > > > > Also erase should come before Send which deletes the message, so > > |pending_replies_| holds pointers to real objects at all times. > > That's a fair comment; and I like the Take.. method you propose dealing with it. > > > > > > https://codereview.chromium.org/498633003/diff/60001/content/browser/android/... > > content/browser/android/java/gin_java_bridge_dispatcher_host.cc:562: > > IPC::Message* GinJavaBridgeDispatcherHost::GetPendingReply( > > I think this should be split. > > > > Have a bool HasPendingReply(...) const that can be used in DCHECKs etc. > > > > Then also have a IPC::Message* TakePendingReply which DCHECKs it's in the map, > > and also does the erase before it returns. > > > > Works better with the above suggestion as well.
OK, cool. Just wanted to check we were on the same page :-)
ptal at ps5
lgtm % last comment https://codereview.chromium.org/498633003/diff/80001/content/browser/android/... File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right): https://codereview.chromium.org/498633003/diff/80001/content/browser/android/... content/browser/android/java/gin_java_bridge_dispatcher_host.cc:383: DCHECK(!HasPendingReply(render_frame_host));; repeated semi colon in all of these DCHECKs
thanks bo! torne, can you take a look please as you are an owner :-)
lgtm https://codereview.chromium.org/498633003/diff/100001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java (right): https://codereview.chromium.org/498633003/diff/100001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java:46: // leave others dunctioning. spelling
The CQ bit was checked by benm@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benm@chromium.org/498633003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 5d3e001f79c137b2ba0f5b0e9489abea616f3431
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/48334b78f249e571cba998184146045114d46ce0 Cr-Commit-Position: refs/heads/master@{#292631} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
