Bug 167415 – Avoid evicting link preload resources when parsing is done.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yoav Weiss]

Avoid evicting link preload resources when parsing is done.

[Yoav Weiss]

Created attachment 299689 [details]
Patch

[Build Bot]

Comment on attachment 299689 [details]
Patch

Attachment 299689 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2946407

New failing tests:
http/tests/preload/dynamic_remove_preload_href.html

[Build Bot]

Created attachment 299693 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299689 [details]
Patch

Attachment 299689 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2946417

New failing tests:
http/tests/preload/dynamic_remove_preload_href.html

[Build Bot]

Created attachment 299694 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299689 [details]
Patch

Attachment 299689 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2946423

New failing tests:
http/tests/preload/dynamic_remove_preload_href.html

[Build Bot]

Created attachment 299695 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299689 [details]
Patch

Attachment 299689 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2946435

New failing tests:
http/tests/preload/dynamic_remove_preload_href.html

[Build Bot]

Created attachment 299696 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Yoav Weiss]

Created attachment 299697 [details]
Patch

[Alex Christensen]

Comment on attachment 299697 [details]
Patch

I have no idea what is going on in this patch.  It is full of suspicious marking and bools that indicates it's probably not designed right.

[Yoav Weiss]

Created attachment 299797 [details]
Patch

[Yoav Weiss]

Created attachment 299798 [details]
Patch

[Yoav Weiss]

