Changeset 214472 in webkit


Ignore:
Timestamp:
Mar 28, 2017 4:56:52 AM (7 years ago)
Author:
yoav@yoav.ws
Message:

Add a warning for unused link preloads.
https://bugs.webkit.org/show_bug.cgi?id=165670

Reviewed by Youenn Fablet.

Source/WebCore:

Tests: http/tests/preload/single_download_preload_headers_charset.php

http/tests/preload/unused_preload_warning.html

  • dom/Document.cpp:

(WebCore::Document::prepareForDestruction): Stop the timer once the document is destructed.

  • loader/LinkPreloadResourceClients.h: Add shouldMarkAsReferenced overides for the LinkPreloadResourceClient classes.
  • loader/cache/CachedResource.cpp:

(WebCore::CachedResource::addClientToSet): Make sure LinkPreloadResourceClients don't set resource to be referenced.

  • loader/cache/CachedResourceClient.h:

(WebCore::CachedResourceClient::shouldMarkAsReferenced): Make sure that ResourceClients mark preloads as referenced by default.

  • loader/cache/CachedResourceLoader.cpp:

(WebCore::CachedResourceLoader::CachedResourceLoader): Initialize timer.
(WebCore::CachedResourceLoader::~CachedResourceLoader): Stop timer.
(WebCore::CachedResourceLoader::documentDidFinishLoadEvent): Trigger a timer if preloads weren't cleared at load time.
(WebCore::CachedResourceLoader::stopUnusedPreloadsTimer): Stop the timer.
(WebCore::CachedResourceLoader::warnUnusedPreloads): Iterate over m_preloads and issue a warning for non-referenced preloads.

  • loader/cache/CachedResourceLoader.h:
  • page/DOMWindow.cpp:

(WebCore::DOMWindow::willDetachDocumentFromFrame): Clear Resource Timing buffer when document detaches, to avoid test flakiness.

LayoutTests:

  • TestExpectations: Added a "Failure Pass" for the flaky charset header test.
  • http/tests/preload/download_resources-expected.txt:
  • http/tests/preload/download_resources.html: Added references to preloaded resources.
  • http/tests/preload/onerror_event-expected.txt:
  • http/tests/preload/onerror_event.html: Added references to preloaded resources.
  • http/tests/preload/onload_event-expected.txt:
  • http/tests/preload/onload_event.html: Added references to preloaded resources.
  • http/tests/preload/single_download_preload.html: Deflaked.
  • http/tests/preload/single_download_preload_headers.php: Removed the charset to avoid double download bug.
  • http/tests/preload/single_download_preload_headers_charset-expected.txt: Added.
  • http/tests/preload/single_download_preload_headers_charset.php: Flaky test showing the double download bug when charset is declared.
  • http/tests/preload/unused_preload_warning-expected.txt: Added.
  • http/tests/preload/unused_preload_warning.html: Added.
