Changeset 214361 in webkit


Ignore:
Timestamp:
Mar 24, 2017 11:41:42 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:

This patch adds a warning message, to warn developers that are using
link preloads in cases where the downloaded resource is likely to
remain unused.

Test: 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::warnUnusedPreloads): Iterate over m_preloads and issue a warning for non-referenced preloads.
(WebCore::CachedResourceLoader::documentDidFinishLoadEvent): Trigger a timer if preloads weren't cleared at load time.
(WebCore::CachedResourceLoader::warnUnusedPreloads): Triggered by the timer, and called CachedResourceLoader::warnUnusedPreloads.
(WebCore::CachedResourceLoader::stopUnusedPreloadsTimer): Stop the timer.

LayoutTests:

This adds a test for the unused preload warning, and adapts some of the other preload tests,
to avoid warnings.

  • http/tests/preload/download_resources-expected.txt: Changed.
  • http/tests/preload/download_resources.html: Added references to preloaded resources.
  • http/tests/preload/onerrer_event-expected.txt: Changed.
  • http/tests/preload/onerror_event.html: Added references to preloaded resources.
  • http/tests/preload/onload_event.html: Added references to preloaded resources.
  • http/tests/preload/resources/single_download_preload.html: Deflaked.
  • http/tests/preload/unused_preload_warning-expected.txt: Added.
  • http/tests/preload/unused_preload_warning.html: Added.
Location:
trunk
Files:
2 added
15 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r214360 r214361  
     12017-03-24  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        This adds a test for the unused preload warning, and adapts some of the other preload tests,
     9        to avoid warnings.
     10
     11        * http/tests/preload/download_resources-expected.txt: Changed.
     12        * http/tests/preload/download_resources.html: Added references to preloaded resources.
     13        * http/tests/preload/onerrer_event-expected.txt: Changed.
     14        * http/tests/preload/onerror_event.html: Added references to preloaded resources.
     15        * http/tests/preload/onload_event.html: Added references to preloaded resources.
     16        * http/tests/preload/resources/single_download_preload.html: Deflaked.
     17        * http/tests/preload/unused_preload_warning-expected.txt: Added.
     18        * http/tests/preload/unused_preload_warning.html: Added.
     19
    1202017-03-24  Brent Fulgham  <bfulgham@apple.com>
    221

  • trunk/LayoutTests/http/tests/preload/download_resources-expected.txt

    r211341 r214361  
    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

    r211341 r214361  
    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    setTimeout(t.step_func(function() {
     18        assert_true(internals.isPreloaded("../resources/dummy.js"));
     19        assert_true(internals.isPreloaded("../resources/dummy.css"));
     20        assert_true(internals.isPreloaded("../resources/square100.png"));
     21        // FIXME: RT doesn't show downloads for the resources below. Need to investigate why.
     22        assert_true(internals.isPreloaded("../resources/Ahem.woff"));
     23        assert_true(internals.isPreloaded("../resources/test.mp4"));
     24        assert_true(internals.isPreloaded("../security/resources/captions.vtt"));
     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();
     42    }));
    2743</script>

  • trunk/LayoutTests/http/tests/preload/onerror_event-expected.txt

    r211341 r214361  
    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

    r211341 r214361  
    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

    r211341 r214361  
    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

    r211341 r214361  
    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

    r212150 r214361  
    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/Source/WebCore/ChangeLog

    r214360 r214361  
     12017-03-24  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        This patch adds a warning message, to warn developers that are using
     9        link preloads in cases where the downloaded resource is likely to
     10        remain unused.
     11
     12        Test: http/tests/preload/unused_preload_warning.html
     13
     14        * dom/Document.cpp:
     15        (WebCore::Document::prepareForDestruction): Stop the timer once the document is destructed.
     16        * loader/LinkPreloadResourceClients.h: Add shouldMarkAsReferenced overides for the LinkPreloadResourceClient classes.
     17        * loader/cache/CachedResource.cpp:
     18        (WebCore::CachedResource::addClientToSet): Make sure LinkPreloadResourceClients don't set resource to be referenced.
     19        * loader/cache/CachedResourceClient.h:
     20        (WebCore::CachedResourceClient::shouldMarkAsReferenced): Make sure that ResourceClients mark preloads as referenced by default.
     21        * loader/cache/CachedResourceLoader.cpp:
     22        (WebCore::CachedResourceLoader::CachedResourceLoader): Initialize timer.
     23        (WebCore::CachedResourceLoader::~CachedResourceLoader): Stop timer.
     24        (WebCore::CachedResourceLoader::warnUnusedPreloads): Iterate over m_preloads and issue a warning for non-referenced preloads.
     25        (WebCore::CachedResourceLoader::documentDidFinishLoadEvent): Trigger a timer if preloads weren't cleared at load time.
     26        (WebCore::CachedResourceLoader::warnUnusedPreloads): Triggered by the timer, and called CachedResourceLoader::warnUnusedPreloads.
     27        (WebCore::CachedResourceLoader::stopUnusedPreloadsTimer): Stop the timer.
     28
    1292017-03-24  Brent Fulgham  <bfulgham@apple.com>
    230

  • trunk/Source/WebCore/dom/Document.cpp

    r214333 r214361  
    22772277#endif
    22782278
     2279    m_cachedResourceLoader->stopUnusedPreloadsTimer();
     2280
    22792281    detachFromFrame();
    22802282

  • trunk/Source/WebCore/loader/LinkPreloadResourceClients.h

    r211755 r214361  
    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

    r213672 r214361  
    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

    r206903 r214361  
    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

    r213672 r214361  
    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

    r213126 r214361  
    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;
Note: See TracChangeset for help on using the changeset viewer.