Bug 165670 – Add a warning for unused link preloads.

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

[Yoav Weiss]

Add a warning for unused link preloads.

[Yoav Weiss]

Created attachment 296673 [details]
Patch

[Build Bot]

Comment on attachment 296673 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 296686 [details]
Archive of layout-test-results from ews115 for mac-yosemite

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

[Build Bot]

Comment on attachment 296673 [details]
Patch

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

New failing tests:
http/tests/misc/video-poster-image-load-outlives-gc-without-crashing.html

[Build Bot]

Created attachment 296690 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Build Bot]

Comment on attachment 296673 [details]
Patch

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

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

[Build Bot]

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

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

[Build Bot]

Comment on attachment 296673 [details]
Patch

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

New failing tests:
http/tests/misc/video-poster-image-load-outlives-gc-without-crashing.html
inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html

[Build Bot]

Created attachment 296696 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

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

[Yoav Weiss]

Created attachment 296844 [details]
Patch

[WebKit Commit Bot]

Attachment 296844 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 7 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yoav Weiss]

Created attachment 296846 [details]
Patch

[Build Bot]

Comment on attachment 296846 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 296849 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Build Bot]

Comment on attachment 296846 [details]
Patch

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

New failing tests:
http/tests/misc/video-poster-image-load-outlives-gc-without-crashing.html
http/tests/preload/onerror_event.html
http/tests/loading/sizes/preload-image-sizes.html

[Build Bot]

Created attachment 296850 [details]
Archive of layout-test-results from ews100 for mac-yosemite

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

[Build Bot]

Comment on attachment 296846 [details]
Patch

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

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

[Build Bot]

Created attachment 296851 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 296846 [details]
Patch

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

New failing tests:
http/tests/preload/download_resources.html
http/tests/local/script-crossorigin-loads-file-scheme.html

[Build Bot]

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

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

[Yoav Weiss]

Created attachment 304494 [details]
Patch

[Yoav Weiss]

Created attachment 304495 [details]
Patch

[Alex Christensen]

Comment on attachment 304495 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceClient.h:48
> +    virtual bool markAsReferenced() const { return true; }

I'm not sure this is named correctly.  shouldMarkAsReferenced?  Isn't there already a way to tell if a load is a preload?

> LayoutTests/TestExpectations:1260
> +http/tests/preload/single_download_preload.html [ DumpJSConsoleLogInStdErr ]

Does this make these tests not accidentally print out a warning message?  Is there no way to do this from within the test?

[Yoav Weiss]

