Notify the HTMLDocumentParser on document element available

Notify the HTMLDocumentParser on document element available

This patch adds logic to the HTMLHtmlElement so that it can notify
the HTMLDocumentParser immediately when the document element is available.
This is triggered in HTMLHtmlElement::insertedByParser. Additionally,
logic to dispatch notifications to the embedder is  refactored into
insertedByParser.

insertedByParser initializes the ApplicationCache, then sends the
notification to the HTMLDocumentParser to issue any queued preloads (the
preloads wait for ApplicationCache so they don't download needlessly).
Next, the method dispatches notifications to embedders, via
FrameLoader::dispatchDocumentElementAvailable and
FrameLoader::runscriptsAtDocumentAvailable.

Note the latter is used by extensions, which can run extremely expensive
content scripts. This patch changes ordering so that preloads can be sent
before this happens.

BUG=618101
Committed: https://crrev.com/d63bd15666f24c048b0caa641cace3eec9460769
Cr-Commit-Position: refs/heads/master@{#398937}

[Charlie Harrison, 2016-06-07 18:53:29]

Description was changed from

==========
Notify the HTMLDocumentParser on document element available

This patch adds a hook to DocumentParser so that the HTMLConstructionSite
can notify the HTMLDocumentParser immediately when the document element
is available. This is used for preloads, which wait for the
ApplicationCache to be initialized before they are sent out.

Before this patch, hooks into //content are called which execute expensive
operations like extension content scripts. This patch executes that code
after preloads are sent out.

BUG=
==========

to

==========
Notify the HTMLDocumentParser on document element available

This patch adds a hook to DocumentParser so that the HTMLConstructionSite
can notify the HTMLDocumentParser immediately when the document element
is available. This is used for preloads, which wait for the
ApplicationCache to be initialized before they are sent out.

Expensive operations like extension content scripts are sent out
upon calling FrameLoader::dispatchDocumentElementAvailable. This
patch sents preloads before that call.

BUG=
==========

[Charlie Harrison, 2016-06-07 21:38:07]

csharrison@chromium.org changed reviewers:
+ kouhei@chromium.org, rdevlin.cronin@chromium.org

[Charlie Harrison, 2016-06-07 21:38:08]

WDYT about this change?

Let's see what the bots think. Devlin, do extension folks have page cyclers /
other canonical means of perf testing?

It like to get a rough sense of the improvement here without running a finch
trial.

[Charlie Harrison, 2016-06-07 21:48:42]

Description was changed from

==========
Notify the HTMLDocumentParser on document element available

This patch adds a hook to DocumentParser so that the HTMLConstructionSite
can notify the HTMLDocumentParser immediately when the document element
is available. This is used for preloads, which wait for the
ApplicationCache to be initialized before they are sent out.

Expensive operations like extension content scripts are sent out
upon calling FrameLoader::dispatchDocumentElementAvailable. This
patch sents preloads before that call.

BUG=
==========

to

==========
Notify the HTMLDocumentParser on document element available

This patch adds a hook to DocumentParser so that the HTMLConstructionSite
can notify the HTMLDocumentParser immediately when the document element
is available. This is used for preloads, which wait for the
ApplicationCache to be initialized before they are sent out.

Expensive operations like extension content scripts are sent out
upon calling FrameLoader::dispatchDocumentElementAvailable. This
patch sents preloads before that call.

BUG=618101
==========

[Devlin, 2016-06-08 00:18:53]

On 2016/06/07 21:38:08, csharrison wrote:
> Let's see what the bots think. Devlin, do extension folks have page cyclers /
> other canonical means of perf testing?
Not really, no - not beyond normal chrome telemetry tests.

Re this change, I'm not very familiar with the intricacies of preload.  We make
guarantees that content scripts injecting at document start run before *any
other DOM is constructed or any other script is run.*  preloading doesn't
trigger the script AIUI, but does it result in any DOM being constructed?

[Charlie Harrison, 2016-06-08 00:33:34]

On 2016/06/08 00:18:53, Devlin (slow until June 9) wrote:
> On 2016/06/07 21:38:08, csharrison wrote:
> > Let's see what the bots think. Devlin, do extension folks have page cyclers
/
> > other canonical means of perf testing?
> Not really, no - not beyond normal chrome telemetry tests.
OK, that may be enough if the telemetry tests actually use extensions.

> 
> Re this change, I'm not very familiar with the intricacies of preload.  We
make
> guarantees that content scripts injecting at document start run before *any
> other DOM is constructed or any other script is run.*  preloading doesn't
> trigger the script AIUI, but does it result in any DOM being constructed?
I'm pretty sure it won't result in any DOM being constructed. If that's the only
guarantee I think this is safe. I'll let kouhei@ confirm though.

[kouhei (in TOK), 2016-06-08 05:09:11]

> > Not really, no - not beyond normal chrome telemetry tests.
> OK, that may be enough if the telemetry tests actually use extensions.
Page Cycler (both v1/v2) doesn't involve extensions.

> > Re this change, I'm not very familiar with the intricacies of preload.  We
make
> > guarantees that content scripts injecting at document start run before *any
> > other DOM is constructed or any other script is run.*  preloading doesn't
> > trigger the script AIUI, but does it result in any DOM being constructed?
> I'm pretty sure it won't result in any DOM being constructed. If that's the
only
> guarantee I think this is safe. I'll let kouhei@ confirm though.
csharrison@ is correct. Preload cannot create any dom.

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/images/width-on-broken-data-src.html
(right):

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/images/width-on-broken-data-src.html:7:
checkLayout("img");
Would you explain this change?

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/passive-mixed-content-iframe.html
(right):

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/passive-mixed-content-iframe.html:6:
}
and this?

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp
(right):

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:386:
m_document->parser()->documentElementAvailable();
I'm not a fan of cyclic-dependency introduced here.
HTMLDocumentParser owns HTMLConstructionSite via HTMLTreeBuilder.

Can we remove dispatchDocumentElementAvailableIfNeeded() and move the logic
including this to HTMLHtmlElement::insertedByParser?

[Charlie Harrison, 2016-06-08 22:25:50]

OK looks like the bots are reasonably happy. Ready for a second pass.

I updated so that all calls to insertedByParser() dispatch these events. In
order for tests to start passing I had to subtly change some ordering behavior.
To get the exact same semantics as PS 3 we would have to accept a less clean
solution that has the XMLDocumentParser still dispatching events (if the element
is not an HTMLHtmlElement), among some other more complicated behavior.

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/images/width-on-broken-data-src.html
(right):

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/images/width-on-broken-data-src.html:7:
checkLayout("img");
On 2016/06/08 at 05:09:11, kouhei wrote:
> Would you explain this change?

This is a timing related issue. I think the problem is that we're querying the
image state before the image is fully loaded (or processed). Moving the check to
onload fixes it. You can also observe the problem if you navigate to:

data:text/html;charset=utf-8,<img id='a' src='data:dsa'><script>
console.log(document.getElementById('a').width)</script>

and

data:text/html;charset=utf-8,<img id='a' src='data:dsa'><script>window.onload =
function () { console.log(document.getElementById('a').width)}</script>

The first one reports 0 width and the second reports 20.

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/passive-mixed-content-iframe.html
(right):

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/passive-mixed-content-iframe.html:6:
}
On 2016/06/08 at 05:09:11, kouhei wrote:
> and this?