(In reply to comment #11)
> Comment on attachment 299697 [details]
> Patch
> 
> I have no idea what is going on in this patch.  It is full of suspicious
> marking and bools that indicates it's probably not designed right.

Sincere apologies for not including a detailed change description in the ChangeLog. An omission on my part. I've now uploaded a new patch that includes one.

Can you look at the patch in light of the description and let me know if it makes more sense? If you have suggestions to improve its design, I'll be happy to apply them.

[Yoav Weiss]

Created attachment 299800 [details]
Patch

[Yoav Weiss]

Removed parts of the patch that weren't tightly related to the main fix. (the markAsReferenced parts)

[Alex Christensen]

Comment on attachment 299800 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:5046
> +    // Parser should have picked up all speculative preloads by now

Is there a difference between a speculative preload and a preload?

> Source/WebCore/loader/LinkLoader.cpp:183
>      linkRequest.setInitiator("link");
> +    linkRequest.setIsLinkPreload();

Do we really need another bool for this?  Can't we just check if the initiator is "link"?

> Source/WebCore/loader/LinkPreloadResourceClients.h:66
> +            m_resource->cancelLoad();
>              m_resource->removeClient(client);

I think removing the client will cancel the load if this was the only client.  If it wasn't the only client, then something else has made the same request and we probably don't want to cancel the load for that other client, right?

> Source/WebCore/loader/cache/CachedResourceLoader.h:131
> +    enum ClearPreloadsMode { ClearSpeculativePreloads, ClearAllPreloads };

enum class

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:48
> +    , m_isLinkPreload(false)

If we added a bool here, we would want to have its initializer be an initializer list in the header
bool m_isLinkPreload { false };

[Yoav Weiss]

(In reply to comment #17)
> Comment on attachment 299800 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299800&action=review
> 
> > Source/WebCore/dom/Document.cpp:5046
> > +    // Parser should have picked up all speculative preloads by now
> 
> Is there a difference between a speculative preload and a preload?

A speculative preload is one issued by the preloadScanner (at least that's the terminology that I used)

> 
> > Source/WebCore/loader/LinkLoader.cpp:183
> >      linkRequest.setInitiator("link");
> > +    linkRequest.setIsLinkPreload();
> 
> Do we really need another bool for this?  Can't we just check if the
> initiator is "link"?

A "link" initiator can also be a stylesheet. A slightly similar flag is the ForPreload flag, which signified if a request and a resource were triggered as a speculative preload.

Maybe we can somehow unite those flags, but a 4 state enum is not necessarily better/clearer than two bools

> 
> > Source/WebCore/loader/LinkPreloadResourceClients.h:66
> > +            m_resource->cancelLoad();
> >              m_resource->removeClient(client);
> 
> I think removing the client will cancel the load if this was the only
> client.  If it wasn't the only client, then something else has made the same
> request and we probably don't want to cancel the load for that other client,
> right?

I'll try that

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.h:131
> > +    enum ClearPreloadsMode { ClearSpeculativePreloads, ClearAllPreloads };
> 
> enum class

OK

> 
> > Source/WebCore/loader/cache/CachedResourceRequest.cpp:48
> > +    , m_isLinkPreload(false)
> 
> If we added a bool here, we would want to have its initializer be an
> initializer list in the header
> bool m_isLinkPreload { false };

OK

[Yoav Weiss]

Created attachment 299837 [details]
Patch

[youenn fablet]

Comment on attachment 299837 [details]
Patch

I guess an alternative would be to restrict m_preloads to speculative preloads only.
Can we do that?
Also, if we do not fully clean m_preloads at onload time, won't we keep references to these resources?
If so, we might not be able to reclaim memory on these, even if MemoryCache is asked to do so.

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

> Source/WebCore/ChangeLog:14
> +        being cleared), said issue is also fixed by clearing previousely preloaded resources if an invalid link preload is later detected.

What happens if a speculative preload is scheduled first and a link preload on the same resource happens after.
Shouldn't the speculative preload be marked as link preload?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:870
> +        resource->setLinkPreload();

This should be done in CachedResource constructor.
Maybe we should in the future make CachedResource have a CachedResourceRequest member.

[Yoav Weiss]

(In reply to comment #20)
> Comment on attachment 299837 [details]
> Patch
> 
> I guess an alternative would be to restrict m_preloads to speculative
> preloads only.
> Can we do that?

We probably could, but that would probably mean duplicating the existing mechanisms that relate to m_preloads, or generalize them so that they can be applied to two separate lists. Do you think such a separation would be valuable enough to justify that?

> Also, if we do not fully clean m_preloads at onload time, won't we keep
> references to these resources?

We fully clear m_preloads when CachedResourceLoader is terminated, so (IIUC) when the document is detached.
 
> If so, we might not be able to reclaim memory on these, even if MemoryCache
> is asked to do so.

That's deliberate, as these resources might be used after onload (e.g. by scripts injecting corresponding DOM nodes that need them)

I'm planning to introduce a warning mechanism for unused preloads (probably at 3 seconds after onload). If we want to reclaim memory for these resources before the document detaches, that might be a good point to introduce clearing mechanism for them.

> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299837&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        being cleared), said issue is also fixed by clearing previousely preloaded resources if an invalid link preload is later detected.
> 
> What happens if a speculative preload is scheduled first and a link preload
> on the same resource happens after.
> Shouldn't the speculative preload be marked as link preload?

I should probably add handling for that scenario for correctness sake, even if I'm not particularly worried about it.

I think such a scenario would be relatively rare, and even if it happens, clearPreloads(ClearSpeculativePreloads) won't evict this resource from MemoryCache, as it would be referenced by the time clearPreloads is first called.

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:870
> > +        resource->setLinkPreload();
> 
> This should be done in CachedResource constructor.

OK

> Maybe we should in the future make CachedResource have a
> CachedResourceRequest member.

[Yoav Weiss]

(In reply to comment #21)
> (In reply to comment #20)
> > Comment on attachment 299837 [details]
> > Patch
> > 
> > I guess an alternative would be to restrict m_preloads to speculative
> > preloads only.
> > Can we do that?
> 
> We probably could, but that would probably mean duplicating the existing
> mechanisms that relate to m_preloads, or generalize them so that they can be
> applied to two separate lists. Do you think such a separation would be
> valuable enough to justify that?

When I think about it some more, there are many cases where the same resource would end up in both lists, which will require deletion from one container before adding to the other. That's likely to add complexity, replace the simple list by maps, etc.

[Yoav Weiss]

Created attachment 299919 [details]
Patch

[Yoav Weiss]

Addressed comments. PTAL

[youenn fablet]

The handling of preloads is a bit complex and it would be good if we could simplify it.
Can we try to align preloads with other loaded resources?
Here are some thoughts below.

Maybe m_preloads could store CachedResourceHandle.

AFAIUI, body link preloads have a loader so they will not be cancelled when the parsing is done. Maybe we should have a way for header link preloads to behave the same.

When we would clean m_preloads at the end of parsing, only the speculative preloads without any other attached loader would actually be cancelled.

[Yoav Weiss]

(In reply to comment #25)
> The handling of preloads is a bit complex and it would be good if we could
> simplify it.
> Can we try to align preloads with other loaded resources?
> Here are some thoughts below.
> 
> Maybe m_preloads could store CachedResourceHandle.

That might be possible IIUC. Seems orthogonal to this particular issue though.

> 
> AFAIUI, body link preloads have a loader so they will not be cancelled when
> the parsing is done. Maybe we should have a way for header link preloads to
> behave the same.
> 
> When we would clean m_preloads at the end of parsing, only the speculative
> preloads without any other attached loader would actually be cancelled.

So, the end goal of this patch is to enable another patch:
* Add warnings for unused preloads - https://bugs.webkit.org/show_bug.cgi?id=165670

That foal requires us to distinguish unused link preloads for used ones. So even if we find a way to declare header preloads as referenced, we'd need another (very similar) mechanism to then say "this is a referenced link preload".

In Blink I used the similar `m_preloadResult != PreloadNotReferenced` mechanism to do that, and excluded link preloads from setting the result to referenced.

I'm happy to discuss alternative designs, but thought said end goal is relevant for the discussion.

One other relevant point: I'm planning to add preloadScanner support for linkPreload, after which we will have speculative link preloads as well.

[youenn fablet]

Thanks for the context, that is helpful.
I guess a possibility might be to make m_preloads a class (or set of classes, one for m_preloads and one for each preloaded resource) that would handle this logic by itself.

We could then migrate some preload logic from CachedResource to these classes: m_preloadCount, when to cancel preloads...

[Ryosuke Niwa]

Comment on attachment 299919 [details]
Patch

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

I think the current approach is fine. CachedResource is supposed to encapsulate these things anyway.
We do have a lot of indications and weird class hierarchy in the loading code through.
Instead of adding a new class, we should consider refactoring them into a more coherent set.

> Source/WebCore/dom/Document.cpp:5047
> +    // Parser should have picked up all speculative preloads by now
> +    m_cachedResourceLoader->clearPreloads(CachedResourceLoader::ClearPreloadsMode::ClearSpeculativePreloads);

We never call this function anywhere else e.g. for link element's preload.
Can't we just rename this function to clearSpeculativePreloads instead?

[Yoav Weiss]

(In reply to comment #28)
> Comment on attachment 299919 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299919&action=review
> 
> I think the current approach is fine. CachedResource is supposed to
> encapsulate these things anyway.
> We do have a lot of indications and weird class hierarchy in the loading
> code through.
> Instead of adding a new class, we should consider refactoring them into a
> more coherent set.
> 
> > Source/WebCore/dom/Document.cpp:5047
> > +    // Parser should have picked up all speculative preloads by now
> > +    m_cachedResourceLoader->clearPreloads(CachedResourceLoader::ClearPreloadsMode::ClearSpeculativePreloads);
> 
> We never call this function anywhere else e.g. for link element's preload.
> Can't we just rename this function to clearSpeculativePreloads instead?

It's also called in CachedResourceLoader's destructor (where all preloads are cleared)

[WebKit Commit Bot]

Comment on attachment 299919 [details]
Patch

Rejecting attachment 299919 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 299919, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
1 succeeded at 11 (offset -2 lines).
patching file LayoutTests/http/tests/preload/not_delaying_window_onload_before_discovery.html
Hunk #1 succeeded at 17 (offset -4 lines).
patching file LayoutTests/http/tests/preload/not_evicting_preload_at_onload-expected.txt
patching file LayoutTests/http/tests/preload/not_evicting_preload_at_onload.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2987773

[Yoav Weiss]

Created attachment 300357 [details]
Patch

[Build Bot]

Comment on attachment 300357 [details]
Patch

Attachment 300357 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2988021

New failing tests:
http/tests/preload/not_evicting_preload_at_onload.html
http/tests/preload/dynamic_removing_preload.html
http/tests/preload/dynamic_remove_preload_href.html

[Build Bot]

Created attachment 300363 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300357 [details]
Patch

Attachment 300357 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2988025

New failing tests:
http/tests/preload/not_evicting_preload_at_onload.html
http/tests/preload/dynamic_removing_preload.html
http/tests/preload/dynamic_remove_preload_href.html

[Build Bot]

Created attachment 300366 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300357 [details]
Patch

Attachment 300357 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2988029

New failing tests:
http/tests/preload/not_evicting_preload_at_onload.html
http/tests/preload/dynamic_removing_preload.html
http/tests/preload/dynamic_remove_preload_href.html

[Build Bot]

Created attachment 300367 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300357 [details]
Patch

Attachment 300357 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2988039

New failing tests:
http/tests/preload/dynamic_remove_preload_href.html
http/tests/preload/dynamic_removing_preload.html
http/tests/preload/not_evicting_preload_at_onload.html

[Build Bot]

Created attachment 300368 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300357 [details]
Patch

Attachment 300357 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2988712

New failing tests:
http/tests/preload/not_evicting_preload_at_onload.html
http/tests/preload/dynamic_removing_preload.html
http/tests/preload/dynamic_remove_preload_href.html

[Build Bot]

Created attachment 300379 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Yoav Weiss]

Created attachment 300408 [details]
Patch

[Yoav Weiss]

One more small change:
One of the tests was previously passing because it was testing that something doesn't download without turning on the LinkPreload feature (so a false pass).
With the move to LinkPreload on-by-default for tests *and* avoiding clearing Link preloads on DCL, this test now failed.

As a result, I added a cancelLoad() method on LinkLoader (called from HTMLLinkElement::removedFrom()) and added a CachedResource::cancelLoad() call in LinkPreloadResourceClient::clear(), as otherwise the load wasn't really cancelled.

PTAL?

[Build Bot]

Comment on attachment 300408 [details]
Patch

Attachment 300408 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2991673

New failing tests:
http/tests/preload/dynamic_removing_preload.html

[Build Bot]

Created attachment 300411 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Yoav Weiss]

Test failure is a result of the latest change. I'll rollback those changes, disable the test for this patch, and land a fix for it in a later patch.

[Yoav Weiss]

Created attachment 300526 [details]
Patch

[Yoav Weiss]

Disabled the failing test, and filed https://bugs.webkit.org/show_bug.cgi?id=167792 to track it.

[WebKit Commit Bot]

Comment on attachment 300526 [details]
Patch

Clearing flags on attachment: 300526

Committed r211649: <http://trac.webkit.org/changeset/211649>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

(In reply to comment #50)
> All reviewed patches have been landed.  Closing bug.

The bots are showing lots of crashes:
https://build.webkit.org/builders/Apple%20El%20Capitan%20%28Leaks%29/builds/10800

I think it was caused by this:
https://trac.webkit.org/changeset/211649

Crashes look like:

>   Process:               DumpRenderTree [63952]
>   
>   Crashed Thread:        0  Dispatch queue: com.apple.main-thread
>   
>   Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
>   Exception Codes:       EXC_I386_GPFLT
>   Exception Note:        EXC_CORPSE_NOTIFY
>   
>   Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
>   0  WebCore::CachedResourceLoader::clearPreloads(WebCore::CachedResourceLoader::ClearPreloadsMode) + 48 (CachedResourceLoader.cpp:1257)
>   1  WebCore::HTMLDocumentParser::prepareToStopParsing() + 162 (HTMLDocumentParser.cpp:136)
>   2  WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution() + 235 (HTMLDocumentParser.cpp:496)
>   3  WebCore::HTMLDocumentParser::notifyFinished(WebCore::PendingScript&) + 84 (HTMLDocumentParser.cpp:539)
>   4  WebCore::PendingScript::notifyFinished(WebCore::LoadableScript&) + 35 (PendingScript.cpp:74)
>   5  WebCore::LoadableScript::notifyClientFinished() + 300 (LoadableScript.cpp:59)
>   6  WebCore::CachedResource::checkNotify() + 153 (CachedResourceClientWalker.h:50)
>   7  WebCore::SubresourceLoader::didFinishLoading(double) + 1167 (SubresourceLoader.cpp:552)
>   8  __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke + 69

[Yoav Weiss]

Yeah, looks like this is the cause. Do you see issues only on the debug bots, or also in the release bots? I suspect some bad interaction with printPreloadStats(), that's only running in debug.

[Yoav Weiss]

I was mistaken, printPreloadStats is not built regardless of debug/release.

[Yoav Weiss]

Apologies, but I'm currently boarding a plane. Please revert if you deem necessary. I'll look into it further once I'm on solid ground.

[Yoav Weiss]

Is there a way for me to run a similar "leak bot" config locally? Is a debug build with MallocStackLogging=YES (as described in https://webkit.org/leak-hunting/) the way to go?

[Yoav Weiss]

I was able to recreate locally. Fix at https://bugs.webkit.org/show_bug.cgi?id=167838