Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2287213002:
Ensure loading PDFs doesn't generate 2 http requests (Closed)
DescriptionEnsure loading PDFs doesn't generate 2 http requests
There is code in MimeHandlerViewContainer::OnReady to make a http request for
the PDF if it is loaded in an embedded context. This code was errorneously
being run for top level PDFs as well, causing two requests to be made. In most
cases this second request would be cancelled straight away but it actually
isn't needed at all.
BUG=587709
Committed: https://crrev.com/1d0b49e4794131e5f53265bf85d961020a725a30
Cr-Commit-Position: refs/heads/master@{#415526}
Patch Set 1 #Patch Set 2 : Ensure loading PDFs doesn't generate 2 http requests #Patch Set 3 : Ensure loading PDFs doesn't generate 2 http requests #Patch Set 4 : Ensure loading PDFs doesn't generate 2 http requests #Patch Set 5 : Ensure loading PDFs doesn't generate 2 http requests #
Total comments: 12
Patch Set 6 : Ensure loading PDFs doesn't generate 2 http requests #
Total comments: 12
Patch Set 7 : Ensure loading PDFs doesn't generate 2 http requests #Patch Set 8 : Ensure loading PDFs doesn't generate 2 http requests #
Messages
Total messages: 28 (12 generated)
The CQ bit was checked by raymes@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...
raymes@chromium.org changed reviewers: + mmenke@chromium.org, sammc@chromium.org
mmenke: I'd like to add a regression test for this. Can we query the net log in a test to ensure that two requests aren't being made?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2016/08/29 04:49:10, raymes wrote: > mmenke: I'd like to add a regression test for this. Can we query the net log in > a test to ensure that two requests aren't being made? A regression test sounds good to me, but rather than use NetLog (Which is really only meant for about:net-internals consumption, and isn't guaranteed to use a consistent format between versions). We could hook into the network stack via URLRequestFilter to count requests (The filter doesn't need to return actual URLRequestJobs). We could also use the EmbeddedTestServer and set up a callback to be informed of requests the server sees, and count requests for the PDF's URL. Or we could hook into wherever devtools gets its data from to make sure there's only one request, though I'm not sure how that's hooked up. There are other ways (Hook in via ResourceDispatcherHostDelegate, hook in via the NetworkDelegate), but they're a bit more complicated, and not used much.
On 2016/08/29 10:08:23, mmenke wrote: > On 2016/08/29 04:49:10, raymes wrote: > > mmenke: I'd like to add a regression test for this. Can we query the net log > in > > a test to ensure that two requests aren't being made? > > A regression test sounds good to me, but rather than use NetLog (Which is really > only meant for about:net-internals consumption, and isn't guaranteed to use a > consistent format between versions). We could hook into the network stack via > URLRequestFilter to count requests (The filter doesn't need to return actual > URLRequestJobs). We could also use the EmbeddedTestServer and set up a callback > to be informed of requests the server sees, and count requests for the PDF's > URL. > > Or we could hook into wherever devtools gets its data from to make sure there's > only one request, though I'm not sure how that's hooked up. > > There are other ways (Hook in via ResourceDispatcherHostDelegate, hook in via > the NetworkDelegate), but they're a bit more complicated, and not used much. The EmbeddedTestServer approach is probably the simplest one
Thanks for the help! I've updated the test, ptal.
Chatted with Sam and it seemed like just checking the requests that hit the server wouldn't always catch the bug (because it would depend on the race condition). So I've updated the CL to use the URLRequestFilter approach instead. Thanks!
On 2016/08/30 02:45:22, raymes wrote: > Chatted with Sam and it seemed like just checking the requests that hit the > server wouldn't always catch the bug (because it would depend on the race > condition). So I've updated the CL to use the URLRequestFilter approach instead. > Thanks! If we're not guaranteed they'll hit the server, are we guaranteed they'll hit the network stack?
On 2016/08/30 02:48:39, mmenke wrote: > On 2016/08/30 02:45:22, raymes wrote: > > Chatted with Sam and it seemed like just checking the requests that hit the > > server wouldn't always catch the bug (because it would depend on the race > > condition). So I've updated the CL to use the URLRequestFilter approach > instead. > > Thanks! > > If we're not guaranteed they'll hit the server, are we guaranteed they'll hit > the network stack? I'm not entirely sure - but we were always seeing the request being made from Chrome with the bug.
https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc (right): https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:72: int* count_; private: https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:77: url_, base::WrapUnique(new SimpleRequestInterceptor(&count_))); base::MakeUnique<SimpleRequestInterceptor>(&count_) https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:84: GURL url_; const https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:85: int count_; You're reading and writing |count_| on different threads. Are you sure there's sufficient synchronisation? https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:86: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:99: StartEmbeddedTestServer(); No ASSERT_TRUE()?
https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc (right): https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:72: int* count_; On 2016/08/30 03:04:28, Sam McNally wrote: > private: Done. https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:77: url_, base::WrapUnique(new SimpleRequestInterceptor(&count_))); On 2016/08/30 03:04:28, Sam McNally wrote: > base::MakeUnique<SimpleRequestInterceptor>(&count_) Done. https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:84: GURL url_; On 2016/08/30 03:04:28, Sam McNally wrote: > const Done. https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:85: int count_; Done - shifted the code around as discussed. https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:86: }; On 2016/08/30 03:04:28, Sam McNally wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2287213002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:99: StartEmbeddedTestServer(); On 2016/08/30 03:04:28, Sam McNally wrote: > No ASSERT_TRUE()? Done.
The CQ bit was checked by raymes@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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Tests LGTM. Didn't verify whether or not there's a race that could mask a regression. https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc (right): https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:39: URLRequestCounter(const GURL& url) : url_(url), count_(0) { explicit https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:54: run_loop.QuitClosure()); Should include bind.h and location.h (locations.h?) https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:70: class SimpleRequestInterceptor : public net::URLRequestInterceptor { Suggest a short comment on what this does. https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:72: SimpleRequestInterceptor(const base::Closure& callback) explicit https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:88: void RequestFired() { ++count_; } Suggest adding DCHECK_CURRENTLY_ONs to all methods to this code, to make it more self-documenting. https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:101: int count_; Should mention count is only used on the UI thread
The CQ bit was checked by raymes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2287213002/#ps140001 (title: "Ensure loading PDFs doesn't generate 2 http requests")
https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc (right): https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:39: URLRequestCounter(const GURL& url) : url_(url), count_(0) { On 2016/08/30 14:51:12, mmenke wrote: > explicit Done. https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:54: run_loop.QuitClosure()); On 2016/08/30 14:51:12, mmenke wrote: > Should include bind.h and location.h (locations.h?) Done. https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:70: class SimpleRequestInterceptor : public net::URLRequestInterceptor { On 2016/08/30 14:51:13, mmenke wrote: > Suggest a short comment on what this does. Done. https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:72: SimpleRequestInterceptor(const base::Closure& callback) On 2016/08/30 14:51:13, mmenke wrote: > explicit Done. https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:88: void RequestFired() { ++count_; } On 2016/08/30 14:51:12, mmenke wrote: > Suggest adding DCHECK_CURRENTLY_ONs to all methods to this code, to make it more > self-documenting. Done. https://codereview.chromium.org/2287213002/diff/100001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:101: int count_; On 2016/08/30 14:51:13, mmenke wrote: > Should mention count is only used on the UI thread Done.
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 #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Ensure loading PDFs doesn't generate 2 http requests There is code in MimeHandlerViewContainer::OnReady to make a http request for the PDF if it is loaded in an embedded context. This code was errorneously being run for top level PDFs as well, causing two requests to be made. In most cases this second request would be cancelled straight away but it actually isn't needed at all. BUG=587709 ========== to ========== Ensure loading PDFs doesn't generate 2 http requests There is code in MimeHandlerViewContainer::OnReady to make a http request for the PDF if it is loaded in an embedded context. This code was errorneously being run for top level PDFs as well, causing two requests to be made. In most cases this second request would be cancelled straight away but it actually isn't needed at all. BUG=587709 Committed: https://crrev.com/1d0b49e4794131e5f53265bf85d961020a725a30 Cr-Commit-Position: refs/heads/master@{#415526} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1d0b49e4794131e5f53265bf85d961020a725a30 Cr-Commit-Position: refs/heads/master@{#415526} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
