Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25713007:
Component updater on-demand logic with ResourceThrottle (Closed)
|
Created:
7 years, 2 months ago by cpu_(ooo_6.6-7.5) Modified:
7 years, 1 month ago CC:
chromium-reviews Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
Descriptionxxx
R=darin@chromium.org, sorin@chromium.org, waffles@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232580
Patch Set 1 : #
Total comments: 5
Patch Set 2 : #
Total comments: 7
Patch Set 3 : #
Total comments: 37
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 8
Patch Set 6 : #
Total comments: 5
Patch Set 7 : xxx #
Messages
Total messages: 30 (0 generated)
https://codereview.chromium.org/25713007/diff/17001/chrome/browser/component_... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/17001/chrome/browser/component_... chrome/browser/component_updater/component_updater_service.cc:1077: *defer = false; note: you could have a redirect to a media type that you wish to defer. the ResourceThrottle is permitted to store the URLRequest that it is given in its constructor. That pointer will remain valid for the lifetime of the ResourceThrottle. You can then inspect the URLRequest to learn what the response meta data looks like. https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:208: crx_id = "hnimpnehoodheedghdeeijklkeaacbdc"; we probably want to base this on mime type instead of file extension. it is possible for the file extension to be anything. the mime type is what matters. That said, the approach you are taking nicely avoids the problem of the TCP socket stream being left hanging for a long time. Hmm... Perhaps we can get a more clear signal that it is NaCl requesting a PEXE.
https://codereview.chromium.org/25713007/diff/17001/chrome/browser/component_... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/17001/chrome/browser/component_... chrome/browser/component_updater/component_updater_service.cc:1051: // item.component.installer->OnDemandUpdate(..) first? What do you imagine 2) is used for? We also need some signal when things are "done" (either successfully or with an error). Downstream, for PNacl, there's two async events that we need to wait for "completion": (a) Start pexe fetch (b) Start loading component files We need to wait until (a) has at least started and (b) is totally ready. For (a) we need the response headers at least to check for LastModified, Etags, etc. for translation caching, and for (b) we need to know when it's okay start the translator process and start streaming the bytes over to that process for streaming translation. https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:208: crx_id = "hnimpnehoodheedghdeeijklkeaacbdc"; On 2013/10/03 21:31:43, darin wrote: > we probably want to base this on mime type instead of file extension. it is > possible for the file extension to be anything. the mime type is what matters. > > That said, the approach you are taking nicely avoids the problem of the TCP > socket stream being left hanging for a long time. Hmm... > > Perhaps we can get a more clear signal that it is NaCl requesting a PEXE. Separately in a mail thread, I suggested adding something to the request headers. E.g., could we make the NaCl pexe URLloader set "Accept: application/x-pnacl" in the request header? Darin, is that the appropriate use of "Accept:" ? What is the issue about having the TCP socket stream being left hanging for a long time? Is it that if we try to get something based on a *response* we will have started the stream, and that will be left hanging, while if we check something that doesn't require a response, we won't have started the stream yet?
On Wed, Oct 9, 2013 at 10:05 AM, <jvoung@chromium.org> wrote: > > https://codereview.chromium.**org/25713007/diff/17001/** > chrome/browser/component_**updater/component_updater_**service.cc<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/component_updater/component_updater_service.cc> > File chrome/browser/component_**updater/component_updater_**service.cc > (right): > > https://codereview.chromium.**org/25713007/diff/17001/** > chrome/browser/component_**updater/component_updater_** > service.cc#newcode1051<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/component_updater/component_updater_service.cc#newcode1051> > chrome/browser/component_**updater/component_updater_**service.cc:1051: // > item.component.installer->**OnDemandUpdate(..) first? > What do you imagine 2) is used for? > > We also need some signal when things are "done" (either successfully or > with an error). > > Downstream, for PNacl, there's two async events that we need to wait for > "completion": > > (a) Start pexe fetch > (b) Start loading component files > > We need to wait until (a) has at least started and (b) is totally ready. > > For (a) we need the response headers at least to check for LastModified, > Etags, etc. for translation caching, and for (b) we need to know when > it's okay start the translator process and start streaming the bytes > over to that process for streaming translation. > > > https://codereview.chromium.**org/25713007/diff/17001/** > chrome/browser/renderer_host/**chrome_resource_dispatcher_** > host_delegate.cc<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc> > File > chrome/browser/renderer_host/**chrome_resource_dispatcher_** > host_delegate.cc > (right): > > https://codereview.chromium.**org/25713007/diff/17001/** > chrome/browser/renderer_host/**chrome_resource_dispatcher_** > host_delegate.cc#newcode208<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc#newcode208> > chrome/browser/renderer_host/**chrome_resource_dispatcher_** > host_delegate.cc:208: > crx_id = "**hnimpnehoodheedghdeeijklkeaacb**dc"; > On 2013/10/03 21:31:43, darin wrote: > >> we probably want to base this on mime type instead of file extension. >> > it is > >> possible for the file extension to be anything. the mime type is what >> > matters. > > That said, the approach you are taking nicely avoids the problem of >> > the TCP > >> socket stream being left hanging for a long time. Hmm... >> > > Perhaps we can get a more clear signal that it is NaCl requesting a >> > PEXE. > > > Separately in a mail thread, I suggested adding something to the request > headers. E.g., could we make the NaCl pexe URLloader set "Accept: > application/x-pnacl" in the request header? > > Darin, is that the appropriate use of "Accept:" ? > We could do that, but what is the use case? Normally, that is used when an URL can be satisfied by multiple different responses and the server has to pick which one the browser likes best. If fetching an URL that was specified in a NMF file, then it seems like we are already down the PNaCl path. > > What is the issue about having the TCP socket stream being left hanging > for a long time? It may get interrupted by the server if the client is VERY slow to read data from the socket. We should probably validate this concern with folks on chrome-network-stack@, but this is what I recall from my years of working on the HTTP stack. > Is it that if we try to get something based on a > *response* we will have started the stream, and that will be left > hanging, while if we check something that doesn't require a response, we > won't have started the stream yet? > Yes. > > https://codereview.chromium.**org/25713007/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
By the way, if we are looking for a clear signal that an URL request is for a PEXE, then maybe we could look at extending the ResourceType enumeration to more precisely indicate that. -Darin On Thu, Oct 10, 2013 at 1:27 PM, Darin Fisher <darin@chromium.org> wrote: > > > > On Wed, Oct 9, 2013 at 10:05 AM, <jvoung@chromium.org> wrote: > >> >> https://codereview.chromium.**org/25713007/diff/17001/** >> chrome/browser/component_**updater/component_updater_**service.cc<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/component_updater/component_updater_service.cc> >> File chrome/browser/component_**updater/component_updater_**service.cc >> (right): >> >> https://codereview.chromium.**org/25713007/diff/17001/** >> chrome/browser/component_**updater/component_updater_** >> service.cc#newcode1051<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/component_updater/component_updater_service.cc#newcode1051> >> chrome/browser/component_**updater/component_updater_**service.cc:1051: >> // >> item.component.installer->**OnDemandUpdate(..) first? >> What do you imagine 2) is used for? >> >> We also need some signal when things are "done" (either successfully or >> with an error). >> >> Downstream, for PNacl, there's two async events that we need to wait for >> "completion": >> >> (a) Start pexe fetch >> (b) Start loading component files >> >> We need to wait until (a) has at least started and (b) is totally ready. >> >> For (a) we need the response headers at least to check for LastModified, >> Etags, etc. for translation caching, and for (b) we need to know when >> it's okay start the translator process and start streaming the bytes >> over to that process for streaming translation. >> >> >> https://codereview.chromium.**org/25713007/diff/17001/** >> chrome/browser/renderer_host/**chrome_resource_dispatcher_** >> host_delegate.cc<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc> >> File >> chrome/browser/renderer_host/**chrome_resource_dispatcher_** >> host_delegate.cc >> (right): >> >> https://codereview.chromium.**org/25713007/diff/17001/** >> chrome/browser/renderer_host/**chrome_resource_dispatcher_** >> host_delegate.cc#newcode208<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc#newcode208> >> chrome/browser/renderer_host/**chrome_resource_dispatcher_** >> host_delegate.cc:208: >> crx_id = "**hnimpnehoodheedghdeeijklkeaacb**dc"; >> On 2013/10/03 21:31:43, darin wrote: >> >>> we probably want to base this on mime type instead of file extension. >>> >> it is >> >>> possible for the file extension to be anything. the mime type is what >>> >> matters. >> >> That said, the approach you are taking nicely avoids the problem of >>> >> the TCP >> >>> socket stream being left hanging for a long time. Hmm... >>> >> >> Perhaps we can get a more clear signal that it is NaCl requesting a >>> >> PEXE. >> >> >> Separately in a mail thread, I suggested adding something to the request >> headers. E.g., could we make the NaCl pexe URLloader set "Accept: >> application/x-pnacl" in the request header? >> >> Darin, is that the appropriate use of "Accept:" ? >> > > We could do that, but what is the use case? Normally, that is used when an > URL can be satisfied by multiple different responses and the server has to > pick which one the browser likes best. If fetching an URL that was > specified in a NMF file, then it seems like we are already down the PNaCl > path. > > > >> >> What is the issue about having the TCP socket stream being left hanging >> for a long time? > > > It may get interrupted by the server if the client is VERY slow to read > data from the socket. We should probably validate this concern with folks > on chrome-network-stack@, but this is what I recall from my years of > working on the HTTP stack. > > > >> Is it that if we try to get something based on a >> *response* we will have started the stream, and that will be left >> hanging, while if we check something that doesn't require a response, we >> won't have started the stream yet? >> > > Yes. > > > >> >> https://codereview.chromium.**org/25713007/<https://codereview.chromium.org/2... >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/10 20:28:46, darin wrote: > By the way, if we are looking for a clear signal that an URL request is for > a PEXE, then maybe we could look at extending the ResourceType enumeration > to more precisely indicate that. > > -Darin > > > On Thu, Oct 10, 2013 at 1:27 PM, Darin Fisher <mailto:darin@chromium.org> wrote: > > > > > > > > > On Wed, Oct 9, 2013 at 10:05 AM, <mailto:jvoung@chromium.org> wrote: > > > >> > >> https://codereview.chromium.**org/25713007/diff/17001/** > >> > chrome/browser/component_**updater/component_updater_**service.cc<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/component_updater/component_updater_service.cc> > >> File chrome/browser/component_**updater/component_updater_**service.cc > >> (right): > >> > >> https://codereview.chromium.**org/25713007/diff/17001/** > >> chrome/browser/component_**updater/component_updater_** > >> > service.cc#newcode1051<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/component_updater/component_updater_service.cc#newcode1051> > >> chrome/browser/component_**updater/component_updater_**service.cc:1051: > >> // > >> item.component.installer->**OnDemandUpdate(..) first? > >> What do you imagine 2) is used for? > >> > >> We also need some signal when things are "done" (either successfully or > >> with an error). > >> > >> Downstream, for PNacl, there's two async events that we need to wait for > >> "completion": > >> > >> (a) Start pexe fetch > >> (b) Start loading component files > >> > >> We need to wait until (a) has at least started and (b) is totally ready. > >> > >> For (a) we need the response headers at least to check for LastModified, > >> Etags, etc. for translation caching, and for (b) we need to know when > >> it's okay start the translator process and start streaming the bytes > >> over to that process for streaming translation. > >> > >> > >> https://codereview.chromium.**org/25713007/diff/17001/** > >> chrome/browser/renderer_host/**chrome_resource_dispatcher_** > >> > host_delegate.cc<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc> > >> File > >> chrome/browser/renderer_host/**chrome_resource_dispatcher_** > >> host_delegate.cc > >> (right): > >> > >> https://codereview.chromium.**org/25713007/diff/17001/** > >> chrome/browser/renderer_host/**chrome_resource_dispatcher_** > >> > host_delegate.cc#newcode208<https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc#newcode208> > >> chrome/browser/renderer_host/**chrome_resource_dispatcher_** > >> host_delegate.cc:208: > >> crx_id = "**hnimpnehoodheedghdeeijklkeaacb**dc"; > >> On 2013/10/03 21:31:43, darin wrote: > >> > >>> we probably want to base this on mime type instead of file extension. > >>> > >> it is > >> > >>> possible for the file extension to be anything. the mime type is what > >>> > >> matters. > >> > >> That said, the approach you are taking nicely avoids the problem of > >>> > >> the TCP > >> > >>> socket stream being left hanging for a long time. Hmm... > >>> > >> > >> Perhaps we can get a more clear signal that it is NaCl requesting a > >>> > >> PEXE. > >> > >> > >> Separately in a mail thread, I suggested adding something to the request > >> headers. E.g., could we make the NaCl pexe URLloader set "Accept: > >> application/x-pnacl" in the request header? > >> > >> Darin, is that the appropriate use of "Accept:" ? > >> > > > > We could do that, but what is the use case? Normally, that is used when an > > URL can be satisfied by multiple different responses and the server has to > > pick which one the browser likes best. If fetching an URL that was > > specified in a NMF file, then it seems like we are already down the PNaCl > > path. > > Oh, I was just looking for a way to tell communicate that the URL fetch is for a pexe. If we could extend the ResourceType enum, then we wouldn't do this "Accept: ..." hack. > > > > > >> > >> What is the issue about having the TCP socket stream being left hanging > >> for a long time? > > > > > > It may get interrupted by the server if the client is VERY slow to read > > data from the socket. We should probably validate this concern with folks > > on chrome-network-stack@, but this is what I recall from my years of > > working on the HTTP stack. > > > > > > > >> Is it that if we try to get something based on a > >> *response* we will have started the stream, and that will be left > >> hanging, while if we check something that doesn't require a response, we > >> won't have started the stream yet? > >> > > > > Yes. > > > > > > > >> > >> > https://codereview.chromium.**org/25713007/%3Chttps://codereview.chromium.org...> > >> > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/17001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:208: crx_id = "hnimpnehoodheedghdeeijklkeaacbdc"; Yeah, a special header sounds nice to me. > What is the issue about having the TCP socket stream being left hanging The server might close the socket or otherwise time out the request.
sorin, waffles please do a preliminary review (use patch set 6). The code is complete sans tests which I can add in this CL or in a following one (your choice). Once I add the tests they code might be harder to follow so you might want to look at it now. I added comments on the strange parts. jvoung, your comments are welcome but I am mostly interested in you trying this patch to see if it does not have bad side effects, in particular see if you can think how to reduce the number of throttles we add in AppendComponentUpaterThrottles because each has a performance impact. https://codereview.chromium.org/25713007/diff/148001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/148001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:267: it has 3 states because there is a strange requirement that triggers a DCHECK. See 1192 for more information. https://codereview.chromium.org/25713007/diff/148001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:612: break; Did I got all the cases in 599 and 604? I think so. Only these two calls delete the weak pointers to throttles so if I got it wrong then we keep accumulating dead weakptrs. https://codereview.chromium.org/25713007/diff/148001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:1193: state_ = UNBLOCKED; if I Resume() without have been blocked it triggers a DCHECK in: void ThrottlingResourceHandler::Resume() { DCHECK(!cancelled_by_resource_throttle_); DeferredStage last_deferred_stage = deferred_stage_; deferred_stage_ = DEFERRED_NONE; switch (last_deferred_stage) { case DEFERRED_NONE: NOTREACHED(); <----- this one break; case DEFERRED_START: ResumeStart(); break; case DEFERRED_REDIRECT: ResumeRedirect(); break; case DEFERRED_RESPONSE: ResumeResponse(); break; } }
thanks I'll try it out -- is there supposed to be a patch for crx_update_item.h also? https://codereview.chromium.org/25713007/diff/148001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/148001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:599: UnblockandReapAllThrottles(&item->throttles); is there a missing file in the patch (crx_update_item.h)? https://codereview.chromium.org/25713007/diff/148001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/148001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:208: if (headers.GetHeader("Accept", &accept_headers)) { If the component is already installed, and there is no version mismatch (between chrome and the component) then we don't really need to do an OnDemand update. Would that help reduce the amount of appended throttles (even though there will be a little more work here to check that)?
I was missing a file CL update at patch set 7 https://codereview.chromium.org/25713007/diff/148001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/148001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:208: if (headers.GetHeader("Accept", &accept_headers)) { On 2013/10/30 18:50:41, jvoung (cr) wrote: > If the component is already installed, and there is no version mismatch (between > chrome and the component) then we don't really need to do an OnDemand update. > Would that help reduce the amount of appended throttles (even though there will > be a little more work here to check that)? That would help a lot, can we check synchronously from the IO thread here?
Just a couple of syntactic comments, still munching on the functional part of the change list. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:252: explicit CUResourceThrottle(const net::URLRequest* request, explicit only makes sense with one param ctors. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:748: empty line not needed. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.h:196: // Returns a resource throttler. It imeans that a component will be downloaded imeans https://codereview.chromium.org/25713007/diff/208001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:198: void AppendComponentUpaterThrottles( Updater
https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:599: UnblockandReapAllThrottles(&item->throttles); Are we sure that we only have to do this if there is an observer? https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.h:197: // and installed before the resource is untrottled. Note that is this called also, untrottled -> unthrottled https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.h:207: friend class PnaclComponentInstaller; Is the plan to remove this friendship and the old PNaCl on-demand flow in a separate changelist? https://codereview.chromium.org/25713007/diff/208001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:348: if (!is_prerendering) { I don't know too much about prerendering; will our system break if we prerender a page that requires some component resource, and then the user navigates to that page? We don't re-render it, I presume, so will we have missed the request that we can throttle?
code has not changed. Just addressing some comments. So far I agree with all the comments and I will change the code tonight, so feel free reviewing the code as is. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:252: explicit CUResourceThrottle(const net::URLRequest* request, On 2013/10/30 21:02:44, Sorin Jianu wrote: > explicit only makes sense with one param ctors. I believe the crx_id_ and crx_id are unnecessary. Vestigial from a previous approach. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:599: UnblockandReapAllThrottles(&item->throttles); On 2013/10/30 21:11:20, waffles wrote: > Are we sure that we only have to do this if there is an observer? No. Good catch. Reaping needs to happen every time. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:348: if (!is_prerendering) { On 2013/10/30 21:11:20, waffles wrote: > I don't know too much about prerendering; will our system break if we prerender > a page that requires some component resource, and then the user navigates to > that page? We don't re-render it, I presume, so will we have missed the request > that we can throttle? Your question is very interesting. Pre-rendering avoids fetching most things that cause perceivable behavior such as video/audio/extensions/plugins/etc. So while possible that a corner case slips by the opposite case is worse because we clog the pipes and since we end up discarding most of the pre-renderings anyway it is a wasted effort. I'll ask Darin to make sure this is the right tradeoff.
Thank you Carlos. My main observation would be to make sure we don't have races between the owner of the resource throttle and the code that posts to it using the weak pointer. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:248: class CUResourceThrottle would it be a good idea to define this class in its own compilation unit and place it in the component_updater namespace? Will we have unit tests for this class? https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:272: void UnblockResourceThrottle(base::WeakPtr<CUResourceThrottle> rt) { is it possible that when we call this function, the owner of |rt| has already destroyed the object and the weak pointer is NULL? https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:283: while (it != throttles->end()) { Is there a reason to write this as a while loop instead of the more idiomatic for? https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:316: virtual content::ResourceThrottle* GetResourceThrottle( Would it be an improvement to rename this function as GetOnDemandResourceThrottle? https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:1158: item->throttles.push_back(rt); DCHECK the item is not NULL or defensively check the condition? Internal functions are refactored many times, maybe a mistake is made during the refactoring. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.h:196: // Returns a resource throttler. It imeans that a component will be downloaded small nit, maybe say "resource throttle" instead of "resource throttler". https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.h:199: virtual content::ResourceThrottle* GetResourceThrottle( Since now this is part of the public interface of the class, how do we address one of the past concerns: devs abusing this interface and calling ondemand aggressively to get ahead of the queue?
https://codereview.chromium.org/25713007/diff/148001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/148001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:208: if (headers.GetHeader("Accept", &accept_headers)) { On 2013/10/30 20:20:25, cpu wrote: > On 2013/10/30 18:50:41, jvoung (cr) wrote: > > If the component is already installed, and there is no version mismatch > (between > > chrome and the component) then we don't really need to do an OnDemand update. > > Would that help reduce the amount of appended throttles (even though there > will > > be a little more work here to check that)? > > That would help a lot, can we check synchronously from the IO thread here? > > I'm thinking that we could set some information when the component registers with the service. A sort of "MayUseOnDemand" bit. Before the component registers with the service, it would do the necessary blocking file and directory checks to determine the bit value. After the component registers, it shouldn't need to flip from false to true so this could just use that bit without more file operations.
https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:1157: if (status == kOk || status == kInProgress) { Hi Carlos, I just ran some tests and here are some observations: I was wondering why (status == kOk || status == kInProgress) vs just status == kOk, here. At first I thought it's because there is a race between this OnDemandUpdateInternal() call, and the one currently in PnaclComponentInstaller::RequestFirstInstall(). That doesn't seem to be the case. The resource throttle blocks the code in ppapi/native_client/src/trusted/plugin/pnacl_coordinator so that PnaclComponentInstaller::RequestFirstInstall() happens later and this happens first. However, the call here currently fails to FindUpdateItemById (at least on Linux), so the code in PnaclComponentInstaller::RequestFirstInstall() does still kick in and sets up Observers, etc. If I change the code to use std::string& instead of char*, then this call works and the call in PnaclComponentInstaller::RequestFirstInstall() never kicks in and it never sets up an Observer, etc. At some point it would be nice to have a way to know the download progress though, not that the current Observer setup helps with that.
appengine seems to be having serious issues but hopefully the new version of the code is up. PTAL. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:248: class CUResourceThrottle On 2013/10/30 22:09:13, Sorin Jianu wrote: > would it be a good idea to define this class in its own compilation unit and > place it in the component_updater namespace? > > Will we have unit tests for this class? By itself I was not planning to. It is so simple at least in my mind. Let's table this idea until we get to the tests which have been giving me a good fight. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:252: explicit CUResourceThrottle(const net::URLRequest* request, On 2013/10/30 21:43:05, cpu wrote: > On 2013/10/30 21:02:44, Sorin Jianu wrote: > > explicit only makes sense with one param ctors. > > I believe the crx_id_ and crx_id are unnecessary. Vestigial from a previous > approach. Done. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:272: void UnblockResourceThrottle(base::WeakPtr<CUResourceThrottle> rt) { On 2013/10/30 22:09:13, Sorin Jianu wrote: > is it possible that when we call this function, the owner of |rt| has already > destroyed the object and the weak pointer is NULL? Yes, sort of. The weak pointer is ours and guaranteed not null, but the actual object could have been destroyed. Bind and PostTask know about weak pointers and simply drop the posttask silently at the last moment. Its pretty nifty. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:283: while (it != throttles->end()) { On 2013/10/30 22:09:13, Sorin Jianu wrote: > Is there a reason to write this as a while loop instead of the more idiomatic > for? I have a secret fear of loops without { and } , I changed it anyhow. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:316: virtual content::ResourceThrottle* GetResourceThrottle( On 2013/10/30 22:09:13, Sorin Jianu wrote: > Would it be an improvement to rename this function as > GetOnDemandResourceThrottle? Done. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:599: UnblockandReapAllThrottles(&item->throttles); On 2013/10/30 21:43:05, cpu wrote: > On 2013/10/30 21:11:20, waffles wrote: > > Are we sure that we only have to do this if there is an observer? > > No. Good catch. Reaping needs to happen every time. Done. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:748: On 2013/10/30 21:02:44, Sorin Jianu wrote: > empty line not needed. Done. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:1157: if (status == kOk || status == kInProgress) { I think you want to change without RequestFirstInstall(), In other words let this code be the only one asking for on-demand install. >> status == kInProgress I check just because it can be in progress because of the timer-based check + download, we still need to block the nexe then. changed char* to string& https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:1158: item->throttles.push_back(rt); On 2013/10/30 22:09:13, Sorin Jianu wrote: > DCHECK the item is not NULL or defensively check the condition? Internal > functions are refactored many times, maybe a mistake is made during the > refactoring. right now it will crash if it is null given (0)->throttles.push_back() is guaranteed to crash. I guess I could add it to the if statement but then I might be masking that FindUpdateItemById is broken? https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.h:196: // Returns a resource throttler. It imeans that a component will be downloaded On 2013/10/30 21:02:44, Sorin Jianu wrote: > imeans Done. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.h:197: // and installed before the resource is untrottled. Note that is this called On 2013/10/30 21:11:20, waffles wrote: > also, untrottled -> unthrottled Done. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.h:199: virtual content::ResourceThrottle* GetResourceThrottle( On 2013/10/30 22:09:13, Sorin Jianu wrote: > Since now this is part of the public interface of the class, how do we address > one of the past concerns: devs abusing this interface and calling ondemand > aggressively to get ahead of the queue? For it to work it requires a fair bit of infrastructure. It is not something one can do by accident, but we could also friend the current client which besides tests should be the only caller. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.h:207: friend class PnaclComponentInstaller; On 2013/10/30 21:11:20, waffles wrote: > Is the plan to remove this friendship and the old PNaCl on-demand flow in a > separate changelist? Yes, possibly in this change-list. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:198: void AppendComponentUpaterThrottles( On 2013/10/30 21:02:44, Sorin Jianu wrote: > Updater Done.
There could be a small race condition due to weak resource ownership. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:272: void UnblockResourceThrottle(base::WeakPtr<CUResourceThrottle> rt) { On 2013/10/31 04:38:26, cpu wrote: > On 2013/10/30 22:09:13, Sorin Jianu wrote: > > is it possible that when we call this function, the owner of |rt| has already > > destroyed the object and the weak pointer is NULL? > > Yes, sort of. The weak pointer is ours and guaranteed not null, but the actual > object could have been destroyed. Bind and PostTask know about weak pointers and > simply drop the posttask silently at the last moment. Its pretty nifty. I could not code search where in the code base that happens but I trust you. I assume that the Bind/PostTask machinery has a test for the weak pointer. Is there a race condition where the test passes, the thread is preempted, the object the weak pointer points to is destroyed on another thread, then the CUResourceThrottle::Unblock is called on an invalid object. We don't control at all what happens with the instance of this object once it is handed out to the caller of the GetOnDemandResourceThrottle. What about the race condition where the Bind and PostTask test for the weak pointer https://codereview.chromium.org/25713007/diff/328001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/328001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:284: UnblockResourceThrottle(*it); I share your concern about loops without {}. That being said, is it not acceptable at all in the code base to fully brace the body of the for in this case? https://codereview.chromium.org/25713007/diff/328001/chrome/browser/component... File chrome/browser/component_updater/crx_update_item.h (right): https://codereview.chromium.org/25713007/diff/328001/chrome/browser/component... chrome/browser/component_updater/crx_update_item.h:108: std::vector<base::WeakPtr<CUResourceThrottle>> throttles; what is the opinion in the codebase relative to >> vs. > > in the context above? Most of the code seems to be using > >.
New (final?) version uploaded please take a look. The unittests are being done in a different CL to speed progress. Sorin, in the unittest CL I specifically excessive the 'dead throttle' case. I am confident that that part of the code is ok. Jan reports success with manual testing and he will remove the previous 'friended' code in a different CL. I tried to but it is fairly involved. https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:272: void UnblockResourceThrottle(base::WeakPtr<CUResourceThrottle> rt) { I believe the test is done in the destination thread. Under the covers there is a sentinel object that is both refcounted and has observers. >> We don't control at all what happens with the instance of this object Correct. https://codereview.chromium.org/25713007/diff/328001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/328001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:284: UnblockResourceThrottle(*it); On 2013/10/31 18:43:58, Sorin Jianu wrote: > I share your concern about loops without {}. That being said, is it not > acceptable at all in the code base to fully brace the body of the for in this > case? Most chromistas would object to that. https://codereview.chromium.org/25713007/diff/328001/chrome/browser/component... File chrome/browser/component_updater/crx_update_item.h (right): https://codereview.chromium.org/25713007/diff/328001/chrome/browser/component... chrome/browser/component_updater/crx_update_item.h:108: std::vector<base::WeakPtr<CUResourceThrottle>> throttles; On 2013/10/31 18:43:58, Sorin Jianu wrote: > what is the opinion in the codebase relative to >> vs. > > in the context above? > Most of the code seems to be using > >. Good catch. VS can handle but did not compile on *nix for this reason. Changed.
lgtm Thank you Carlos! https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:272: void UnblockResourceThrottle(base::WeakPtr<CUResourceThrottle> rt) { On 2013/10/31 21:36:08, cpu wrote: > I believe the test is done in the destination thread. Under the covers there is > a sentinel object that is both refcounted and has observers. > > >> We don't control at all what happens with the instance of this object > > Correct. > At this moment, I am not convinced we don't have a race condition here but I don't know enough about the code base nor I have the time to dig further. I will trust your judgement here.
lgtm https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/208001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:272: void UnblockResourceThrottle(base::WeakPtr<CUResourceThrottle> rt) { On 2013/10/31 21:59:32, Sorin Jianu wrote: > On 2013/10/31 21:36:08, cpu wrote: > > I believe the test is done in the destination thread. Under the covers there > is > > a sentinel object that is both refcounted and has observers. > > > > >> We don't control at all what happens with the instance of this object > > > > Correct. > > > > At this moment, I am not convinced we don't have a race condition here but I > don't know enough about the code base nor I have the time to dig further. I will > trust your judgement here. To me it seems like this is safe only if the IO thread owns the CUResourceThrottle in question. Otherwise, we might begin execution of the throttle but have the throttle destroyed midway through executing Unblock(). According to the contract of the class, "This class is touched solely from the IO thread", so as long as callers obey that, it should be good. (But this means that users of this interface have to be careful about where/how they store this pointer, since they may need to post a task to destroy it while their owning object is being destroyed down in another thread. A scoped_refptr seems safer to me, but I don't think this is a bug... yet.)
https://codereview.chromium.org/25713007/diff/408001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/408001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:1142: // from the UI thread without having to track lifetime directly. Should we add a DCHECK here (and maybe in the throttle's destructor) that we are on the IO thread?
ty! https://codereview.chromium.org/25713007/diff/408001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/408001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:245: // updated. This class is touched solely from the IO thread. The UI thread Consider adding checks in the members of this class to validate this assumption, especially the ctor and dtor?
https://codereview.chromium.org/25713007/diff/408001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/408001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:12: #include "base/strings/string_util.h" Just checking, what is string_util.h used for? (in case it was used for something more sophisticated that headers.find("...") in an old incarnation of the code). https://codereview.chromium.org/25713007/diff/408001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:210: crx_id = "hnimpnehoodheedghdeeijklkeaacbdc"; FYI: with GCC I got the following error: ../../chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc: In function 'void {anonymous}::AppendComponentUpdaterThrottles(net::URLRequest*, content::ResourceContext*, ResourceType::Type, ScopedVector<content::ResourceThrottle>*)': ../../chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:210:18: error: deprecated conversion from string constant to 'char*' [-Werror=write-strings]
thanks, checking in soon. https://codereview.chromium.org/25713007/diff/408001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/408001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:245: // updated. This class is touched solely from the IO thread. The UI thread On 2013/10/31 22:39:36, Sorin Jianu wrote: > Consider adding checks in the members of this class to validate this assumption, > especially the ctor and dtor? Done. https://codereview.chromium.org/25713007/diff/408001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:1142: // from the UI thread without having to track lifetime directly. On 2013/10/31 22:34:43, waffles wrote: > Should we add a DCHECK here (and maybe in the throttle's destructor) that we are > on the IO thread? Done. The ctor has it so we keep both dchecks in the CUResourceThrottle. https://codereview.chromium.org/25713007/diff/408001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/408001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:12: #include "base/strings/string_util.h" On 2013/10/31 22:45:17, jvoung (cr) wrote: > Just checking, what is string_util.h used for? (in case it was used for > something more sophisticated that headers.find("...") in an old incarnation of > the code). Not needed. removed. https://codereview.chromium.org/25713007/diff/408001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:210: crx_id = "hnimpnehoodheedghdeeijklkeaacbdc"; On 2013/10/31 22:45:17, jvoung (cr) wrote: > FYI: with GCC I got the following error: > > ../../chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc: > In function 'void {anonymous}::AppendComponentUpdaterThrottles(net::URLRequest*, > content::ResourceContext*, ResourceType::Type, > ScopedVector<content::ResourceThrottle>*)': > ../../chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:210:18: > error: deprecated conversion from string constant to 'char*' > [-Werror=write-strings] Done.
https://codereview.chromium.org/25713007/diff/538001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/25713007/diff/538001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:266: enum State { I think this is indented by 3 spaces.
LGTM, just one concern: https://codereview.chromium.org/25713007/diff/538001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/538001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:347: if (!is_prerendering) { we should probably make sure that the presence of a pnacl module causes prerendering to be disabled, or we should not exclude prerendering here. https://codereview.chromium.org/25713007/diff/538001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:349: resource_context, nit: indentation
https://codereview.chromium.org/25713007/diff/538001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/25713007/diff/538001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:347: if (!is_prerendering) { Yeah, nacl is disabled since the infamous PinkyPie exploit. On 2013/11/01 21:14:41, darin wrote: > we should probably make sure that the presence of a pnacl module causes > prerendering to be disabled, or we should not exclude prerendering here. https://codereview.chromium.org/25713007/diff/538001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:349: resource_context, On 2013/11/01 21:14:41, darin wrote: > nit: indentation Done.
Message was sent while issue was closed.
Committed patchset #6 manually as r232517 (presubmit successful).
Message was sent while issue was closed.
xxx
Message was sent while issue was closed.
Committed patchset #7 manually as r232580. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
