Fix Runtime.executionContextCreated for crafted iframes.

Fix Runtime.executionContextCreated for crafted iframes.

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

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

[eustas, 2014-10-28 14:07:12]

eustas@chromium.org changed reviewers:
+ sergeyv@chromium.org, vsevik@chromium.org, yurys@chromium.org

[eustas, 2014-10-28 15:03:55]

https://codereview.chromium.org/146693005

[eustas, 2014-10-29 15:03:03]

PTAL

[aandrey, 2014-10-29 15:18:50]

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

[aandrey, 2014-10-29 15:18:50]

needs a test

https://codereview.chromium.org/686763002/diff/20001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/686763002/diff/20001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:108: if
(!frame->script().initializeMainWorld()) {
I don't think we should initialize main world from inspector code.
Maybe this is a bad naming and should be actually named
isMainWorldContextInitialized() ?

[eustas, 2014-10-30 10:08:05]

PTAL

https://codereview.chromium.org/686763002/diff/20001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/686763002/diff/20001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:108: if
(!frame->script().initializeMainWorld()) {
On 2014/10/29 15:18:50, aandrey wrote:
> I don't think we should initialize main world from inspector code.
> Maybe this is a bad naming and should be actually named
> isMainWorldContextInitialized() ?

Actually, we need to initialize main world sooner or later, to provide ability
to manipulate the page.
This patch makes is explicit. Implicitly main world is initialized with
ScriptState::forMainWorld

[yurys, 2014-10-30 10:23:43]

https://codereview.chromium.org/686763002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/686763002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/WindowProxy.cpp:229: SecurityOrigin* origin =
m_frame->document()->securityOrigin();
When is SecurityOrigin null?

https://codereview.chromium.org/686763002/diff/40001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/686763002/diff/40001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:109: registerMainWorldContext(frame);
In this case we should not force initialization, it should happen regular way
without inspector's intervention. Can add an assert that on this path the
context is already created?

[aandrey, 2014-10-30 10:27:16]

lgtm + test

https://codereview.chromium.org/686763002/diff/40001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/686763002/diff/40001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:114: // Force creation on main world
contect.
maybe add a comment that creation otherwise will happen anyway in
ScriptState::forMainWorld() and we need to know whether it was created or not at
this time

https://codereview.chromium.org/686763002/diff/40001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:115: // Positive result means that
contect was just initialized,
typo: contect

https://codereview.chromium.org/686763002/diff/40001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:168: void
PageRuntimeAgent::reportExecutionContextCreation()
nit: reportExecutionContextsToFrontend

[eustas, 2014-10-31 06:59:54]

https://codereview.chromium.org/686763002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/686763002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/WindowProxy.cpp:229: SecurityOrigin* origin =
m_frame->document()->securityOrigin();
On 2014/10/30 10:23:43, yurys wrote:
> When is SecurityOrigin null?

According to setSecurityToken implementation, it can't be null for main world.

https://codereview.chromium.org/686763002/diff/40001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/686763002/diff/40001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:109: registerMainWorldContext(frame);
On 2014/10/30 10:23:43, yurys wrote:
> In this case we should not force initialization, it should happen regular way
> without inspector's intervention. Can add an assert that on this path the
> context is already created?

Done.

https://codereview.chromium.org/686763002/diff/40001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:114: // Force creation on main world
contect.
On 2014/10/30 10:27:16, aandrey wrote:
> maybe add a comment that creation otherwise will happen anyway in
> ScriptState::forMainWorld() and we need to know whether it was created or not
at
> this time

Done.

https://codereview.chromium.org/686763002/diff/40001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:115: // Positive result means that
contect was just initialized,
On 2014/10/30 10:27:16, aandrey wrote:
> typo: contect

Done.

https://codereview.chromium.org/686763002/diff/40001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:168: void
PageRuntimeAgent::reportExecutionContextCreation()
On 2014/10/30 10:27:16, aandrey wrote:
> nit: reportExecutionContextsToFrontend

Done.

[eustas, 2014-10-31 07:01:14]

PTAL

[vsevik, 2014-10-31 14:58:07]

https://codereview.chromium.org/686763002/diff/60001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/686763002/diff/60001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:125:
base::debug::StackTrace().Print();
remove

[yurys, 2014-10-31 15:08:03]

Also please provide description of what this patch is doing.

https://codereview.chromium.org/686763002/diff/60001/LayoutTests/http/tests/i...
File
LayoutTests/http/tests/inspector/runtime/runtime-crafted-frame-execution-context.html
(right):

https://codereview.chromium.org/686763002/diff/60001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/runtime/runtime-crafted-frame-execution-context.html:21:
WebInspector.runtimeModel.addEventListener(WebInspector.RuntimeModel.Events.ExecutionContextCreated,
onExecutionCintextCreated, {});
Can you turn this into a protocol test so that we are sure that it works when no
other agents are enabled?

https://codereview.chromium.org/686763002/diff/60001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/686763002/diff/60001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:175:
registerMainWorldContext(localFrame);
Am I right that now we may end up with 0 contexts reported to the front-end if
there are no scripts in the inspected page?

[eustas, 2014-11-03 20:14:16]

PTAL

https://codereview.chromium.org/686763002/diff/60001/LayoutTests/http/tests/i...
File
LayoutTests/http/tests/inspector/runtime/runtime-crafted-frame-execution-context.html
(right):

https://codereview.chromium.org/686763002/diff/60001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/runtime/runtime-crafted-frame-execution-context.html:21:
WebInspector.runtimeModel.addEventListener(WebInspector.RuntimeModel.Events.ExecutionContextCreated,
onExecutionCintextCreated, {});
On 2014/10/31 15:08:03, yurys wrote:
> Can you turn this into a protocol test so that we are sure that it works when
no
> other agents are enabled?

Great idea. Updading protocol test I've found that mainWorld for non-main frame
shouldn't be reported when "windowCleared" notification arrives.

Done.

https://codereview.chromium.org/686763002/diff/60001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/686763002/diff/60001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:125:
base::debug::StackTrace().Print();
On 2014/10/31 14:58:07, vsevik wrote:
> remove

Done.

https://codereview.chromium.org/686763002/diff/60001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:175:
registerMainWorldContext(localFrame);
On 2014/10/31 15:08:03, yurys wrote:
> Am I right that now we may end up with 0 contexts reported to the front-end if
> there are no scripts in the inspected page?

Yes and no.
Theoretically - yes.
But I've examined different cases (see stack-trace dumper Vsevolod requests to
remove), and found that there are no way for loaded page to live without context
(even on scriptless page). So, paractically - no.

[aandrey, 2014-11-04 11:21:08]

https://codereview.chromium.org/686763002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/686763002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/WindowProxy.cpp:231:
InspectorInstrumentation::didCreateMainWorldContext(m_frame,
m_scriptState.get(), origin);
ASSERT(origin);

[eustas, 2014-11-05 21:28:34]

yurys@ ping

https://codereview.chromium.org/686763002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/686763002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/WindowProxy.cpp:231:
InspectorInstrumentation::didCreateMainWorldContext(m_frame,
m_scriptState.get(), origin);
On 2014/11/04 11:21:07, aandrey wrote:
> ASSERT(origin);

Done.

[yurys, 2014-11-06 05:13:06]

On 2014/11/03 20:14:16, eustas wrote:
>
https://codereview.chromium.org/686763002/diff/60001/Source/core/inspector/Pa...
> Source/core/inspector/PageRuntimeAgent.cpp:175:
> registerMainWorldContext(localFrame);
> On 2014/10/31 15:08:03, yurys wrote:
> > Am I right that now we may end up with 0 contexts reported to the front-end
if
> > there are no scripts in the inspected page?
> 
> Yes and no.
> Theoretically - yes.
> But I've examined different cases (see stack-trace dumper Vsevolod requests to
> remove), and found that there are no way for loaded page to live without
context
> (even on scriptless page). So, paractically - no.

