Add 'error' property to exceptions thrown via Worker::importScripts.

Add 'error' property to exceptions thrown via Worker::importScripts.

If an imported script throws an exception, and the script is same-origin
with the Worker, we should provide the exception object when handling
the exception via 'self.onerror'. The exception should not, however, be
available to the page-level 'window.onerror' (we'd be leaking state
across worlds otherwise (also, the spec says not to[1])).

This patch pulls the logic governing the exception object's storage
(wrapping the ErrorEvent, storing the value on a hidden property) out of
V8Initializer, and moves it to a static method on V8ErrorHandler so that
it's easily reusable inside WorkerScriptController.

[1]: http://www.whatwg.org/specs/web-apps/current-work/multipage/workers.html#runtime-script-errors-0

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

[Mike West, 2013-08-09 12:01:20]

Hi Christophe! Would you mind taking a first pass at this CL?

I'd also like Adam (hi Adam!) to take a look at the hidden property stuff, just
to sanity check the move (and because haraken@ made some changes yesterday that
I'd appreciate his opinion on).

Thanks!

-mike

[adamk, 2013-08-09 17:11:32]

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8ErrorHan...
File Source/bindings/v8/V8ErrorHandler.cpp (right):

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8ErrorHan...
Source/bindings/v8/V8ErrorHandler.cpp:84: // This method might be called while
we're creating a new context. Since we
Why move this code? It seems like it would work fine to leave it in
messageHandlerInMainThread, which would allow this comment to continue making
sense. Right now, the only time it could be called at a "bad" time is by that
function anyway. Basically, you could just guard the
storeExceptionOnErrorEventWrapper() call with the existingWindowShell() check.

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8Initiali...
File Source/bindings/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8Initiali...
Source/bindings/v8/V8Initializer.cpp:52: #include <v8-debug.h>
This #include seems wrong...

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/WorkerScri...
File Source/bindings/v8/WorkerScriptController.cpp (left):

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/WorkerScri...
Source/bindings/v8/WorkerScriptController.cpp:194:
m_errorEventFromImportedScript.clear();
Looks like this line was a no-op because of the .release() call above, correct?

[Mike West, 2013-08-09 20:34:51]

Thanks for taking a look!

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8ErrorHan...
File Source/bindings/v8/V8ErrorHandler.cpp (right):

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8ErrorHan...
Source/bindings/v8/V8ErrorHandler.cpp:84: // This method might be called while
we're creating a new context. Since we
On 2013/08/09 17:11:32, adamk wrote:
> Why move this code? It seems like it would work fine to leave it in
> messageHandlerInMainThread, which would allow this comment to continue making
> sense. Right now, the only time it could be called at a "bad" time is by that
> function anyway. Basically, you could just guard the
> storeExceptionOnErrorEventWrapper() call with the existingWindowShell() check.

It seemed better to have one spot where the wrapping/hidden value work happened,
rather than copy/pasting a few lines a few times. If you feel like that's
overcomplicating things, I can go back to copy/paste. :)

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8Initiali...
File Source/bindings/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8Initiali...
Source/bindings/v8/V8Initializer.cpp:52: #include <v8-debug.h>
On 2013/08/09 17:11:32, adamk wrote:
> This #include seems wrong...

It's already included, this CL just moves it to stop the stylebot from
complaining.

That said, yes. It seems wrong.

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/WorkerScri...
File Source/bindings/v8/WorkerScriptController.cpp (left):

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/WorkerScri...
Source/bindings/v8/WorkerScriptController.cpp:194:
m_errorEventFromImportedScript.clear();
On 2013/08/09 17:11:32, adamk wrote:
> Looks like this line was a no-op because of the .release() call above,
correct?

Correct. I inadvertently left it in a previous CL.

[adamk, 2013-08-09 21:29:24]

