Bug 49236 – Link prefetch isn't compatible with subresources

• Component: DOM
• Status: RESOLVED DUPLICATE

[Gavin Peters]

If you both prefetch a resource, and use it as a subresource, the second of these two fetches fails due to a type mismatch on the CachedResource.

See Also: http://code.google.com/p/chromium/issues/detail?id=61468

[Adam Barth]

I think Eric might have just fixed this bug.  Can you check if it still happens at top-of-tree?

[Gavin Peters]

I just tested with tip-of-tree, and I think Eric has not fixed this bug.  :/

[Gavin Peters]

Created attachment 74952 [details]
Patch

[Gavin Peters]

Created attachment 74953 [details]
Patch

[Gavin Peters]

This is not my favourite fix ever.  I think this works: the WebKit cache doesn't get subresources loaded into it, and so we rely on the disk cache from the network stack to be primed when doing prefetches.

I'd be happier if instead, we were letting the prefetch data into the memory cache, and then just either mutated the cache entry to a different type, or something like that, if a subresource request came.

Also, don't we have a bug if you use the same file as both an image and a JS file, or an image and a css file, etc..., in the same page?  I suppose that's less significant than what I fixed though.

[Gavin Peters]

N.B.: I haven't added test exclusions for my new unit test, since it runs fine if you don't have link prefetch.  The test only fails if you both have link prefetch enabled, and don't have this patch.

[Adam Barth]

Comment on attachment 74953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74953&action=review

This seems fine.  I wonder if we can avoid the CachedResouceLoader now that the ResourceLoadScheduler isn't tied so tightly to the MemoryCache...

> LayoutTests/fast/dom/HTMLLinkElement/link-and-subresource-test.html:8
> +  setTimeout("layoutTestController.notifyDone()",500);

boo timeouts lead to flakiness.

[WebKit Commit Bot]

Comment on attachment 74953 [details]
Patch

Rejecting patch 74953 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
dom/HTMLHeadElement .......
fast/dom/HTMLHtmlElement ..
fast/dom/HTMLImageElement ..........
fast/dom/HTMLInputElement ..........
fast/dom/HTMLKeygenElement .
fast/dom/HTMLLabelElement ...
fast/dom/HTMLLabelElement/form .
fast/dom/HTMLLinkElement .
fast/dom/HTMLLinkElement/link-and-subresource-test.html -> failed

Exiting early after 1 failures. 7167 tests run.
152.91s total testing time

7166 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6296083

[Gavin Peters]

Interesting: there's a failure here that I was not seeing in my testing on gtk, or my earlier testing on mac.  I'm now investigating.

[Gavin Peters]

Created attachment 74976 [details]
Patch

[Gavin Peters]

This new upload fixes two mistakes I made

1. I have added my new tests to the skipped section.  Yes, the tests when run manually look correct if you don't have prefetching, however the media list is different, since of course the prefetch didn't occur.

2. I have added code in CachedResourceLoader.cpp to not remove prefetches from the cache.  They never make it in, and there was a debug mode assertion failure caused by my unit tests here: the wrong element was being removed from the cache in the case that a prefetched resource was also a subresource.

My apologies for not catching these earlier.

[Eric Seidel (no email)]

Um... Turns out the fix I made, never landed: https://bugs.webkit.org/show_bug.cgi?id=35404

Gavin's fix may be unnecessary here.

[Adam Barth]

Comment on attachment 74976 [details]
Patch

Gavin, can you try with Eric's patch applied?  His fix is more general, so if it works for this case, that's probably better.

[Gavin Peters]

Eric's more general fix definitely does it.  I'll obsolete my patch.

Eric, can you please land yours?

*** This bug has been marked as a duplicate of bug 35404 ***

[Adam Barth]

> Eric's more general fix definitely does it.  I'll obsolete my patch.

Should we land the test from your patch after we land Eric's patch?

[Gavin Peters]

Yes, we should, but it should *also* be after I land an "onload" handler to <link rel=prefetch>, since that will make all our tests for prefetching less flakey.