Trigger `window.onerror` only for exceptions thrown in the same world.

Trigger `window.onerror` only for exceptions thrown in the same world.

This patch addresses half of crbug.com/225513 by grabbing the currently
active DOMWrapperWorld when creating ErrorEvent objects, and checking
that we're still in the same world when triggering `window.onerror`
handlers.

The next step will be to adjust event dispatching to ensure that 'error'
event listeners only trigger in the same world as the exception. I'll address
that in a separate patch; this patch adds several FAILing test results to make
that gap clear.

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

[Mike West, 2013-09-05 08:32:21]

Rebase.

[Mike West, 2013-09-05 09:11:33]

Hi Adam, would you mind taking a look at this patch? It addresses only
`window.onerror`. I'd like to split out event dispatch (that is,
`window.addEventListener('error', ...)` to a separate patch just to keep things
clear and simple.

WDYT?

-mike

[adamk, 2013-09-05 19:24:19]

https://codereview.chromium.org/21071003/diff/7001/Source/bindings/v8/WorkerS...
File Source/bindings/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/21071003/diff/7001/Source/bindings/v8/WorkerS...
Source/bindings/v8/WorkerScriptController.cpp:192:
ErrorEvent::createSanitizedError() : ErrorEvent::create(state.errorMessage,
state.sourceURL, state.lineNumber, state.columnNumber, 0);
The magic-ness of "workers don't have worlds" is beginning to bug me a bit, but
I don't know what the right solution is...

https://codereview.chromium.org/21071003/diff/7001/Source/core/dom/ErrorEvent...
File Source/core/dom/ErrorEvent.cpp (right):

https://codereview.chromium.org/21071003/diff/7001/Source/core/dom/ErrorEvent...
Source/core/dom/ErrorEvent.cpp:57: , m_world(0)
Why 0?

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

https://codereview.chromium.org/21071003/diff/7001/Source/core/dom/ErrorEvent...
Source/core/dom/ErrorEvent.h:64: static PassRefPtr<ErrorEvent>
createSanitizedError()
What is this used for? Why is passing 0 for world always correct?

https://codereview.chromium.org/21071003/diff/7001/Source/core/dom/ErrorEvent...
Source/core/dom/ErrorEvent.h:96: DOMWrapperWorld* m_world;
Seems a little bit scary to skip a RefPtr here. As you're currently using it,
only the object identity is important, but is there any downside to holding a
Ref?

[Mike West, 2013-09-06 11:57:03]

Thanks for taking a look, Adam. Would you mind taking another pass?

-mike

https://codereview.chromium.org/21071003/diff/7001/Source/core/dom/ErrorEvent...
File Source/core/dom/ErrorEvent.cpp (right):

https://codereview.chromium.org/21071003/diff/7001/Source/core/dom/ErrorEvent...
Source/core/dom/ErrorEvent.cpp:57: , m_world(0)
On 2013/09/05 19:24:20, adamk wrote:
> Why 0?

Laziness. I've changed this to pull in the current DOMWrapperWorld. I'd prefer
for that to be done in the generated bindings code, but I don't think it's worth
adding a custom constructor to V8ErrorEvent to make that happen. Pulling
'DOMWrapperWorld::current()' here is the lesser of those two evils.

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

https://codereview.chromium.org/21071003/diff/7001/Source/core/dom/ErrorEvent...
Source/core/dom/ErrorEvent.h:64: static PassRefPtr<ErrorEvent>
createSanitizedError()
On 2013/09/05 19:24:20, adamk wrote:
> What is this used for? Why is passing 0 for world always correct?

This is used for cross-origin onerror messages: they are sanitized to prevent
information leakage about the state of cross-origin script.

I don't think there's any case where we'd sanitize an extension's script, since
it's injected into the page directly. That said, you're probably right to say
that we should covering the edge-case explicitly by adding a world parameter.
I'll do that.

https://codereview.chromium.org/21071003/diff/7001/Source/core/dom/ErrorEvent...
Source/core/dom/ErrorEvent.h:96: DOMWrapperWorld* m_world;
On 2013/09/05 19:24:20, adamk wrote:
> Seems a little bit scary to skip a RefPtr here. As you're currently using it,
> only the object identity is important, but is there any downside to holding a
> Ref?

No good reason, as long as there aren't any side-effects I can't think of to
holding a reference. I'll change the implementation.

[adamk, 2013-09-06 16:13:04]

lgtm

Thanks for the quick bugfix!

[commit-bot: I haz the power, 2013-09-06 16:16:58]

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

[commit-bot: I haz the power, 2013-09-06 17:37:37]

Change committed as 157376