Add 'error' parameter to 'window.onerror' handlers.

Add 'error' parameter to 'window.onerror' and 'ErrorEvent'.

The HTML5 spec recently added the error object to both the
'window.onerror' callback and the 'ErrorEvent' interface in order to
improve developers' ability to debug issues that cause unexpected
exceptions in the wild[1]. This patch brings Blink's behavior into line
with that update.

Currently, we generate an ErrorEvent object after doing sanitization.
This patch refactors that code such that we generate an ErrorEvent when
V8 calls back into the bindings (V8Initializer::messageHandlerInWorker
and V8Initializer::messageHandlerInMainThread). Before we hand that
event off to core for sanitization and dispatch, we grab the wrapper
for the world in which the exception was thrown, and set the exception
object as a hidden value (V8HiddenPropertyName::error()).

On the other end, we grab that exception object in the custom 'error'
getter of 'V8ErrorEvent', and when triggering the 'onerror' handler
via 'V8ErrorHandler::callListenerFunction'. If the value exists, we're
still in the same isolated world, so we can safely return the exception
without leakage. If the value is empty, we've crossed worlds, and
return 'null' instead.

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

[Mike West, 2013-07-25 16:52:23]

Hi Dan, Jochen, and Adam. I'd appreciate some feedback on this preliminary pass
at adding an 'error' parameter to 'window.onerror' by passing the exception
object through core. I'm not entirely sure that it's a) technically correct (I
don't understand why the generated ErrorEvent bindings require changes to
V8HiddenPropertyName, for instance), or b) advisable (are there interesting
properties hanging off the exception object that we should sanitize? Should we
copy the object rather than passing exactly that object around?).

I don't intend to land the patch like this, but if you could find some time to
take a quick look at the patch to let me know if the direction is reasonable,
I'd appreciate it.

Thanks!

-mike

[Mike West, 2013-07-25 16:53:30]

+marja, since she knows oh so many things about handles. :)

[abarth-chromium, 2013-07-25 18:24:08]

https://codereview.chromium.org/20351002/diff/1/Source/core/dom/ErrorEvent.h
File Source/core/dom/ErrorEvent.h (right):

https://codereview.chromium.org/20351002/diff/1/Source/core/dom/ErrorEvent.h#...
Source/core/dom/ErrorEvent.h:72: const ScriptValue& error() const { return
m_error; }
Doesn't this create a leak between isolated worlds?  We need to do something
like what we've done with CustomEvent here.

[Mike West, 2013-07-30 07:03:44]

On 2013/07/25 18:24:08, abarth wrote:
> https://codereview.chromium.org/20351002/diff/1/Source/core/dom/ErrorEvent.h
> File Source/core/dom/ErrorEvent.h (right):
> 
>
https://codereview.chromium.org/20351002/diff/1/Source/core/dom/ErrorEvent.h#...
> Source/core/dom/ErrorEvent.h:72: const ScriptValue& error() const { return
> m_error; }
> Doesn't this create a leak between isolated worlds?  We need to do something
> like what we've done with CustomEvent here.

You're right. We should do something here to prevent leakage across isolated
worlds. Error events, unfortunately, are not serializable. Passing them between
worlds safely is going to be a pain in the butt.

