Use the Document rather than the Frame's execution context to install JS-based marquee elements

Use the Document rather than the Frame's execution context to install JS-based marquee elements

Depending on the frame results in a crash inside <template> tags, which don't have one.

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

[haraken, 2014-10-20 23:47:54]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2014-10-20 23:47:54]

Thanks for looking into this!

I also have a CL here (https://codereview.chromium.org/661693002/), but not yet
complete :-/

https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/PrivateScriptRunner.cpp:179: return
v8::Handle<v8::Value>();

Won't your CL hit this branch and just return an empty handle? I'm afraid we'll
just fail to install private scripts.

[rhogan, 2014-10-21 17:32:30]

https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/PrivateScriptRunner.cpp:179: return
v8::Handle<v8::Value>();
On 2014/10/20 at 23:47:54, haraken wrote:
> Won't your CL hit this branch and just return an empty handle? I'm afraid
we'll just fail to install private scripts.

No, document->executionContext() isn't empty AFAICT. I tried this out with and
got a working marquee:

<button onclick="useIt()">Use me</button>
<div id="container"></div>
<script>
function useIt() {
    var content = document.querySelector('template').content;
    document.querySelector('#container').appendChild(
    document.importNode(content, true));
}
</script>

<template>
<marquee>dsadsa</marquee>
</template>

[haraken, 2014-10-22 12:13:25]

haraken@chromium.org changed reviewers:
+ adamk@chromium.org

[haraken, 2014-10-22 12:13:26]

Oh, you're clever! We can use document->executionContext() (instead of document
itself) to get the executionContext of the context Document (and thus get the
frame on which the private script runs).

LGTM, but let's wait for Adamk's approval.

[adamk, 2014-10-22 17:38:44]

adamk@chromium.org changed reviewers:
+ morrita@chromium.org

[adamk, 2014-10-22 17:38:45]

This general approach looks promising, but the call to
document->executionContext() is super-weird. Adding morrita to get some
understanding of the state of ExecutionContexts and Documents.

https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/PrivateScriptRunner.cpp:174: v8::Handle<v8::Value>
PrivateScriptRunner::installClassIfNeeded(Document* document, String className)
Asking for a Document here is odd, there are likely non-DOM Blink-in-JS features
in the future. But I see from below that it really is a Document that you need.

https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/PrivateScriptRunner.cpp:177: v8::Handle<v8::Context>
context = toV8Context(document->executionContext(),
DOMWrapperWorld::privateScriptIsolatedWorld());
This is pretty magical-looking. I had to go read through a lot of files to
figure out that this was calling Node::executionContext(), which was then
calling Document::contextDocument(). It makes me think we're in a very strange
place with the ExecutionContext refactor. If this is the right way to get ahold
of the ExecutionContext for a Document, it really seems like Document should
stop subclassing ExecutionContext...

I don't feel like holding up Blink-in-JS on this refactoring issue, but I'm
CCing morrita to get an idea of how close we are to getting rid of Document as
an ExecutionContext.

https://codereview.chromium.org/654583003/diff/20001/Source/core/html/HTMLMar...
File Source/core/html/HTMLMarqueeElement.cpp (right):

https://codereview.chromium.org/654583003/diff/20001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.cpp:38:
RELEASE_ASSERT(!classObject.IsEmpty());
This RELEASE_ASSERT still looks scary to me. There's code in
Document::contextDocument() that'll sometimes return NULL, which would result in
a crash here. Why are we sure that it's impossible for a contextDocument's
m_frame to be non-empty? Or are we just sure that we won't be running the
HTMLMarqueeElement constructor at that time?

[Hajime Morrita, 2014-10-22 17:46:22]

https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/PrivateScriptRunner.cpp:177: v8::Handle<v8::Context>
context = toV8Context(document->executionContext(),
DOMWrapperWorld::privateScriptIsolatedWorld());
On 2014/10/22 17:38:45, adamk wrote:
> This is pretty magical-looking. I had to go read through a lot of files to
> figure out that this was calling Node::executionContext(), which was then
> calling Document::contextDocument(). It makes me think we're in a very strange
> place with the ExecutionContext refactor. If this is the right way to get
ahold
> of the ExecutionContext for a Document, it really seems like Document should
> stop subclassing ExecutionContext...
> 
> I don't feel like holding up Blink-in-JS on this refactoring issue, but I'm
> CCing morrita to get an idea of how close we are to getting rid of Document as
> an ExecutionContext.

I think the function is an leftover of my failed attempt. Document is a
ExecutionContext.

[adamk, 2014-10-22 17:51:35]

