Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349443003:
Listen to didReceiveResponse() to get the response in WebViewPlugin. (Closed)
|
Created:
4 years, 3 months ago by Nate Chapin Modified:
4 years, 3 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionListen to didReceiveResponse() to get the response in WebViewPlugin.
This is a partial revert of the WebViewPlugin changes in
https://crrev.com/8489f1bf78c2c6f296e0df6c529751d29e8a0188
BUG=641028
Committed: https://crrev.com/8ed4bf69eca5e2696aeab731a8e9a15e8f379f2b
Cr-Commit-Position: refs/heads/master@{#419387}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Get rid of the whole response thing #
Total comments: 6
Patch Set 3 : +comment #
Messages
Total messages: 38 (19 generated)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Listen to didReceiveResponse() to get the response in WebViewPlugin. BUG=641028 ========== to ========== Listen to didReceiveResponse() to get the response in WebViewPlugin. This is a partial revert of the WebViewPlugin changes in https://crrev.com/8489f1bf78c2c6f296e0df6c529751d29e8a0188 BUG=641028 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
japhet@chromium.org changed reviewers: + bauerb@chromium.org
bauerb, would you mind reviewing this partial revert? It appears that WebViewPlugin::response_ was not the same as web_frame_->dataSource().response() as I asserted in https://codereview.chromium.org/2211283004. In fact, response_ was never set, because the only user of WebViewPlugin creates it with a data: url, which guarantees that blink never creates a PluginDocument, and therefore no one is sending WebPlugin::didReceiveResponse() callbacks. This patch puts the code as close as possible to the way it was before, pending further diagnostics. I'm suspicious that there's a bunch of dead code in WebViewPlugin that can be deleted, but I wanted to keep this small enough to be a merge candidate. My more invasive experiment is at https://codereview.chromium.org/2344023002/
On 2016/09/15 22:49:39, Nate Chapin wrote: > bauerb, would you mind reviewing this partial revert? > > It appears that WebViewPlugin::response_ was not the same as > web_frame_->dataSource().response() as I asserted in > https://codereview.chromium.org/2211283004. In fact, response_ was never set, > because the only user of WebViewPlugin creates it with a data: url, which > guarantees that blink never creates a PluginDocument, and therefore no one is > sending WebPlugin::didReceiveResponse() callbacks. > > This patch puts the code as close as possible to the way it was before, pending > further diagnostics. I'm suspicious that there's a bunch of dead code in > WebViewPlugin that can be deleted, but I wanted to keep this small enough to be > a merge candidate. My more invasive experiment is at > https://codereview.chromium.org/2344023002/ If this looks ok, please go ahead and CQ it.
tommycli@chromium.org changed reviewers: + tommycli@chromium.org
https://codereview.chromium.org/2349443003/diff/1/components/plugins/renderer... File components/plugins/renderer/webview_plugin.cc (right): https://codereview.chromium.org/2349443003/diff/1/components/plugins/renderer... components/plugins/renderer/webview_plugin.cc:127: if (!response_.isNull()) This line used to be DCHECK(response_.isNull()); Did you mean "response" the function argument instead of the member var?
https://codereview.chromium.org/2349443003/diff/1/components/plugins/renderer... File components/plugins/renderer/webview_plugin.cc (right): https://codereview.chromium.org/2349443003/diff/1/components/plugins/renderer... components/plugins/renderer/webview_plugin.cc:127: if (!response_.isNull()) On 2016/09/15 23:53:35, tommycli wrote: > This line used to be DCHECK(response_.isNull()); > > Did you mean "response" the function argument instead of the member var? Restoring the old DCHECK crashes, so I'm guessing something has changed about how these responses work. The patch as-is works, but I'm not sure, since it would seem that response_.isNull() would be initially true, and the member variable would never be populated.
https://codereview.chromium.org/2349443003/diff/1/components/plugins/renderer... File components/plugins/renderer/webview_plugin.cc (right): https://codereview.chromium.org/2349443003/diff/1/components/plugins/renderer... components/plugins/renderer/webview_plugin.cc:127: if (!response_.isNull()) On 2016/09/16 00:01:38, tommycli wrote: > On 2016/09/15 23:53:35, tommycli wrote: > > This line used to be DCHECK(response_.isNull()); > > > > Did you mean "response" the function argument instead of the member var? > > Restoring the old DCHECK crashes, so I'm guessing something has changed about > how these responses work. Huh, that's strange. Where would the non-null value come from? Is this being called more than once now? > The patch as-is works, but I'm not sure, since it would seem that > response_.isNull() would be initially true, and the member variable would never > be populated.
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2349443003/diff/1/components/plugins/renderer... File components/plugins/renderer/webview_plugin.cc (right): https://codereview.chromium.org/2349443003/diff/1/components/plugins/renderer... components/plugins/renderer/webview_plugin.cc:127: if (!response_.isNull()) On 2016/09/16 10:28:01, Bernhard (slow until Sep 27) wrote: > On 2016/09/16 00:01:38, tommycli wrote: > > On 2016/09/15 23:53:35, tommycli wrote: > > > This line used to be DCHECK(response_.isNull()); > > > > > > Did you mean "response" the function argument instead of the member var? > > > > Restoring the old DCHECK crashes, so I'm guessing something has changed about > > how these responses work. > > Huh, that's strange. Where would the non-null value come from? Is this being > called more than once now? > > > The patch as-is works, but I'm not sure, since it would seem that > > response_.isNull() would be initially true, and the member variable would > never > > be populated. > Yeah this patchset only works accidentally: I inverted the if() statement, and therefore response_ will always be null. To understand why this fixes the issue, let's start at the beginning! While I was working on https://crrev.com/8489f1bf78c2c6f296e0df6c529751d29e8a0188, I ran into a problem: I had changed WebFrameClient::didReceiveResponse() to just have a single const WebURLResponse& parameter. This meant that WebViewPlugin had a diamond inheritance problem, as it implements both WebFrameClient and WebPlugin, and both of these functions now had a didReceiveResponse() callback with identical parameters. I believe the key is that the WebPlugin::didReceiveResponse() callback was *never* getting called for a WebViewPlugin. WebViewPlugin initializes its frame with a data: url, so a PluginDocument isn't created in blink, and without a PluginDocument, there are no WebPlugin::didReceiveResponse() callbacks. But with the diamond inheritance, it was now picking up the response from the WebFrameClient::didReceiveResponse() callbacks, none of which are actually the plugin data. I settled on "fixing" this in https://crrev.com/8489f1bf78c2c6f296e0df6c529751d29e8a0188 by getting an irrelevant WebURLResponse a different way, not realizing that all of WebViewPlugin's response handling is dead code. So I goofed in that patch, and it appears this patchset fixes the issue by introducing different, subtly dead code. I believe the right solution is to simply remove any logic that handles a WebURLResponse. Updated patch to reflect that. There is probably more to remove, but I'll stick to that in this patch.
https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... File components/plugins/renderer/webview_plugin.cc (left): https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... components/plugins/renderer/webview_plugin.cc:99: if (!response.isNull()) { If I understand correctly, WebPlugin::didReceiveResponse() is never called, and WebFrameClient::didReceiveResponse() IS called, but with irrelevant, inappropriate data... I realize there's a hairy diamond inheritance problem -- but can we add an explicit NOTREACHED() for WebPlugin::didReceiveResponse() that won't be triggered by the WebFrameClient::didReceiveResponse()? https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... components/plugins/renderer/webview_plugin.cc:108: UMA_HISTOGRAM_MEMORY_KB( Are these the only places these UMAs are logged? If so, can we remove from histograms.xml also?
https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... File components/plugins/renderer/webview_plugin.cc (left): https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... components/plugins/renderer/webview_plugin.cc:99: if (!response.isNull()) { On 2016/09/16 17:38:03, tommycli wrote: > If I understand correctly, WebPlugin::didReceiveResponse() is never called, and > WebFrameClient::didReceiveResponse() IS called, but with irrelevant, > inappropriate data... Correct. > > I realize there's a hairy diamond inheritance problem -- but can we add an > explicit NOTREACHED() for WebPlugin::didReceiveResponse() that won't be > triggered by the WebFrameClient::didReceiveResponse()? There may be a way, but I'd need to do some research or get some guidance: my C++ knowledge is minimal in this area. https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... components/plugins/renderer/webview_plugin.cc:108: UMA_HISTOGRAM_MEMORY_KB( On 2016/09/16 17:38:03, tommycli wrote: > Are these the only places these UMAs are logged? If so, can we remove from > histograms.xml also? I don't see any related histograms.xml entries?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... File components/plugins/renderer/webview_plugin.cc (left): https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... components/plugins/renderer/webview_plugin.cc:99: if (!response.isNull()) { On 2016/09/16 17:49:40, Nate Chapin wrote: > On 2016/09/16 17:38:03, tommycli wrote: > > If I understand correctly, WebPlugin::didReceiveResponse() is never called, > and > > WebFrameClient::didReceiveResponse() IS called, but with irrelevant, > > inappropriate data... > > Correct. > > > > > I realize there's a hairy diamond inheritance problem -- but can we add an > > explicit NOTREACHED() for WebPlugin::didReceiveResponse() that won't be > > triggered by the WebFrameClient::didReceiveResponse()? > > There may be a way, but I'd need to do some research or get some guidance: my > C++ knowledge is minimal in this area. Okay, here's one suggestion: Make the WebFrameClient portion a delegate. Have WebViewPlugin own a member frame_client_ that implements the interface. Maybe it's an inner class or something. Now WebViewPlugin::didReceiveResponse should only override WebPlugin::didReceiveResponse and you can put NOTREACHED in there. Thanks for investigating, Tommy https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... components/plugins/renderer/webview_plugin.cc:108: UMA_HISTOGRAM_MEMORY_KB( On 2016/09/16 17:49:39, Nate Chapin wrote: > On 2016/09/16 17:38:03, tommycli wrote: > > Are these the only places these UMAs are logged? If so, can we remove from > > histograms.xml also? > > I don't see any related histograms.xml entries? They don't appear under UMA either. These must just be orphans (safe to delete).
On 2016/09/16 18:08:26, tommycli wrote: > https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... > File components/plugins/renderer/webview_plugin.cc (left): > > https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... > components/plugins/renderer/webview_plugin.cc:99: if (!response.isNull()) { > On 2016/09/16 17:49:40, Nate Chapin wrote: > > On 2016/09/16 17:38:03, tommycli wrote: > > > If I understand correctly, WebPlugin::didReceiveResponse() is never called, > > and > > > WebFrameClient::didReceiveResponse() IS called, but with irrelevant, > > > inappropriate data... > > > > Correct. > > > > > > > > I realize there's a hairy diamond inheritance problem -- but can we add an > > > explicit NOTREACHED() for WebPlugin::didReceiveResponse() that won't be > > > triggered by the WebFrameClient::didReceiveResponse()? > > > > There may be a way, but I'd need to do some research or get some guidance: my > > C++ knowledge is minimal in this area. > > Okay, here's one suggestion: > > Make the WebFrameClient portion a delegate. Have WebViewPlugin own a member > frame_client_ that implements the interface. Maybe it's an inner class or > something. > > Now WebViewPlugin::didReceiveResponse should only override > WebPlugin::didReceiveResponse and you can put NOTREACHED in there. Would it be alright if I did that in my cleanup-oriented followup? I'd like to take a deeper look to see if WebViewPlugin can simply not be a WebFrameClient. > > Thanks for investigating, Tommy > > https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... > components/plugins/renderer/webview_plugin.cc:108: UMA_HISTOGRAM_MEMORY_KB( > On 2016/09/16 17:49:39, Nate Chapin wrote: > > On 2016/09/16 17:38:03, tommycli wrote: > > > Are these the only places these UMAs are logged? If so, can we remove from > > > histograms.xml also? > > > > I don't see any related histograms.xml entries? > > They don't appear under UMA either. These must just be orphans (safe to delete).
On 2016/09/16 20:11:57, Nate Chapin wrote: > On 2016/09/16 18:08:26, tommycli wrote: > > > https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... > > File components/plugins/renderer/webview_plugin.cc (left): > > > > > https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... > > components/plugins/renderer/webview_plugin.cc:99: if (!response.isNull()) { > > On 2016/09/16 17:49:40, Nate Chapin wrote: > > > On 2016/09/16 17:38:03, tommycli wrote: > > > > If I understand correctly, WebPlugin::didReceiveResponse() is never > called, > > > and > > > > WebFrameClient::didReceiveResponse() IS called, but with irrelevant, > > > > inappropriate data... > > > > > > Correct. > > > > > > > > > > > I realize there's a hairy diamond inheritance problem -- but can we add an > > > > explicit NOTREACHED() for WebPlugin::didReceiveResponse() that won't be > > > > triggered by the WebFrameClient::didReceiveResponse()? > > > > > > There may be a way, but I'd need to do some research or get some guidance: > my > > > C++ knowledge is minimal in this area. > > > > Okay, here's one suggestion: > > > > Make the WebFrameClient portion a delegate. Have WebViewPlugin own a member > > frame_client_ that implements the interface. Maybe it's an inner class or > > something. > > > > Now WebViewPlugin::didReceiveResponse should only override > > WebPlugin::didReceiveResponse and you can put NOTREACHED in there. > > Would it be alright if I did that in my cleanup-oriented followup? I'd like to > take a deeper look to see if WebViewPlugin can simply not be a WebFrameClient. > > > > > Thanks for investigating, Tommy > > > > > https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... > > components/plugins/renderer/webview_plugin.cc:108: UMA_HISTOGRAM_MEMORY_KB( > > On 2016/09/16 17:49:39, Nate Chapin wrote: > > > On 2016/09/16 17:38:03, tommycli wrote: > > > > Are these the only places these UMAs are logged? If so, can we remove from > > > > histograms.xml also? > > > > > > I don't see any related histograms.xml entries? > > > > They don't appear under UMA either. These must just be orphans (safe to > delete). I'm fine with cleaning it up in a separate CL. lgtm. For this CL, can we add some comments and a TODO in the code regarding this subtle behavior?
On 2016/09/16 20:34:50, tommycli wrote: > On 2016/09/16 20:11:57, Nate Chapin wrote: > > On 2016/09/16 18:08:26, tommycli wrote: > > > > > > https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... > > > File components/plugins/renderer/webview_plugin.cc (left): > > > > > > > > > https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... > > > components/plugins/renderer/webview_plugin.cc:99: if (!response.isNull()) { > > > On 2016/09/16 17:49:40, Nate Chapin wrote: > > > > On 2016/09/16 17:38:03, tommycli wrote: > > > > > If I understand correctly, WebPlugin::didReceiveResponse() is never > > called, > > > > and > > > > > WebFrameClient::didReceiveResponse() IS called, but with irrelevant, > > > > > inappropriate data... > > > > > > > > Correct. > > > > > > > > > > > > > > I realize there's a hairy diamond inheritance problem -- but can we add > an > > > > > explicit NOTREACHED() for WebPlugin::didReceiveResponse() that won't be > > > > > triggered by the WebFrameClient::didReceiveResponse()? > > > > > > > > There may be a way, but I'd need to do some research or get some guidance: > > my > > > > C++ knowledge is minimal in this area. > > > > > > Okay, here's one suggestion: > > > > > > Make the WebFrameClient portion a delegate. Have WebViewPlugin own a member > > > frame_client_ that implements the interface. Maybe it's an inner class or > > > something. > > > > > > Now WebViewPlugin::didReceiveResponse should only override > > > WebPlugin::didReceiveResponse and you can put NOTREACHED in there. > > > > Would it be alright if I did that in my cleanup-oriented followup? I'd like to > > take a deeper look to see if WebViewPlugin can simply not be a WebFrameClient. > > > > > > > > Thanks for investigating, Tommy > > > > > > > > > https://codereview.chromium.org/2349443003/diff/20001/components/plugins/rend... > > > components/plugins/renderer/webview_plugin.cc:108: UMA_HISTOGRAM_MEMORY_KB( > > > On 2016/09/16 17:49:39, Nate Chapin wrote: > > > > On 2016/09/16 17:38:03, tommycli wrote: > > > > > Are these the only places these UMAs are logged? If so, can we remove > from > > > > > histograms.xml also? > > > > > > > > I don't see any related histograms.xml entries? > > > > > > They don't appear under UMA either. These must just be orphans (safe to > > delete). > > I'm fine with cleaning it up in a separate CL. lgtm. For this CL, can we add > some comments and a TODO in the code > regarding this subtle behavior? Done.
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2349443003/#ps40001 (title: "+comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by laforge@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Listen to didReceiveResponse() to get the response in WebViewPlugin. This is a partial revert of the WebViewPlugin changes in https://crrev.com/8489f1bf78c2c6f296e0df6c529751d29e8a0188 BUG=641028 ========== to ========== Listen to didReceiveResponse() to get the response in WebViewPlugin. This is a partial revert of the WebViewPlugin changes in https://crrev.com/8489f1bf78c2c6f296e0df6c529751d29e8a0188 BUG=641028 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Listen to didReceiveResponse() to get the response in WebViewPlugin. This is a partial revert of the WebViewPlugin changes in https://crrev.com/8489f1bf78c2c6f296e0df6c529751d29e8a0188 BUG=641028 ========== to ========== Listen to didReceiveResponse() to get the response in WebViewPlugin. This is a partial revert of the WebViewPlugin changes in https://crrev.com/8489f1bf78c2c6f296e0df6c529751d29e8a0188 BUG=641028 Committed: https://crrev.com/8ed4bf69eca5e2696aeab731a8e9a15e8f379f2b Cr-Commit-Position: refs/heads/master@{#419387} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8ed4bf69eca5e2696aeab731a8e9a15e8f379f2b Cr-Commit-Position: refs/heads/master@{#419387} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