Location:
trunk
Files:
3 added
18 edited
1 copied

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r214471 r214472  
     12017-03-28  Yoav Weiss  <yoav@yoav.ws>
     2
     3        Add a warning for unused link preloads.
     4        https://bugs.webkit.org/show_bug.cgi?id=165670
     5
     6        Reviewed by Youenn Fablet.
     7
     8        * TestExpectations: Added a "Failure Pass" for the flaky charset header test.
     9        * http/tests/preload/download_resources-expected.txt:
     10        * http/tests/preload/download_resources.html: Added references to preloaded resources.
     11        * http/tests/preload/onerror_event-expected.txt:
     12        * http/tests/preload/onerror_event.html: Added references to preloaded resources.
     13        * http/tests/preload/onload_event-expected.txt:
     14        * http/tests/preload/onload_event.html: Added references to preloaded resources.
     15        * http/tests/preload/single_download_preload.html: Deflaked.
     16        * http/tests/preload/single_download_preload_headers.php: Removed the charset to avoid double download bug.
     17        * http/tests/preload/single_download_preload_headers_charset-expected.txt: Added.
     18        * http/tests/preload/single_download_preload_headers_charset.php: Flaky test showing the double download bug when charset is declared.
     19        * http/tests/preload/unused_preload_warning-expected.txt: Added.
     20        * http/tests/preload/unused_preload_warning.html: Added.
     21
    1222017-03-28  Antoine Quint  <graouts@apple.com>
    223
  • trunk/LayoutTests/TestExpectations

    r214291 r214472  
    10871087webkit.org/b/168238 imported/w3c/web-platform-tests/dom/events/EventListener-invoke-legacy.html [ Pass Failure ]
    10881088
     1089webkit.org/b/170122 http/tests/preload/single_download_preload_headers_charset.php [ Pass Failure ]
     1090
    10891091########################################
    10901092### START OF -disabled tests
  • trunk/LayoutTests/http/tests/preload/download_resources-expected.txt

    r214388 r214472  
    1 CONSOLE MESSAGE: line 11: <link rel=preload> must have a valid `as` value
    2 PASS internals.isPreloaded('../resources/dummy.js'); is true
    3 PASS internals.isPreloaded('../resources/dummy.css'); is true
    4 PASS internals.isPreloaded('../resources/square.png'); is true
    5 PASS internals.isPreloaded('../resources/Ahem.ttf'); is true
    6 PASS internals.isPreloaded('../resources/test.mp4'); is true
    7 PASS internals.isPreloaded('../security/resources/captions.vtt'); is true
    8 PASS internals.isPreloaded('../resources/dummy.xml?badvalue'); is false
    9 PASS internals.isPreloaded('../resources/dummy.xml'); is true
    10 PASS successfullyParsed is true
     1CONSOLE MESSAGE: line 13: <link rel=preload> must have a valid `as` value
     2This test makes sure that link preload preloads resources
    113
    12 TEST COMPLETE
     4PASS Makes sure that preloaded resources are downloaded
    135
  • trunk/LayoutTests/http/tests/preload/download_resources.html

    r214388 r214472  
    11<!DOCTYPE html>
    2 <html>
    3 <head>
    4 <script src="/js-test-resources/js-test.js"></script>
     2<script src="/js-test-resources/testharness.js"></script>
     3<script src="/js-test-resources/testharnessreport.js"></script>
     4<script>
     5    var t = async_test('Makes sure that preloaded resources are downloaded');
     6</script>
    57<link rel=preload href="../resources/dummy.js" as=script>
    68<link rel=preload href="../resources/dummy.css" as=style>
    7 <link rel=preload href="../resources/square.png" as=image>
    8 <link rel=preload href="../resources/Ahem.ttf" as=font crossorigin>
     9<link rel=preload href="../resources/square100.png" as=image>
     10<link rel=preload href="../resources/Ahem.woff" as=font crossorigin>
    911<link rel=preload href="../resources/test.mp4" as=media>
    1012<link rel=preload href="../security/resources/captions.vtt" as=track>
    1113<link rel=preload href="../resources/dummy.xml?badvalue" as=foobarxmlthing>
    1214<link rel=preload href="../resources/dummy.xml">
    13 <script src="http://127.0.0.1:8000/resources/slow-script.pl?delay=100"></script>
    14 </head>
    15 <body>
     15<script src="http://127.0.0.1:8000/resources/slow-script.pl?delay=400"></script>
    1616<script>
    17     if (window.testRunner)
    18         testRunner.dumpAsText();
    19     shouldBeTrue("internals.isPreloaded('../resources/dummy.js');");
    20     shouldBeTrue("internals.isPreloaded('../resources/dummy.css');");
    21     shouldBeTrue("internals.isPreloaded('../resources/square.png');");
    22     shouldBeTrue("internals.isPreloaded('../resources/Ahem.ttf');");
    23     shouldBeTrue("internals.isPreloaded('../resources/test.mp4');");
    24     shouldBeTrue("internals.isPreloaded('../security/resources/captions.vtt');");
    25     shouldBeFalse("internals.isPreloaded('../resources/dummy.xml?badvalue');");
    26     shouldBeTrue("internals.isPreloaded('../resources/dummy.xml');");
     17    assert_true(internals.isPreloaded("../resources/dummy.js"));
     18    assert_true(internals.isPreloaded("../resources/dummy.css"));
     19    assert_true(internals.isPreloaded("../resources/square100.png"));
     20    // FIXME: RT doesn't show downloads for the resources below. Need to investigate why.
     21    assert_true(internals.isPreloaded("../resources/Ahem.woff"));
     22    assert_true(internals.isPreloaded("../resources/test.mp4"));
     23    assert_true(internals.isPreloaded("../security/resources/captions.vtt"));
     24
     25    assert_false(internals.isPreloaded("../resources/dummy.xml?badvalue"));
     26    assert_true(internals.isPreloaded("../resources/dummy.xml"));
     27    document.write('<script src="../resources/dummy.js"></scr' + 'ipt>' +
     28                   '<link rel=stylesheet href="../resources/dummy.css">' +
     29                   '<img src="../resources/square100.png">' +
     30                   '<video><source src="../resources/test.mp4">' +
     31                   '<track kind=subtitles src="../security/resources/captions.vtt" srclang=en>' +
     32                   '</video>' +
     33                   '<style>' +
     34                   '    @font-face { font-family:ahem; src: url(../resources/Ahem.woff); }' +
     35                   '    span { font-family: ahem, Arial; }' +
     36                   '</style>' +
     37                   '<span>This test makes sure that link preload preloads resources</span>');
     38    var xhr = new XMLHttpRequest();
     39    xhr.open("GET", "../resources/dummy.xml");
     40    xhr.send();
     41    t.done();
    2742</script>
  • trunk/LayoutTests/http/tests/preload/onerror_event-expected.txt

    r214388 r214472  
    1 CONSOLE MESSAGE: line 27: <link rel=preload> must have a valid `as` value
    2 PASS successfullyParsed is true
     1CONSOLE MESSAGE: line 25: <link rel=preload> must have a valid `as` value
     2This test makes sure that link preload triggers error events for non-existing resources.
    33
    4 TEST COMPLETE
    5 PASS styleFailed is true
    6 PASS scriptFailed is true
    7 PASS imageFailed is true
    8 PASS fontFailed is true
    9 PASS trackFailed is true
    10 PASS gibrishFailed is true
    11 PASS noTypeFailed is true
     4PASS Makes sure that preloaded resources trigger onerror
    125
  • trunk/LayoutTests/http/tests/preload/onerror_event.html

    r214388 r214472  
    11<!DOCTYPE html>
     2<script src="/js-test-resources/testharness.js"></script>
     3<script src="/js-test-resources/testharnessreport.js"></script>
    24<script>
    3     if (window.testRunner) {
    4         testRunner.dumpAsText()
    5         testRunner.waitUntilDone();
    6     }
     5    var t = async_test('Makes sure that preloaded resources trigger onerror');
    76</script>
    8 <script src="/js-test-resources/js-test.js"></script>
    97<script>
    108    var scriptFailed = false;
     
    2826<link rel=preload href="http://127.0.0.1:9999/non-existent/dummy.xml" onerror="count();noTypeFailed = true;" onload="count();">
    2927<script>
     28    document.write('<script src="../non-existent/dummy.js"></scr' + 'ipt>' +
     29                   '<link rel=stylesheet href="../non-existent/dummy.css">' +
     30                   '<img src="../non-existent/square.png">' +
     31                   '<video><source src="test.mp4">' +
     32                   '<track kind=subtitles src="../../non-existent/security/captions.vtt" srclang=en>' +
     33                   '</video>' +
     34                   '<style>' +
     35                   '    @font-face { font-family:ahem; src: url(../../non-existent/Ahem.ttf); }' +
     36                   '    span { font-family: ahem, Arial; }' +
     37                   '</style>' +
     38                   '<span>This test makes sure that link preload triggers error events for non-existing resources.</span>');
     39    var xhr = new XMLHttpRequest();
     40    xhr.open("GET", "../non-existent/dummy.xml");
     41    xhr.send();
     42    var xhr2 = new XMLHttpRequest();
     43    xhr2.open("GET", "http://127.0.0.1:9999/non-existent/dummy.xml");
     44    xhr2.send();
    3045    function test() {
    31         shouldBeTrue("styleFailed");
    32         shouldBeTrue("scriptFailed");
    33         shouldBeTrue("imageFailed");
    34         shouldBeTrue("fontFailed");
    35         shouldBeTrue("trackFailed");
    36         shouldBeTrue("gibrishFailed");
    37         shouldBeTrue("noTypeFailed");
    38         if (window.testRunner)
    39             testRunner.notifyDone();
     46        assert_true(styleFailed);
     47        assert_true(scriptFailed);
     48        assert_true(imageFailed);
     49        assert_true(fontFailed);
     50        assert_true(trackFailed);
     51        assert_true(gibrishFailed);
     52        assert_true(noTypeFailed);
     53        t.done();
    4054    };
    41     setInterval(function() {
     55    setInterval(t.step_func(function() {
    4256        if (window.counter >= 7)
    4357            test();
    44     }, 100);
     58    }, 100));
    4559</script>
    4660
  • trunk/LayoutTests/http/tests/preload/onload_event-expected.txt

    r214388 r214472  
    1616PASS noTypeLoaded is true
    1717PASS emptyTypeLoaded is true
    18 
     18This test makes sure that link preload events are fired
  • trunk/LayoutTests/http/tests/preload/onload_event.html

    r214388 r214472  
    3030<link rel=preload href="../resources/Ahem.woff" as=font crossorigin onload="count(); fontLoaded = true;" onerror="count()">
    3131<link rel=preload href="../resources/test.ogv" as=media onload="count(); mediaLoaded = true;" onerror="count()">
    32 <link rel=preload href="../security/resources/captions.vtt" as=track onload="count(); trackLoaded = true;" onerror="count()">
     32<link rel=preload href="../security/resources/captions.vtt" as=track onload="count(); trackLoaded = true;" onerror="count();">
    3333<link rel=preload href="../resources/dummy.xml" as=foobarxmlthing onload="count(); gibberishLoaded = true;" onerror="count(); gibberishErrored = true;">
    3434<link rel=preload href="../resources/dummy.xslt" as=xslt onload="count(); xsltLoaded = true;" onerror="count(); xsltErrored = true;">
     
    5252            testRunner.notifyDone();
    5353    }
    54      addEventListener("load", function(){
     54    document.write('<script src="../resources/dummy.js"></scr' + 'ipt>' +
     55                   '<link rel=stylesheet href="../resources/dummy.css">' +
     56                   '<img src="../resources/square.png">' +
     57                   '<video><source src="test.mp4">' +
     58                   '<track kind=subtitles src="../../security/resources/captions.vtt" srclang=en>' +
     59                   '</video>' +
     60                   '<style>' +
     61                   '    @font-face { font-family:ahem; src: url(../../w3c/webperf/resources/Ahem.ttf); }' +
     62                   '    span { font-family: ahem, Arial; }' +
     63                   '</style>' +
     64                   '<span>This test makes sure that link preload events are fired</span>');
     65    var xhr = new XMLHttpRequest();
     66    xhr.open("GET", "../resources/dummy.xml");
     67    xhr.send();
     68    var xhr2 = new XMLHttpRequest();
     69    xhr2.open("GET", "../resources/dummy.xml?badvalue");
     70    xhr2.send();
     71    addEventListener("load", function(){
    5572        setInterval(function() {
    5673            if (window.counter >= 10)
  • trunk/LayoutTests/http/tests/preload/single_download_preload.html

    r214388 r214472  
    5656            verifyDownloadNumber("http://127.0.0.1:8000/resources/dummy.xml", 2);
    5757            // FIXME: We should verify for video and audio as well, but they seem to (flakily?) trigger multiple partial requests.
     58            var xhr = new XMLHttpRequest();
     59            xhr.open("GET", "../resources/dummy.xml");
     60            xhr.send();
     61            var xhr2 = new XMLHttpRequest();
     62            xhr2.open("GET", "../resources/dummy.xml?badvalue");
     63            xhr2.send();
    5864            t.done();
    5965            }), 100);
  • trunk/LayoutTests/http/tests/preload/single_download_preload_headers.php

    r213672 r214472  
    1212?>
    1313<!DOCTYPE html>
    14 <meta charset="utf-8">
    1514<script src="/js-test-resources/testharness.js"></script>
    1615<script src="/js-test-resources/testharnessreport.js"></script>
  • trunk/Source/WebCore/ChangeLog

    r214471 r214472  
     12017-03-28  Yoav Weiss  <yoav@yoav.ws>
     2
     3        Add a warning for unused link preloads.
     4        https://bugs.webkit.org/show_bug.cgi?id=165670
     5
     6        Reviewed by Youenn Fablet.
     7
     8        Tests: http/tests/preload/single_download_preload_headers_charset.php
     9               http/tests/preload/unused_preload_warning.html
     10
     11        * dom/Document.cpp:
     12        (WebCore::Document::prepareForDestruction): Stop the timer once the document is destructed.
     13        * loader/LinkPreloadResourceClients.h: Add shouldMarkAsReferenced overides for the LinkPreloadResourceClient classes.
     14        * loader/cache/CachedResource.cpp:
     15        (WebCore::CachedResource::addClientToSet): Make sure LinkPreloadResourceClients don't set resource to be referenced.
     16        * loader/cache/CachedResourceClient.h:
     17        (WebCore::CachedResourceClient::shouldMarkAsReferenced): Make sure that ResourceClients mark preloads as referenced by default.
     18        * loader/cache/CachedResourceLoader.cpp:
     19        (WebCore::CachedResourceLoader::CachedResourceLoader): Initialize timer.
     20        (WebCore::CachedResourceLoader::~CachedResourceLoader): Stop timer.
     21        (WebCore::CachedResourceLoader::documentDidFinishLoadEvent): Trigger a timer if preloads weren't cleared at load time.
     22        (WebCore::CachedResourceLoader::stopUnusedPreloadsTimer): Stop the timer.
     23        (WebCore::CachedResourceLoader::warnUnusedPreloads): Iterate over m_preloads and issue a warning for non-referenced preloads.
     24        * loader/cache/CachedResourceLoader.h:
     25        * page/DOMWindow.cpp:
     26        (WebCore::DOMWindow::willDetachDocumentFromFrame): Clear Resource Timing buffer when document detaches, to avoid test flakiness.
     27
    1282017-03-28  Antoine Quint  <graouts@apple.com>
    229
  • trunk/Source/WebCore/dom/Document.cpp

    r214435 r214472  
    22662266#endif
    22672267
     2268    m_cachedResourceLoader->stopUnusedPreloadsTimer();
     2269
    22682270    detachFromFrame();
    22692271
  • trunk/Source/WebCore/loader/LinkPreloadResourceClients.h

    r214388 r214472  
    8989
    9090    void clear() override { clearResource(*this); }
     91    bool shouldMarkAsReferenced() const override { return false; }
    9192
    9293private:
     
    115116
    116117    void clear() override { clearResource(*this); }
     118    bool shouldMarkAsReferenced() const override { return false; }
    117119
    118120private:
     
    136138
    137139    void clear() override { clearResource(*this); }
     140    bool shouldMarkAsReferenced() const override { return false; }
    138141
    139142private:
     
    161164
    162165    void clear() override { clearResource(*this); }
     166    bool shouldMarkAsReferenced() const override { return false; }
    163167
    164168private:
     
    182186
    183187    void clear() override { clearResource(*this); }
     188    bool shouldMarkAsReferenced() const override { return false; }
    184189
    185190private:
  • trunk/Source/WebCore/loader/cache/CachedResource.cpp

    r214388 r214472  
    466466bool CachedResource::addClientToSet(CachedResourceClient& client)
    467467{
    468     if (m_preloadResult == PreloadNotReferenced) {
     468    if (m_preloadResult == PreloadNotReferenced && client.shouldMarkAsReferenced()) {
    469469        if (isLoaded())
    470470            m_preloadResult = PreloadReferencedWhileComplete;
  • trunk/Source/WebCore/loader/cache/CachedResourceClient.h

    r214388 r214472  
    4646    static CachedResourceClientType expectedType() { return BaseResourceType; }
    4747    virtual CachedResourceClientType resourceClientType() const { return expectedType(); }
     48    virtual bool shouldMarkAsReferenced() const { return true; }
    4849
    4950protected:
  • trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp

    r214388 r214472  
    8686namespace WebCore {
    8787
     88// Timeout for link preloads to be used after window.onload
     89static const int unusedPreloadTimeoutInSeconds = 3;
     90
    8891static CachedResource* createResource(CachedResource::Type type, CachedResourceRequest&& request, SessionID sessionID)
    8992{
     
    130133    , m_documentLoader(documentLoader)
    131134    , m_requestCount(0)
     135    , m_unusedPreloadsTimer(*this, &CachedResourceLoader::warnUnusedPreloads)
    132136    , m_garbageCollectDocumentResourcesTimer(*this, &CachedResourceLoader::garbageCollectDocumentResources)
    133137    , m_autoLoadImages(true)
     
    148152    // Make sure no requests still point to this CachedResourceLoader
    149153    ASSERT(m_requestCount == 0);
     154    m_unusedPreloadsTimer.stop();
    150155}
    151156
     
    822827{
    823828    m_validatedURLs.clear();
     829
     830    // If m_preloads is not empty here, it's full of link preloads,
     831    // as speculative preloads were cleared at DCL.
     832    if (m_preloads && m_preloads->size() && !m_unusedPreloadsTimer.isActive())
     833        m_unusedPreloadsTimer.startOneShot(unusedPreloadTimeoutInSeconds);
     834}
     835
     836void CachedResourceLoader::stopUnusedPreloadsTimer()
     837{
     838    m_unusedPreloadsTimer.stop();
    824839}
    825840
     
    12271242#endif
    12281243    return resource;
     1244}
     1245
     1246void CachedResourceLoader::warnUnusedPreloads()
     1247{
     1248    if (!m_preloads)
     1249        return;
     1250    for (const auto& resource : *m_preloads) {
     1251        if (resource && resource->isLinkPreload() && resource->preloadResult() == CachedResource::PreloadNotReferenced && document()) {
     1252            document()->addConsoleMessage(MessageSource::Other, MessageLevel::Warning,
     1253                "The resource " + resource->url().string() +
     1254                " was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it wasn't preloaded for nothing.");
     1255        }
     1256    }
    12291257}
    12301258
  • trunk/Source/WebCore/loader/cache/CachedResourceLoader.h

    r214388 r214472  
    134134    CachedResourceHandle<CachedResource> preload(CachedResource::Type, CachedResourceRequest&&);
    135135    void printPreloadStats();
     136    void warnUnusedPreloads();
     137    void stopUnusedPreloadsTimer();
    136138
    137139    bool updateRequestAfterRedirection(CachedResource::Type, ResourceRequest&, const ResourceLoaderOptions&);
     
    189191   
    190192    std::unique_ptr<ListHashSet<CachedResource*>> m_preloads;
     193    Timer m_unusedPreloadsTimer;
    191194
    192195    Timer m_garbageCollectDocumentResourcesTimer;
  • trunk/Source/WebCore/page/DOMWindow.cpp

    r214251 r214472  
    516516    for (auto& property : properties)
    517517        property->willDetachGlobalObjectFromFrame();
     518
     519    if (m_performance)
     520        m_performance->clearResourceTimings();
    518521}
    519522
Note: See TracChangeset for help on using the changeset viewer.