I assume there's already a test that non-same-origin importScripts don't get to
share errors?

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8ErrorHan...
File Source/bindings/v8/V8ErrorHandler.cpp (right):

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8ErrorHan...
Source/bindings/v8/V8ErrorHandler.cpp:84: // This method might be called while
we're creating a new context. Since we
On 2013/08/09 20:34:51, Mike West wrote:
> On 2013/08/09 17:11:32, adamk wrote:
> > Why move this code? It seems like it would work fine to leave it in
> > messageHandlerInMainThread, which would allow this comment to continue
making
> > sense. Right now, the only time it could be called at a "bad" time is by
that
> > function anyway. Basically, you could just guard the
> > storeExceptionOnErrorEventWrapper() call with the existingWindowShell()
check.
> 
> It seemed better to have one spot where the wrapping/hidden value work
happened,
> rather than copy/pasting a few lines a few times. If you feel like that's
> overcomplicating things, I can go back to copy/paste. :)

I'm confused: I'm suggesting sharing the wrapping/hidden value work, but moving
this bail-out back to the original caller. That is, keep the last 5 lines of
this function, but move the first 8 lines back to where they were before.

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8Initiali...
File Source/bindings/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8Initiali...
Source/bindings/v8/V8Initializer.cpp:52: #include <v8-debug.h>
On 2013/08/09 20:34:51, Mike West wrote:
> On 2013/08/09 17:11:32, adamk wrote:
> > This #include seems wrong...
> 
> It's already included, this CL just moves it to stop the stylebot from
> complaining.
> 
> That said, yes. It seems wrong.

Ah, sorry, didn't see this was just a move.

[Mike West, 2013-08-09 21:38:45]

Regarding tests:
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/workers/wor...
and
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/workers/wor...
test the cross-origin behavior. They're both asserting that 'error' is null. The
other bit inline.

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8ErrorHan...
File Source/bindings/v8/V8ErrorHandler.cpp (right):

https://codereview.chromium.org/22650008/diff/1/Source/bindings/v8/V8ErrorHan...
Source/bindings/v8/V8ErrorHandler.cpp:84: // This method might be called while
we're creating a new context. Since we
On 2013/08/09 21:29:25, adamk wrote:
> On 2013/08/09 20:34:51, Mike West wrote:
> > On 2013/08/09 17:11:32, adamk wrote:
> > > Why move this code? It seems like it would work fine to leave it in
> > > messageHandlerInMainThread, which would allow this comment to continue
> making
> > > sense. Right now, the only time it could be called at a "bad" time is by
> that
> > > function anyway. Basically, you could just guard the
> > > storeExceptionOnErrorEventWrapper() call with the existingWindowShell()
> check.
> > 
> > It seemed better to have one spot where the wrapping/hidden value work
> happened,
> > rather than copy/pasting a few lines a few times. If you feel like that's
> > overcomplicating things, I can go back to copy/paste. :)
> 
> I'm confused: I'm suggesting sharing the wrapping/hidden value work, but
moving
> this bail-out back to the original caller. That is, keep the last 5 lines of
> this function, but move the first 8 lines back to where they were before.

Ah, sorry. I thought you were asking "Why move all this code", not "Why move
this bailout."

That makes sense, I'll rework it on Monday.

[Mike West, 2013-08-12 08:42:54]

On 2013/08/09 21:38:45, Mike West wrote:

> That makes sense, I'll rework it on Monday.

I've updated the patch. Would you mind taking another look?

Thanks!

-mike

[adamk, 2013-08-12 17:12:00]

lgtm

[do-not-use, 2013-08-12 17:14:36]

lgtm

[Mike West, 2013-08-12 18:57:26]

On 2013/08/12 17:14:36, Christophe Dumez wrote:
> lgtm

Thank you both.

[commit-bot: I haz the power, 2013-08-12 18:57:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/22650008/11001

[commit-bot: I haz the power, 2013-08-12 21:27:21]

Change committed as 155953