The problem was that line numbers were being reported non-deterministically for
the image element. This flakiness has happened before and dcheng@ recommended
this approach to insert the element dynamically.

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp
(right):

https://codereview.chromium.org/2042413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:386:
m_document->parser()->documentElementAvailable();
On 2016/06/08 at 05:09:11, kouhei wrote:
> I'm not a fan of cyclic-dependency introduced here.
> HTMLDocumentParser owns HTMLConstructionSite via HTMLTreeBuilder.
> 
> Can we remove dispatchDocumentElementAvailableIfNeeded() and move the logic
including this to HTMLHtmlElement::insertedByParser?

Yeah sure. I agree this is a non ideal architecture. One thing to note is that
|insertedByParser| is called in more places than this method. I've had to make
some changes to get this to work.

[kouhei (in TOK), 2016-06-09 01:13:47]

lgtm % description update

Thanks for doing the refactoring work. I really like PS7. Yes, XMLDocumentParser
is a bit unfortunate, but I think the new code is a lot easier to read and
maintain.

[Charlie Harrison, 2016-06-09 02:56:25]

Description was changed from

==========
Notify the HTMLDocumentParser on document element available

This patch adds a hook to DocumentParser so that the HTMLConstructionSite
can notify the HTMLDocumentParser immediately when the document element
is available. This is used for preloads, which wait for the
ApplicationCache to be initialized before they are sent out.