(In reply to comment #23)
> Comment on attachment 304495 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304495&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceClient.h:48
> > +    virtual bool markAsReferenced() const { return true; }
> 
> I'm not sure this is named correctly.  shouldMarkAsReferenced?

Yeah, I'll rename

> Isn't there already a way to tell if a load is a preload?

There is already a way to tell if a certain request is a preload request or a preloaded resource.
However, a preloaded resource can be referenced (e.g. `<link rel=preload href=foo as=script><script src=foo>`).
Current tracking if a resource is referenced is based on the addition of CachedResourceClients to it. So I figured it'd be cleanest to add that "is this referencing" info to the resource client.
 
> 
> > LayoutTests/TestExpectations:1260
> > +http/tests/preload/single_download_preload.html [ DumpJSConsoleLogInStdErr ]
> 
> Does this make these tests not accidentally print out a warning message?

Yeah

> Is there no way to do this from within the test?

Don't know. If there is, I'd be happy to switch to that method

[Yoav Weiss]

Created attachment 304543 [details]
Patch

[Build Bot]

Comment on attachment 304543 [details]
Patch

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

New failing tests:
http/tests/preload/single_download_preload_headers.php

[Build Bot]

Created attachment 304559 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

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

[Yoav Weiss]

(In reply to comment #24)
> (In reply to comment #23)
> > Comment on attachment 304495 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=304495&action=review
> > 
> > > Source/WebCore/loader/cache/CachedResourceClient.h:48
> > > +    virtual bool markAsReferenced() const { return true; }
> > 
> > I'm not sure this is named correctly.  shouldMarkAsReferenced?
> 
> Yeah, I'll rename
> 
> > Isn't there already a way to tell if a load is a preload?
> 
> There is already a way to tell if a certain request is a preload request or
> a preloaded resource.
> However, a preloaded resource can be referenced (e.g. `<link rel=preload
> href=foo as=script><script src=foo>`).
> Current tracking if a resource is referenced is based on the addition of
> CachedResourceClients to it. So I figured it'd be cleanest to add that "is
> this referencing" info to the resource client.
>  
> > 
> > > LayoutTests/TestExpectations:1260
> > > +http/tests/preload/single_download_preload.html [ DumpJSConsoleLogInStdErr ]
> > 
> > Does this make these tests not accidentally print out a warning message?
> 
> Yeah
> 
> > Is there no way to do this from within the test?
> 
> Don't know. If there is, I'd be happy to switch to that method

Hmm, the debug bot failure seems to indicate that it's not taking this directive into account. Is that a known issue? Is there a way to get around that?

[Alex Christensen]

I don't know much about DumpJSConsoleLogInStdErr, but I think it would be cleaner to add something to TestRunner.idl or Internals.idl to disable the logs from within the test rather than having to know about this magic value in TestExpectations to figure out what is going on.

[youenn fablet]

(In reply to Alex Christensen from comment #29)
> I don't know much about DumpJSConsoleLogInStdErr, but I think it would be
> cleaner to add something to TestRunner.idl or Internals.idl to disable the
> logs from within the test rather than having to know about this magic value
> in TestExpectations to figure out what is going on.

DumpJSConsoleLogInStdErr is mostly for imported W3C tests.

In most cases, if you can tweak the test, you can fix the flakiness issues, which makes TestRunner method not necessary, although we can add it for convenience.

If we were able to inject WebKit specific scripts on imported W3C tests without modifying them (like testXX.webkit.js file next to textXX.js), we could probably get rid of DumpJSConsoleLogInStdErr and use TestRunner.

[youenn fablet]

> Hmm, the debug bot failure seems to indicate that it's not taking this
> directive into account. Is that a known issue? Is there a way to get around
> that?

It is not known, can you reproduce it locally?

[youenn fablet]

(In reply to youenn fablet from comment #31)
> > Hmm, the debug bot failure seems to indicate that it's not taking this
> > directive into account. Is that a known issue? Is there a way to get around
> > that?
> 
> It is not known, can you reproduce it locally?

I am not seeing DumpJSConsoleLogInStdErr in the patch or in TestExpectations though.

[Yoav Weiss]

I removed DumpJSConsoleLogInStdErr and modified the tests to be more resilient, as well as stopped the timer when the document is detached.

Unfortunately, I'm currently failing to upload a patch... I'll try to upload it again tomorrow.

[Yoav Weiss]

Created attachment 305269 [details]
Patch

[Yoav Weiss]

Uploaded the patch. PTAL?

[Build Bot]

Comment on attachment 305269 [details]
Patch

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

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

[Build Bot]

Created attachment 305271 [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

[Build Bot]

Comment on attachment 305269 [details]
Patch

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

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

[Build Bot]

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

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

[Yoav Weiss]

Created attachment 305273 [details]
Patch

[youenn fablet]

Comment on attachment 305273 [details]
Patch

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

> Source/WebCore/page/DOMWindow.h:390
> +    Timer m_unusedPreloadsTimer;

Why not having this timer in CachedResourceLoader and letting DOMWindow/Document notify the cached resource loader when to start/stop its timer?
(I still think the preload management logic should be somehow isolated to ease understanding and future evolution of this mechanism)

[Yoav Weiss]

(In reply to youenn fablet from comment #41)
> Comment on attachment 305273 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305273&action=review
> 
> > Source/WebCore/page/DOMWindow.h:390
> > +    Timer m_unusedPreloadsTimer;
> 
> Why not having this timer in CachedResourceLoader and letting
> DOMWindow/Document notify the cached resource loader when to start/stop its
> timer?

I'll move it if you think it's cleaner.

[youenn fablet]

Comment on attachment 305273 [details]
Patch

r=me, provided the timer is moved to CachedResourceLoader.

[youenn fablet]

(In reply to Yoav Weiss from comment #42)
> (In reply to youenn fablet from comment #41)
> > Comment on attachment 305273 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=305273&action=review
> > 
> > > Source/WebCore/page/DOMWindow.h:390
> > > +    Timer m_unusedPreloadsTimer;
> > 
> > Why not having this timer in CachedResourceLoader and letting
> > DOMWindow/Document notify the cached resource loader when to start/stop its
> > timer?
> 
> I'll move it if you think it's cleaner.

I think it will avoid leaking some CachedResourceLoader logic in DOMWindow.
Please let me know if there is something worse when moving the logic.

[Yoav Weiss]

Created attachment 305286 [details]
Patch

[Yoav Weiss]

Comment on attachment 305286 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:2263
> +    m_cachedResourceLoader->stopUnusedPreloadsTimer();

Youenn - can you just confirm that I can assume m_cachedResourceLoader here can never be nullptr? I'm almost sure that's the case...

[Yoav Weiss]

(In reply to youenn fablet from comment #44)
> (In reply to Yoav Weiss from comment #42)
> > (In reply to youenn fablet from comment #41)
> > > Comment on attachment 305273 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=305273&action=review
> > > 
> > > > Source/WebCore/page/DOMWindow.h:390
> > > > +    Timer m_unusedPreloadsTimer;
> > > 
> > > Why not having this timer in CachedResourceLoader and letting
> > > DOMWindow/Document notify the cached resource loader when to start/stop its
> > > timer?
> > 
> > I'll move it if you think it's cleaner.
> 
> I think it will avoid leaking some CachedResourceLoader logic in DOMWindow.
> Please let me know if there is something worse when moving the logic.

Much cleaner now with the timer moved. Good call! :)

[youenn fablet]

Comment on attachment 305286 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:2263
>> +    m_cachedResourceLoader->stopUnusedPreloadsTimer();
> 
> Youenn - can you just confirm that I can assume m_cachedResourceLoader here can never be nullptr? I'm almost sure that's the case...

This is fine.

[Build Bot]

Comment on attachment 305286 [details]
Patch

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

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-styles.html

[Build Bot]

Created attachment 305295 [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]

Comment on attachment 305286 [details]
Patch

The failing test doesn't seem related so I'm retrying the CQ.

Thanks for reviewing! :)

[WebKit Commit Bot]

Comment on attachment 305286 [details]
Patch

Clearing flags on attachment: 305286

Committed r214361: <http://trac.webkit.org/changeset/214361>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #52)
> Comment on attachment 305286 [details]
> Patch
> 
> Clearing flags on attachment: 305286
> 
> Committed r214361: <http://trac.webkit.org/changeset/214361>

This change appears to have caused flakiness in these tests:

http/tests/preload/onload_event.html
http/tests/preload/single_download_preload.html
http/tests/preload/single_download_preload_headers.php

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r214378%20(189)/results.html

[Ryan Haddad]

Reverted r214361 for reason:

This change caused flakiness in http/tests/preload tests.

Committed r214388: <http://trac.webkit.org/changeset/214388>

[Yoav Weiss]

Created attachment 305493 [details]
Patch

[Yoav Weiss]

Uploaded a new version of the patch with improved tests, and which is also clearing the ResourceTiming buffer when the document is detached in order to reduce test flakiness.

PTAL?

[Yoav Weiss]

Comment on attachment 305493 [details]
Patch

Thanks! :)

[WebKit Commit Bot]

Comment on attachment 305493 [details]
Patch

Rejecting attachment 305493 [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-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 305493, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
21bf338bedb2f38ccec2141744769e2e7b 8d9ae72ccc5ab5bf039c78da119722bab7f1ddb0 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.
Total errors found: 0 in 1 files

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

[Yoav Weiss]

Created attachment 305579 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 305579 [details]
Patch

Clearing flags on attachment: 305579

Committed r214472: <http://trac.webkit.org/changeset/214472>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.