So you are telling that you examined some cases and didn't find any pages
without the context which doesn't necessarily mean that it will hold in other
cases. Can you either point at the code path that is always executed and leads
to the context initialization even for a scriptless page or come up with a way
to explicitly force the context initialization for inspector's needs.

[eustas, 2014-11-06 21:11:52]

> So you are telling that you examined some cases and didn't find any pages
> without the context which doesn't necessarily mean that it will hold in other
> cases. Can you either point at the code path that is always executed and leads
> to the context initialization even for a scriptless page or come up with a way
> to explicitly force the context initialization for inspector's needs.

Here is the stack-trace:

blink::ScriptController::updateDocument()
blink::LocalDOMWindow::installNewDocument(WTF::String const&,
blink::DocumentInit const&, bool)
blink::DocumentLoader::createWriterFor(blink::Document const*,
blink::DocumentInit const&, WTF::AtomicString const&, WTF::AtomicString const&,
bool)
blink::DocumentLoader::ensureWriter(WTF::AtomicString const&, blink::KURL
const&)
blink::DocumentLoader::commitData(char const*, unsigned long)
blink::DocumentLoader::finishedLoading(double)

Sooner or later, document is created even for non-html.

[eustas, 2014-11-07 14:25:18]

Ping

[yurys, 2014-11-07 16:17:07]

