Always preload all tokens before parsing

Always preload all tokens before parsing

If Android WebView tests break from this change,
please disable those tests: crbug.com/387101

This change is in preparation for re-writing the path
through which tokens are handled on the main thread
to allow them to be batched and deferred until more
important work is done:
https://codereview.chromium.org/258013009/

Currently the threaded parser uses the main thread's
MessageQueue as its work-queue and we'll commonly
queue 10 chunks of work in the main thread MessageQueue
even if the first chunk takes a long time we'll still have to
process the next 9 before we can do more important things
like put up a frame because we don't have a separate
queue for parser work.  258013009 will fix that.

http/tests/security/script-crossorigin-loads-correctly-credentials-2.html
originally started failing with this change and it
turns out that always-preloading found a real bug
in CORS handling with preloads. The ResourceFetcher
was incorrectly always reusing an in-flight preload
request, even if the CORS state didn't match. Nate
helped me re-order the calls in requestResource to fix it.

This change also discovered 2 other bugs in the preloader.
1. a preload request != LinkPrefect request
and one will cancel the other. crbug.com/379893
2. The preloader has no concept of script type and will
blindly issue preloads for all <script href> regardless
of <script type>.  I decided this didn't matter in practice.
Making the <script type> handling threadsafe isn't
worth the trouble.

I would anticipate that this change might make some
speed-index scores better (we'll now be preloading
all tokens slightly sooner, especially those
which are in the same chunk as a long inline
script block for instance), but may make non-network
PLT scores lower (due to extra mallocs required
to process every chunk since more loads will
be preloaded just before their actually loaded).

BUG=356292
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175427

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176642

[eseidel, 2014-05-29 21:23:40]

This change shouldn't affect layout tests (other than the ones which log preload
events), but it does.  Suggesting we have a bug in our preload handling.

[eseidel, 2014-05-29 22:59:50]

I believe this is now ready for review.

[tonyg, 2014-05-29 23:27:25]

everything other than the two layout test failures lg2m. And it is very possible
those tests are just confused.

Pat, if low-overhead, it would be really interesting to get speed index of this
change. I bet it helps a lot.

https://codereview.chromium.org/302063002/diff/40001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLLinkElement/link-and-subresource-test-expected.txt
(right):

https://codereview.chromium.org/302063002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLLinkElement/link-and-subresource-test-expected.txt:3:
nick.jpg has MIME type image/jpeg
I'm confused why we see 3 loads here. Do you understand why?

https://codereview.chromium.org/302063002/diff/40001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLScriptElement/dont-load-unknown-type-expected.txt
(right):

https://codereview.chromium.org/302063002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLScriptElement/dont-load-unknown-type-expected.txt:2:
Unknown script types should not be loaded nor executed. If this test fails, an
extra line will appear in the output with the script's MIME type.
According to this message, this test is failing now.

https://codereview.chromium.org/302063002/diff/40001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/302063002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:363:
m_speculations.append(chunk);
These two lines are now identical to the other code path above. Is there a
cleaner way to structure this or is it so temporary that it doesn't matter.

[blink-reviews, 2014-05-29 23:47:26]

I'll kick it off in the morning. Should have some early data by eod
tomorrow.
On May 29, 2014 7:27 PM, <tonyg@chromium.org> wrote:

> everything other than the two layout test failures lg2m. And it is very
> possible
> those tests are just confused.
>
> Pat, if low-overhead, it would be really interesting to get speed index of
> this
> change. I bet it helps a lot.
>
>
> https://codereview.chromium.org/302063002/diff/40001/LayoutTests/fast/dom/
> HTMLLinkElement/link-and-subresource-test-expected.txt
> File
> LayoutTests/fast/dom/HTMLLinkElement/link-and-
> subresource-test-expected.txt
> (right):
>
> https://codereview.chromium.org/302063002/diff/40001/LayoutTests/fast/dom/
> HTMLLinkElement/link-and-subresource-test-expected.txt#newcode3
> LayoutTests/fast/dom/HTMLLinkElement/link-and-
> subresource-test-expected.txt:3:
> nick.jpg has MIME type image/jpeg
> I'm confused why we see 3 loads here. Do you understand why?
>
> https://codereview.chromium.org/302063002/diff/40001/LayoutTests/fast/dom/
> HTMLScriptElement/dont-load-unknown-type-expected.txt
> File
> LayoutTests/fast/dom/HTMLScriptElement/dont-load-unknown-type-expected.txt
> (right):
>
> https://codereview.chromium.org/302063002/diff/40001/LayoutTests/fast/dom/
> HTMLScriptElement/dont-load-unknown-type-expected.txt#newcode2
> LayoutTests/fast/dom/HTMLScriptElement/dont-load-
> unknown-type-expected.txt:2:
> Unknown script types should not be loaded nor executed. If this test
> fails, an extra line will appear in the output with the script's MIME
> type.
> According to this message, this test is failing now.
>
> https://codereview.chromium.org/302063002/diff/40001/
> Source/core/html/parser/HTMLDocumentParser.cpp
> File Source/core/html/parser/HTMLDocumentParser.cpp (right):
>
> https://codereview.chromium.org/302063002/diff/40001/
> Source/core/html/parser/HTMLDocumentParser.cpp#newcode363
> Source/core/html/parser/HTMLDocumentParser.cpp:363:
> m_speculations.append(chunk);
> These two lines are now identical to the other code path above. Is there
> a cleaner way to structure this or is it so temporary that it doesn't
> matter.
>
> https://codereview.chromium.org/302063002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Pat Meenan, 2014-05-30 12:37:31]