Expensive operations like extension content scripts are sent out
upon calling FrameLoader::dispatchDocumentElementAvailable. This
patch sents preloads before that call.

BUG=618101
==========

to

==========
Notify the HTMLDocumentParser on document element available

This patch adds logic to the HTMLHtmlElement so that it can notify
the HTMLDocumentParser immediately when the document element is available.
This is triggered in HTMLHtmlElement::insertedByParser. Additionally,
logic to dispatch notifications to the embedder is  refactored into
insertedByParser.

insertedByParser initializes the ApplicationCache, then sends the
notification to the HTMLDocumentParser to issue any queued preloads (the
preloads wait for ApplicationCache so they don't download needlessly).
Next, the method dispatches notifications to embedders, via
FrameLoader::dispatchDocumentElementAvailable and
FrameLoader::runscriptsAtDocumentAvailable.

Note the latter is used by extensions, which can run extremely expensive
content scripts. This patch changes ordering so that preloads can be sent
before this happens.

TBR=rdevlin.cronin@chromium.org
BUG=618101
==========

[Charlie Harrison, 2016-06-09 02:58:02]

Description was changed from

==========
Notify the HTMLDocumentParser on document element available

This patch adds logic to the HTMLHtmlElement so that it can notify
the HTMLDocumentParser immediately when the document element is available.
This is triggered in HTMLHtmlElement::insertedByParser. Additionally,
logic to dispatch notifications to the embedder is  refactored into
insertedByParser.

insertedByParser initializes the ApplicationCache, then sends the
notification to the HTMLDocumentParser to issue any queued preloads (the
preloads wait for ApplicationCache so they don't download needlessly).
Next, the method dispatches notifications to embedders, via
FrameLoader::dispatchDocumentElementAvailable and
FrameLoader::runscriptsAtDocumentAvailable.

Note the latter is used by extensions, which can run extremely expensive
content scripts. This patch changes ordering so that preloads can be sent
before this happens.

TBR=rdevlin.cronin@chromium.org
BUG=618101
==========

to

==========
Notify the HTMLDocumentParser on document element available

This patch adds logic to the HTMLHtmlElement so that it can notify
the HTMLDocumentParser immediately when the document element is available.
This is triggered in HTMLHtmlElement::insertedByParser. Additionally,
logic to dispatch notifications to the embedder is  refactored into
insertedByParser.

insertedByParser initializes the ApplicationCache, then sends the
notification to the HTMLDocumentParser to issue any queued preloads (the
preloads wait for ApplicationCache so they don't download needlessly).
Next, the method dispatches notifications to embedders, via
FrameLoader::dispatchDocumentElementAvailable and
FrameLoader::runscriptsAtDocumentAvailable.

Note the latter is used by extensions, which can run extremely expensive
content scripts. This patch changes ordering so that preloads can be sent
before this happens.

BUG=618101
==========

[Charlie Harrison, 2016-06-09 03:00:47]

On 2016/06/09 at 01:13:47, kouhei wrote:
> lgtm % description update
> 
> Thanks for doing the refactoring work. I really like PS7. Yes,
XMLDocumentParser is a bit unfortunate, but I think the new code is a lot easier
to read and maintain.

Thanks! Updated the description. Devlin, would you like to review this or should
I go ahead and land?

Concerning performance. I talked with jkarlin@ about this and I think I'm fine
with just landing this and observing any change in UMA. If we want to run an
experiment with issuing preloads before appcache then we could do a 3-pronged
experiment
1. This behavior
2. Old behavior
3. Before appcache

It should be fairly easy to simulate the old behavior.

[Charlie Harrison, 2016-06-09 03:01:10]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-09 03:01:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2042413002/120001

[kinuko, 2016-06-09 03:55:32]

Since this is not adding tech debt but rather simplifying things (I like the
resulting code better too) I'm fine landing this just as is (and revert if this
breaks something / has clear regression). OTOH if you want to claim a clear win
in some metrics by this change we could also run an experiment.

[Charlie Harrison, 2016-06-09 04:01:31]

On 2016/06/09 at 03:55:32, kinuko wrote:
> Since this is not adding tech debt but rather simplifying things (I like the
resulting code better too) I'm fine landing this just as is (and revert if this
breaks something / has clear regression). OTOH if you want to claim a clear win
in some metrics by this change we could also run an experiment.

Yeah I was thinking if we wanted to prove clear wins we could run an experiment
after the fact, by disabling this feature (possibly in conjunction with
preloading before AppCache or something).

[commit-bot: I haz the power, 2016-06-09 04:09:46]

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

[commit-bot: I haz the power, 2016-06-09 04:09:47]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-06-09 17:32:10]