lgtm

[eustas, 2014-11-07 16:53:54]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-11-07 16:54:27]

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

[commit-bot: I haz the power, 2014-11-07 19:09:19]

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

[commit-bot: I haz the power, 2014-11-07 19:09:21]

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

[eustas, 2014-11-07 19:56:33]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-11-07 19:56:40]

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

[commit-bot: I haz the power, 2014-11-07 21:43:51]

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

[commit-bot: I haz the power, 2014-11-07 21:43:53]

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

[eustas, 2014-11-07 22:18:47]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-11-07 22:19:48]

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

[commit-bot: I haz the power, 2014-11-08 00:10:07]

Committed patchset #6 (id:100001) as 184995

[dcheng, 2014-11-08 03:57:25]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/711853002/ by dcheng@chromium.org.

The reason for reverting is: Speculatively reverting to fix telemetry_unittests
failures..

[dcheng, 2014-11-08 03:58:29]

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

[dcheng, 2014-11-08 03:58:29]

Here's a log of a failing test:
[ RUN      ]
backends.chrome.inspector_runtime_unittest.InspectorRuntimeTest.testIFrame

Traceback (most recent call last):
  run at
/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py:331
    testMethod()
  testIFrame at
tools/telemetry/telemetry/core/backends/chrome/inspector_runtime_unittest.py:48
    timeout=10)
  WaitFor at tools/telemetry/telemetry/core/util.py:84
    (timeout, GetConditionString()))
TimeoutException: Timed out while waiting 10s for util.WaitFor(lambda:
self._tab.EnableAllContexts() == expected_contexts,
                 timeout=10).

Locals:
  GetConditionString       : <function GetConditionString at 0x102548848>
  condition                : <function <lambda> at 0x102548758>
  elapsed_time             : 10.05207109451294
  last_output_elapsed_time : 10.05207109451294
  last_output_time         : 1415412519.995524
  max_poll_interval        : 5
  min_poll_interval        : 0.1
  now                      : 1415412530.047595
  output_interval          : 300
  poll_interval            : 0.9137236118316651
  res                      : False
  start_time               : 1415412519.995524
  timeout                  : 10

[  FAILED  ]
backends.chrome.inspector_runtime_unittest.InspectorRuntimeTest.testIFrame
(10745 ms)

Failing builders:
https://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/...
https://build.chromium.org/p/chromium.webkit/builders/Mac10.8%20Tests/builds/...
https://build.chromium.org/p/chromium.webkit/builders/Win7%20Tests/builds/6699

[eustas, 2014-11-12 06:47:15]

PTAL

[yurys, 2014-11-12 08:47:22]

https://codereview.chromium.org/686763002/diff/120001/Source/core/inspector/I...
File Source/core/inspector/InspectorPageAgent.cpp (left):

