Propagate nested importScripts() error events outwards.

Propagate nested importScripts() error events outwards.

If importScripts() evaluation fails, we report the exception wrt the
URL and location of the failing script. Extend that to handle the
nested case, propagating any inner importScripts-induced error event
outwards rather than construct a new one wrt the outer script URL.

R=haraken@chromium.org
BUG=394615
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178432

[sof, 2014-07-17 13:52:00]

Please take a look.

[haraken, 2014-07-17 15:19:06]

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

https://codereview.chromium.org/404513002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:206:
m_errorEventFromImportedScript.clear();

Is it possible that m_errorEventFromImportedScript is not cleared at this point?
Maybe can we add ASSERT(!m_errorEventFromImportedScript)?

[sof, 2014-07-17 15:22:18]

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

https://codereview.chromium.org/404513002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:206:
m_errorEventFromImportedScript.clear();
On 2014/07/17 15:19:05, haraken wrote:
> 
> Is it possible that m_errorEventFromImportedScript is not cleared at this
point?
> Maybe can we add ASSERT(!m_errorEventFromImportedScript)?

It won't be cleared whenever a previous (not nested) importScripts() calls
rethrowExceptionFromImportedScript().

[haraken, 2014-07-17 15:47:24]

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

https://codereview.chromium.org/404513002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:206:
m_errorEventFromImportedScript.clear();
On 2014/07/17 15:22:17, sof wrote:
> On 2014/07/17 15:19:05, haraken wrote:
> > 
> > Is it possible that m_errorEventFromImportedScript is not cleared at this
> point?
> > Maybe can we add ASSERT(!m_errorEventFromImportedScript)?
> 
> It won't be cleared whenever a previous (not nested) importScripts() calls
> rethrowExceptionFromImportedScript().

Then does it mean that a persistent handle to the error event is kept alive
until the next script is evaluated by the WorkerScriptController? If yes, that
wouldn't be desirable.

[sof, 2014-07-17 15:53:19]

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

https://codereview.chromium.org/404513002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:206:
m_errorEventFromImportedScript.clear();
On 2014/07/17 15:47:24, haraken wrote:
> On 2014/07/17 15:22:17, sof wrote:
> > On 2014/07/17 15:19:05, haraken wrote:
> > > 
> > > Is it possible that m_errorEventFromImportedScript is not cleared at this
> > point?
> > > Maybe can we add ASSERT(!m_errorEventFromImportedScript)?
> > 
> > It won't be cleared whenever a previous (not nested) importScripts() calls
> > rethrowExceptionFromImportedScript().
> 
> Then does it mean that a persistent handle to the error event is kept alive
> until the next script is evaluated by the WorkerScriptController? If yes, that
> wouldn't be desirable.

Yes, or via a RefPtr non-Oilpan if that exception ends up being caught.

[haraken, 2014-07-17 15:58:54]

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

https://codereview.chromium.org/404513002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:206:
m_errorEventFromImportedScript.clear();
On 2014/07/17 15:53:19, sof wrote:
> On 2014/07/17 15:47:24, haraken wrote:
> > On 2014/07/17 15:22:17, sof wrote:
> > > On 2014/07/17 15:19:05, haraken wrote:
> > > > 
> > > > Is it possible that m_errorEventFromImportedScript is not cleared at
this
> > > point?
> > > > Maybe can we add ASSERT(!m_errorEventFromImportedScript)?
> > > 
> > > It won't be cleared whenever a previous (not nested) importScripts() calls
> > > rethrowExceptionFromImportedScript().
> > 
> > Then does it mean that a persistent handle to the error event is kept alive
> > until the next script is evaluated by the WorkerScriptController? If yes,
that
> > wouldn't be desirable.
> 
> Yes, or via a RefPtr non-Oilpan if that exception ends up being caught.

I'm a bit afraid that it will cause leaks. Why can't we clear
m_errorEventFromImportedScript at the end of WorkerScriptController::evaluate()?
I wonder why we need to keep it alive after WorkerScriptController::evaluate()
finishes.