On 2016/06/09 03:00:47, csharrison wrote:
> Thanks! Updated the description. Devlin, would you like to review this or
should
> I go ahead and land?

As long as all the guarantees we make for content scripts continue to hold true
(which it sounds like they will, and we should have at least one test for), I'm
happy to defer to the far more knowledgeable blink owners. :)

[Charlie Harrison, 2016-06-09 17:33:20]

On 2016/06/09 at 17:32:10, rdevlin.cronin wrote:
> On 2016/06/09 03:00:47, csharrison wrote:
> > Thanks! Updated the description. Devlin, would you like to review this or
should
> > I go ahead and land?
> 
> As long as all the guarantees we make for content scripts continue to hold
true (which it sounds like they will, and we should have at least one test for),
I'm happy to defer to the far more knowledgeable blink owners. :)

Great, thanks!

[Charlie Harrison, 2016-06-09 17:33:25]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-06-09 17:34:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2042413002/120001

[commit-bot: I haz the power, 2016-06-09 17:40:43]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-09 17:40:50]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-09 17:42:38]

Description was changed from

==========
Notify the HTMLDocumentParser on document element available

This patch adds logic to the HTMLHtmlElement so that it can notify
the HTMLDocumentParser immediately when the document element is available.
This is triggered in HTMLHtmlElement::insertedByParser. Additionally,
logic to dispatch notifications to the embedder is  refactored into
insertedByParser.

insertedByParser initializes the ApplicationCache, then sends the
notification to the HTMLDocumentParser to issue any queued preloads (the
preloads wait for ApplicationCache so they don't download needlessly).
Next, the method dispatches notifications to embedders, via
FrameLoader::dispatchDocumentElementAvailable and
FrameLoader::runscriptsAtDocumentAvailable.

Note the latter is used by extensions, which can run extremely expensive
content scripts. This patch changes ordering so that preloads can be sent
before this happens.

BUG=618101
==========

to

==========
Notify the HTMLDocumentParser on document element available

This patch adds logic to the HTMLHtmlElement so that it can notify
the HTMLDocumentParser immediately when the document element is available.
This is triggered in HTMLHtmlElement::insertedByParser. Additionally,
logic to dispatch notifications to the embedder is  refactored into
insertedByParser.

insertedByParser initializes the ApplicationCache, then sends the
notification to the HTMLDocumentParser to issue any queued preloads (the
preloads wait for ApplicationCache so they don't download needlessly).
Next, the method dispatches notifications to embedders, via
FrameLoader::dispatchDocumentElementAvailable and
FrameLoader::runscriptsAtDocumentAvailable.

Note the latter is used by extensions, which can run extremely expensive
content scripts. This patch changes ordering so that preloads can be sent
before this happens.

BUG=618101

Committed: https://crrev.com/d63bd15666f24c048b0caa641cace3eec9460769
Cr-Commit-Position: refs/heads/master@{#398937}
==========

[commit-bot: I haz the power, 2016-06-09 17:42:40]

Patchset 7 (id:??) landed as
https://crrev.com/d63bd15666f24c048b0caa641cace3eec9460769
Cr-Commit-Position: refs/heads/master@{#398937}

[Charlie Harrison, 2016-06-12 11:18:47]

On 2016/06/09 at 17:42:40, commit-bot wrote:
> Patchset 7 (id:??) landed as
https://crrev.com/d63bd15666f24c048b0caa641cace3eec9460769
> Cr-Commit-Position: refs/heads/master@{#398937}

This may have caused a regression if the site has some expensive inline scripts
first, see
http://stevesouders.com/cuzillion/?c0=hb0hfff0_2_f&c1=hc1hfff2_0_f&c2=hc1hfff...

I have some ideas about how to fix this, but I think we should keep this patch.
Initial UMA indicates an improvement of WebCore.PreloadDelayMS.

[pdknsk, 2016-10-18 02:57:38]

This has partially broken content scripts which insert a <script> tag at
document_start.

https://bugs.chromium.org/p/chromium/issues/detail?id=634381#c6