Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6611023:
The optional Malware Details also collects HTTP cache information. See design... (Closed)
|
Created:
9 years, 9 months ago by panayiotis Modified:
9 years, 7 months ago CC:
chromium-reviews, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionThe optional Malware Details also collects HTTP cache information. See design doc for background (http://goo.gl/omvhq)
Bug=None
TEST=None
Patch Set 1 #Patch Set 2 : '' #
Total comments: 16
Patch Set 3 : '' #
Total comments: 1
Patch Set 4 : '' #
Total comments: 55
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 9
Patch Set 7 : '' #
Total comments: 7
Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 4
Patch Set 11 : '' #Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #Patch Set 16 : '' #Patch Set 17 : '' #Messages
Total messages: 25 (0 generated)
Panayiotis: Sorry for the late reply. I did not drill into too much details on the caching code. But I got a couple of high level feedbacks. Mainly: 1. I would suggest to wrap the cache code in its own class; 2. Start to collect cache from the blocking page, so we could limit the consumers of the MalwareDetails class. Let me know if you have questions or want to discuss more. Thanks! Lei http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/malware_details.cc (right): http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/malware_details.cc:219: request_context_getter_ = tab_contents()->profile()->GetRequestContext(); Might be better to put this in the constructor. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/malware_details.cc:237: // If we have already started collecting data from the HTTP cache, don't nit: add an empty line above. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/malware_details.cc:393: entry_callback_->AddRef(); Since MalwareDetails is refcounted, while waiting for callback, entry_callack_ won't go away (however, in this case, you have to delete entry_callback_ to let it release the ref count for MalwareDetails, otherwise, both of them will stay forever). I did not look carefully about the other callback. However, I do hope we could avoid AddRef and Release. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/malware_details.h (right): http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/malware_details.h:81: virtual ~MalwareDetails(); I think ~MalwareDetails() should be right after MalwareDetails(xxx). http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/malware_details.h:93: ResourceMap; I would suggest to move the cache-mining code to its own class. It will make the MalwareDetails class easier to follow. Besides the cache relate code is relative complicated and also self-containing. You might want to add some comments regarding the relations among the threads. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/report.proto (right): http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/report.proto:91: // Whether the report is complete. Not very clear what "complete" is, can you explain more? http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/safe_browsing_service.cc:1039: details->AddRef(); We should avoid AddRef() and Release(). And I don't think you need the AddRef() and Release() below. The MalwareDetails uses 'this' in the run(xx), right? Also, I think we should move the StartCacheCollection() call and its callback to SafeBrowsingBlockingPage: the malware_details is created there and it is easier to follow if that class drives this action. So, here SafeBrowsingService only acts as a proxy to pass the result one. Maybe we could change this API as acception string only too. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/safe_browsing_service.h (right): http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/safe_browsing_service.h:236: // When the MalwareDetails is done, OnReportReady is called. Maybe more explanation on what is "MalwareDetails is done"?
Thanks Lei, both suggestions sound reasonable, will get back to this next week. On Fri, Mar 4, 2011 at 4:37 PM, <lzheng@chromium.org> wrote: > Panayiotis: > > Sorry for the late reply. I did not drill into too much details on the > caching > code. But I got a couple of high level feedbacks. Mainly: > 1. I would suggest to wrap the cache code in its own class; > 2. Start to collect cache from the blocking page, so we could limit the > consumers of the MalwareDetails class. > > Let me know if you have questions or want to discuss more. > > Thanks! > > Lei > > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > File chrome/browser/safe_browsing/malware_details.cc (right): > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > chrome/browser/safe_browsing/malware_details.cc:219: > request_context_getter_ = > tab_contents()->profile()->GetRequestContext(); > Might be better to put this in the constructor. > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > chrome/browser/safe_browsing/malware_details.cc:237: // If we have > already started collecting data from the HTTP cache, don't > nit: add an empty line above. > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > chrome/browser/safe_browsing/malware_details.cc:393: > entry_callback_->AddRef(); > Since MalwareDetails is refcounted, while waiting for callback, > entry_callack_ won't go away (however, in this case, you have to delete > entry_callback_ to let it release the ref count for MalwareDetails, > otherwise, both of them will stay forever). I did not look carefully > about the other callback. However, I do hope we could avoid AddRef and > Release. > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > File chrome/browser/safe_browsing/malware_details.h (right): > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > chrome/browser/safe_browsing/malware_details.h:81: virtual > ~MalwareDetails(); > I think ~MalwareDetails() should be right after MalwareDetails(xxx). > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > chrome/browser/safe_browsing/malware_details.h:93: ResourceMap; > I would suggest to move the cache-mining code to its own class. It will > make the MalwareDetails class easier to follow. Besides the cache relate > code is relative complicated and also self-containing. > > You might want to add some comments regarding the relations among the > threads. > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > File chrome/browser/safe_browsing/report.proto (right): > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > chrome/browser/safe_browsing/report.proto:91: // Whether the report is > complete. > Not very clear what "complete" is, can you explain more? > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > File chrome/browser/safe_browsing/safe_browsing_service.cc (right): > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > chrome/browser/safe_browsing/safe_browsing_service.cc:1039: > details->AddRef(); > We should avoid AddRef() and Release(). And I don't think you need the > AddRef() and Release() below. The MalwareDetails uses 'this' in the > run(xx), right? > > Also, I think we should move the StartCacheCollection() call and its > callback to SafeBrowsingBlockingPage: the malware_details is created > there and it is easier to follow if that class drives this action. So, > here SafeBrowsingService only acts as a proxy to pass the result one. > Maybe we could change this API as acception string only too. > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > File chrome/browser/safe_browsing/safe_browsing_service.h (right): > > http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... > chrome/browser/safe_browsing/safe_browsing_service.h:236: // When the > MalwareDetails is done, OnReportReady is called. > Maybe more explanation on what is "MalwareDetails is done"? > > http://codereview.chromium.org/6611023/ >
http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/malware_details.cc (right): http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/malware_details.cc:219: request_context_getter_ = tab_contents()->profile()->GetRequestContext(); On 2011/03/05 00:37:50, lzheng wrote: > Might be better to put this in the constructor. Done. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/malware_details.cc:237: // If we have already started collecting data from the HTTP cache, don't On 2011/03/05 00:37:50, lzheng wrote: > nit: add an empty line above. Done. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/malware_details.cc:393: entry_callback_->AddRef(); I managed to not refcount this callback. But the cache_callback_ is not refcounted, which requires that both MalwareDetailsCache and MalwareDetails need to AddRef() to themselves ... I think. I can take another look. On 2011/03/05 00:37:50, lzheng wrote: > Since MalwareDetails is refcounted, while waiting for callback, entry_callack_ > won't go away (however, in this case, you have to delete entry_callback_ to let > it release the ref count for MalwareDetails, otherwise, both of them will stay > forever). I did not look carefully about the other callback. However, I do hope > we could avoid AddRef and Release. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/malware_details.h (right): http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/malware_details.h:81: virtual ~MalwareDetails(); On 2011/03/05 00:37:50, lzheng wrote: > I think ~MalwareDetails() should be right after MalwareDetails(xxx). Done. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/malware_details.h:93: ResourceMap; On 2011/03/05 00:37:50, lzheng wrote: > I would suggest to move the cache-mining code to its own class. It will make the > MalwareDetails class easier to follow. Besides the cache relate code is relative > complicated and also self-containing. > > You might want to add some comments regarding the relations among the threads. Done. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/report.proto (right): http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/report.proto:91: // Whether the report is complete. On 2011/03/05 00:37:50, lzheng wrote: > Not very clear what "complete" is, can you explain more? Done. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/safe_browsing_service.cc:1039: details->AddRef(); On 2011/03/05 00:37:50, lzheng wrote: > We should avoid AddRef() and Release(). And I don't think you need the AddRef() > and Release() below. The MalwareDetails uses 'this' in the run(xx), right? > > Also, I think we should move the StartCacheCollection() call and its callback to > SafeBrowsingBlockingPage: the malware_details is created there and it is easier > to follow if that class drives this action. So, here SafeBrowsingService only > acts as a proxy to pass the result one. Maybe we could change this API as > acception string only too. Done. http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/safe_browsing_service.h (right): http://codereview.chromium.org/6611023/diff/1010/chrome/browser/safe_browsing... chrome/browser/safe_browsing/safe_browsing_service.h:236: // When the MalwareDetails is done, OnReportReady is called. On 2011/03/05 00:37:50, lzheng wrote: > Maybe more explanation on what is "MalwareDetails is done"? Done.
http://codereview.chromium.org/6611023/diff/8001/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/malware_details.cc (right): http://codereview.chromium.org/6611023/diff/8001/chrome/browser/safe_browsing... chrome/browser/safe_browsing/malware_details.cc:256: AddRef(); // Ensures we remain alive during cache collection. I was able to get rid of this AddRef. Now the only one that remains is inside the cache collector, due to the calls to http_cache->GetBackend()
+ Matt Matt: Would you have to review this patch? I am a little bit distracted and not confident I can do a good review. On 2011/03/29 21:18:17, panayiotis wrote: > http://codereview.chromium.org/6611023/diff/8001/chrome/browser/safe_browsing... > File chrome/browser/safe_browsing/malware_details.cc (right): > > http://codereview.chromium.org/6611023/diff/8001/chrome/browser/safe_browsing... > chrome/browser/safe_browsing/malware_details.cc:256: AddRef(); // Ensures we > remain alive during cache collection. > I was able to get rid of this AddRef. Now the only one that remains is inside > the cache collector, due to the calls to http_cache->GetBackend()
Hi Panos, Looks really nice. i haven't looked at all the tests yet since they might still change a bit. nice work, noe. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.cc (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:268: std::string serialized; can you explain why you're serializing here instead of in the protocol manager for example? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:270: DLOG(ERROR) << "Unable to serialize the malware report."; Do you want to return here? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.h (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.h:110: SafeBrowsingService* sb_service_; I think you need a scoped ref pointer here just to be safe. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.cc (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:29: static const int kMaxBodySize = 1024; Is this in bytes or KB? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:45: // We modify |resources| and |result|, so it must remain alive. Consider moving this comment to the header file. It's more useful there. And what about the other pointers? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:71: return cache_state_ != STATE_NONE; I assume this is also always called on the IO thread? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:115: } while (rv != net::ERR_IO_PENDING && cache_state_ != STATE_NONE); I'm wondering if it's OK to do all these operations without breaking them up into multiple smaller bits. As far as I can tell it's possible and all these methods are called one after another if net::ERR_IO_PENDING is never returned. What is the likelihood of that happening? Wouldn't you take too much time on the IO thread in that case? I'm wondering if you don't want to force each of these methods to run as a separate task on the IO message loop. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:137: LOG(ERROR) << "Could not get URLRequestContext"; Is there a particular reason you LOG(ERROR) here but not below? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:182: return cache_->OpenEntry(resources_it_->first, &entry_, &cache_callback_); Maybe mention what resources_it_ refers to? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:188: return net::OK; I think there might be a bug here. I think OpenEntry might return et::ERR_IO_PENDING and use the cache_callback_ to say when the entry is available. What if an error occurs and rv is an error? In that case entry_ is probably invalid in which case you should probably abort, no? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:195: if (!entry_) { // No entry in the cache for this url. Is that how you handle errors mentioned above? Are you guaranteed that entry_ will not be assigned if there was an error in OpenEntry? It might be nicer to skip some of the stages (i.e., change the value of the cache_state_ if you know that the next stage isn't going to do anything anyways. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:200: buf_len_ = entry_->GetDataSize(0); why not use the constant then? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:203: return buf_len_; This is confusing. You always return net::something except here. Can you return net::OK instead? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:257: cache_state_ = STATE_READ_DATA_COMPLETE; skip this stage too if entry_ wasn't set? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:264: buf_len_ = entry_->GetDataSize(1); why not use the contant here too? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:271: return entry_->ReadData(1, 0, buf_, buf_len_, entry_callback_); also use the contant here? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:283: if (buf_len_ <= kMaxBodySize) { // Only send small bodies for now. you know in the previous stage (DoReadData) that you won't read that response. Would it make sense to skip it in DoReadData already and avoid a call to ReadData? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:290: entry_->Close(); If you do what I mentioned above make sure to close the entry :). Also, I'm not sure this always works. What if the entry_ was read properly but the second ReadData failed and result here is negative? Don't you still have to call entry_->Close() in this case? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:293: ++resources_it_; // Advance to the next entry. A few things here: - Before you go read the next entry do you want to set everything to NULL (buf_, entry_, buf_len_)? - I think you also need to release the IOBuffer object, no? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.h (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:41: : public base::RefCountedThreadSafe<MalwareDetailsCacheCollector> { include base/memory/ref_counted.h? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:44: nit: remove extra nl? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:46: ~MalwareDetailsCacheCollector(); I think the destructor needs to be private? See ref_counted.h which gives an example. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:48: void StartCacheCollection( I think this methods needs a comment. E.g., how long do these pointers need to live, can they be null, what thread is this method called on. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:57: int DoOpenCache(); Do all these methods have to be public? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:67: bool InProgress(); Maybe add a comment for all public methods? http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/safe_browsing_service.h (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/safe_browsing_service.h:240: // so the service can send it over. maybe mention that you expect this to be called on the IO thread?
Thanks Noe. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.cc (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:268: std::string serialized; On 2011/04/06 20:34:19, noelutz wrote: > can you explain why you're serializing here instead of in the protocol manager > for example? This was Lei's suggestion, it makes it simpler that the service doesn't need to know about our protocol buffers etc, it just takes a string. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:270: DLOG(ERROR) << "Unable to serialize the malware report."; On 2011/04/06 20:34:19, noelutz wrote: > Do you want to return here? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.h (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.h:110: SafeBrowsingService* sb_service_; On 2011/04/06 20:34:19, noelutz wrote: > I think you need a scoped ref pointer here just to be safe. Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.cc (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:29: static const int kMaxBodySize = 1024; On 2011/04/06 20:34:19, noelutz wrote: > Is this in bytes or KB? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:45: // We modify |resources| and |result|, so it must remain alive. On 2011/04/06 20:34:19, noelutz wrote: > Consider moving this comment to the header file. It's more useful there. And > what about the other pointers? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:71: return cache_state_ != STATE_NONE; On 2011/04/06 20:34:19, noelutz wrote: > I assume this is also always called on the IO thread? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:115: } while (rv != net::ERR_IO_PENDING && cache_state_ != STATE_NONE); On 2011/04/06 20:34:19, noelutz wrote: > I'm wondering if it's OK to do all these operations without breaking them up > into multiple smaller bits. As far as I can tell it's possible and all these > methods are called one after another if net::ERR_IO_PENDING is never returned. > What is the likelihood of that happening? Wouldn't you take too much time on > the IO thread in that case? I'm wondering if you don't want to force each of > these methods to run as a separate task on the IO message loop. Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:137: LOG(ERROR) << "Could not get URLRequestContext"; On 2011/04/06 20:34:19, noelutz wrote: > Is there a particular reason you LOG(ERROR) here but not below? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:182: return cache_->OpenEntry(resources_it_->first, &entry_, &cache_callback_); On 2011/04/06 20:34:19, noelutz wrote: > Maybe mention what resources_it_ refers to? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:188: return net::OK; On 2011/04/06 20:34:19, noelutz wrote: > I think there might be a bug here. I think OpenEntry might return > et::ERR_IO_PENDING and use the cache_callback_ to say when the entry is > available. What if an error occurs and rv is an error? In that case entry_ is > probably invalid in which case you should probably abort, no? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:195: if (!entry_) { // No entry in the cache for this url. On 2011/04/06 20:34:19, noelutz wrote: > Is that how you handle errors mentioned above? Are you guaranteed that entry_ > will not be assigned if there was an error in OpenEntry? > > It might be nicer to skip some of the stages (i.e., change the value of the > cache_state_ if you know that the next stage isn't going to do anything anyways. Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:200: buf_len_ = entry_->GetDataSize(0); On 2011/04/06 20:34:19, noelutz wrote: > why not use the constant then? It's not public. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:203: return buf_len_; On 2011/04/06 20:34:19, noelutz wrote: > This is confusing. You always return net::something except here. Can you > return net::OK instead? well, now we return net::ERR_PENDING but DoReadResponseComplete actually gets the size of the buffer ... confusing, indeed. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:257: cache_state_ = STATE_READ_DATA_COMPLETE; On 2011/04/06 20:34:19, noelutz wrote: > skip this stage too if entry_ wasn't set? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:264: buf_len_ = entry_->GetDataSize(1); On 2011/04/06 20:34:19, noelutz wrote: > why not use the contant here too? same reason as above, not public. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:271: return entry_->ReadData(1, 0, buf_, buf_len_, entry_callback_); On 2011/04/06 20:34:19, noelutz wrote: > also use the contant here? same http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:283: if (buf_len_ <= kMaxBodySize) { // Only send small bodies for now. On 2011/04/06 20:34:19, noelutz wrote: > you know in the previous stage (DoReadData) that you won't read that response. > Would it make sense to skip it in DoReadData already and avoid a call to > ReadData? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:290: entry_->Close(); On 2011/04/06 20:34:19, noelutz wrote: > If you do what I mentioned above make sure to close the entry :). Also, I'm not > sure this always works. What if the entry_ was read properly but the second > ReadData failed and result here is negative? Don't you still have to call > entry_->Close() in this case? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:293: ++resources_it_; // Advance to the next entry. On 2011/04/06 20:34:19, noelutz wrote: > A few things here: > - Before you go read the next entry do you want to set everything to NULL > (buf_, entry_, buf_len_)? > - I think you also need to release the IOBuffer object, no? setting buf_ = NULL now. I verified this releases & deletes the IOBuffer. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.h (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:41: : public base::RefCountedThreadSafe<MalwareDetailsCacheCollector> { On 2011/04/06 20:34:19, noelutz wrote: > include base/memory/ref_counted.h? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:44: On 2011/04/06 20:34:19, noelutz wrote: > nit: remove extra nl? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:46: ~MalwareDetailsCacheCollector(); On 2011/04/06 20:34:19, noelutz wrote: > I think the destructor needs to be private? See ref_counted.h which gives an > example. Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:48: void StartCacheCollection( On 2011/04/06 20:34:19, noelutz wrote: > I think this methods needs a comment. E.g., how long do these pointers need to > live, can they be null, what thread is this method called on. Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:57: int DoOpenCache(); On 2011/04/06 20:34:19, noelutz wrote: > Do all these methods have to be public? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:67: bool InProgress(); On 2011/04/06 20:34:19, noelutz wrote: > Maybe add a comment for all public methods? Done. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/safe_browsing_service.h (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/safe_browsing_service.h:240: // so the service can send it over. On 2011/04/06 20:34:19, noelutz wrote: > maybe mention that you expect this to be called on the IO thread? Done.
Looks very nice. Just some minor comments. Thanks, noe. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.cc (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:283: if (buf_len_ <= kMaxBodySize) { // Only send small bodies for now. On 2011/04/07 20:08:32, panayiotis wrote: > On 2011/04/06 20:34:19, noelutz wrote: > > you know in the previous stage (DoReadData) that you won't read that response. > > > Would it make sense to skip it in DoReadData already and avoid a call to > > ReadData? > > Done. I don't think this is done. Is there any reason not do move this check to DoREadData? http://codereview.chromium.org/6611023/diff/26003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:222: &truncated)) { Before you set buf_ to NULL do you need to delete the IOBuffer object here? http://codereview.chromium.org/6611023/diff/26003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:262: Same here? Delete buf_? http://codereview.chromium.org/6611023/diff/26003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:301: Same here? http://codereview.chromium.org/6611023/diff/26003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:312: Maybe set entry_ to NULL?
http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.cc (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:283: if (buf_len_ <= kMaxBodySize) { // Only send small bodies for now. Oh sorry, I missed this. I actually do read the response, so that I can calculate the md5 digest. I just skip adding it to the protocol buffer if it's too big. On 2011/04/11 20:32:10, noelutz wrote: > On 2011/04/07 20:08:32, panayiotis wrote: > > On 2011/04/06 20:34:19, noelutz wrote: > > > you know in the previous stage (DoReadData) that you won't read that > response. > > > > > Would it make sense to skip it in DoReadData already and avoid a call to > > > ReadData? > > > > Done. > > I don't think this is done. Is there any reason not do move this check to > DoREadData? http://codereview.chromium.org/6611023/diff/26003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:222: &truncated)) { On 2011/04/11 20:32:11, noelutz wrote: > Before you set buf_ to NULL do you need to delete the IOBuffer object here? Since buf_ is a scoped_refptr, we shouldn't delete it ourselves. http://codereview.chromium.org/6611023/diff/26003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:262: On 2011/04/11 20:32:11, noelutz wrote: > Same here? Delete buf_? same http://codereview.chromium.org/6611023/diff/26003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:301: On 2011/04/11 20:32:11, noelutz wrote: > Same here? same http://codereview.chromium.org/6611023/diff/26003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:312: On 2011/04/11 20:32:11, noelutz wrote: > Maybe set entry_ to NULL? Done.
LGTM thanks, noe. http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.cc (right): http://codereview.chromium.org/6611023/diff/14001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:283: if (buf_len_ <= kMaxBodySize) { // Only send small bodies for now. On 2011/04/11 21:42:16, panayiotis wrote: > Oh sorry, I missed this. I actually do read the response, so that I can > calculate the md5 digest. I just skip adding it to the protocol buffer if it's > too big. > > On 2011/04/11 20:32:10, noelutz wrote: > > On 2011/04/07 20:08:32, panayiotis wrote: > > > On 2011/04/06 20:34:19, noelutz wrote: > > > > you know in the previous stage (DoReadData) that you won't read that > > response. > > > > > > > Would it make sense to skip it in DoReadData already and avoid a call to > > > > ReadData? > > > > > > Done. > > > > I don't think this is done. Is there any reason not do move this check to > > DoREadData? > sounds good. i missed the digest part. http://codereview.chromium.org/6611023/diff/26003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:222: &truncated)) { On 2011/04/11 21:42:16, panayiotis wrote: > On 2011/04/11 20:32:11, noelutz wrote: > > Before you set buf_ to NULL do you need to delete the IOBuffer object here? > > Since buf_ is a scoped_refptr, we shouldn't delete it ourselves. Woops. I missed that. LG.
http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.cc (right): http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:202: // net::HttpCache::kResponseInfoIndex = 0. Do you know if there is a reason why the enum where this is defined isn't public? http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:244: // Strip any Set-Cookie headers. Is this for privacy reasons? Seems like it should be mentioned in the design doc. http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:279: return entry_->ReadData(1, 0, buf_, buf_len_, entry_callback_); I don't quite grok the disk cache interface, but is it ok that we don't check for / do something with sparse entries?
http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.cc (right): http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:202: // net::HttpCache::kResponseInfoIndex = 0. On 2011/04/12 01:47:26, mattm wrote: > Do you know if there is a reason why the enum where this is defined isn't > public? I don't know :/ http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:244: // Strip any Set-Cookie headers. On 2011/04/12 01:47:26, mattm wrote: > Is this for privacy reasons? Seems like it should be mentioned in the design > doc. Update the doc. http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:279: return entry_->ReadData(1, 0, buf_, buf_len_, entry_callback_); On 2011/04/12 01:47:26, mattm wrote: > I don't quite grok the disk cache interface, but is it ok that we don't check > for / do something with sparse entries? Do you mean that the entries we lookup could be spread in sparse locations on the disk and we could cause too many seeks? I think since the majority of reports have <=3 entries, it shouldn't be a problem. In addition, most of these URLs should have been added to the cache very recently -- they are all part of the page that was loaded. So maybe there cache is smart enough to keep these entries together ...
On 2011/04/12 17:23:10, panayiotis wrote: > http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... > File chrome/browser/safe_browsing/malware_details_cache.cc (right): > > http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/malware_details_cache.cc:202: // > net::HttpCache::kResponseInfoIndex = 0. > On 2011/04/12 01:47:26, mattm wrote: > > Do you know if there is a reason why the enum where this is defined isn't > > public? > > I don't know :/ > > http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/malware_details_cache.cc:244: // Strip any > Set-Cookie headers. > On 2011/04/12 01:47:26, mattm wrote: > > Is this for privacy reasons? Seems like it should be mentioned in the design > > doc. > > Update the doc. I meant: I updated the doc. > > http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/malware_details_cache.cc:279: return > entry_->ReadData(1, 0, buf_, buf_len_, entry_callback_); > On 2011/04/12 01:47:26, mattm wrote: > > I don't quite grok the disk cache interface, but is it ok that we don't check > > for / do something with sparse entries? > > Do you mean that the entries we lookup could be spread in sparse locations on > the disk and we could cause too many seeks? > > I think since the majority of reports have <=3 entries, it shouldn't be a > problem. In addition, most of these URLs should have been added to the cache > very recently -- they are all part of the page that was loaded. So maybe there > cache is smart enough to keep these entries together ...
http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.cc (right): http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:279: return entry_->ReadData(1, 0, buf_, buf_len_, entry_callback_); On 2011/04/12 17:23:10, panayiotis wrote: > On 2011/04/12 01:47:26, mattm wrote: > > I don't quite grok the disk cache interface, but is it ok that we don't check > > for / do something with sparse entries? > > Do you mean that the entries we lookup could be spread in sparse locations on > the disk and we could cause too many seeks? > > I think since the majority of reports have <=3 entries, it shouldn't be a > problem. In addition, most of these URLs should have been added to the cache > very recently -- they are all part of the page that was loaded. So maybe there > cache is smart enough to keep these entries together ... > I was refering to ReadSparseData in disk_cache.h. It has a comment saying trying to read sparse entries with the non-sparse function gives undefined results, but I dunno if any of the cache entries this will get are sparse.
Got it. Good find, I wasn't aware of the sparse entries. According to src/net/http/partial_data.h these entries are generated for partial HTTP responses. My hunch is that partial HTTP responses are more common in large downloads rather than common browsing, so I'm guessing our entries won't be sparse. But, the "mistake with implementation specific behavior" comment is worrisome. I emailed the owners of net/ for suggestions on how to check if an entry is sparse, let's see what they think. If you have a suggestion please let me know. On Tue, Apr 12, 2011 at 3:18 PM, <mattm@chromium.org> wrote: > > http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... > File chrome/browser/safe_browsing/malware_details_cache.cc (right): > > http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/malware_details_cache.cc:279: return > entry_->ReadData(1, 0, buf_, buf_len_, entry_callback_); > On 2011/04/12 17:23:10, panayiotis wrote: >> >> On 2011/04/12 01:47:26, mattm wrote: >> > I don't quite grok the disk cache interface, but is it ok that we > > don't check >> >> > for / do something with sparse entries? > >> Do you mean that the entries we lookup could be spread in sparse > > locations on >> >> the disk and we could cause too many seeks? > >> I think since the majority of reports have <=3 entries, it shouldn't > > be a >> >> problem. In addition, most of these URLs should have been added to the > > cache >> >> very recently -- they are all part of the page that was loaded. So > > maybe there >> >> cache is smart enough to keep these entries together ... > > > I was refering to ReadSparseData in disk_cache.h. It has a comment > saying trying to read sparse entries with the non-sparse function gives > undefined results, but I dunno if any of the cache entries this will get > are sparse. > > http://codereview.chromium.org/6611023/ >
Ok, please have another look Matt. The code is now much simpler, and the tests didn't require a lot of modifications. The URLFetcher API is much nicer than talking to the cache directly. On 2011/04/12 23:30:25, panayiotis wrote: > Got it. Good find, I wasn't aware of the sparse entries. According to > src/net/http/partial_data.h these entries are generated for partial > HTTP responses. My hunch is that partial HTTP responses are more > common in large downloads rather than common browsing, so I'm guessing > our entries won't be sparse. > > But, the "mistake with implementation specific behavior" comment is > worrisome. I emailed the owners of net/ for suggestions on how to > check if an entry is sparse, let's see what they think. If you have a > suggestion please let me know. > > On Tue, Apr 12, 2011 at 3:18 PM, <mailto:mattm@chromium.org> wrote: > > > > > http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... > > File chrome/browser/safe_browsing/malware_details_cache.cc (right): > > > > > http://codereview.chromium.org/6611023/diff/31002/chrome/browser/safe_browsin... > > chrome/browser/safe_browsing/malware_details_cache.cc:279: return > > entry_->ReadData(1, 0, buf_, buf_len_, entry_callback_); > > On 2011/04/12 17:23:10, panayiotis wrote: > >> > >> On 2011/04/12 01:47:26, mattm wrote: > >> > I don't quite grok the disk cache interface, but is it ok that we > > > > don't check > >> > >> > for / do something with sparse entries? > > > >> Do you mean that the entries we lookup could be spread in sparse > > > > locations on > >> > >> the disk and we could cause too many seeks? > > > >> I think since the majority of reports have <=3 entries, it shouldn't > > > > be a > >> > >> problem. In addition, most of these URLs should have been added to the > > > > cache > >> > >> very recently -- they are all part of the page that was loaded. So > > > > maybe there > >> > >> cache is smart enough to keep these entries together ... > > > > > > I was refering to ReadSparseData in disk_cache.h. It has a comment > > saying trying to read sparse entries with the non-sparse function gives > > undefined results, but I dunno if any of the cache entries this will get > > are sparse. > > > > http://codereview.chromium.org/6611023/ > >
http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.cc (right): http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:116: status.os_error() == net::ERR_CACHE_MISS) { do we need a test for status != SUCCESS && os_error != CACHE_MISS? http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.h (right): http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:60: // tODO: needs to be public? shouldn't need to be public
Thanks Matt, all done http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.cc (right): http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.cc:116: status.os_error() == net::ERR_CACHE_MISS) { On 2011/04/18 02:19:24, mattm wrote: > do we need a test for status != SUCCESS && os_error != CACHE_MISS? Done. http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_cache.h (right): http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_cache.h:60: // tODO: needs to be public? On 2011/04/18 02:19:24, mattm wrote: > shouldn't need to be public Done.
LGTM On 2011/04/18 17:26:40, panayiotis wrote: > Thanks Matt, all done > > http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... > File chrome/browser/safe_browsing/malware_details_cache.cc (right): > > http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/malware_details_cache.cc:116: status.os_error() == > net::ERR_CACHE_MISS) { > On 2011/04/18 02:19:24, mattm wrote: > > do we need a test for status != SUCCESS && os_error != CACHE_MISS? > > Done. > > http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... > File chrome/browser/safe_browsing/malware_details_cache.h (right): > > http://codereview.chromium.org/6611023/diff/37003/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/malware_details_cache.h:60: // tODO: needs to be > public? > On 2011/04/18 02:19:24, mattm wrote: > > shouldn't need to be public > > Done.
Try job failure for 6611023-37006 on mac: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Try job failure for 6611023-37006 on win: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Try job failure for 6611023-37006 on linux: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...
Try job failure for 6611023-51006 on linux: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...
Change committed as 82070 |
