[HTML Imports] Let script of imported document running.

[HTML Imports] Let script of imported document running.

This change allows <script> inside imported document running.
The implementation is based on a proposal at
https://www.w3.org/Bugs/Public/show_bug.cgi?id=22413.

This CL teaches ScriptLoader about HTML Imports so that it loads and
evaluates script if the document is an import.

For that purpose, now ScriptLoader distinguishes "executing document" which
is a master and "element" document which is an import, then executes scripts
on "executing" document.

Here are some remarks:
- This change doesn't make other resources, including stylesheets and images,
   being loaded on imports. The spec doesn't mention about them.
- This change doesn't take care of execution order. It will be attacked in
   following changes.
- Some of new CSP related tests capture some FAILures. This represents the 
   current limitation of Blink's CSP handling of HTML Import:
   It cannot prohibit eval() in imports and just follows master's context.
   This is because the imported scripts run on master's context and
   there is no way to the source of the eval() calls.

BUG=240592
TEST=csp-import-block-but-*.html, import-script-nosniff.html, import-script.html,
          import-script-prototypes.html
R=dglazkov,abarth

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

[Hajime Morrita, 2013-07-19 00:23:22]

Oops. I meant to have sent this yeasterday :-(
Could you take a look?

[Hajime Morrita, 2013-07-19 00:23:22]

Oops. I meant to have sent this yeasterday :-(
Could you take a look?

[abarth-chromium, 2013-07-19 07:55:55]

This is a scary change...  I've noted several places where we should have more
tests.  The only major issue is the one related to CSP and inline scripts.

https://codereview.chromium.org/19762002/diff/9001/LayoutTests/fast/html/impo...
File LayoutTests/fast/html/imports/import-master.html (right):

https://codereview.chromium.org/19762002/diff/9001/LayoutTests/fast/html/impo...
LayoutTests/fast/html/imports/import-master.html:10: testRunner.dumpAsText();
Normally we wrap this call in an

if (window.testRunner)
    testRunner.dumpAsText();

That way you can run the test case in another browser (like Firefox).

https://codereview.chromium.org/19762002/diff/9001/Source/core/dom/DocumentIn...
File Source/core/dom/DocumentInit.cpp (right):

https://codereview.chromium.org/19762002/diff/9001/Source/core/dom/DocumentIn...
Source/core/dom/DocumentInit.cpp:70: inline Frame*
DocumentInit::contextSourceFrame() const
Why not just sourceFrame() or just frame() ?  I don't understand what the words
"context" and "source" mean here.

https://codereview.chromium.org/19762002/diff/9001/Source/core/dom/DocumentIn...
File Source/core/dom/DocumentInit.h (right):

https://codereview.chromium.org/19762002/diff/9001/Source/core/dom/DocumentIn...
Source/core/dom/DocumentInit.h:52: bool hasSecurityContextSource() const;
What does "source" mean here?  Why not just "hasSecurityContext()?"

https://codereview.chromium.org/19762002/diff/9001/Source/core/dom/ScriptLoad...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/19762002/diff/9001/Source/core/dom/ScriptLoad...
Source/core/dom/ScriptLoader.cpp:325: return;
I'm not sure whether this check is right.  I think the idea is that inline
script should be allowed in imported documents because the import itself is
controlled by the script-src directive.  That means we should set
shouldBypassMainWorldContentSecurityPolicy to true if executingDocument !=
elementDocument.

Please add a test for this case.

https://codereview.chromium.org/19762002/diff/9001/Source/core/dom/ScriptLoad...
Source/core/dom/ScriptLoader.cpp:328:
executingDocument->addConsoleMessage(SecurityMessageSource, ErrorMessageLevel,
"Refused to execute script from '" + m_cachedScript->url().elidedString() + "'
because its MIME type ('" + m_cachedScript->mimeType() + "') is not executable,
and strict MIME type checking is enabled.");
Please add a test for this case as well.

https://codereview.chromium.org/19762002/diff/9001/Source/core/html/parser/HT...
File Source/core/html/parser/HTMLResourcePreloader.cpp (right):

https://codereview.chromium.org/19762002/diff/9001/Source/core/html/parser/HT...
Source/core/html/parser/HTMLResourcePreloader.cpp:85: Document*
executingDocument = m_document->import() ? m_document->import()->master() :
m_document;
What if the document that's imported itself imports another HTML document?  If
you don't already have tests for that case, please add them.

[abarth-chromium, 2013-07-19 07:58:43]

I think we need a lot more tests for this CL.  For example, we should test the
transitive case of an imported document importing more documents.  Also, we
should test that the prototype chains of DOM Nodes look sane from a script
executing in an imported document.  We should also look at the prototype chains
of other objects related to the imported document that aren't DOM nodes.

This CL breaks a lot of assumptions in a lot of code.  We can do that, but we
need to have good test coverage so that we don't forget this strange case in the
future.

[Hajime Morrita, 2013-07-19 08:09:09]

Thanks much for the thoughtful review, Adam!
I should think more about CSP cases and add tests for covering it.
WIll come back with them.

[dglazkov, 2013-07-19 16:02:04]

Adam, do you think we should bring this up for discussion in public-webappsec?
Or is this along the lines of what you guys already understood about HTML
imports?

[abarth-chromium, 2013-07-19 18:25:32]

On 2013/07/19 16:02:04, Dimitri Glazkov wrote:
> Adam, do you think we should bring this up for discussion in public-webappsec?
> Or is this along the lines of what you guys already understood about HTML
> imports?

We can discuss in public-webappsec if you like, but this CL is playing out the
way I expected from our previous conversation.  We should make sure the behavior
is defined in one or another of the specs.

[dglazkov, 2013-07-19 18:26:51]

On 2013/07/19 18:25:32, abarth wrote:
> On 2013/07/19 16:02:04, Dimitri Glazkov wrote:
> > Adam, do you think we should bring this up for discussion in
public-webappsec?
> > Or is this along the lines of what you guys already understood about HTML
> > imports?
> 
> We can discuss in public-webappsec if you like, but this CL is playing out the
> way I expected from our previous conversation.  We should make sure the
behavior
> is defined in one or another of the specs.

Sounds good, let's keep going then. Morrita-san's implementation is a practical
way to determine the details of the spec.

[Hajime Morrita, 2013-07-26 09:42:24]

And I'm back!

Adam, could you please take a look again? This one becomes much smaller than
before
because CSP is addressed by previous CL. This change just unblocks script
loading and execution for imports.

CSP related tests are added though, and it found a limitation about eval()
handling.
I updated the CL description to highlight it.

[abarth-chromium, 2013-07-26 17:17:34]

I'm sure there are going to be be issues with this code over time, but this CL
is a great start.  Thank you for the tests.

It's a bit hard to see which parts of the CL have test coverage because this CL
is a bit "all at once".  However, I don't have advice for how break it up into
smaller pieces.  We could land purposefully wrong code and then correct it piece
by piece with a test, but that doesn't seem worthwhile.

LGTM.  Let's land this CL and iterate as needed.  Thank you for breaking out a
bunch of smaller CLs already.

https://codereview.chromium.org/19762002/diff/25001/LayoutTests/http/tests/ht...
File LayoutTests/http/tests/htmlimports/import-script-nosniff.html (right):

https://codereview.chromium.org/19762002/diff/25001/LayoutTests/http/tests/ht...
LayoutTests/http/tests/htmlimports/import-script-nosniff.html:5: </script>
Is there a reason to have this empty script block?

https://codereview.chromium.org/19762002/diff/25001/LayoutTests/http/tests/ht...
File LayoutTests/http/tests/htmlimports/resources/having-domain-policy.html
(right):

https://codereview.chromium.org/19762002/diff/25001/LayoutTests/http/tests/ht...
LayoutTests/http/tests/htmlimports/resources/having-domain-policy.html:6:
<script src="http://localhost:8000//htmlimports/resources/cors-js.cgi"></script>
You should also test the case where the script element has the crossorigin
attribute.

https://codereview.chromium.org/19762002/diff/25001/Source/core/dom/ScriptLoa...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/19762002/diff/25001/Source/core/dom/ScriptLoa...
Source/core/dom/ScriptLoader.cpp:281:
request.setPotentiallyCrossOriginEnabled(elementDocument->securityOrigin(),
allowCredentials);
This one is slightly tricky.  I guess this is correct.  Is there a test that
covers the difference between using elementDocument and executingDocument here?

https://codereview.chromium.org/19762002/diff/25001/Source/core/dom/ScriptLoa...
Source/core/dom/ScriptLoader.cpp:386: if
(!elementDocument->fetcher()->canAccess(m_cachedScript.get())) {
This one is important to test too

https://codereview.chromium.org/19762002/diff/25001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/19762002/diff/25001/Source/core/html/parser/H...
Source/core/html/parser/HTMLDocumentParser.cpp:466: if (isStopped() ||
isWaitingForScripts())
Can you fix the instance on line 274 as well?

The idiom we use elsewhere in this file is to make these two separate if
statements (so that it's clear the order is important).

Good catch.  Thank you.

[commit-bot: I haz the power, 2013-07-29 05:32:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/19762002/33001

[Hajime Morrita, 2013-07-29 05:38:13]

Adam, thanks for your LGTM!

To address the feedback, I added a test for covering tricky case.
Then I realized that chrome's @crossorigin support for <script> is partial and
mkwst is working on fixing it. Such effort might help us to attack the eval()
limitation
since both needs to know the originated <script> element of specific
invocations, like throwing exception or invoking eval().

[commit-bot: I haz the power, 2013-07-29 07:44:59]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-07-30 01:10:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/19762002/33001

[commit-bot: I haz the power, 2013-07-30 06:34:17]

Failed to trigger a try job on win_blink_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-07-30 06:34:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/19762002/53001

[commit-bot: I haz the power, 2013-07-30 08:54:25]

Change committed as 155149