(Probably I'm not understanding how things are working.)

[sof, 2014-07-17 18:13:11]

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

https://codereview.chromium.org/404513002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:206:
m_errorEventFromImportedScript.clear();
On 2014/07/17 15:58:54, haraken wrote:
> On 2014/07/17 15:53:19, sof wrote:
> > On 2014/07/17 15:47:24, haraken wrote:
> > > On 2014/07/17 15:22:17, sof wrote:
> > > > On 2014/07/17 15:19:05, haraken wrote:
> > > > > 
> > > > > Is it possible that m_errorEventFromImportedScript is not cleared at
> this
> > > > point?
> > > > > Maybe can we add ASSERT(!m_errorEventFromImportedScript)?
> > > > 
> > > > It won't be cleared whenever a previous (not nested) importScripts()
calls
> > > > rethrowExceptionFromImportedScript().
> > > 
> > > Then does it mean that a persistent handle to the error event is kept
alive
> > > until the next script is evaluated by the WorkerScriptController? If yes,
> that
> > > wouldn't be desirable.
> > 
> > Yes, or via a RefPtr non-Oilpan if that exception ends up being caught.
> 
> I'm a bit afraid that it will cause leaks. Why can't we clear
> m_errorEventFromImportedScript at the end of
WorkerScriptController::evaluate()?
> I wonder why we need to keep it alive after WorkerScriptController::evaluate()
> finishes.
> 
> (Probably I'm not understanding how things are working.)

I'll have another look if we can tidy this up here, but this CL actually
improves the situation ErrorEvent leak-wise.

It's brought about by the need to sanitize uncaught errors when reporting them
out of the global scope and onto the worker object.

[sof, 2014-07-17 22:05:31]

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

https://codereview.chromium.org/404513002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:206:
m_errorEventFromImportedScript.clear();
On 2014/07/17 18:13:11, sof wrote:
> On 2014/07/17 15:58:54, haraken wrote:
> > On 2014/07/17 15:53:19, sof wrote:
> > > On 2014/07/17 15:47:24, haraken wrote:
> > > > On 2014/07/17 15:22:17, sof wrote:
> > > > > On 2014/07/17 15:19:05, haraken wrote:
> > > > > > 
> > > > > > Is it possible that m_errorEventFromImportedScript is not cleared at
> > this
> > > > > point?
> > > > > > Maybe can we add ASSERT(!m_errorEventFromImportedScript)?
> > > > > 
> > > > > It won't be cleared whenever a previous (not nested) importScripts()
> calls
> > > > > rethrowExceptionFromImportedScript().
> > > > 
> > > > Then does it mean that a persistent handle to the error event is kept
> alive
> > > > until the next script is evaluated by the WorkerScriptController? If
yes,
> > that
> > > > wouldn't be desirable.
> > > 
> > > Yes, or via a RefPtr non-Oilpan if that exception ends up being caught.
> > 
> > I'm a bit afraid that it will cause leaks. Why can't we clear
> > m_errorEventFromImportedScript at the end of
> WorkerScriptController::evaluate()?
> > I wonder why we need to keep it alive after
WorkerScriptController::evaluate()
> > finishes.
> > 
> > (Probably I'm not understanding how things are working.)
> 
> I'll have another look if we can tidy this up here, but this CL actually
> improves the situation ErrorEvent leak-wise.
> 
> It's brought about by the need to sanitize uncaught errors when reporting them
> out of the global scope and onto the worker object.

Reworked a bit to let these ErrorEvents be stack allocated instead.

The ExecutionState stack objects are now chained to handle nested invocation --
please take a look.

[haraken, 2014-07-18 01:12:38]

Mostly looks good.

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:62: DISALLOW_ALLOCATION();

Can we use STACK_ALLOCATED()?

I agree that this object is stack-allocated, but another way would be using
NoBaseWillBeGarbageCollected. Then we can make
WorkerGlobalScopeExecutionState::m_outerState a Member and
WorkerScriptController::m_globalScopeExecutionState a Persistent, which will
make the lifetime relationship clearer.

I don't have a strong opinion on this though.

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:93:
WorkerGlobalScopeExecutionState* m_outerState;

Shall we add a comment about why we don't need to trace this?

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WorkerScriptController.h (right):

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.h:103:
WorkerGlobalScopeExecutionState* m_globalScopeExecutionState;

Shall we add a comment about why we don't need to trace this?

[sof, 2014-07-18 07:00:47]

Thanks for having a second careful look also; comments below and in the updated
patchset.

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:62: DISALLOW_ALLOCATION();
On 2014/07/18 01:12:38, haraken wrote:
> 
> Can we use STACK_ALLOCATED()?
> 
> I agree that this object is stack-allocated, but another way would be using
> NoBaseWillBeGarbageCollected. Then we can make
> WorkerGlobalScopeExecutionState::m_outerState a Member and
> WorkerScriptController::m_globalScopeExecutionState a Persistent, which will
> make the lifetime relationship clearer.
> 
> I don't have a strong opinion on this though.

The use of STACK_ALLOCATED() makes the GC plugin complain that the off-heap
WorkerScriptController has a field with a stack allocation. I don't see the
problem with that, at least not here.

Moving WorkerGlobalScopeExecutionState onto the heap (along with keeping a
Persistent) is how to make the GC plugin content, as you suggest, but that
strikes me as a workaround. These objects have a clear stack lifetime and the
stack is there and will be scanned should a GC happen. Are we leaving ourselves
open to vulnerabilities by not doing so?

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:93:
WorkerGlobalScopeExecutionState* m_outerState;
On 2014/07/18 01:12:38, haraken wrote:
> 
> Shall we add a comment about why we don't need to trace this?

Yes, these two deserve some explanation :)

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WorkerScriptController.h (right):

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.h:103:
WorkerGlobalScopeExecutionState* m_globalScopeExecutionState;
On 2014/07/18 01:12:38, haraken wrote:
> 
> Shall we add a comment about why we don't need to trace this?

Good idea; tried to explain.

[haraken, 2014-07-18 07:15:34]

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:62: DISALLOW_ALLOCATION();
On 2014/07/18 07:00:46, sof wrote:
> On 2014/07/18 01:12:38, haraken wrote:
> > 
> > Can we use STACK_ALLOCATED()?
> > 
> > I agree that this object is stack-allocated, but another way would be using
> > NoBaseWillBeGarbageCollected. Then we can make
> > WorkerGlobalScopeExecutionState::m_outerState a Member and
> > WorkerScriptController::m_globalScopeExecutionState a Persistent, which will
> > make the lifetime relationship clearer.
> > 
> > I don't have a strong opinion on this though.
> 
> The use of STACK_ALLOCATED() makes the GC plugin complain that the off-heap
> WorkerScriptController has a field with a stack allocation. I don't see the
> problem with that, at least not here.
> 
> Moving WorkerGlobalScopeExecutionState onto the heap (along with keeping a
> Persistent) is how to make the GC plugin content, as you suggest, but that
> strikes me as a workaround. These objects have a clear stack lifetime and the
> stack is there and will be scanned should a GC happen. Are we leaving
ourselves
> open to vulnerabilities by not doing so?

Makes sense.

I don't think it's a bug of the plugin. The plugin is just complaining that the
WorkerScriptController has a raw pointer to a stack-allocated object. This is
not safe in general, so the complaint makes sense.

Probably a right fix would be:

- Make WorkerGlobalScopeExecutionState STACK_ALLOCATED().

- Add IGNORE_GC_PLUGIN() to the raw pointers with a comment.

What do you think?

[sof, 2014-07-18 07:36:17]

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/404513002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:62: DISALLOW_ALLOCATION();
On 2014/07/18 07:15:34, haraken wrote:
> On 2014/07/18 07:00:46, sof wrote:
> > On 2014/07/18 01:12:38, haraken wrote:
> > > 
> > > Can we use STACK_ALLOCATED()?
> > > 
> > > I agree that this object is stack-allocated, but another way would be
using
> > > NoBaseWillBeGarbageCollected. Then we can make
> > > WorkerGlobalScopeExecutionState::m_outerState a Member and
> > > WorkerScriptController::m_globalScopeExecutionState a Persistent, which
will
> > > make the lifetime relationship clearer.
> > > 
> > > I don't have a strong opinion on this though.
> > 
> > The use of STACK_ALLOCATED() makes the GC plugin complain that the off-heap
> > WorkerScriptController has a field with a stack allocation. I don't see the
> > problem with that, at least not here.
> > 
> > Moving WorkerGlobalScopeExecutionState onto the heap (along with keeping a
> > Persistent) is how to make the GC plugin content, as you suggest, but that
> > strikes me as a workaround. These objects have a clear stack lifetime and
the
> > stack is there and will be scanned should a GC happen. Are we leaving
> ourselves
> > open to vulnerabilities by not doing so?
> 
> Makes sense.
> 
> I don't think it's a bug of the plugin. The plugin is just complaining that
the
> WorkerScriptController has a raw pointer to a stack-allocated object. This is
> not safe in general, so the complaint makes sense.
> 
> Probably a right fix would be:
> 
> - Make WorkerGlobalScopeExecutionState STACK_ALLOCATED().
> 
> - Add IGNORE_GC_PLUGIN() to the raw pointers with a comment.
> 
> What do you think?
> 

I think that is an improvement, the natural annotation for it is
STACK_ALLOCATED(). Added now.

[haraken, 2014-07-18 07:40:15]

LGTM.

[sof, 2014-07-18 07:55:15]

Thanks for the review. We could consider moving WorkerScriptController to the
heap at some point.

[sof, 2014-07-18 07:55:30]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-18 07:56:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/404513002/100001

[commit-bot: I haz the power, 2014-07-18 09:01:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17658)

[commit-bot: I haz the power, 2014-07-18 10:04:53]

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

[commit-bot: I haz the power, 2014-07-18 10:04:54]

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

[sof, 2014-07-18 10:06:29]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-18 10:07:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/404513002/100001

[sof, 2014-07-18 10:43:21]

The CQ bit was unchecked by sigbjornf@opera.com

[sof, 2014-07-18 10:50:53]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-18 10:51:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/404513002/100001

[commit-bot: I haz the power, 2014-07-18 11:13:49]

Change committed as 178432