I was a bit surprised to see that `onerror` events are triggered across worlds
at all, honestly. It seems like exceptions thrown in one world should only
trigger listeners in that world. Do you happen to know if that was a conscious
decision? Skimming the comments at https://bugs.webkit.org/show_bug.cgi?id=8519
(https://bugs.webkit.org/show_bug.cgi?id=8519#c93 for example), it appeared that
there was agreement to treat onerror as a world-specific property. That seems
like a pretty reasonable thing to do.

[abarth-chromium, 2013-07-30 07:27:56]

On 2013/07/30 07:03:44, Mike West wrote:
> You're right. We should do something here to prevent leakage across isolated
> worlds. Error events, unfortunately, are not serializable. Passing them
between
> worlds safely is going to be a pain in the butt.

Maybe we should start out just exposing them in the main world.  You can store
them on the main world wrapper as in the CustomEvent case but skip the
serialization for now.

> I was a bit surprised to see that `onerror` events are triggered across worlds
> at all, honestly. It seems like exceptions thrown in one world should only
> trigger listeners in that world. Do you happen to know if that was a conscious
> decision? Skimming the comments at
https://bugs.webkit.org/show_bug.cgi?id=8519
> (https://bugs.webkit.org/show_bug.cgi?id=8519#c93 for example), it appeared
that
> there was agreement to treat onerror as a world-specific property. That seems
> like a pretty reasonable thing to do.

We can do that if you like.  I think we just followed the normal event model. 
Feel free to choose either pathn.

[Mike West, 2013-07-30 10:14:59]

Hi Jochen, Dan, and Adam,

I'd appreciate you taking a few moments to skim the newest patchset.

Given the way this event is created, I don't think it's possible (or at least
not at all straightforward) to store the exception object on the ErrorEvent
wrapper in the same way that CustomEvent does. The approach I'm running with
right now is to set a hidden "worldId" value on the exception object before
storing it on the ErrorEvent. I check the current DOMWrapperWorld's ID against
the stored ID before passing that object back out to JavaScript in
V8ErrorHandler and V8ErrorEventCustom.

This is a bit ugly, not least of which because the IDL parser forces me to add
the SerializedScriptValue pipework that I can't actually do anything with.

Assuming that the approach makes sense (does it?), I'd appreciate any
suggestions around cleaning this up. :)

+adamk, who I talked with briefly about this yesterday. Hi Adam!

[dcarney, 2013-07-30 10:48:55]

seems reasonable.

would be really nice to get rid of a lot of the unnecessary code around SSV and
HiddenProperty::error

https://chromiumcodereview.appspot.com/20351002/diff/9001/Source/bindings/v8/...
File Source/bindings/v8/V8HiddenPropertyName.h (right):

https://chromiumcodereview.appspot.com/20351002/diff/9001/Source/bindings/v8/...
Source/bindings/v8/V8HiddenPropertyName.h:55: V(error) \
would be nice to be able to get rid of this

https://chromiumcodereview.appspot.com/20351002/diff/9001/Source/bindings/v8/...
File Source/bindings/v8/custom/V8ErrorEventCustom.cpp (right):

https://chromiumcodereview.appspot.com/20351002/diff/9001/Source/bindings/v8/...
Source/bindings/v8/custom/V8ErrorEventCustom.cpp:49: {
this function can go

[Mike West, 2013-07-30 12:10:04]

Rebase.

[adamk, 2013-07-30 15:46:40]

I think this code would be simpler if we just relaxed our restrictions on where
one can use v8 types (see my second comment).

https://chromiumcodereview.appspot.com/20351002/diff/15001/Source/core/dom/Er...
File Source/core/dom/ErrorEvent.idl (right):

https://chromiumcodereview.appspot.com/20351002/diff/15001/Source/core/dom/Er...
Source/core/dom/ErrorEvent.idl:38: [InitializedByEventConstructor, Custom]
readonly attribute any error;
Maybe add some new extended attribute here to avoid that SerializedScriptValue
boilerplate (which was only very recently added to the code gen).

https://chromiumcodereview.appspot.com/20351002/diff/15001/Source/core/dom/Sc...
File Source/core/dom/ScriptExecutionContext.cpp (right):

https://chromiumcodereview.appspot.com/20351002/diff/15001/Source/core/dom/Sc...
Source/core/dom/ScriptExecutionContext.cpp:254: RefPtr<ErrorEvent> errorEvent =
ErrorEvent::create(message, sourceName, line, column, error);
It seems like the thing holding you back from just storing the error on the
wrapper is a layering concern about calling toV8() here in
ScriptExecutionContext.cpp. But the amount of code would be tiny, something
like:

toV8(errorEvent)->SetHiddenValue(V8HiddenPropertyName::error(), error.v8Value())

I'm convinced sure hiding the V8 stuff behind ScriptValue is providing any value
here. The worldID workaround just seems weird.

[Mike West, 2013-07-30 15:53:36]

https://chromiumcodereview.appspot.com/20351002/diff/15001/Source/core/dom/Er...
File Source/core/dom/ErrorEvent.idl (right):

https://chromiumcodereview.appspot.com/20351002/diff/15001/Source/core/dom/Er...
Source/core/dom/ErrorEvent.idl:38: [InitializedByEventConstructor, Custom]
readonly attribute any error;
On 2013/07/30 15:46:41, adamk wrote:
> Maybe add some new extended attribute here to avoid that SerializedScriptValue
> boilerplate (which was only very recently added to the code gen).

Makes sense. "Unserializable"?

https://chromiumcodereview.appspot.com/20351002/diff/15001/Source/core/dom/Sc...
File Source/core/dom/ScriptExecutionContext.cpp (right):

https://chromiumcodereview.appspot.com/20351002/diff/15001/Source/core/dom/Sc...
Source/core/dom/ScriptExecutionContext.cpp:254: RefPtr<ErrorEvent> errorEvent =
ErrorEvent::create(message, sourceName, line, column, error);
On 2013/07/30 15:46:41, adamk wrote:
> I'm convinced sure hiding the V8 stuff behind ScriptValue is providing any
value
> here. The worldID workaround just seems weird.

Yup. You're entirely correct. But there are only a handful of places in core
that use 'toV8', and it generally feels like a hack. This code shouldn't need to
know about worlds or wrappers. I'm not sure introducing those concepts is a good
idea.

[adamk, 2013-07-30 16:00:47]

https://chromiumcodereview.appspot.com/20351002/diff/15001/Source/core/dom/Sc...
File Source/core/dom/ScriptExecutionContext.cpp (right):

https://chromiumcodereview.appspot.com/20351002/diff/15001/Source/core/dom/Sc...
Source/core/dom/ScriptExecutionContext.cpp:254: RefPtr<ErrorEvent> errorEvent =
ErrorEvent::create(message, sourceName, line, column, error);
On 2013/07/30 15:53:37, Mike West wrote:
> On 2013/07/30 15:46:41, adamk wrote:
> > I'm convinced sure hiding the V8 stuff behind ScriptValue is providing any
> value
> > here. The worldID workaround just seems weird.
> 
> Yup. You're entirely correct. But there are only a handful of places in core
> that use 'toV8', and it generally feels like a hack. This code shouldn't need
to
> know about worlds or wrappers. I'm not sure introducing those concepts is a
good
> idea.

No need to teach it about worlds (though note that DOMWrapperWorld is already
used in core), but hiding the existence of wrappers seems hardly worth it to me.
I guess this is just a difference of opinion about the aesthetics (as long as
your version doesn't leak memory, that is; where do you break the reference
cycle from script -> ErrorEvent -> script?).

[Mike West, 2013-07-30 17:47:16]

I've reworked the patch after talking with Adam and Dan: now the ErrorEvent is
created in the Message callback in V8Initializer, wrapped there, and sent on its
merry way.

This feels much cleaner.

WDYT?

-mike

[adamk, 2013-07-30 17:55:41]

Approach looks good, happy to see the ownership of the V8 object disconnected
from C++.

https://codereview.chromium.org/20351002/diff/24001/Source/core/dom/ErrorEvent.h
File Source/core/dom/ErrorEvent.h (right):

https://codereview.chromium.org/20351002/diff/24001/Source/core/dom/ErrorEven...
Source/core/dom/ErrorEvent.h:49: ScriptValue error;
Is this doing anything other than satisfying the code generator?

[dcarney, 2013-07-30 18:51:58]

much cleaner now.  I added some nits in case you're done reworking this and
ready to commit

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Erro...
File Source/bindings/v8/V8ErrorHandler.cpp (right):

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Erro...
Source/bindings/v8/V8ErrorHandler.cpp:64: error =
v8::Local<v8::Value>(v8::Undefined(isolate));
nit: shouldn't need the extra Local wrapper

https://codereview.chromium.org/20351002/diff/28001/Source/core/dom/ErrorEven...
File Source/core/dom/ErrorEvent.cpp (right):

https://codereview.chromium.org/20351002/diff/28001/Source/core/dom/ErrorEven...
Source/core/dom/ErrorEvent.cpp:34: #include "bindings/v8/ScriptValue.h"
nit: not needed

https://codereview.chromium.org/20351002/diff/28001/Source/core/dom/ErrorEvent.h
File Source/core/dom/ErrorEvent.h (right):

https://codereview.chromium.org/20351002/diff/28001/Source/core/dom/ErrorEven...
Source/core/dom/ErrorEvent.h:86: 
nit: space

[Mike West, 2013-07-30 19:48:12]

Thanks Dan and Adam. I do think this is a pretty good approach, and would like
to run with it. I'd appreciate a "real" review. No rush, though, as I'm out of
office tomorrow. :)

Thanks!

-mike

[adamk, 2013-07-31 16:37:30]

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Init...
File Source/bindings/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Init...
Source/bindings/v8/V8Initializer.cpp:102: if (!wrappedEvent.IsEmpty() &&
wrappedEvent->IsObject())
wrappedEvent->IsObject() can just be an ASSERT, I think.

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Init...
Source/bindings/v8/V8Initializer.cpp:103:
wrappedEvent->ToObject()->SetHiddenValue(V8HiddenPropertyName::error(), data);
Rather than ToObject, I think this should be v8::Local<v8::Object>::Cast(...);
ToObject() is slightly heavier-weight.

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Init...
Source/bindings/v8/V8Initializer.cpp:175: if (!wrappedEvent.IsEmpty() &&
wrappedEvent->IsObject())
Same here as above

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Init...
Source/bindings/v8/V8Initializer.cpp:176:
wrappedEvent->ToObject()->SetHiddenValue(V8HiddenPropertyName::error(), data);
and here too

https://codereview.chromium.org/20351002/diff/28001/Source/core/dom/ErrorEvent.h
File Source/core/dom/ErrorEvent.h (right):

https://codereview.chromium.org/20351002/diff/28001/Source/core/dom/ErrorEven...
Source/core/dom/ErrorEvent.h:49: ScriptValue error;
I asked before but I don't think I got an answer: what is this used for?

https://codereview.chromium.org/20351002/diff/28001/Source/core/dom/ErrorEven...
Source/core/dom/ErrorEvent.h:77: void
setSerializedError(PassRefPtr<SerializedScriptValue>) { }
I thought you were going to add something to the code generator to make this
unnecessary?

[Mike West, 2013-08-01 07:52:15]

PTAL.

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Erro...
File Source/bindings/v8/V8ErrorHandler.cpp (right):

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Erro...
Source/bindings/v8/V8ErrorHandler.cpp:64: error =
v8::Local<v8::Value>(v8::Undefined(isolate));
On 2013/07/30 18:51:58, dcarney wrote:
> nit: shouldn't need the extra Local wrapper

Done.

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Init...
File Source/bindings/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Init...
Source/bindings/v8/V8Initializer.cpp:102: if (!wrappedEvent.IsEmpty() &&
wrappedEvent->IsObject())
On 2013/07/31 16:37:30, adamk wrote:
> wrappedEvent->IsObject() can just be an ASSERT, I think.

Done.

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Init...
Source/bindings/v8/V8Initializer.cpp:103:
wrappedEvent->ToObject()->SetHiddenValue(V8HiddenPropertyName::error(), data);
On 2013/07/31 16:37:30, adamk wrote:
> Rather than ToObject, I think this should be v8::Local<v8::Object>::Cast(...);
> ToObject() is slightly heavier-weight.

Done.

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Init...
Source/bindings/v8/V8Initializer.cpp:175: if (!wrappedEvent.IsEmpty() &&
wrappedEvent->IsObject())
On 2013/07/31 16:37:30, adamk wrote:
> Same here as above

Done.

https://codereview.chromium.org/20351002/diff/28001/Source/bindings/v8/V8Init...
Source/bindings/v8/V8Initializer.cpp:176:
wrappedEvent->ToObject()->SetHiddenValue(V8HiddenPropertyName::error(), data);
On 2013/07/31 16:37:30, adamk wrote:
> and here too

Done.

https://codereview.chromium.org/20351002/diff/28001/Source/core/dom/ErrorEvent.h
File Source/core/dom/ErrorEvent.h (right):

https://codereview.chromium.org/20351002/diff/28001/Source/core/dom/ErrorEven...
Source/core/dom/ErrorEvent.h:49: ScriptValue error;
On 2013/07/31 16:37:30, adamk wrote:
> I asked before but I don't think I got an answer: what is this used for?

Nothing! Killed it.

https://codereview.chromium.org/20351002/diff/28001/Source/core/dom/ErrorEven...
Source/core/dom/ErrorEvent.h:77: void
setSerializedError(PassRefPtr<SerializedScriptValue>) { }
On 2013/07/31 16:37:30, adamk wrote:
> I thought you were going to add something to the code generator to make this
> unnecessary?

I was! And I have!

[dcarney, 2013-08-01 08:05:21]

lgtm

https://codereview.chromium.org/20351002/diff/37001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8ErrorEventCustom.cpp (right):

https://codereview.chromium.org/20351002/diff/37001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8ErrorEventCustom.cpp:57: v8SetReturnValue(info,
v8::Null(info.GetIsolate()));
v8SetReturnValueNull

