Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1924383002:
BufferedDataSource now caches redirects for subsequent requests (Closed)
DescriptionBufferedDataSource now caches redirects for subsequent requests on Android
Previously BDS would always make requests to the initial URL. So if the
response was a redirect it would be followed on every subsequent request.
Now we save the destination after redirects and use it for future
requests. This will speed up seeking because we need to do
fewer http requests, and is more robust because we can be more sure that
we're requesting bytes from the same resource each time. See the bug for
an example.
This is Android only for now because Android has historically had this
behavior. MultiBufferDataSource, which will replace BDS, also
implements this logic, so all platforms will soon have it.
BUG=606666
Committed: https://crrev.com/7ca0b0cfb005a022f08346a778b4481b985078c1
Cr-Commit-Position: refs/heads/master@{#391121}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Reword comment #Patch Set 3 : rebase #Patch Set 4 : Converted to android only #Messages
Total messages: 28 (10 generated)
Description was changed from ========== BufferedDataSource now caches redirects for subsequent requests Previously BDS would always make requests to the initial URL. So if the response was a redirect it would be followed on every subsequent request. Now we save the destination after redirects and use it for future subsequent requests. This will speed up seeking because we need to do fewer http requests, and is more robust because we can be more sure that we're requesting bytes from the same resource each time. See the bug for an example. Note: MultiBufferDataSource, which will replace BDS already implements this logic. BUG=606666 ========== to ========== BufferedDataSource now caches redirects for subsequent requests Previously BDS would always make requests to the initial URL. So if the response was a redirect it would be followed on every subsequent request. Now we save the destination after redirects and use it for future subsequent requests. This will speed up seeking because we need to do fewer http requests, and is more robust because we can be more sure that we're requesting bytes from the same resource each time. See the bug for an example. Note: MultiBufferDataSource, which will replace BDS, already implements this logic. BUG=606666 ==========
watk@chromium.org changed reviewers: + hubbe@chromium.org
PTAL. I didn't restrict this to Android only because MultiBuffer is already doing this, so maybe it's okay to enable everywhere? I don't like that these tests inspect the url_ member, but since they're going to be deleted soon, I don't think it's worth the effort to write them properly. (Probably the BufferedResourceLoader constructor should be a mockable factory function so we can assert that it's constructed for the right URL.)
https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... media/blink/buffered_data_source.cc:395: // If it's empty, it was a response from a Service Worker, so keep the Why do we care if it's a service worker?
https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... media/blink/buffered_data_source.cc:395: // If it's empty, it was a response from a Service Worker, so keep the On 2016/04/28 19:34:17, hubbe wrote: > Why do we care if it's a service worker? I guess my phrasing is slightly misleading. It's not that it was a service worker that's important, but that it's empty. The only reason I mentioned Service Workers is because otherwise it's not obvious why we'd check for an empty url. Should I reword it?
Description was changed from ========== BufferedDataSource now caches redirects for subsequent requests Previously BDS would always make requests to the initial URL. So if the response was a redirect it would be followed on every subsequent request. Now we save the destination after redirects and use it for future subsequent requests. This will speed up seeking because we need to do fewer http requests, and is more robust because we can be more sure that we're requesting bytes from the same resource each time. See the bug for an example. Note: MultiBufferDataSource, which will replace BDS, already implements this logic. BUG=606666 ========== to ========== BufferedDataSource now caches redirects for subsequent requests Previously BDS would always make requests to the initial URL. So if the response was a redirect it would be followed on every subsequent request. Now we save the destination after redirects and use it for future requests. This will speed up seeking because we need to do fewer http requests, and is more robust because we can be more sure that we're requesting bytes from the same resource each time. See the bug for an example. Note: MultiBufferDataSource, which will replace BDS, already implements this logic. BUG=606666 ==========
On 2016/04/28 20:05:16, watk wrote: > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > File media/blink/buffered_data_source.cc (right): > > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > media/blink/buffered_data_source.cc:395: // If it's empty, it was a response > from a Service Worker, so keep the > On 2016/04/28 19:34:17, hubbe wrote: > > Why do we care if it's a service worker? > > I guess my phrasing is slightly misleading. It's not that it was a service > worker that's important, but that it's empty. The only reason I mentioned > Service Workers is because otherwise it's not obvious why we'd check for an > empty url. > > Should I reword it? We don't have this check in the multibuffer code, so I'm trying to understand if we need it. However, I don't understand the significance of the empty url.
On 2016/04/28 20:17:27, hubbe wrote: > On 2016/04/28 20:05:16, watk wrote: > > > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > > File media/blink/buffered_data_source.cc (right): > > > > > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > > media/blink/buffered_data_source.cc:395: // If it's empty, it was a response > > from a Service Worker, so keep the > > On 2016/04/28 19:34:17, hubbe wrote: > > > Why do we care if it's a service worker? > > > > I guess my phrasing is slightly misleading. It's not that it was a service > > worker that's important, but that it's empty. The only reason I mentioned > > Service Workers is because otherwise it's not obvious why we'd check for an > > empty url. > > > > Should I reword it? > > We don't have this check in the multibuffer code, so I'm trying to understand if > we need it. > However, I don't understand the significance of the empty url. Oh, gotcha. If you look at where the response_original_url is set it calls originalURLViaServiceWorker: https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer... And that's documented to be possibly empty: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... So it seems like we probably do need it with multibuffer, right?
On 2016/04/28 20:23:15, watk wrote: > On 2016/04/28 20:17:27, hubbe wrote: > > On 2016/04/28 20:05:16, watk wrote: > > > > > > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > > > File media/blink/buffered_data_source.cc (right): > > > > > > > > > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > > > media/blink/buffered_data_source.cc:395: // If it's empty, it was a response > > > from a Service Worker, so keep the > > > On 2016/04/28 19:34:17, hubbe wrote: > > > > Why do we care if it's a service worker? > > > > > > I guess my phrasing is slightly misleading. It's not that it was a service > > > worker that's important, but that it's empty. The only reason I mentioned > > > Service Workers is because otherwise it's not obvious why we'd check for an > > > empty url. > > > > > > Should I reword it? > > > > We don't have this check in the multibuffer code, so I'm trying to understand > if > > we need it. > > However, I don't understand the significance of the empty url. > > Oh, gotcha. If you look at where the response_original_url is set it calls > originalURLViaServiceWorker: > https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer... > > And that's documented to be possibly empty: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > So it seems like we probably do need it with multibuffer, right? Ehh, I feel impossibly stupid, but I still don't understand why we need a special case for service workers. If a service worker gives us a redirect, why should we handle that any different from any other redirect?
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
The service worker special case is for the security issue we had a while back where data was accessible across origins.
I thought the solution to that was to not accept redirects from origin A to B unless B has the proper cross-origin headers. Service workers were merely a convenient way to exploit the bug, no? Or am I thinking of something else? On Thu, Apr 28, 2016 at 2:01 PM, <dalecurtis@chromium.org> wrote: > The service worker special case is for the security issue we had a while > back > where data was accessible across origins. > > https://codereview.chromium.org/1924383002/ > -- 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.
On 2016/04/28 20:48:15, hubbe wrote: > On 2016/04/28 20:23:15, watk wrote: > > On 2016/04/28 20:17:27, hubbe wrote: > > > On 2016/04/28 20:05:16, watk wrote: > > > > > > > > > > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > > > > File media/blink/buffered_data_source.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > > > > media/blink/buffered_data_source.cc:395: // If it's empty, it was a > response > > > > from a Service Worker, so keep the > > > > On 2016/04/28 19:34:17, hubbe wrote: > > > > > Why do we care if it's a service worker? > > > > > > > > I guess my phrasing is slightly misleading. It's not that it was a service > > > > worker that's important, but that it's empty. The only reason I mentioned > > > > Service Workers is because otherwise it's not obvious why we'd check for > an > > > > empty url. > > > > > > > > Should I reword it? > > > > > > We don't have this check in the multibuffer code, so I'm trying to > understand > > if > > > we need it. > > > However, I don't understand the significance of the empty url. > > > > Oh, gotcha. If you look at where the response_original_url is set it calls > > originalURLViaServiceWorker: > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer... > > > > And that's documented to be possibly empty: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > So it seems like we probably do need it with multibuffer, right? > > Ehh, I feel impossibly stupid, but I still don't understand why we need a > special case for service workers. > If a service worker gives us a redirect, why should we handle that any different > from any other redirect? I guess there are two cases though. SW redirect to SW -> response url may be empty. SW redirect to a non-SW -> response url is non-empty. For the former case, we don't know at BDS that a redirect occurred I don't think. So we'd have to plumb that up. For the latter case, it will behave like a non-SW redirect.
On 2016/04/28 20:48:15, hubbe wrote: > On 2016/04/28 20:23:15, watk wrote: > > On 2016/04/28 20:17:27, hubbe wrote: > > > On 2016/04/28 20:05:16, watk wrote: > > > > > > > > > > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > > > > File media/blink/buffered_data_source.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > > > > media/blink/buffered_data_source.cc:395: // If it's empty, it was a > response > > > > from a Service Worker, so keep the > > > > On 2016/04/28 19:34:17, hubbe wrote: > > > > > Why do we care if it's a service worker? > > > > > > > > I guess my phrasing is slightly misleading. It's not that it was a service > > > > worker that's important, but that it's empty. The only reason I mentioned > > > > Service Workers is because otherwise it's not obvious why we'd check for > an > > > > empty url. > > > > > > > > Should I reword it? > > > > > > We don't have this check in the multibuffer code, so I'm trying to > understand > > if > > > we need it. > > > However, I don't understand the significance of the empty url. > > > > Oh, gotcha. If you look at where the response_original_url is set it calls > > originalURLViaServiceWorker: > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer... > > > > And that's documented to be possibly empty: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > So it seems like we probably do need it with multibuffer, right? > > Ehh, I feel impossibly stupid, but I still don't understand why we need a > special case for service workers. > If a service worker gives us a redirect, why should we handle that any different > from any other redirect?
On 2016/04/28 21:23:51, watk wrote: > On 2016/04/28 20:48:15, hubbe wrote: > > On 2016/04/28 20:23:15, watk wrote: > > > On 2016/04/28 20:17:27, hubbe wrote: > > > > On 2016/04/28 20:05:16, watk wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > > > > > File media/blink/buffered_data_source.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1924383002/diff/1/media/blink/buffered_data_s... > > > > > media/blink/buffered_data_source.cc:395: // If it's empty, it was a > > response > > > > > from a Service Worker, so keep the > > > > > On 2016/04/28 19:34:17, hubbe wrote: > > > > > > Why do we care if it's a service worker? > > > > > > > > > > I guess my phrasing is slightly misleading. It's not that it was a > service > > > > > worker that's important, but that it's empty. The only reason I > mentioned > > > > > Service Workers is because otherwise it's not obvious why we'd check for > > an > > > > > empty url. > > > > > > > > > > Should I reword it? > > > > > > > > We don't have this check in the multibuffer code, so I'm trying to > > understand > > > if > > > > we need it. > > > > However, I don't understand the significance of the empty url. > > > > > > Oh, gotcha. If you look at where the response_original_url is set it calls > > > originalURLViaServiceWorker: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer... > > > > > > And that's documented to be possibly empty: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > So it seems like we probably do need it with multibuffer, right? > > > > Ehh, I feel impossibly stupid, but I still don't understand why we need a > > special case for service workers. > > If a service worker gives us a redirect, why should we handle that any > different > > from any other redirect? Looks like I need to port these tests over to the multibuffer code as well. LGTM
The CQ bit was checked by watk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1924383002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1924383002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== BufferedDataSource now caches redirects for subsequent requests Previously BDS would always make requests to the initial URL. So if the response was a redirect it would be followed on every subsequent request. Now we save the destination after redirects and use it for future requests. This will speed up seeking because we need to do fewer http requests, and is more robust because we can be more sure that we're requesting bytes from the same resource each time. See the bug for an example. Note: MultiBufferDataSource, which will replace BDS, already implements this logic. BUG=606666 ========== to ========== BufferedDataSource now caches redirects for subsequent requests on Android Previously BDS would always make requests to the initial URL. So if the response was a redirect it would be followed on every subsequent request. Now we save the destination after redirects and use it for future requests. This will speed up seeking because we need to do fewer http requests, and is more robust because we can be more sure that we're requesting bytes from the same resource each time. See the bug for an example. This is Android only for now because Android has historically had this behavior. MultiBufferDataSource, which will replace BDS, also implements this logic, so all platforms will soon have it. BUG=606666 ==========
PTAL
lgtm
The CQ bit was checked by watk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1924383002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1924383002/60001
Message was sent while issue was closed.
Description was changed from ========== BufferedDataSource now caches redirects for subsequent requests on Android Previously BDS would always make requests to the initial URL. So if the response was a redirect it would be followed on every subsequent request. Now we save the destination after redirects and use it for future requests. This will speed up seeking because we need to do fewer http requests, and is more robust because we can be more sure that we're requesting bytes from the same resource each time. See the bug for an example. This is Android only for now because Android has historically had this behavior. MultiBufferDataSource, which will replace BDS, also implements this logic, so all platforms will soon have it. BUG=606666 ========== to ========== BufferedDataSource now caches redirects for subsequent requests on Android Previously BDS would always make requests to the initial URL. So if the response was a redirect it would be followed on every subsequent request. Now we save the destination after redirects and use it for future requests. This will speed up seeking because we need to do fewer http requests, and is more robust because we can be more sure that we're requesting bytes from the same resource each time. See the bug for an example. This is Android only for now because Android has historically had this behavior. MultiBufferDataSource, which will replace BDS, also implements this logic, so all platforms will soon have it. BUG=606666 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== BufferedDataSource now caches redirects for subsequent requests on Android Previously BDS would always make requests to the initial URL. So if the response was a redirect it would be followed on every subsequent request. Now we save the destination after redirects and use it for future requests. This will speed up seeking because we need to do fewer http requests, and is more robust because we can be more sure that we're requesting bytes from the same resource each time. See the bug for an example. This is Android only for now because Android has historically had this behavior. MultiBufferDataSource, which will replace BDS, also implements this logic, so all platforms will soon have it. BUG=606666 ========== to ========== BufferedDataSource now caches redirects for subsequent requests on Android Previously BDS would always make requests to the initial URL. So if the response was a redirect it would be followed on every subsequent request. Now we save the destination after redirects and use it for future requests. This will speed up seeking because we need to do fewer http requests, and is more robust because we can be more sure that we're requesting bytes from the same resource each time. See the bug for an example. This is Android only for now because Android has historically had this behavior. MultiBufferDataSource, which will replace BDS, also implements this logic, so all platforms will soon have it. BUG=606666 Committed: https://crrev.com/7ca0b0cfb005a022f08346a778b4481b985078c1 Cr-Commit-Position: refs/heads/master@{#391121} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7ca0b0cfb005a022f08346a778b4481b985078c1 Cr-Commit-Position: refs/heads/master@{#391121} |
