Reland "Correctly set ScriptState in the image loader microtask"

Reland "Correctly set ScriptState in the image loader microtask"

Original review: https://codereview.chromium.org/1153233002/

BUG=487155
R=haraken@chromium.org

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

[jochen (gone - plz use gerrit), 2015-05-28 10:24:15]

this time, I'm taking the correct ScriptContext (the image might be created from
an isolated world), and I'm running the task also when the scriptContext is
invalid meanwhile, as the task has to do some cleanup work that would otherwise
result in leaks if skipped.

ptal

[haraken, 2015-05-28 10:57:38]

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
Source/core/loader/ImageLoader.cpp:96: m_scriptState =
ScriptState::from(toV8Context(&loader->element()->document(),
DOMWrapperWorld::mainWorld()));

Why can't we always use the line 96?

ScriptState::hasCurrentScriptState(isolate) is a hacky API whose usage should be
limited. "if(ScriptState::hasCurrentScriptState(isolate))" means that "if the
debugger context is not on top of the context stack".

So what we're doing here is:

  if (debugger context is not on top of the context stack)
    use ScriptState::current();
  else
    use ScriptState::from(toV8Context());

Here are two problems:

- ScriptState::current() is not guaranteed to be the same as
ScriptState::from(toV8Context()). I think the correct ScriptState we should use
is ScriptState::from(toV8Context()).

- The debugger context shouldn't be on top of the context stack when you reach
here. (It can rarely happen.)

So, in short, my suggestion is to unconditionally use
ScriptState::from(toV8Context()) here.

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
Source/core/loader/ImageLoader.cpp:120: v8::HandleScope
handleScope(m_scriptState->isolate());

This wouldn't be needed, since the ScriptState::Scope will create the handle
scope for you.

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
Source/core/loader/ImageLoader.cpp:124:
m_loader->doUpdateFromElement(m_shouldBypassMainWorldCSP, m_updateBehavior);

Do we really want to execute this when the context associated with this task is
already disposed?

[jochen (gone - plz use gerrit), 2015-05-28 11:16:49]

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
Source/core/loader/ImageLoader.cpp:96: m_scriptState =
ScriptState::from(toV8Context(&loader->element()->document(),
DOMWrapperWorld::mainWorld()));
there might be no context at all, so I need two cases

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
Source/core/loader/ImageLoader.cpp:120: v8::HandleScope
handleScope(m_scriptState->isolate());
On 2015/05/28 at 10:57:38, haraken wrote:
> This wouldn't be needed, since the ScriptState::Scope will create the handle
scope for you.

done

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
Source/core/loader/ImageLoader.cpp:124:
m_loader->doUpdateFromElement(m_shouldBypassMainWorldCSP, m_updateBehavior);
On 2015/05/28 at 10:57:38, haraken wrote:
> Do we really want to execute this when the context associated with this task
is already disposed?

yes, the loader needs to clean itself up.

[haraken, 2015-05-28 11:37:50]

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
Source/core/loader/ImageLoader.cpp:92: v8::HandleScope scope(isolate);

Nit: I think we should have the handle scope in hasCurrentScriptState.

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
Source/core/loader/ImageLoader.cpp:96: m_scriptState =
ScriptState::from(toV8Context(&loader->element()->document(),
DOMWrapperWorld::mainWorld()));
On 2015/05/28 11:16:49, jochen wrote:
> there might be no context at all, so I need two cases

OK, understood.

If JS creates a micro task, we should run the micro task in the context of the
JS.

If DOM creates a micro task, there can be no associated context at this point.
Since this can happen only in the main world, we should run the micro task in
the context associated with the document and the main world.

Maybe we can add a comment.

https://codereview.chromium.org/1163543002/diff/1/Source/core/loader/ImageLoa...
Source/core/loader/ImageLoader.cpp:124:
m_loader->doUpdateFromElement(m_shouldBypassMainWorldCSP, m_updateBehavior);
On 2015/05/28 11:16:49, jochen wrote:
> On 2015/05/28 at 10:57:38, haraken wrote:
> > Do we really want to execute this when the context associated with this task
> is already disposed?
> 
> yes, the loader needs to clean itself up.

In that scenario, wouldn't there be any risk of executing doUpdateFromElement in
an unexpected (or exploited) ScriptState?

Maybe we want to create a dummy ScriptState or set m_shouldBypassMainWorldCSP to
false?

[haraken, 2015-05-28 11:48:58]

LGTM

https://codereview.chromium.org/1163543002/diff/40001/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1163543002/diff/40001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:125:
m_loader->doUpdateFromElement(m_shouldBypassMainWorldCSP, m_updateBehavior);

It's not clear that this never reaches shouldBypassMainWorldCSP(). Shall we add
some comments or ASSERTs to verify that this code doesn't reach anything that
requires a context?

[jochen (gone - plz use gerrit), 2015-05-28 12:12:41]

On 2015/05/28 at 11:48:58, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/1163543002/diff/40001/Source/core/loader/Imag...
> File Source/core/loader/ImageLoader.cpp (right):
> 
>
https://codereview.chromium.org/1163543002/diff/40001/Source/core/loader/Imag...
> Source/core/loader/ImageLoader.cpp:125:
m_loader->doUpdateFromElement(m_shouldBypassMainWorldCSP, m_updateBehavior);
> 
> It's not clear that this never reaches shouldBypassMainWorldCSP(). Shall we
add some comments or ASSERTs to verify that this code doesn't reach anything
that requires a context?

what kind of assert do we have that does this?

[haraken, 2015-05-28 12:39:55]

On 2015/05/28 12:12:41, jochen wrote:
> On 2015/05/28 at 11:48:58, haraken wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/1163543002/diff/40001/Source/core/loader/Imag...
> > File Source/core/loader/ImageLoader.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1163543002/diff/40001/Source/core/loader/Imag...
> > Source/core/loader/ImageLoader.cpp:125:
> m_loader->doUpdateFromElement(m_shouldBypassMainWorldCSP, m_updateBehavior);
> > 
> > It's not clear that this never reaches shouldBypassMainWorldCSP(). Shall we
> add some comments or ASSERTs to verify that this code doesn't reach anything
> that requires a context?
> 
> what kind of assert do we have that does this?

Yeah, I have no idea.

If we have similar issues in other places, it would be worth creating a dummy
ScriptState (per Isolate) and use the dummy one when we're going to execute
something that may interact with JS. It would be better than executing the thing
with an unexpected ScriptState. (Another customer of the dummy ScriptState would
be V8GCController::collectGarbage().)

I think we can work on the issue in a follow up. Even if this CL is not perfect,
it definitely improves our situation.

[jochen (gone - plz use gerrit), 2015-05-28 12:40:36]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2015-05-28 12:40:54]

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

[commit-bot: I haz the power, 2015-05-28 13:42:34]

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

[commit-bot: I haz the power, 2015-05-28 13:42:35]

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

[jochen (gone - plz use gerrit), 2015-05-28 14:47:57]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-05-28 14:47:58]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1163543002/#ps60001
(title: "updates")

[commit-bot: I haz the power, 2015-05-28 14:48:05]

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

[commit-bot: I haz the power, 2015-05-28 16:08:15]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196071