Bug 167415 - Avoid evicting link preload resources when parsing is done.
Summary: Avoid evicting link preload resources when parsing is done.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-25 04:13 PST by Yoav Weiss
Modified: 2017-02-04 12:00 PST (History)
13 users (show)

See Also:


Attachments
Patch (17.84 KB, patch)
2017-01-25 04:26 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (728.54 KB, application/zip)
2017-01-25 05:32 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (835.42 KB, application/zip)
2017-01-25 05:34 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.62 MB, application/zip)
2017-01-25 05:40 PST, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (809.83 KB, application/zip)
2017-01-25 05:57 PST, Build Bot
no flags Details
Patch (18.50 KB, patch)
2017-01-25 06:00 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (19.09 KB, patch)
2017-01-26 01:35 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (19.36 KB, patch)
2017-01-26 01:40 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (16.19 KB, patch)
2017-01-26 02:38 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (15.07 KB, patch)
2017-01-26 13:00 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (16.13 KB, patch)
2017-01-27 01:04 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (15.55 KB, patch)
2017-02-01 15:00 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (877.47 KB, application/zip)
2017-02-01 16:04 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (985.29 KB, application/zip)
2017-02-01 16:07 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.77 MB, application/zip)
2017-02-01 16:12 PST, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (871.18 KB, application/zip)
2017-02-01 16:19 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-elcapitan (1.02 MB, application/zip)
2017-02-01 18:03 PST, Build Bot
no flags Details
Patch (20.60 KB, patch)
2017-02-02 07:03 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (1.82 MB, application/zip)
2017-02-02 08:19 PST, Build Bot
no flags Details
Patch (16.95 KB, patch)
2017-02-03 06:27 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yoav Weiss 2017-01-25 04:13:38 PST
Avoid evicting link preload resources when parsing is done.
Comment 1 Yoav Weiss 2017-01-25 04:26:41 PST
Created attachment 299689 [details]
Patch
Comment 2 Build Bot 2017-01-25 05:32:18 PST
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
Comment 3 Build Bot 2017-01-25 05:32:22 PST
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
Comment 4 Build Bot 2017-01-25 05:34:43 PST
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
Comment 5 Build Bot 2017-01-25 05:34:47 PST
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
Comment 6 Build Bot 2017-01-25 05:40:51 PST
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
Comment 7 Build Bot 2017-01-25 05:40:55 PST
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
Comment 8 Build Bot 2017-01-25 05:57:43 PST
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
Comment 9 Build Bot 2017-01-25 05:57:47 PST
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 10 Yoav Weiss 2017-01-25 06:00:33 PST
Created attachment 299697 [details]
Patch
Comment 11 Alex Christensen 2017-01-25 14:19:33 PST
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.
Comment 12 Yoav Weiss 2017-01-26 01:35:22 PST
Created attachment 299797 [details]
Patch
Comment 13 Yoav Weiss 2017-01-26 01:40:23 PST
Created attachment 299798 [details]
Patch
Comment 14 Yoav Weiss 2017-01-26 01:45:01 PST
(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 15 Yoav Weiss 2017-01-26 02:38:54 PST
Created attachment 299800 [details]
Patch
Comment 16 Yoav Weiss 2017-01-26 02:39:54 PST
Removed parts of the patch that weren't tightly related to the main fix. (the markAsReferenced parts)
Comment 17 Alex Christensen 2017-01-26 11:00:26 PST
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 };
Comment 18 Yoav Weiss 2017-01-26 11:55:01 PST
(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 19 Yoav Weiss 2017-01-26 13:00:14 PST
Created attachment 299837 [details]
Patch
Comment 20 youenn fablet 2017-01-26 23:08:46 PST
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.
Comment 21 Yoav Weiss 2017-01-27 00:03:01 PST
(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.
Comment 22 Yoav Weiss 2017-01-27 00:54:08 PST
(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.
Comment 23 Yoav Weiss 2017-01-27 01:04:59 PST
Created attachment 299919 [details]
Patch
Comment 24 Yoav Weiss 2017-01-27 01:05:36 PST
Addressed comments. PTAL
Comment 25 youenn fablet 2017-01-29 20:08:17 PST
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.
Comment 26 Yoav Weiss 2017-01-29 23:59:28 PST
(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.
Comment 27 youenn fablet 2017-01-31 08:36:34 PST
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 28 Ryosuke Niwa 2017-01-31 13:02:56 PST
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?
Comment 29 Yoav Weiss 2017-01-31 14:10:29 PST
(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)
Comment 30 WebKit Commit Bot 2017-02-01 14:36:10 PST
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
Comment 31 Yoav Weiss 2017-02-01 15:00:20 PST
Created attachment 300357 [details]
Patch
Comment 32 Build Bot 2017-02-01 16:03:53 PST
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
Comment 33 Build Bot 2017-02-01 16:04:00 PST
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
Comment 34 Build Bot 2017-02-01 16:07:01 PST
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
Comment 35 Build Bot 2017-02-01 16:07:06 PST
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
Comment 36 Build Bot 2017-02-01 16:12:44 PST
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
Comment 37 Build Bot 2017-02-01 16:12:49 PST
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
Comment 38 Build Bot 2017-02-01 16:19:25 PST
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
Comment 39 Build Bot 2017-02-01 16:19:30 PST
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
Comment 40 Build Bot 2017-02-01 18:03:41 PST
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
Comment 41 Build Bot 2017-02-01 18:03:46 PST
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
Comment 42 Yoav Weiss 2017-02-02 07:03:06 PST
Created attachment 300408 [details]
Patch
Comment 43 Yoav Weiss 2017-02-02 07:09:48 PST
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?
Comment 44 Build Bot 2017-02-02 08:19:22 PST
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
Comment 45 Build Bot 2017-02-02 08:19:28 PST
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
Comment 46 Yoav Weiss 2017-02-02 10:05:06 PST
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.
Comment 47 Yoav Weiss 2017-02-03 06:27:21 PST
Created attachment 300526 [details]
Patch
Comment 48 Yoav Weiss 2017-02-03 06:30:10 PST
Disabled the failing test, and filed https://bugs.webkit.org/show_bug.cgi?id=167792 to track it.
Comment 49 WebKit Commit Bot 2017-02-03 13:54:17 PST
Comment on attachment 300526 [details]
Patch

Clearing flags on attachment: 300526

Committed r211649: <http://trac.webkit.org/changeset/211649>
Comment 50 WebKit Commit Bot 2017-02-03 13:54:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 51 Joseph Pecoraro 2017-02-03 19:58:16 PST
(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
Comment 52 Yoav Weiss 2017-02-03 20:11:21 PST
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.
Comment 53 Yoav Weiss 2017-02-03 20:23:32 PST
I was mistaken, printPreloadStats is not built regardless of debug/release.
Comment 54 Yoav Weiss 2017-02-03 20:29:10 PST
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.
Comment 55 Yoav Weiss 2017-02-04 09:13:15 PST
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?
Comment 56 Yoav Weiss 2017-02-04 12:00:38 PST
I was able to recreate locally. Fix at https://bugs.webkit.org/show_bug.cgi?id=167838