Test is up and running and I'll post results when they're ready (probably Monday
or Tuesday for the complete results).

[Pat Meenan, 2014-05-30 22:59:48]

On 2014/05/30 12:37:31, Pat Meenan wrote:
> Test is up and running and I'll post results when they're ready (probably
Monday
> or Tuesday for the complete results).

Not going to be a huge difference at slower network speeds (at least if the
initial data holds out).  With 50 pages completed in both configs the
differences are < 1%.  We'll see what happens as we get more data.

[eseidel, 2014-06-02 20:30:15]

Tony:

You're right that both of those tests look like failures.  I understand both of
them now, but neither of them look worth fixing at this time.  I've filed one
bug and decided that we don't care that we issue an extra load from the
preloader in the invalid-script-type case.  Let me know if you disagree.

[eseidel, 2014-06-02 20:30:20]

The CQ bit was checked by eseidel@chromium.org

[eseidel, 2014-06-02 20:30:22]

The CQ bit was unchecked by eseidel@chromium.org

[eseidel, 2014-06-02 20:35:43]

PTAL.

[tonyg, 2014-06-02 20:43:05]

lgtm

[Nate Chapin, 2014-06-02 20:43:58]

On 2014/06/02 20:35:43, eseidel wrote:
> PTAL.

LGTM

[eseidel, 2014-06-02 20:57:39]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-06-02 20:58:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eseidel@chromium.org/302063002/60001

[commit-bot: I haz the power, 2014-06-02 22:11:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9927)

[commit-bot: I haz the power, 2014-06-02 22:51:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 22:51:55]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9940)

[eseidel, 2014-06-03 18:59:34]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-06-03 18:59:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eseidel@chromium.org/302063002/80001

[commit-bot: I haz the power, 2014-06-03 20:25:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10434)

[commit-bot: I haz the power, 2014-06-03 20:49:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-03 20:49:43]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[eseidel, 2014-06-03 21:54:29]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-06-03 21:54:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eseidel@chromium.org/302063002/80001

[eseidel, 2014-06-03 22:59:28]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-06-03 23:00:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eseidel@chromium.org/302063002/90001

[commit-bot: I haz the power, 2014-06-04 02:21:17]

Change committed as 175427

[Pat Meenan, 2014-06-04 12:18:09]

On 2014/05/30 22:59:48, Pat Meenan wrote:
> On 2014/05/30 12:37:31, Pat Meenan wrote:
> > Test is up and running and I'll post results when they're ready (probably
> Monday
> > or Tuesday for the complete results).
> 
> Not going to be a huge difference at slower network speeds (at least if the
> initial data holds out).  With 50 pages completed in both configs the
> differences are < 1%.  We'll see what happens as we get more data.

The tests completed and showed ~1% improvement in Speed Index and Start render
without degrading the other metrics.  It's not huge but if it opens up the
opportunities for better preempting for more important it's going to be a nice
win.  I can pull together the full spreadsheet if anyone is interested in
looking at the details

[Julien - ping for review, 2014-06-04 20:21:03]

A revert of this CL has been created in
https://codereview.chromium.org/317703005/ by jchaffraix@chromium.org.

The reason for reverting is: This change breaks:
org.chromium.android_webview.test.AwSettingsTest#testBlockNetworkImagesDoesNotBlockDataUrlImage

Probably because it pre-loads images when "automatically load images" is
disabled on Android.

See
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered....

[eseidel, 2014-06-20 19:59:58]

relanding

[eseidel, 2014-06-20 20:00:18]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-06-20 20:01:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eseidel@chromium.org/302063002/90001

[commit-bot: I haz the power, 2014-06-20 20:13:49]

Change committed as 176642