Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829356:
Make sure that AppCache tests subresource URLrequests against NETWORK whitelist (Closed)
|
Created:
8 years, 4 months ago by vabr (Chromium) Modified:
8 years, 3 months ago Reviewers:
michaeln CC:
chromium-reviews, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake sure that AppCache tests subresource URLrequests against NETWORK whitelist
even if this is a redirect.
BUG=141114
TEST=Follow the reproduction steps in the BUG description and fail to reproduce the bug.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155588
Patch Set 1 #
Total comments: 6
Patch Set 2 : MaybeLoadFallbackForRedirect uses MaybeLoadResource #
Total comments: 8
Patch Set 3 : Giving up on Redirect fallback if the request has not been seen before #
Total comments: 4
Patch Set 4 : TODO added #Messages
Total messages: 12 (0 generated)
https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... File webkit/appcache/appcache_request_handler.cc (right): https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... webkit/appcache/appcache_request_handler.cc:102: I think there's an additional condition that has to be taken care of... if (host_->is_selection_pending()) { // Stuff is being read out of the db on a background thread, // FindResponseForSubRequest() can't be used until it's loaded up. // See MaybeLoadSubResource() and OnCacheSelectionComplete() for // how that's handled when this class gets a chance to intercept // the request from the Start(). We provide a 'job' in that case, // but the 'job' just sits there and waits for OnCacheSelectionComplete() // prior to continuing. } https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... webkit/appcache/appcache_request_handler.cc:106: &found_entry_, &found_fallback_entry_, &found_network_namespace_); What should we do if found_entry_.has_response_id() at this point? The originally requested url is in the appcache, and we've been redirected via WebRequest (presumably) to go elsewhere. There's no clause for that particular case below. As coded, the appcache will synthesize an error and the page won't work (despite there being a cached resource ready to go). https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... webkit/appcache/appcache_request_handler.cc:109: if (found_fallback_entry_.has_response_id()) { As coded we'll will load an appcached fallback resource and disregard the redirect in this case where we've found a fallback resource. From the appcache consumers point of view, the loading of a fallback resource is a network error handling behavior. Not really sure that behavior is appropriate in this case.
Hi Michael, When trying to address your comments, it looked to me like like calling MaybeLoadResource from within MayleLoadFallbackForRedirect is the most appropriate way. Please let me know what do you think about patch set 2, Vaclav https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... File webkit/appcache/appcache_request_handler.cc (right): https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... webkit/appcache/appcache_request_handler.cc:102: On 2012/08/17 00:50:43, michaeln wrote: > I think there's an additional condition that has to be taken care of... > > if (host_->is_selection_pending()) { > // Stuff is being read out of the db on a background thread, > // FindResponseForSubRequest() can't be used until it's loaded up. > // See MaybeLoadSubResource() and OnCacheSelectionComplete() for > // how that's handled when this class gets a chance to intercept > // the request from the Start(). We provide a 'job' in that case, > // but the 'job' just sits there and waits for OnCacheSelectionComplete() > // prior to continuing. > } This should now be addressed by the call to MaybeLoadResources. https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... webkit/appcache/appcache_request_handler.cc:106: &found_entry_, &found_fallback_entry_, &found_network_namespace_); On 2012/08/17 00:50:43, michaeln wrote: > What should we do if found_entry_.has_response_id() at this point? The > originally requested url Here I realised that I should use original_url(), not url(). > is in the appcache, and we've been redirected via > WebRequest (presumably) to go elsewhere. There's no clause for that particular > case below. As coded, the appcache will synthesize an error and the page won't > work (despite there being a cached resource ready to go). Ideally, we should let the user know what is happening, but this is beyond this CL and bug 141114 (it is within the scope of 121325). As a workaround, I chose to let AppCache use the cached page. This would happen, if AppCache got its hands on the request before it was hijacked via NetworkDelegate. https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... webkit/appcache/appcache_request_handler.cc:109: if (found_fallback_entry_.has_response_id()) { On 2012/08/17 00:50:43, michaeln wrote: > As coded we'll will load an appcached fallback resource and disregard the > redirect in this case where we've found a fallback resource. From the appcache > consumers point of view, the loading of a fallback resource is a network error > handling behavior. Not really sure that behavior is appropriate in this case. Through the use of MaybeLoadResource within MaybeLoadFallbackForRedirect we end up serving fallback resource if the original_url is not cached, and also not network-whitelisted (if I understood correctly that found_fallback_entry_.has_response_id() and found_network_namespace_ cannot both be true). Are there still situations here where serving the fallback would be inappropriate?
Hi Michael, A friendly ping about this CL. If you feel I am going totally in a wrong direction, please do not hesitate to tell me. Thanks, Vaclav On 2012/08/17 16:22:35, vabr (Chromium) wrote: > Hi Michael, > > When trying to address your comments, it looked to me like like calling > MaybeLoadResource from within MayleLoadFallbackForRedirect is the most > appropriate way. > > Please let me know what do you think about patch set 2, > > Vaclav > > https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... > File webkit/appcache/appcache_request_handler.cc (right): > > https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... > webkit/appcache/appcache_request_handler.cc:102: > On 2012/08/17 00:50:43, michaeln wrote: > > I think there's an additional condition that has to be taken care of... > > > > if (host_->is_selection_pending()) { > > // Stuff is being read out of the db on a background thread, > > // FindResponseForSubRequest() can't be used until it's loaded up. > > // See MaybeLoadSubResource() and OnCacheSelectionComplete() for > > // how that's handled when this class gets a chance to intercept > > // the request from the Start(). We provide a 'job' in that case, > > // but the 'job' just sits there and waits for OnCacheSelectionComplete() > > // prior to continuing. > > } > > This should now be addressed by the call to MaybeLoadResources. > > https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... > webkit/appcache/appcache_request_handler.cc:106: &found_entry_, > &found_fallback_entry_, &found_network_namespace_); > On 2012/08/17 00:50:43, michaeln wrote: > > What should we do if found_entry_.has_response_id() at this point? The > > originally requested url > > Here I realised that I should use original_url(), not url(). > > > is in the appcache, and we've been redirected via > > WebRequest (presumably) to go elsewhere. There's no clause for that particular > > case below. As coded, the appcache will synthesize an error and the page won't > > work (despite there being a cached resource ready to go). > > Ideally, we should let the user know what is happening, but this is beyond this > CL and bug 141114 (it is within the scope of 121325). As a workaround, I chose > to let AppCache use the cached page. This would happen, if AppCache got its > hands on the request before it was hijacked via NetworkDelegate. > > https://chromiumcodereview.appspot.com/10829356/diff/1/webkit/appcache/appcac... > webkit/appcache/appcache_request_handler.cc:109: if > (found_fallback_entry_.has_response_id()) { > On 2012/08/17 00:50:43, michaeln wrote: > > As coded we'll will load an appcached fallback resource and disregard the > > redirect in this case where we've found a fallback resource. From the appcache > > consumers point of view, the loading of a fallback resource is a network error > > handling behavior. Not really sure that behavior is appropriate in this case. > > Through the use of MaybeLoadResource within MaybeLoadFallbackForRedirect we end > up serving fallback resource if the original_url is not cached, and also not > network-whitelisted (if I understood correctly that > found_fallback_entry_.has_response_id() and found_network_namespace_ cannot both > be true). Are there still situations here where serving the fallback would be > inappropriate?
https://chromiumcodereview.appspot.com/10829356/diff/4001/webkit/appcache/app... File webkit/appcache/appcache_request_handler.cc (right): https://chromiumcodereview.appspot.com/10829356/diff/4001/webkit/appcache/app... webkit/appcache/appcache_request_handler.cc:97: return NULL; a very small change that might workaround the immediate failure mode (if not actually 'fix' the problem of how the heck to integrate these two things together) could be to return NULL here if !maybe_load_resource_executed_ ? https://chromiumcodereview.appspot.com/10829356/diff/4001/webkit/appcache/app... webkit/appcache/appcache_request_handler.cc:108: return MaybeLoadResource(request); How is the 'fallback' behavior triggered if we go down the main resource codepath, i guess there's no case for which we should ever invoke the fallback? https://chromiumcodereview.appspot.com/10829356/diff/4001/webkit/appcache/app... webkit/appcache/appcache_request_handler.cc:132: return NULL; ditto if (!maybe_load_resource_executed_) return NULL? https://chromiumcodereview.appspot.com/10829356/diff/4001/webkit/appcache/app... webkit/appcache/appcache_request_handler.cc:317: const GURL& url = job_->request()->original_url(); It doesn't look like will work when following a chain of redirects. The 'handler' instance lives for the duration of of the URLRequest. When being queried about intercepting the 2nd location in a chain, this logic will look for a hit for the original url. https://chromiumcodereview.appspot.com/10829356/diff/4001/webkit/appcache/app... File webkit/appcache/appcache_request_handler.h (right): https://chromiumcodereview.appspot.com/10829356/diff/4001/webkit/appcache/app... webkit/appcache/appcache_request_handler.h:122: bool maybe_load_resource_executed_; ah... this is a much clearer name
Hi Michael, Thanks for your comments. Before addressing them, I have one more question to one of them. Vaclav http://codereview.chromium.org/10829356/diff/4001/webkit/appcache/appcache_re... File webkit/appcache/appcache_request_handler.cc (right): http://codereview.chromium.org/10829356/diff/4001/webkit/appcache/appcache_re... webkit/appcache/appcache_request_handler.cc:97: return NULL; On 2012/08/25 01:33:41, michaeln wrote: > a very small change that might workaround the immediate failure mode (if not > actually 'fix' the problem of how the heck to integrate these two things > together) could be to return NULL here if !maybe_load_resource_executed_ ? I like very much this suggestion. However, it will mean that app-caching will be switched off for pages which get redirected. Do you think it would make sense to put here something like the following? if (!maybe_load_resource_executed_ && IsNetworkWhitelisted(request->original_url())) return NULL;
https://chromiumcodereview.appspot.com/10829356/diff/4001/webkit/appcache/app... File webkit/appcache/appcache_request_handler.cc (right): https://chromiumcodereview.appspot.com/10829356/diff/4001/webkit/appcache/app... webkit/appcache/appcache_request_handler.cc:97: return NULL; > I like very much this suggestion. > > However, it will mean that app-caching will be switched off for pages which get > redirected. No worse than the current state of things where both appcache'ing and webrequest'ing is busted. The result is that for now at least, webrequest based redirection trumps whatever the appcache may have to say. > Do you think it would make sense to put here something like the > following? > > if (!maybe_load_resource_executed_ && > IsNetworkWhitelisted(request->original_url())) > return NULL; I'm not sure what would that accomplish really? Suppose the original url is appcached (and not in a network or fallback namespace), if we proceed, the request will result in an error response. I don't see that as an improvement.
Hi Michael, I followed your suggestion about AppCache giving up in the redirect fallback method, if the request was not seen during MaybeLoadResource. I also tested that this gets rid of the issue from the associated bug (141114). Does this look like an acceptable fix? It will need to be there until the solution to the more general problem. At that point we will need to make sure AppCache will handle all requests it sees for the first time as non-redirects, independent of whether they are whitelisted or not. But this is not part of this CL, I only state it to get an idea for how long this temporary fix will need to be in the code. What do you think? Thanks, Vaclav https://chromiumcodereview.appspot.com/10829356/diff/4001/webkit/appcache/app... File webkit/appcache/appcache_request_handler.cc (right): https://chromiumcodereview.appspot.com/10829356/diff/4001/webkit/appcache/app... webkit/appcache/appcache_request_handler.cc:97: return NULL; On 2012/09/05 19:24:21, michaeln wrote: > > I like very much this suggestion. > > > > However, it will mean that app-caching will be switched off for pages which > get > > redirected. > > No worse than the current state of things where both appcache'ing and > webrequest'ing is busted. The result is that for now at least, webrequest based > redirection trumps whatever the appcache may have to say. > > > Do you think it would make sense to put here something like the > > following? > > > > if (!maybe_load_resource_executed_ && > > IsNetworkWhitelisted(request->original_url())) > > return NULL; > > I'm not sure what would that accomplish really? Suppose the original url is > appcached (and not in a network or fallback namespace), if we proceed, the > request will result in an error response. I don't see that as an improvement. > I agree that this is not making things worse. When I suggested the "IsNetworkWhitelisted" I was thinking of not messing up redirects from other sources than WebRequest. But for external redirects |maybe_load_resource_executed_| will always be true, so that's not an issue. https://chromiumcodereview.appspot.com/10829356/diff/16001/webkit/appcache/ap... File webkit/appcache/appcache_request_handler.cc (right): https://chromiumcodereview.appspot.com/10829356/diff/16001/webkit/appcache/ap... webkit/appcache/appcache_request_handler.cc:130: return NULL; I did not add any condition "!maybe_load_resource_executed_" here, because this path is not influencing the issue in http://crbug.com/141114, and I wanted to keep my changes minimal.
I think this lgtm, but i don't understand this comment... > At that point we will need to make sure AppCache will handle all requests it > sees for the first time as non-redirects, independent of whether they are > whitelisted or not. ... can you explain what you mean here? Are you saying more is needed to resolve the more limited crbug/141114? https://chromiumcodereview.appspot.com/10829356/diff/16001/webkit/appcache/ap... File webkit/appcache/appcache_request_handler.cc (right): https://chromiumcodereview.appspot.com/10829356/diff/16001/webkit/appcache/ap... webkit/appcache/appcache_request_handler.cc:100: if (!maybe_load_resource_executed_) Please add a TODO here points at crbug/121325 https://chromiumcodereview.appspot.com/10829356/diff/16001/webkit/appcache/ap... webkit/appcache/appcache_request_handler.cc:130: return NULL; On 2012/09/06 17:34:21, vabr (Chromium) wrote: > I did not add any condition "!maybe_load_resource_executed_" here, because this > path is not influencing the issue in http://crbug.com/141114, and I wanted to > keep my changes minimal. Ok. Since found_fallback_entry_ will not have anything unless we've gone thru the MaybeLoadResource, this s/b functionally equivalent w/o the check. I was looking for this check more as a matter of documenting that we can get here w/o having gone thru that (but its pretty clear given the data member and all that).
Thanks, Michael. Please check that you like the wording of the TODO. Sorry for the confusing comment which you cited. I did not mean that more is needed to fix crbug/141114. The only reason I made that comment was to give an idea of how long will the fix need to be in the code. Let me please know if that clarifies the comment, and also whether you have any additional questions/issues with this CL. Many thanks! Vaclav https://chromiumcodereview.appspot.com/10829356/diff/16001/webkit/appcache/ap... File webkit/appcache/appcache_request_handler.cc (right): https://chromiumcodereview.appspot.com/10829356/diff/16001/webkit/appcache/ap... webkit/appcache/appcache_request_handler.cc:100: if (!maybe_load_resource_executed_) On 2012/09/06 19:49:05, michaeln wrote: > Please add a TODO here points at crbug/121325 Done.
thnx, lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/10829356/24001
Change committed as 155588 |
