Revalidate using the same Resource, attempt #3

Revalidate using the same Resource, attempt #3

Currently, when we perform a revalidation, we create a new resource and put it in the MemoryCache in place of the resource being revalidated. If the revalidation results in a 304, we silently swap all of the clients of the revalidation to the now-revalidated resource and swap which Resource is in the MemoryCache.

This changes the behavior to use the existing resource to revalidate itself, which simplifies the logic quite a bit. It also gives us the option of removing ResourcePtr (which is used to implement the silent client swap in the 304 case) and make Resources simply RefCounted in a future change.

BUG=

Committed: https://crrev.com/71adea27213b2268bda765abccd5a3bf9517b860
Cr-Commit-Position: refs/heads/master@{#354615}

[Nate Chapin, 2015-10-15 17:32:33]

japhet@chromium.org changed reviewers:
+ mkwst@chromium.org

[Nate Chapin, 2015-10-15 17:32:34]

Since we have several weeks before the next branch point, it's time to try this
again :D

Last try was https://codereview.chromium.org/1237983003. Substantive changes
from the previous version are in Resource.h/cpp, ImageResource.cpp, and
RawResource.cpp. Also, a setup tweak to ScriptStreamingTest.cpp.

I've verified that it fixed the following regressions from the previous version:
http://crbug.com/512405
http://crbug.com/511939
http://crbug.com/514115
http://crbug.com/513572

[Mike West, 2015-10-16 20:45:06]

Sure, why not? :) LGTM.

[Nate Chapin, 2015-10-16 20:54:45]

On 2015/10/16 20:45:06, Mike West (slow) wrote:
> Sure, why not? :) LGTM.

That's a ringing endorsement :)

[Nate Chapin, 2015-10-16 20:55:00]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-10-16 20:56:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1398523004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1398523004/20001

[commit-bot: I haz the power, 2015-10-16 22:06:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-16 22:07:09]

Patchset 2 (id:??) landed as
https://crrev.com/71adea27213b2268bda765abccd5a3bf9517b860
Cr-Commit-Position: refs/heads/master@{#354615}

[Ken Russell (switch to Gerrit), 2015-10-18 23:23:17]

On 2015/10/16 22:07:09, commit-bot: I haz the power wrote:
> Patchset 2 (id:??) landed as
> https://crrev.com/71adea27213b2268bda765abccd5a3bf9517b860
> Cr-Commit-Position: refs/heads/master@{#354615}

I think this caused the following assertion failure on this bot:

http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%...

	W/WebKit  ( 5479): ASSERTION FAILED: buffer->size() >= m_readPosition
	W/WebKit  ( 5479):
../../third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.cpp(47)
: void blink::ThreadSafeDataTransport::setData(blink::SharedBuffer*, bool)

	Stack Trace:
	  RELADDR   FUNCTION                                                           
                                                                                
                                              FILE:LINE
	  00db370a  blink::ThreadSafeDataTransport::setData(blink::SharedBuffer*, bool)
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  00d9a57b  blink::DeferredImageDecoder::setData(blink::SharedBuffer&, bool)   
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  00daccd5  blink::ImageSource::setData(blink::SharedBuffer&, bool)            
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  00d94f41  blink::BitmapImage::dataChanged(bool)                              
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  016570d9  blink::ImageResource::updateImage(bool)                            
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  01d83993 
content::WebURLLoaderImpl::Context::OnReceivedData(scoped_ptr<content::RequestPeer::ReceivedData,
base::DefaultDeleter<content::RequestPeer::ReceivedData> >)                     
               
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  01d73e83  content::ResourceDispatcher::OnReceivedData(int, int, int, int)    
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  01d750d3  content::ResourceDispatcher::DispatchMessage(IPC::Message const&)  
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  01d75397  content::ResourceDispatcher::OnMessageReceived(IPC::Message const&)
[clone .part.171]                                                               
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  01d75ba1  content::(anonymous namespace)::DispatchMessageTask::run()         
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  003ab6f5  base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask
const&)                                                                         
                                              
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  01d9b8c1 
scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(scheduler::internal::TaskQueueImpl*,
scheduler::internal::TaskQueueImpl::Task*)                                      
                      
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  01d9bd17  scheduler::TaskQueueManager::DoWork(bool)                          
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  003ab6f5  base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask
const&)                                                                         
                                              
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  003c1a35  base::MessageLoop::RunTask(base::PendingTask const&)               
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  003c1ecf  base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) 
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  003c1faf  base::MessageLoop::DoWork()                                        
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  003c394b  base::MessagePumpDefault::Run(base::MessagePump::Delegate*)        
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  003c2ff3  base::MessageLoop::RunHandler()                                    
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  003d35f9  base::RunLoop::Run()                                               
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  003c1787  base::MessageLoop::Run()                                           
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  01ed9c95  content::RendererMain(content::MainFunctionParams const&)          
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  002f52cb  content::RunNamedProcessTypeMain(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
content::MainFunctionParams const&, content::ContentMainDelegate*) 
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  002f5403  content::ContentMainRunnerImpl::Run()                              
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  002f4e17  Java_org_chromium_content_app_ContentMain_nativeStart              
                                                                                
                                             
/data/app/org.chromium.content_shell_apk-1/lib/arm/libcontent_shell_content_view.so
	  000d0795  <unknown>                                                          
                                                                                
                                             
