Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2343623002:
Add a warning whenever link preloads are not used (Closed)
|
Created:
4 years, 3 months ago by Yoav Weiss Modified:
4 years ago CC:
chromium-reviews, tyoshino+watch_chromium.org, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a warning whenever link preloads are not used
This CL adds a console warning if 3 seconds after the page's onload,
there are still preloaded resources that were not used.
BUG=646797
Committed: https://crrev.com/4fcc2d6bfddd9592403148e752620a987d1c15ae
Cr-Commit-Position: refs/heads/master@{#420015}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Review comments #
Total comments: 3
Patch Set 3 : more comments #
Total comments: 6
Patch Set 4 : moar review comments #Patch Set 5 : Fix crash #Patch Set 6 : rebase #Patch Set 7 : rebase2 #Patch Set 8 : removed dcheck to test it on the bots #Patch Set 9 : trying assert instead #Patch Set 10 : replaced ASSERT with a cowardly if #Patch Set 11 : Added failing unit tests #
Total comments: 2
Patch Set 12 : Removed the counter, since it was not accurate #
Total comments: 2
Patch Set 13 : Added comment #Messages
Total messages: 87 (59 generated)
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yoav@yoav.ws changed reviewers: + csharrison@chromium.org, japhet@chromium.org
Hey! While looking into the twitter memory issues, I figured there's no current way for developers to know if a preload was used or not. This CL adds a console warning if 3 seconds after onload, the link preloaded resources are still not used. https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/FetchContext.h (right): https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/FetchContext.h:39: #include "core/inspector/ConsoleTypes.h" I shouldn't be including this (as fetch shouldn't include anything from the rest of core/), btu found no other way to define MessageLevel below with a default value. Advice on the proper way to do this would be appreciated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Add a warning whenever preloads are not used This CL adds a console warning if 3 seconds after the page's onload, there are still preloaded resources that were not used. BUG= ========== to ========== Add a warning whenever link preloads are not used This CL adds a console warning if 3 seconds after the page's onload, there are still preloaded resources that were not used. BUG=646797 ==========
Great patch! This is not only useful to web devs but to us :P A few comments: https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/FetchContext.h (right): https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/FetchContext.h:39: #include "core/inspector/ConsoleTypes.h" On 2016/09/14 11:53:35, Yoav Weiss wrote: > I shouldn't be including this (as fetch shouldn't include anything from the rest > of core/), btu found no other way to define MessageLevel below with a default > value. Advice on the proper way to do this would be appreciated. It isn't a great solution but you could just add another method "addConsoleErrorMessage" / "addConsoleWarningMessage". Maybe a better strategy would be to use an enum that translates to the MessageLevel. Think: there are multiple consumers of this API that should conform, and inspector is just one of them. https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:913: for (auto resource : *m_preloads) { const auto& ? https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:915: context().addConsoleMessage("The resource " + resource->url().getString() + " was preloaded but not used within a few seconds from the window's load event. Please make sure it wasn't preloaded for nothing.", WarningMessageLevel); Do you think you should include the fact that it was a link preload or do you think it's obvious? https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (left): https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1348: } else { Remove empty line. https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:88: static const int unusedPreloadTimeout = 3; Make it clear this is in seconds in the name or comment. https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1361: m_unusedPreloadsTimer.startOneShot(unusedPreloadTimeout, BLINK_FROM_HERE); I think it makes sense to only dispatch this timer if there are link preloads currently unmatched. The other cases seem extremely rare (like inserting it into the dom after onload).
https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/FetchContext.h (right): https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/FetchContext.h:39: #include "core/inspector/ConsoleTypes.h" On 2016/09/14 12:34:58, Charlie Harrison wrote: > On 2016/09/14 11:53:35, Yoav Weiss wrote: > > I shouldn't be including this (as fetch shouldn't include anything from the > rest > > of core/), btu found no other way to define MessageLevel below with a default > > value. Advice on the proper way to do this would be appreciated. > > It isn't a great solution but you could just add another method > "addConsoleErrorMessage" / "addConsoleWarningMessage". Maybe a better strategy > would be to use an enum that translates to the MessageLevel. Think: there are > multiple consumers of this API that should conform, and inspector is just one of > them. Yeah, I'll go with the latter. https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:913: for (auto resource : *m_preloads) { On 2016/09/14 12:34:58, Charlie Harrison wrote: > const auto& ? Yeah. I'll also modify all the other iterations of m_preloads in the file. https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:915: context().addConsoleMessage("The resource " + resource->url().getString() + " was preloaded but not used within a few seconds from the window's load event. Please make sure it wasn't preloaded for nothing.", WarningMessageLevel); On 2016/09/14 12:34:58, Charlie Harrison wrote: > Do you think you should include the fact that it was a link preload or do you > think it's obvious? nothing is obvious :) I'll add it to the comment https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (left): https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1348: } else { On 2016/09/14 12:34:58, Charlie Harrison wrote: > Remove empty line. sure https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:88: static const int unusedPreloadTimeout = 3; On 2016/09/14 12:34:58, Charlie Harrison wrote: > Make it clear this is in seconds in the name or comment. good call https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1361: m_unusedPreloadsTimer.startOneShot(unusedPreloadTimeout, BLINK_FROM_HERE); On 2016/09/14 12:34:58, Charlie Harrison wrote: > I think it makes sense to only dispatch this timer if there are link preloads > currently unmatched. The other cases seem extremely rare (like inserting it into > the dom after onload). It makes sense, but I really don't want to iterate over m_preloads twice... I can add a linkPreloads counter that will be used here and at warnUnusedPreloads() itself
https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1361: m_unusedPreloadsTimer.startOneShot(unusedPreloadTimeout, BLINK_FROM_HERE); On 2016/09/14 13:28:31, Yoav Weiss wrote: > On 2016/09/14 12:34:58, Charlie Harrison wrote: > > I think it makes sense to only dispatch this timer if there are link preloads > > currently unmatched. The other cases seem extremely rare (like inserting it > into > > the dom after onload). > > It makes sense, but I really don't want to iterate over m_preloads twice... > I can add a linkPreloads counter that will be used here and at > warnUnusedPreloads() itself That's a good point. A counter is not great but I think it's better than issuing no-op tasks for most page loads.
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/14 13:33:00, Charlie Harrison wrote: > https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): > > https://codereview.chromium.org/2343623002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1361: > m_unusedPreloadsTimer.startOneShot(unusedPreloadTimeout, BLINK_FROM_HERE); > On 2016/09/14 13:28:31, Yoav Weiss wrote: > > On 2016/09/14 12:34:58, Charlie Harrison wrote: > > > I think it makes sense to only dispatch this timer if there are link > preloads > > > currently unmatched. The other cases seem extremely rare (like inserting it > > into > > > the dom after onload). > > > > It makes sense, but I really don't want to iterate over m_preloads twice... > > I can add a linkPreloads counter that will be used here and at > > warnUnusedPreloads() itself > > That's a good point. A counter is not great but I think it's better than issuing > no-op tasks for most page loads. Added a counter and addressed all other points raised. The counter required integrating https://codereview.chromium.org/2319483002/patch/140001/150002 into this patch (or I can hold this patch until the other one lands)
Quick comments https://codereview.chromium.org/2343623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2343623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:922: context().addConsoleMessage("The resource " + resource->url().getString() + " was preloaded using `<link rel=preload>` but not used within a few seconds from the window's load event. Please make sure it wasn't preloaded for nothing.", FetchContext::LogWarningMessage); Hm... but what about Link preload headers? https://codereview.chromium.org/2343623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2343623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:99: bool isUnusedLinkPreloads() { return m_linkPreloadCounter > 0; } hasUnusedLinkPreloads? https://codereview.chromium.org/2343623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:185: unsigned m_linkPreloadCounter; m_unusedLinkPreloadCounter?
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
On 2016/09/14 21:07:59, Charlie Harrison wrote: > Quick comments > > https://codereview.chromium.org/2343623002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/2343623002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:922: > context().addConsoleMessage("The resource " + resource->url().getString() + " > was preloaded using `<link rel=preload>` but not used within a few seconds from > the window's load event. Please make sure it wasn't preloaded for nothing.", > FetchContext::LogWarningMessage); > Hm... but what about Link preload headers? > > https://codereview.chromium.org/2343623002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): > > https://codereview.chromium.org/2343623002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ResourceFetcher.h:99: bool > isUnusedLinkPreloads() { return m_linkPreloadCounter > 0; } > hasUnusedLinkPreloads? > > https://codereview.chromium.org/2343623002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ResourceFetcher.h:185: unsigned > m_linkPreloadCounter; > m_unusedLinkPreloadCounter? Good points. Addressed
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
non owner lgtm. This patch is great!
LGTM with a few nitpicks. https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:480: DCHECK_GT(static_cast<int>(m_unusedLinkPreloadCounter), 0); Why static_cast and not 0u? https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1363: DCHECK(documentLoader->fetcher()); I'd skip scheduling the timer here if documentLoader != frame()->loader().documentLoader() after dispatchEvent(). Also, should the Timer be cancelled somewhere in the case where a new navigation commits before it fires? https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:659: MessageLevel level = ErrorMessageLevel; MessageLevel level = messageType == LogWarningMessage ? WarningMessageLevel : ErrorMessageLevel; ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks both for reviewing! :) https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:480: DCHECK_GT(static_cast<int>(m_unusedLinkPreloadCounter), 0); On 2016/09/14 22:14:31, Nate Chapin wrote: > Why static_cast and not 0u? changed https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1363: DCHECK(documentLoader->fetcher()); On 2016/09/14 22:14:31, Nate Chapin wrote: > I'd skip scheduling the timer here if documentLoader != > frame()->loader().documentLoader() after dispatchEvent(). added such a check > > Also, should the Timer be cancelled somewhere in the case where a new navigation > commits before it fires? Normally a detach would call clearPreloads which would result in the counter being 0. So in such a case the timer would fire but do nothing. Nevertheless, now stopping the timer on clearDocument(). https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2343623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:659: MessageLevel level = ErrorMessageLevel; On 2016/09/14 22:14:31, Nate Chapin wrote: > MessageLevel level = messageType == LogWarningMessage ? WarningMessageLevel : > ErrorMessageLevel; > > ? ok
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yoav@yoav.ws
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2343623002/#ps80001 (title: "Fix crash")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/15 10:31:53, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) So, looks like the android bots are failing because of the DCHECK that's making sure that the counter never goes negative... :/ I'll try to recreate and resolve
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/15 16:23:19, Yoav Weiss wrote: > On 2016/09/15 10:31:53, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > > So, looks like the android bots are failing because of the DCHECK that's making > sure that the counter never goes negative... :/ > I'll try to recreate and resolve I'm failing to recreate and apparently need google-internal permissions to run this benchmark locally :/ I could add a check that makes sure that the counter doesn't go below 0 without asserting on it. Alternatively, I could try to recreate if I had access to the test's WPR (or if I was aware of what's there in terms of preload).
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/15 22:14:08, Yoav Weiss wrote: > On 2016/09/15 16:23:19, Yoav Weiss wrote: > > On 2016/09/15 10:31:53, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > > > > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > > > > So, looks like the android bots are failing because of the DCHECK that's > making > > sure that the counter never goes negative... :/ > > I'll try to recreate and resolve > > I'm failing to recreate and apparently need google-internal permissions to run > this benchmark locally :/ > I could add a check that makes sure that the counter doesn't go below 0 without > asserting on it. Alternatively, I could try to recreate if I had access to the > test's WPR (or if I was aware of what's there in terms of preload). Have you talked to folks on speed-infra to ask for access?
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:662: EXPECT_FALSE(fetcher2->hasUnusedLinkPreloads()); This fails because the resource->isLinkPreload() is no longer true, which doesn't decrement the counter on the second fetcher, which makes me think a counter may be the wrong way to approach this :/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:662: EXPECT_FALSE(fetcher2->hasUnusedLinkPreloads()); On 2016/09/19 10:57:41, Yoav Weiss wrote: > This fails because the resource->isLinkPreload() is no longer true, which > doesn't decrement the counter on the second fetcher, which makes me think a > counter may be the wrong way to approach this :/ Yeah you're right. Good thing this test showed that. To go with the counter approach you'd have to propagate the link preload change to all fetchers. That's a lot of complexity. I'm willing to relax my previous push-back on the timeout, in the face of this. What do you think Nate?
On 2016/09/19 14:06:45, Charlie Harrison wrote: > https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): > > https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:662: > EXPECT_FALSE(fetcher2->hasUnusedLinkPreloads()); > On 2016/09/19 10:57:41, Yoav Weiss wrote: > > This fails because the resource->isLinkPreload() is no longer true, which > > doesn't decrement the counter on the second fetcher, which makes me think a > > counter may be the wrong way to approach this :/ > > Yeah you're right. Good thing this test showed that. To go with the counter > approach you'd have to propagate the link preload change to all fetchers. That's > a lot of complexity. > > I'm willing to relax my previous push-back on the timeout, in the face of this. > What do you think Nate? A more relaxed approach would be to call the timeout only in cases where a link preload resource existed and m_preloads is not empty. That would minimize the timeout calls, without much complexity. WDYT?
On 2016/09/19 15:22:24, Yoav Weiss wrote: > On 2016/09/19 14:06:45, Charlie Harrison wrote: > > > https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): > > > > > https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:662: > > EXPECT_FALSE(fetcher2->hasUnusedLinkPreloads()); > > On 2016/09/19 10:57:41, Yoav Weiss wrote: > > > This fails because the resource->isLinkPreload() is no longer true, which > > > doesn't decrement the counter on the second fetcher, which makes me think a > > > counter may be the wrong way to approach this :/ > > > > Yeah you're right. Good thing this test showed that. To go with the counter > > approach you'd have to propagate the link preload change to all fetchers. > That's > > a lot of complexity. > > > > I'm willing to relax my previous push-back on the timeout, in the face of > this. > > What do you think Nate? > > A more relaxed approach would be to call the timeout only in cases where a link > preload resource existed and m_preloads is not empty. That would minimize the > timeout calls, without much complexity. WDYT? Would that require a member "hadLinkPreload"? That seems strange, I'll defer to Nate though. Gating this on m_preloads not being empty SGTM though.
On 2016/09/19 15:27:15, Charlie Harrison wrote: > On 2016/09/19 15:22:24, Yoav Weiss wrote: > > On 2016/09/19 14:06:45, Charlie Harrison wrote: > > > > > > https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): > > > > > > > > > https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:662: > > > EXPECT_FALSE(fetcher2->hasUnusedLinkPreloads()); > > > On 2016/09/19 10:57:41, Yoav Weiss wrote: > > > > This fails because the resource->isLinkPreload() is no longer true, which > > > > doesn't decrement the counter on the second fetcher, which makes me think > a > > > > counter may be the wrong way to approach this :/ > > > > > > Yeah you're right. Good thing this test showed that. To go with the counter > > > approach you'd have to propagate the link preload change to all fetchers. > > That's > > > a lot of complexity. > > > > > > I'm willing to relax my previous push-back on the timeout, in the face of > > this. > > > What do you think Nate? > > > > A more relaxed approach would be to call the timeout only in cases where a > link > > preload resource existed and m_preloads is not empty. That would minimize the > > timeout calls, without much complexity. WDYT? > > Would that require a member "hadLinkPreload"? That seems strange, I'll defer to > Nate though. It would. My thought behind that is that at least currently most ResourceFetchers don't have preload links, so we could skip that timer for them, even if they have preloads. At the same time, non-link-preload preloads would get cleared on DCL, so such a flag may not be necessary in order to skip it in all practical cases of pages with no link preload. > Gating this on m_preloads not being empty SGTM though.
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/20 08:24:02, Yoav Weiss wrote: > On 2016/09/19 15:27:15, Charlie Harrison wrote: > > On 2016/09/19 15:22:24, Yoav Weiss wrote: > > > On 2016/09/19 14:06:45, Charlie Harrison wrote: > > > > > > > > > > https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2343623002/diff/200001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:662: > > > > EXPECT_FALSE(fetcher2->hasUnusedLinkPreloads()); > > > > On 2016/09/19 10:57:41, Yoav Weiss wrote: > > > > > This fails because the resource->isLinkPreload() is no longer true, > which > > > > > doesn't decrement the counter on the second fetcher, which makes me > think > > a > > > > > counter may be the wrong way to approach this :/ > > > > > > > > Yeah you're right. Good thing this test showed that. To go with the > counter > > > > approach you'd have to propagate the link preload change to all fetchers. > > > That's > > > > a lot of complexity. > > > > > > > > I'm willing to relax my previous push-back on the timeout, in the face of > > > this. > > > > What do you think Nate? > > > > > > A more relaxed approach would be to call the timeout only in cases where a > > link > > > preload resource existed and m_preloads is not empty. That would minimize > the > > > timeout calls, without much complexity. WDYT? > > > > Would that require a member "hadLinkPreload"? That seems strange, I'll defer > to > > Nate though. > It would. My thought behind that is that at least currently most > ResourceFetchers don't have preload links, so we could skip that timer for them, > even if they have preloads. > > At the same time, non-link-preload preloads would get cleared on DCL, so such a > flag may not be necessary in order to skip it in all practical cases of pages > with no link preload. > > > Gating this on m_preloads not being empty SGTM though. Uploaded a new patch which only looks at m_preloads' size. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yeah the fact that we clear m_preloads on DCL helps here. I would maybe say that in a comment near the one shot timer to be clear about when it actually fires though. Otherwise, the latest patch still lgtm https://codereview.chromium.org/2343623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h (right): https://codereview.chromium.org/2343623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:72: setResource(resource, Resource::DontMarkAsReferenced); side note: I'm glad this new enum is proving useful.
https://codereview.chromium.org/2343623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h (right): https://codereview.chromium.org/2343623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:72: setResource(resource, Resource::DontMarkAsReferenced); On 2016/09/20 12:15:19, Charlie Harrison wrote: > side note: I'm glad this new enum is proving useful. Very useful! :)
The CQ bit was checked by yoav@yoav.ws
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2343623002/#ps240001 (title: "Added comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a warning whenever link preloads are not used This CL adds a console warning if 3 seconds after the page's onload, there are still preloaded resources that were not used. BUG=646797 ========== to ========== Add a warning whenever link preloads are not used This CL adds a console warning if 3 seconds after the page's onload, there are still preloaded resources that were not used. BUG=646797 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add a warning whenever link preloads are not used This CL adds a console warning if 3 seconds after the page's onload, there are still preloaded resources that were not used. BUG=646797 ========== to ========== Add a warning whenever link preloads are not used This CL adds a console warning if 3 seconds after the page's onload, there are still preloaded resources that were not used. BUG=646797 Committed: https://crrev.com/4fcc2d6bfddd9592403148e752620a987d1c15ae Cr-Commit-Position: refs/heads/master@{#420015} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/4fcc2d6bfddd9592403148e752620a987d1c15ae Cr-Commit-Position: refs/heads/master@{#420015}
Message was sent while issue was closed.
FYI: this happened to flake a layout test in debug mode - I guess the test is slow and took more than 3 seconds to use preload. Not sure we should do anything. https://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_dbg/...
Message was sent while issue was closed.
On 2016/12/05 16:30:49, dgozman wrote: > FYI: this happened to flake a layout test in debug mode - I guess the test is > slow and took more than 3 seconds to use preload. Not sure we should do > anything. > https://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_dbg/... What I did elsewhere was to move tests that may trigger this (so tests that use link preload without actually using the resource) to testharness (which is a good thing in general) |
