Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2353603002:
Fix DomDistillerViewerSource to work with PlzNavigate. (Closed)
DescriptionFix DomDistillerViewerSource to work with PlzNavigate.
In particular, the render frame ID isn't known at navigate time. I'll change content::URLDataSource::StartDataRequest to take a WebContentsGetter in a followup which will complete this fix, but I wanted to separate that mechanical change to 40+ consumers from this actual fix. The other fix here is to delay the code which makes a Mojo call and sets up an interface factory until the load commits, since that's when the main frame will be committed with PlzNavigate. I've verified that this is still early enough for dom distiller's needs.
BUG=576275
Committed: https://crrev.com/8d58263f115b3d9eaa166ee50e9bed051365c791
Cr-Commit-Position: refs/heads/master@{#419635}
Patch Set 1 #
Total comments: 5
Patch Set 2 : review comments #
Total comments: 5
Patch Set 3 : undo bad merge #Messages
Total messages: 25 (15 generated)
The CQ bit was checked by jam@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...
jam@chromium.org changed reviewers: + nyquist@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2353603002/diff/1/components/dom_distiller/co... File components/dom_distiller/content/browser/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/2353603002/diff/1/components/dom_distiller/co... components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:145: CHECK_EQ(0, render_view_host->GetEnabledBindings()); This is our security check to ensure we never, ever, mess up and give any resource rendered through the distiller access to things it should not have access to. Would it somehow be possible to keep this verification in StartDataRequest, or does that not work correctly with PlzNavigate? Maybe the RenderViewHost is not available anymore at that time? https://codereview.chromium.org/2353603002/diff/1/components/dom_distiller/co... components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:260: std::move(distiller_ui_handle_)); Maybe I'm misunderstanding this, but I thought that we would create the DomDistillerViewerSource once through the ProfileImpl in //chrome: https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_impl.cc?... And then, the StartDataRequest would be invoked for any calls to the distiller after that for that profile. Moving the distiller_ui_handle_ to the one-off RequestViewerHandle seems to me like it might make the second call to StartDataRequest not work?
https://codereview.chromium.org/2353603002/diff/1/components/dom_distiller/co... File components/dom_distiller/content/browser/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/2353603002/diff/1/components/dom_distiller/co... components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:145: CHECK_EQ(0, render_view_host->GetEnabledBindings()); On 2016/09/19 21:39:51, nyquist wrote: > This is our security check to ensure we never, ever, mess up and give any > resource rendered through the distiller access to things it should not have > access to. > > Would it somehow be possible to keep this verification in StartDataRequest, or > does that not work correctly with PlzNavigate? Maybe the RenderViewHost is not > available anymore at that time? Right, the RVH is not available at that time so we can't check. I chatted with Nasko about this in our PlzNavigate meeting and he was fine with it. My logic was: this check is for catching developer regressions (i.e. somehow dom distiller gets hosted in a webui process), and so if this occurs we'll get a lot of crash reports which will alert us to the bug. https://codereview.chromium.org/2353603002/diff/1/components/dom_distiller/co... components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:260: std::move(distiller_ui_handle_)); On 2016/09/19 21:39:51, nyquist wrote: > Maybe I'm misunderstanding this, but I thought that we would create the > DomDistillerViewerSource once through the ProfileImpl in //chrome: > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_impl.cc?... > > And then, the StartDataRequest would be invoked for any calls to the distiller > after that for that profile. > Moving the distiller_ui_handle_ to the one-off RequestViewerHandle seems to me > like it might make the second call to StartDataRequest not work? Thanks for catching that (somehow I misremembered and thought the data source was per request, double bad since I made this interface!). Fixed.
The CQ bit was checked by jam@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_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_...)
Could you also update the CL description now that you've in fact started using the WebContentsGetter already? https://codereview.chromium.org/2353603002/diff/1/components/dom_distiller/co... File components/dom_distiller/content/browser/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/2353603002/diff/1/components/dom_distiller/co... components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:145: CHECK_EQ(0, render_view_host->GetEnabledBindings()); On 2016/09/19 22:13:05, jam wrote: > On 2016/09/19 21:39:51, nyquist wrote: > > This is our security check to ensure we never, ever, mess up and give any > > resource rendered through the distiller access to things it should not have > > access to. > > > > Would it somehow be possible to keep this verification in StartDataRequest, or > > does that not work correctly with PlzNavigate? Maybe the RenderViewHost is not > > available anymore at that time? > > Right, the RVH is not available at that time so we can't check. > > I chatted with Nasko about this in our PlzNavigate meeting and he was fine with > it. My logic was: this check is for catching developer regressions (i.e. somehow > dom distiller gets hosted in a webui process), and so if this occurs we'll get a > lot of crash reports which will alert us to the bug. OK. Thanks for checking! https://codereview.chromium.org/2353603002/diff/20001/components/dom_distille... File components/dom_distiller/content/browser/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/2353603002/diff/20001/components/dom_distille... components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:226: void DomDistillerViewerSource::StartDataRequest( Since you're changing which method we override, should you also change the header file? https://codereview.chromium.org/2353603002/diff/20001/components/dom_distille... components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:228: const content::ResourceRequestInfo::WebContentsGetter& wc_getter, Does this require including content/public/browser/resource_request_info.h ? https://codereview.chromium.org/2353603002/diff/20001/components/dom_distille... components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:230: content::WebContents* web_contents = wc_getter.Run(); Are we guaranteed that the info used to create the getter is still valid (i.e. never returning nullptr here)? I would really hope so, since otherwise passing it to StartDataRequest would kind of be a not-so-nice API :-)
https://codereview.chromium.org/2353603002/diff/20001/components/dom_distille... File components/dom_distiller/content/browser/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/2353603002/diff/20001/components/dom_distille... components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:226: void DomDistillerViewerSource::StartDataRequest( On 2016/09/19 22:26:59, nyquist wrote: > Since you're changing which method we override, should you also change the > header file? sorry that was a bad merge, this change shouldn't be using WebContentsGetter. unreverted now. https://codereview.chromium.org/2353603002/diff/20001/components/dom_distille... components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:230: content::WebContents* web_contents = wc_getter.Run(); On 2016/09/19 22:27:00, nyquist wrote: > Are we guaranteed that the info used to create the getter is still valid (i.e. > never returning nullptr here)? I would really hope so, since otherwise passing > it to StartDataRequest would kind of be a not-so-nice API :-) (it's removed in this patch now, but yes it's guaranteed)
The CQ bit was checked by jam@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...
lgtm
The CQ bit was unchecked by jam@chromium.org
The CQ bit was checked by jam@chromium.org
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix DomDistillerViewerSource to work with PlzNavigate. In particular, the render frame ID isn't known at navigate time. I'll change content::URLDataSource::StartDataRequest to take a WebContentsGetter in a followup which will complete this fix, but I wanted to separate that mechanical change to 40+ consumers from this actual fix. The other fix here is to delay the code which makes a Mojo call and sets up an interface factory until the load commits, since that's when the main frame will be committed with PlzNavigate. I've verified that this is still early enough for dom distiller's needs. BUG=576275 ========== to ========== Fix DomDistillerViewerSource to work with PlzNavigate. In particular, the render frame ID isn't known at navigate time. I'll change content::URLDataSource::StartDataRequest to take a WebContentsGetter in a followup which will complete this fix, but I wanted to separate that mechanical change to 40+ consumers from this actual fix. The other fix here is to delay the code which makes a Mojo call and sets up an interface factory until the load commits, since that's when the main frame will be committed with PlzNavigate. I've verified that this is still early enough for dom distiller's needs. BUG=576275 Committed: https://crrev.com/8d58263f115b3d9eaa166ee50e9bed051365c791 Cr-Commit-Position: refs/heads/master@{#419635} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8d58263f115b3d9eaa166ee50e9bed051365c791 Cr-Commit-Position: refs/heads/master@{#419635}
Message was sent while issue was closed.
mdjones@chromium.org changed reviewers: + mdjones@chromium.org
Message was sent while issue was closed.
This appears to have broken navigations from inside distiller (i.e. clicking on a link). The error I get is: [FATAL:navigation_handle_impl.cc(204)] Check failed: state_ == DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE. This accessor should not be called before the navigation has committed. It looks like it is complaining about "navigation_handle->IsSamePage()". Is this a quick fix? |