/data/dalvik-cache/arm/data@app@org.chromium.content_shell_apk-1@base.apk@cla...

Could you please link to bug IDs in CL descriptions in the future? A bug ID is a
much better place to post this sort of information.

This bot's been reliably red since this CL landed. I'm going to try sending a
revert through the CQ to see if it turns it green again.

[Ken Russell (switch to Gerrit), 2015-10-18 23:24:50]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1410313002/ by kbr@chromium.org.

The reason for reverting is: Seems to have caused assertion failures on
http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%...
as described on the original CL..

[Ken Russell (switch to Gerrit), 2015-10-19 05:53:12]

On 2015/10/18 23:24:50, Ken Russell wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/1410313002/ by mailto:kbr@chromium.org.
> 
> The reason for reverting is: Seems to have caused assertion failures on
>
http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%...
> as described on the original CL..

The first build containing this revert -
http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%...
- was green, so it's clear this was the cause. Please ask me, zmo or bajones if
you need any help reproducing this problem. It should be straightforward to
reproduce using the command line from a failing build like
http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%...
and the command line argument --story-filter=[failing test name] ; in this case,
WebglConformance.conformance_more_functions_texSubImage2DHTML .

[Ken Russell (switch to Gerrit), 2015-10-19 06:16:11]

On 2015/10/19 05:53:12, Ken Russell wrote:
> On 2015/10/18 23:24:50, Ken Russell wrote:
> > A revert of this CL (patchset #2 id:20001) has been created in
> > https://codereview.chromium.org/1410313002/ by mailto:kbr@chromium.org.
> > 
> > The reason for reverting is: Seems to have caused assertion failures on
> >
>
http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%...
> > as described on the original CL..
> 
> The first build containing this revert -
>
http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%...
> - was green, so it's clear this was the cause. Please ask me, zmo or bajones
if
> you need any help reproducing this problem. It should be straightforward to
> reproduce using the command line from a failing build like
>
http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%...
> and the command line argument --story-filter=[failing test name] ; in this
case,
> WebglConformance.conformance_more_functions_texSubImage2DHTML .

Note: this also caused SecurityStateModelTest.MixedContentWithBrokenSHA1 to fail
in this try job:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...

so it may have introduced random flakiness on a bunch of bots.

Brief excerpt (corrupted due to multithreaded test execution):

[ RUN      ] SecurityStateModelTest.MixedContentWithBrokenSHA1
HTTP server started on http://127.0.0.1:58295...
sending server_data: {"host": "127.0.0.1", "port": 58295} (36 bytes)
HTTPS server started on https://127.0.0.1:58297...
sending server_data: {"host": "127.0.0.1", "port": 58297} (36 bytes)
[2272:2420:1018/175510:INFO:CONSOLE(12)] "Mixed Content: The page at
'https://127.0.0.1:58297/files/ssl/page_displays_insecure_content.html?replace_text=UkVQTEFDRV9XSVRIX0hPU1RfQU5EX1BPUlQ=:MTI3LjAuMC4xOjU4Mjk1'
was loaded over HTTPS, but requested an insecure image
'http://127.0.0.1:58295/files/ssl/google_files/logo.gif'. This content should
also be served over HTTPS.", source:
https://127.0.0.1:58297/files/ssl/page_displays_insecure_content.html?replace...
(12)
[2272:2420:1018/175510:INFO:CONSOLE(0)] "Mixed Content: The page at
'https://127.0.0.1:58297/files/ssl/page_with_dynamic_insecure_content.html?replace_text=UkVQTEFDRV9XSVRIX0hPU1RfQU5EX1BPUlQ=:MTI3LjAuMC4xOjU4Mjk1'
was loaded over HTTPS, but requested an insecure image
'http://127.0.0.1:58295/files/ssl/google_files/logo.gif'. This content should
also be served over HTTPS.", source:
https://127.0.0.1:58297/files/ssl/page_with_dynamic_insecure_content.html?rep...
(0)
ASSERTION FAILED: buffer->size() >= m_readPosition
e:uild\slave\winuild\src	hird_party\webkit\source\platform\graphics	hreadsafedatatransport.cpp(47)
: void __thiscall blink::ThreadSafeDataTransport::setData(class
blink::SharedBuffer *,bool)
Backtrace:
	blink::ThreadSafeDataTransport::setData [0x02FB3714+52]
	blink::ImageFrameGenerator::setData [0x02F92662+18]
	blink::DeferredImageDecoder::setData [0x02F6EA1A+170]
	blink::ImageSource::setData [0x02F82600+144]
	blink::BitmapImage::dataChanged [0x02F660E7+535]
	blink::Image::setData [0x02F41653+83]
	blink::ImageResource::updateImage [0x0388C813+195]

Again, I strongly suggest filing a bug and referencing it from the CL for any
change as major as this. This conversation should be permanently archived on a
bug, not an individual CL.