Bug 49236 - Link prefetch isn't compatible with subresources
Summary: Link prefetch isn't compatible with subresources
Status: RESOLVED DUPLICATE of bug 35404
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://www.belshe.com/test/prefetch.html
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-08 20:29 PST by Gavin Peters
Modified: 2010-11-29 08:58 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.59 KB, patch)
2010-11-26 11:55 PST, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (4.59 KB, patch)
2010-11-26 11:57 PST, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2010-11-28 08:42 PST, Gavin Peters
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2010-11-08 20:29:15 PST
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
Comment 1 Adam Barth 2010-11-08 21:00:45 PST
I think Eric might have just fixed this bug.  Can you check if it still happens at top-of-tree?
Comment 2 Gavin Peters 2010-11-08 21:31:46 PST
I just tested with tip-of-tree, and I think Eric has not fixed this bug.  :/
Comment 3 Gavin Peters 2010-11-26 11:55:09 PST
Created attachment 74952 [details]
Patch
Comment 4 Gavin Peters 2010-11-26 11:57:19 PST
Created attachment 74953 [details]
Patch
Comment 5 Gavin Peters 2010-11-26 12:13:03 PST
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.
Comment 6 Gavin Peters 2010-11-27 07:39:18 PST
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.
Comment 7 Adam Barth 2010-11-27 14:34:01 PST
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.
Comment 8 WebKit Commit Bot 2010-11-27 15:21:09 PST
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
Comment 9 Gavin Peters 2010-11-28 07:00:43 PST
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.
Comment 10 Gavin Peters 2010-11-28 08:42:40 PST
Created attachment 74976 [details]
Patch
Comment 11 Gavin Peters 2010-11-28 08:45:11 PST
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.
Comment 12 Eric Seidel (no email) 2010-11-28 09:34:01 PST
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.
Comment 13 Adam Barth 2010-11-28 10:35:13 PST
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.
Comment 14 Gavin Peters 2010-11-28 11:26:30 PST
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 ***
Comment 15 Adam Barth 2010-11-29 08:46:38 PST
> 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?
Comment 16 Gavin Peters 2010-11-29 08:58:16 PST
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.