|
|
DescriptionPDF: Don't follow redirects in the plugin.
The MimeHandler should have done it already.
BUG=653749
Committed: https://crrev.com/65c30fee167a96761f0bb9929dc44996d9e546fe
Cr-Commit-Position: refs/heads/master@{#427452}
Patch Set 1 #
Messages
Total messages: 20 (9 generated)
The CQ bit was checked by thestig@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: This issue passed the CQ dry run.
raymes@chromium.org changed reviewers: + raymes@chromium.org
lgtm - I think we should land this. We should add a test though.
On 2016/10/25 05:46:00, raymes wrote: > lgtm - I think we should land this. We should add a test though. What kind of a test did you have in mind? Something in chrome/ with an embedded web server?
> What kind of a test did you have in mind? Something in chrome/ with an embedded web server? I was just trying to figure out how we could write one. It looks like we already have a way to trigger redirects in browsertests, e.g. https://cs.chromium.org/chromium/src/chrome/browser/history/redirect_browsert... If we add a hook which allows us to load a custom URL during tests (rather than through the mime handler interface), then we could verify that the redirect fails and nothing gets loaded? I think that might not be too burdensome? Thoughts? On Tue, 25 Oct 2016 at 17:00 <thestig@chromium.org> wrote: > Reviewers: raymes > CL: https://codereview.chromium.org/2409423004/ > > Message: > > On 2016/10/25 05:46:00, raymes wrote: > > lgtm - I think we should land this. We should add a test though. > > What kind of a test did you have in mind? Something in chrome/ with an > embedded > web server? > > Description: > PDF: Don't follow redirects in the plugin. > > The MimeHandler should have done it already. > > BUG=653749 > > Affected files (+1, -1 lines): > M pdf/document_loader.cc > > > Index: pdf/document_loader.cc > diff --git a/pdf/document_loader.cc b/pdf/document_loader.cc > index > ae608eff93e6e45bb7db06517afb70a403f80da5..ea4b664b27b53afccc3f6ccb50e23eae456a7ab9 > 100644 > --- a/pdf/document_loader.cc > +++ b/pdf/document_loader.cc > @@ -335,7 +335,7 @@ pp::URLRequestInfo DocumentLoader::GetRequest(uint32_t > position, > pp::URLRequestInfo request(client_->GetPluginInstance()); > request.SetURL(url_); > request.SetMethod("GET"); > - request.SetFollowRedirects(true); > + request.SetFollowRedirects(false); > request.SetCustomReferrerURL(url_); > > const size_t kBufSize = 100; > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> If we add a hook which allows us to load a custom URL during tests I can imagine a couple of ways to do this: 1) Trigger the chrome://print path to be taken in the test, allowing us to specify the URL in the querystring 2) Load the ppapi plugin directly in the test. (1) might be slightly preferable. On Tue, 25 Oct 2016 at 17:06 Raymes Khoury <raymes@chromium.org> wrote: > > What kind of a test did you have in mind? Something in chrome/ with an > embedded > web server? > > I was just trying to figure out how we could write one. > > It looks like we already have a way to trigger redirects in browsertests, > e.g. > https://cs.chromium.org/chromium/src/chrome/browser/history/redirect_browsert... > > If we add a hook which allows us to load a custom URL during tests (rather > than through the mime handler interface), then we could verify that the > redirect fails and nothing gets loaded? I think that might not be too > burdensome? Thoughts? > > On Tue, 25 Oct 2016 at 17:00 <thestig@chromium.org> wrote: > > Reviewers: raymes > CL: https://codereview.chromium.org/2409423004/ > > Message: > > On 2016/10/25 05:46:00, raymes wrote: > > lgtm - I think we should land this. We should add a test though. > > What kind of a test did you have in mind? Something in chrome/ with an > embedded > web server? > > Description: > PDF: Don't follow redirects in the plugin. > > The MimeHandler should have done it already. > > BUG=653749 > > Affected files (+1, -1 lines): > M pdf/document_loader.cc > > > Index: pdf/document_loader.cc > diff --git a/pdf/document_loader.cc b/pdf/document_loader.cc > index > ae608eff93e6e45bb7db06517afb70a403f80da5..ea4b664b27b53afccc3f6ccb50e23eae456a7ab9 > 100644 > --- a/pdf/document_loader.cc > +++ b/pdf/document_loader.cc > @@ -335,7 +335,7 @@ pp::URLRequestInfo DocumentLoader::GetRequest(uint32_t > position, > pp::URLRequestInfo request(client_->GetPluginInstance()); > request.SetURL(url_); > request.SetMethod("GET"); > - request.SetFollowRedirects(true); > + request.SetFollowRedirects(false); > request.SetCustomReferrerURL(url_); > > const size_t kBufSize = 100; > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Assuming we land this, do you have time to work on a test separately? Unfortunately I really don't at the moment.
Yeah, I'm happy to write a test - I don't think it will be too hard. On Tue, 25 Oct 2016 at 17:11 <thestig@chromium.org> wrote: > Assuming we land this, do you have time to work on a test separately? > Unfortunately I really don't at the moment. > > https://codereview.chromium.org/2409423004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by thestig@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by thestig@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 #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== PDF: Don't follow redirects in the plugin. The MimeHandler should have done it already. BUG=653749 ========== to ========== PDF: Don't follow redirects in the plugin. The MimeHandler should have done it already. BUG=653749 Committed: https://crrev.com/65c30fee167a96761f0bb9929dc44996d9e546fe Cr-Commit-Position: refs/heads/master@{#427452} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/65c30fee167a96761f0bb9929dc44996d9e546fe Cr-Commit-Position: refs/heads/master@{#427452} |