https://codereview.chromium.org/686763002/diff/120001/Source/core/inspector/I...
Source/core/inspector/InspectorPageAgent.cpp:986:
m_injectedScriptManager->discardInjectedScripts();
Let's move it at some earlier point on the navigation to make sure that all
injected scripts are discarded and we don't leak them along with the window
context.

[yurys, 2014-11-13 13:58:30]

https://codereview.chromium.org/686763002/diff/140001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptManager.cpp (right):

https://codereview.chromium.org/686763002/diff/140001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptManager.cpp:129:
m_scriptStateToId.remove(scriptState);
m_scriptStateToId.remove(it);

https://codereview.chromium.org/686763002/diff/140001/Source/core/inspector/P...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/686763002/diff/140001/Source/core/inspector/P...
Source/core/inspector/PageRuntimeAgent.cpp:104:
frame->script().initializeMainWorld();
How can we be sure that this is a scriptless document and main world context is
not expected to be created later?

https://codereview.chromium.org/686763002/diff/140001/Source/core/inspector/P...
Source/core/inspector/PageRuntimeAgent.cpp:163: if
(!localFrame->script().initializeMainWorld())
Mind adding a comment?

[vsevik, 2014-11-20 09:20:50]

vsevik@chromium.org changed reviewers:
- vsevik@chromium.org

[aandrey, 2014-12-04 11:03:28]

what's happening with this work?

[dcheng, 2014-12-04 18:58:56]

https://codereview.chromium.org/686763002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/686763002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/WindowProxy.cpp:109:
InspectorInstrumentation::willReleaseScriptContext(m_frame,
m_scriptState.get());
I'm changing WindowProxy to hold a Frame, not a LocalFrame. If possible, it'd be
nice if this patch would take that into account.

https://codereview.chromium.org/686763002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/WindowProxy.cpp:232: setSecurityToken(origin);
Please note I've removed this call to setSecurityToken(), since it's redundant
with updateDocument(). Unfortunately, that means the approach in this patch
won't work. Would it be possible to call
InspectorInstrumentation::didCreateScriptContext() in setSecurityToken()?

[eustas, 2014-12-05 10:01:37]

https://codereview.chromium.org/686763002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/686763002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/WindowProxy.cpp:109:
InspectorInstrumentation::willReleaseScriptContext(m_frame,
m_scriptState.get());
OK, but let's do it in separate patch.

https://codereview.chromium.org/686763002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/WindowProxy.cpp:232: setSecurityToken(origin);
On 2014/12/04 18:58:55, dcheng wrote:
> Please note I've removed this call to setSecurityToken(), since it's redundant
> with updateDocument(). Unfortunately, that means the approach in this patch
> won't work. Would it be possible to call
> InspectorInstrumentation::didCreateScriptContext() in setSecurityToken()?

I've updated this place.
setSecurityToken doesn't seem to be right place, because there is no 1:1
relation between creation on execution context and setting origin.

https://codereview.chromium.org/686763002/diff/140001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptManager.cpp (right):

https://codereview.chromium.org/686763002/diff/140001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptManager.cpp:129:
m_scriptStateToId.remove(scriptState);
On 2014/11/13 13:58:30, yurys wrote:
> m_scriptStateToId.remove(it);

Done.

https://codereview.chromium.org/686763002/diff/140001/Source/core/inspector/P...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/686763002/diff/140001/Source/core/inspector/P...
Source/core/inspector/PageRuntimeAgent.cpp:104:
frame->script().initializeMainWorld();
On 2014/11/13 13:58:30, yurys wrote:
> How can we be sure that this is a scriptless document and main world context
is
> not expected to be created later?

No guarantees. But this worked for ages...

https://codereview.chromium.org/686763002/diff/140001/Source/core/inspector/P...
Source/core/inspector/PageRuntimeAgent.cpp:163: if
(!localFrame->script().initializeMainWorld())
On 2014/11/13 13:58:30, yurys wrote:
> Mind adding a comment?

Done.

[eustas, 2014-12-09 06:36:16]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-12-09 06:36:25]

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

[commit-bot: I haz the power, 2014-12-09 07:42:57]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186762