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
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
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
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
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.
(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.
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 };
(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
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.
(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.
(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.
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.
(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.
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...
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?
(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)
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
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
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
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
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
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?
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
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.
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?
2017-01-25 04:26 PST, Yoav Weiss
2017-01-25 05:32 PST, Build Bot
2017-01-25 05:34 PST, Build Bot
2017-01-25 05:40 PST, Build Bot
2017-01-25 05:57 PST, Build Bot
2017-01-25 06:00 PST, Yoav Weiss
2017-01-26 01:35 PST, Yoav Weiss
2017-01-26 01:40 PST, Yoav Weiss
2017-01-26 02:38 PST, Yoav Weiss
2017-01-26 13:00 PST, Yoav Weiss
2017-01-27 01:04 PST, Yoav Weiss
2017-02-01 15:00 PST, Yoav Weiss
2017-02-01 16:04 PST, Build Bot
2017-02-01 16:07 PST, Build Bot
2017-02-01 16:12 PST, Build Bot
2017-02-01 16:19 PST, Build Bot
2017-02-01 18:03 PST, Build Bot
2017-02-02 07:03 PST, Yoav Weiss
2017-02-02 08:19 PST, Build Bot
2017-02-03 06:27 PST, Yoav Weiss