[adamk, 2013-08-01 15:44:02]

lgtm, but see below; it's not clear to me what the correct behavior is when
constructing an ErrorEvent from JS.

https://codereview.chromium.org/20351002/diff/37001/Source/core/dom/ErrorEven...
File Source/core/dom/ErrorEvent.idl (right):

https://codereview.chromium.org/20351002/diff/37001/Source/core/dom/ErrorEven...
Source/core/dom/ErrorEvent.idl:38: [InitializedByEventConstructor, Custom,
Unserializable] readonly attribute any error;
So is it possible to actually initialize this from JS by calling "new
ErrorEvent(..., { error: something })"? That's what the
[InitializedByEventConstructor] attribute is supposed to be about.

[Mike West, 2013-08-01 19:24:37]

On 2013/08/01 15:44:02, adamk wrote:
> lgtm, but see below; it's not clear to me what the correct behavior is when
> constructing an ErrorEvent from JS.

I'll add a test case before landing, but I believe the code would do the
reasonable thing at the moment. The constructor would set the error parameter as
a hidden field on the newly created object, and the getter code would perform
the same world I validation it does for the Blink-fired case.


> 
>
https://codereview.chromium.org/20351002/diff/37001/Source/core/dom/ErrorEven...
> File Source/core/dom/ErrorEvent.idl (right):
> 
>
https://codereview.chromium.org/20351002/diff/37001/Source/core/dom/ErrorEven...
> Source/core/dom/ErrorEvent.idl:38: [InitializedByEventConstructor, Custom,
> Unserializable] readonly attribute any error;
> So is it possible to actually initialize this from JS by calling "new
> ErrorEvent(..., { error: something })"? That's what the
> [InitializedByEventConstructor] attribute is supposed to be about.

[commit-bot: I haz the power, 2013-08-02 10:19:58]

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

[commit-bot: I haz the power, 2013-08-02 15:18:04]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[commit-bot: I haz the power, 2013-08-02 19:43:38]

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

[commit-bot: I haz the power, 2013-08-02 21:51:17]

Change committed as 155454