Not networking issue. Moving to Blink>Loader and Blink>HTML>Parser.

My gut is that this is due to the "type=invalid". There was an issue a while back with type= altering preload match semantics. I'll try to find it.

If I change type=invalid to type=text/javascript then it does the right thing, preloading once and then using that result: http://www.trycontra.com/preload-bug/with-valid-type

(What mod_pagespeed's defer_javsascript filter does is change all the types to something invalid to prevent the javascript from executing initially, and then starts them executing in a tweaked environment by re-inserting them with the right type.)

It seems like either an invalid type should keep the preload scanner from recognizing the url, or it should match things with a different type.  The current behavior kind of ends up being the worst of both worlds in this case!

OK I narrowed this down using tracing.

Essentially, marking the script as "invalid" prevents it from being run. This means that we never "use" the script resource in Blink. When HTML parsing ends, we clear all preloads that we never used, this evicts the resource from memory cache.

By the time the timer runs, we don't have the resource in memory cache, so we re-request it. At least, we'll populate HTTP cache :/

So, this is not a matching thing.

I'm not sure what the ideal solution is. What might work for your case is to replace the script tags with link rel="preload". What is the purpose of including scripts with type="invalid" ?

cc hiroshige for memory cache stuff.

> marking the script as "invalid" prevents it from being run.
> This means that we never "use" the script resource in Blink.
> When HTML parsing ends, we clear all preloads that we never
> used, this evicts the resource from memory cache.

Could we make the preload scanner skip any scripts that are marked with a type that would keep them from being run?

> At least, we'll populate HTTP cache :/

It doesn't look to me like we even do that, since I see a second load going out over the network.

> What might work for your case is to replace the script
> tags with link rel="preload". What is the purpose of
> including scripts with type="invalid"

We're running into this bug as part of mod_pagespeed's defer_javascript filter [1].  The goal of this filter is to rewrite web pages that use javascript in a synchronous way to instead use it asynchronously, as a fully automated transformation.  The filter rewrites the page server-side (in C++) from:

  <script src="a.js"></script>
  <script src="b.js"></script>
  <script src="c.js"></script>

  [web page body]

To:

  <script src="a.js" type="text/psajs" orig_index="0"></script>
  <script src="b.js" type="text/psajs" orig_index="1"></script>
  <script src="c.js" type="text/psajs" orig_index="2"></script>

  [web page body]

  <script src="js_defer.js">

The browser skips over a.js, b.js, and c.js while loading the page, because the type marks it as invalid and unrunnable.  Then js_defer.js goes back over the page, and runs each script in turn.  It has a bunch of tricky code to preserve the illusion for these scripts that they're running earlier in the loading process, for example it keeps "document.write()" able to write out the text in the right part of the DOM, and it keeps "s=document.getElementsByTagName("script"); s[s.length-1]" working.

In order to fix our usage it may be enough for me to change it from <script type=text/psajs ...> to <pagespeed-script ...>, though that's not as good as the preload scanner keeping the js data around for when we'll need it.

[1] https://developers.google.com/speed/pagespeed/module/filter-js-defer

The HTTP cache should follow typical semantics. You might have loaded the page bypassing the cache? In any case, that's a non-ideal solution.

We could make the preload scanner skip scripts with invalid types, but I want to come to a solution where we can preload your scripts before parsing the entire body. What if you used <pagespeed-script> in combination with link rel preload? link rel preload is smart enough to delay memory cache eviction by the end of the parse.

> You might have loaded the page bypassing the cache?

You're right, sorry!  I had devtools open with "disable cache" and I was also using Ctrl+Shift+R.  Testing properly I no longer see multiple loads over the network; the later loads of the js file are all pulled from cache.

This is actually making me wonder whether there's actually anything that needs fixing in Chrome or in mod_pagespeed here: as long as you're not testing in a debugging environment where you've disabled caching it loads efficiently.  Yes, it would be a bit more efficient to keep it in the memory cache instead of having to go out to the disk cache, but it's not huge.

You're right, this isn't a huge deal. I see two things that are possibly worth changing:
1. Use <pagespeed-script> + <link rel="preload"> to avoid IPC round trips to fetch from cache.
2. Maybe chrome should stop preloading things it won't use? If we go this way, pagespeed should move to solution (1) or else have it's scripts invisible to the preload scanner.

I wonder how many preload misses we have due to weird type= problems. Might be interesting to log some metrics there.

Let's keep restrict-view-google due to netlog containing internal URLs.

I think it's reasonable to not preload scripts with bad types. I will make that change. japhet, kouhei, what do you think? Best case scenario this improves PLT and data usage for broken pages. Worst case this makes page loads a bit slower for developers doing tricky things.

I can include a counter here to measure impact.

Actually, just deleting the net log
cc yoav@, I'll upload a patch set momentarily.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/23f4df7587f338837a709afb192900102584fc0a

commit 23f4df7587f338837a709afb192900102584fc0a
Author: csharrison <csharrison@chromium.org>
Date: Thu Jun 30 15:30:45 2016

Don't preload scripts with invalid type/language attributes

This patch keeps track of a script tag's type and language attributes,
and gates preloading if they are invalid. This aligns the preload scanner's
policy with the ScriptLoader's policy.

BUG= 623109 

Review-Url: https://codereview.chromium.org/2099853002
Cr-Commit-Position: refs/heads/master@{#403182}

[modify] https://crrev.com/23f4df7587f338837a709afb192900102584fc0a/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
[modify] https://crrev.com/23f4df7587f338837a709afb192900102584fc0a/third_party/WebKit/Source/core/dom/ScriptLoader.h
[modify] https://crrev.com/23f4df7587f338837a709afb192900102584fc0a/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
[modify] https://crrev.com/23f4df7587f338837a709afb192900102584fc0a/third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerTest.cpp

Going to close this for now. jefftk@ don't hesitate to open up another bug if something like this comes up again!

Awesome, thanks for the rapid fix!

Could someone please help us with the steps so that test team can verify this issue. As of now loading the URL: http://www.trycontra.com/preload-bug/with-set-timeout gives a blank page and in console it gives an error:

failed to load resource:the server responded with a status of 404 (Not found)
example.js executing

Try loading http://www.trycontra.com/preload-bug/with-set-timeout with developer tools on the network tab and "disable cache" checked.  In currently released versions of Chrome you'll see two loads for example.js [image attached]. In a fixed version, you should only see one load for example.js.

Deleted: tmp.png 14.6 KB

	tmp.png 14.6 KB View Download

FYI we are reverting this change due to a small compat risk where developers relied on this feature. I will be sending out an official intent to deprecate/remove today to see what the status is.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a825f90e8fdbd9e41bd2dcafffa88662fb50878f

commit a825f90e8fdbd9e41bd2dcafffa88662fb50878f
Author: csharrison <csharrison@chromium.org>
Date: Thu Jul 07 13:58:40 2016

Revert of Don't preload scripts with invalid type/language attributes (patchset #5 id:80001 of https://codereview.chromium.org/2099853002/ )

Reason for revert:
Some compat risk for this change was brought up. Going to send a note to blink-dev before re-landing.

Original issue's description:
> Don't preload scripts with invalid type/language attributes
>
> This patch keeps track of a script tag's type and language attributes,
> and gates preloading if they are invalid. This aligns the preload scanner's
> policy with the ScriptLoader's policy.
>
> BUG= 623109 
>
> Committed: https://crrev.com/23f4df7587f338837a709afb192900102584fc0a
> Cr-Commit-Position: refs/heads/master@{#403182}

TBR=yoav@yoav.ws
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 623109 

Review-Url: https://codereview.chromium.org/2130773002
Cr-Commit-Position: refs/heads/master@{#404153}

[modify] https://crrev.com/a825f90e8fdbd9e41bd2dcafffa88662fb50878f/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
[modify] https://crrev.com/a825f90e8fdbd9e41bd2dcafffa88662fb50878f/third_party/WebKit/Source/core/dom/ScriptLoader.h
[modify] https://crrev.com/a825f90e8fdbd9e41bd2dcafffa88662fb50878f/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
[modify] https://crrev.com/a825f90e8fdbd9e41bd2dcafffa88662fb50878f/third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerTest.cpp

csharrison@: This is fixed by  issue 626321 , right?

Yeah let's merge them