https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/PrivateScriptRunner.cpp:177: v8::Handle<v8::Context>
context = toV8Context(document->executionContext(),
DOMWrapperWorld::privateScriptIsolatedWorld());
On 2014/10/22 at 17:46:22, morrita wrote:
> On 2014/10/22 17:38:45, adamk wrote:
> > This is pretty magical-looking. I had to go read through a lot of files to
> > figure out that this was calling Node::executionContext(), which was then
> > calling Document::contextDocument(). It makes me think we're in a very
strange
> > place with the ExecutionContext refactor. If this is the right way to get
ahold
> > of the ExecutionContext for a Document, it really seems like Document should
> > stop subclassing ExecutionContext...
> > 
> > I don't feel like holding up Blink-in-JS on this refactoring issue, but I'm
> > CCing morrita to get an idea of how close we are to getting rid of Document
as
> > an ExecutionContext.
> 
> I think the function is an leftover of my failed attempt. Document is a
ExecutionContext.

And yet it can (and does in this case) sometimes return a _different_ Document
than the one passed-in as |this|.

I think I'd rather see this code be more explicit for now:

document->contextDocument()

reads better to me than

document->executionContext()

rhogan, haraken, how does that sound?

[Hajime Morrita, 2014-10-22 18:17:47]

On 2014/10/22 17:51:35, adamk wrote:
>
https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
> File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/654583003/diff/20001/Source/bindings/core/v8/...
> Source/bindings/core/v8/PrivateScriptRunner.cpp:177: v8::Handle<v8::Context>
> context = toV8Context(document->executionContext(),
> DOMWrapperWorld::privateScriptIsolatedWorld());
> On 2014/10/22 at 17:46:22, morrita wrote:
> > On 2014/10/22 17:38:45, adamk wrote:
> > > This is pretty magical-looking. I had to go read through a lot of files to
> > > figure out that this was calling Node::executionContext(), which was then
> > > calling Document::contextDocument(). It makes me think we're in a very
> strange
> > > place with the ExecutionContext refactor. If this is the right way to get
> ahold
> > > of the ExecutionContext for a Document, it really seems like Document
should
> > > stop subclassing ExecutionContext...
> > > 
> > > I don't feel like holding up Blink-in-JS on this refactoring issue, but
I'm
> > > CCing morrita to get an idea of how close we are to getting rid of
Document
> as
> > > an ExecutionContext.
> > 
> > I think the function is an leftover of my failed attempt. Document is a
> ExecutionContext.
> 
> And yet it can (and does in this case) sometimes return a _different_ Document
> than the one passed-in as |this|.
Oops. My comment was wrong.
IIRC it was added to make event dispatch in documents that don't have frames
(like one retrieved from XHR... and an import).


> 
> I think I'd rather see this code be more explicit for now:
> 
> document->contextDocument()
> 
> reads better to me than
> 
> document->executionContext()
> 
> rhogan, haraken, how does that sound?

[rhogan, 2014-10-22 18:48:53]

On 2014/10/22 at 17:51:35, adamk wrote:
> I think I'd rather see this code be more explicit for now:
> 
> document->contextDocument()
> 
> reads better to me than
> 
> document->executionContext()
> 
> rhogan, haraken, how does that sound?

Sure. That's the only update to the patch right?

[adamk, 2014-10-22 18:51:01]

On 2014/10/22 at 18:48:53, robhogan wrote:
> On 2014/10/22 at 17:51:35, adamk wrote:
> > I think I'd rather see this code be more explicit for now:
> > 
> > document->contextDocument()
> > 
> > reads better to me than
> > 
> > document->executionContext()
> > 
> > rhogan, haraken, how does that sound?
> 
> Sure. That's the only update to the patch right?

Yes, lgtm with that change. If you disagree that it's clearer, I'm also OK with
the patch as-is (as noted above, it's not your fault that this ExecutionContext
notion is so complicated).

[rhogan, 2014-10-22 22:54:21]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2014-10-22 22:54:42]

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

[haraken, 2014-10-23 00:52:06]

https://codereview.chromium.org/654583003/diff/20001/Source/core/html/HTMLMar...
File Source/core/html/HTMLMarqueeElement.cpp (right):

https://codereview.chromium.org/654583003/diff/20001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.cpp:38:
RELEASE_ASSERT(!classObject.IsEmpty());
On 2014/10/22 17:38:45, adamk wrote:
> This RELEASE_ASSERT still looks scary to me. There's code in
> Document::contextDocument() that'll sometimes return NULL, which would result
in
> a crash here. Why are we sure that it's impossible for a contextDocument's
> m_frame to be non-empty? Or are we just sure that we won't be running the
> HTMLMarqueeElement constructor at that time?

I think we can just remove installClassIfNeeded() completely, because
installClassIfNeeded() is lazily called when any private script API is invoked.

In this particular case, installClassIfNeeded() will be called when we call
V8HTMLMarqueeElement::PrivateScript::createdCallbackMethod(). If we don't have
any frame at that point, createdCallbackMethod() will return immediately without
doing nothing. (This is better than crashing, but it's still a wrong behavior.
We need a frame to execute a private script.)

I'll remove installClassIfNeeded() in a follow-up CL.

[commit-bot: I haz the power, 2014-10-23 02:41:05]

Committed patchset #4 (id:60001) as 184229