Throw 'SecurityError' upon cross-origin Location access.

Throw 'SecurityError' upon cross-origin Location access.

As suggested in the HTML specification[1], IE and Firefox (and old
Opera) throw a 'SecurityError' exception upon attempts to access to
Location object properties cross-origin. Chrome and WebKit do not,
instead returning 'undefined' and writing an error to the console.

This has a few negative effects: developers are forced to hack around
access violations in two ways rather than having a single code path, and
(more annoyingly) developers are unable to avoid generating the error
message. See every ad on the internet for the effect on the console. :)

This patch aligns Blink's behavior to IE and Firefox's by adding
'SecurityError' exception to these access violations, which is the first
step towards getting rid of the console spam. A subsequent patch will
adjust the exception's message to contain some sanitized version of the
current access violation detail message, and drop the unavoidable console
messages. There will be much rejoicing.

[1]: http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#security-location

R=abarth@chromium.org
BUG=17325
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154376

[Mike West, 2013-07-12 09:35:14]

This patch depends on V8 landing something like requires something like
https://codereview.chromium.org/19053002/ and rolling it into Chromium. I'll
fail in interesting ways if handed to the bots, but I'm uploading it for review
anyway. :)

Please put it somewhere near the bottom of your pile, Adam. Once the V8 bits
roll in, I'll throw it to the bots to see how many other tests need rebaselines.

-mike

[abarth-chromium, 2013-07-12 22:57:08]

Looks reasonable to me.  Let me know when you're further along.

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

https://codereview.chromium.org/19095003/diff/1/Source/bindings/v8/V8Initiali...
Source/bindings/v8/V8Initializer.cpp:111: setDOMException(SecurityError,
v8::Isolate::GetCurrent());
Why only location?  Why not have every security violation throw an exception?  I
guess we can start with location and see how it goes.

[Mike West, 2013-07-16 12:55:13]

On 2013/07/12 22:57:08, abarth wrote:
> Looks reasonable to me.  Let me know when you're further along.

The V8 patch landed, and has rolled into ToT. Would you mind taking another look
at the patch when you have some time?

>
https://codereview.chromium.org/19095003/diff/1/Source/bindings/v8/V8Initiali...
> File Source/bindings/v8/V8Initializer.cpp (right):
> 
>
https://codereview.chromium.org/19095003/diff/1/Source/bindings/v8/V8Initiali...
> Source/bindings/v8/V8Initializer.cpp:111: setDOMException(SecurityError,
> v8::Isolate::GetCurrent());
> Why only location?  Why not have every security violation throw an exception? 
I
> guess we can start with location and see how it goes.

Location was specifically called out in the HTML spec. I'm not sure about
History's and Window's behavior. I think I agree with you that throwing an
exception for those accesses would also be quite reasonable, but I'd like to
look at other browsers' behavior before going that route here.

It'd be nice to have those in separate patches anyway, given the number of
changes required to layout tests. It's probably already a bit overwhelming to
review. :)

[abarth-chromium, 2013-07-16 21:33:19]

The code change LGTM.  I didn't review the test changes.

[Mike West, 2013-07-17 06:27:17]

Thank you, Adam.

Marja, Jochen, would you mind spot-checking a few of the test changes? I'm
fairly confident they don't change behavior in any way other than catching the
exceptions we're now generating, but I'd appreciate another pair of eyes.

[haraken, 2013-07-17 06:36:41]

LGTM

[Mike West, 2013-07-17 07:12:42]

On 2013/07/17 06:36:41, haraken wrote:
> LGTM

Thanks Kentaro-san. :)

[commit-bot: I haz the power, 2013-07-17 07:12:56]

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

[commit-bot: I haz the power, 2013-07-17 07:21:30]

Change committed as 154376

[abarth-chromium, 2013-07-17 17:51:30]

https://codereview.chromium.org/19095003/diff/7001/Source/bindings/v8/V8Initi...
File Source/bindings/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/19095003/diff/7001/Source/bindings/v8/V8Initi...
Source/bindings/v8/V8Initializer.cpp:106:
targetWindow->printErrorMessage(targetWindow->crossDomainAccessErrorMessage(activeDOMWindow()));
I'm sorry I didn't mention this before, but my understanding is that developers
want to be able to catch this exception so that they can quiet this console
message.  Perhaps we should print out this message only when we don't throw the
exception?  Ideally we'd print the message if no one catches the exception as
well.

[levin, 2013-07-17 18:06:42]

On 2013/07/17 17:51:30, abarth wrote:
>
https://codereview.chromium.org/19095003/diff/7001/Source/bindings/v8/V8Initi...
> File Source/bindings/v8/V8Initializer.cpp (right):
> 
>
https://codereview.chromium.org/19095003/diff/7001/Source/bindings/v8/V8Initi...
> Source/bindings/v8/V8Initializer.cpp:106:
>
targetWindow->printErrorMessage(targetWindow->crossDomainAccessErrorMessage(activeDOMWindow()));
> I'm sorry I didn't mention this before, but my understanding is that
developers
> want to be able to catch this exception so that they can quiet this console
> message.  Perhaps we should print out this message only when we don't throw
the
> exception?  Ideally we'd print the message if no one catches the exception as
> well.
Yes! Just make it like every other exception.


But I thought that was going to be in a subsequent patch "A subsequent patch
will
adjust the exception's message to contain some sanitized version of the
current access violation detail message, and drop the unavoidable console
messages."

[abarth-chromium, 2013-07-17 18:25:48]

Ah, I fail at reading.  :)