Populate cross-origin location access SecurityError messages.

Populate cross-origin location access SecurityError messages.

We currently add a message to the console each time a Location object
is accessed cross-origin, in addition to throwing a SecurityError exception.
We should allow developers to suppress the console message by catching
and properly handling any exception that's generated.

This patch adds the ability to pipe a message through 'setDOMException()'
such that it's applied as the exception's message property. When we deny
access to a Location object, we can use this new method, and avoid
writing an unsuppressable message out to the console.

BUG=17325
R=abarth@chromium.org

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

[Mike West, 2013-07-17 18:12:03]

Still have some rebaselining to do, but what do you think about this change,
Adam?

[abarth-chromium, 2013-07-17 18:27:58]

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

https://codereview.chromium.org/19564003/diff/1/Source/bindings/v8/V8Initiali...
Source/bindings/v8/V8Initializer.cpp:110: setDOMException(SecurityError,
targetWindow->crossDomainAccessErrorMessage(activeDOMWindow()).utf8().data(),
v8::Isolate::GetCurrent());
Why utf8().data()?  Can't we just pass in the string?  That way we won't have to
make a bunch of copies.

[Mike West, 2013-07-17 18:37:36]

On 2013/07/17 18:27:58, abarth wrote:
>
https://codereview.chromium.org/19564003/diff/1/Source/bindings/v8/V8Initiali...
> File Source/bindings/v8/V8Initializer.cpp (right):
> 
>
https://codereview.chromium.org/19564003/diff/1/Source/bindings/v8/V8Initiali...
> Source/bindings/v8/V8Initializer.cpp:110: setDOMException(SecurityError,
> targetWindow->crossDomainAccessErrorMessage(activeDOMWindow()).utf8().data(),
> v8::Isolate::GetCurrent());
> Why utf8().data()?  Can't we just pass in the string?  That way we won't have
to
> make a bunch of copies.

We eventually store the char* on a DOMException in V8ThrowException, which is
then thrown at V8. Would you prefer to grab the characters then rather than
here?

+arv

[Mike West, 2013-07-17 18:38:10]

Actually +arv

[abarth-chromium, 2013-07-17 18:40:36]

DOMException::m_message is a String...

[Mike West, 2013-07-17 18:43:22]

On 2013/07/17 18:40:36, abarth wrote:
> DOMException::m_message is a String...

Oh. Hrm. Then why the heck am I passing around char*? :)

[arv (Not doing code reviews), 2013-07-17 18:48:55]

https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
File
LayoutTests/http/tests/security/cross-frame-access-location-put-expected.txt
(right):

https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
LayoutTests/http/tests/security/cross-frame-access-location-put-expected.txt:2:
SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from
accessing a frame with origin "http://localhost:8000". Protocols, domains, and
ports must match.
I think it would read better if the message was "A frame with ..." instead of
"Blocked a frame with ..."

https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
File LayoutTests/http/tests/security/svg-image-leak-expected.txt (right):

https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
LayoutTests/http/tests/security/svg-image-leak-expected.txt:1: ALERT: FAIL!
Cookies received: img=PASS; import=FAIL
Is this change intentional?

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

https://codereview.chromium.org/19564003/diff/1/Source/bindings/v8/V8Initiali...
Source/bindings/v8/V8Initializer.cpp:110: setDOMException(SecurityError,
targetWindow->crossDomainAccessErrorMessage(activeDOMWindow()).utf8().data(),
v8::Isolate::GetCurrent());
On 2013/07/17 18:27:59, abarth wrote:
> Why utf8().data()?  Can't we just pass in the string?  That way we won't have
to
> make a bunch of copies.

I'm fine with us changing all the exception methods to be changed to String
instead of char* if Adam prefers that.

[arv (Not doing code reviews), 2013-07-17 18:50:14]

Lets clean up the char* vs String. It looks like my bad.

On Wed, Jul 17, 2013 at 2:48 PM,  <arv@chromium.org> wrote:
>
>
https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
> File
> LayoutTests/http/tests/security/cross-frame-access-location-put-expected.txt
> (right):
>
>
https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
>
LayoutTests/http/tests/security/cross-frame-access-location-put-expected.txt:2:
> SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from
> accessing a frame with origin "http://localhost:8000". Protocols,
> domains, and ports must match.
> I think it would read better if the message was "A frame with ..."
> instead of "Blocked a frame with ..."
>
>
https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
> File LayoutTests/http/tests/security/svg-image-leak-expected.txt
> (right):
>
>
https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
> LayoutTests/http/tests/security/svg-image-leak-expected.txt:1: ALERT:
> FAIL! Cookies received: img=PASS; import=FAIL
> Is this change intentional?
>
>
https://codereview.chromium.org/19564003/diff/1/Source/bindings/v8/V8Initiali...
> File Source/bindings/v8/V8Initializer.cpp (right):
>
>
https://codereview.chromium.org/19564003/diff/1/Source/bindings/v8/V8Initiali...
> Source/bindings/v8/V8Initializer.cpp:110: setDOMException(SecurityError,
> targetWindow->crossDomainAccessErrorMessage(activeDOMWindow()).utf8().data(),
> v8::Isolate::GetCurrent());
> On 2013/07/17 18:27:59, abarth wrote:
>>
>> Why utf8().data()?  Can't we just pass in the string?  That way we
>
> won't have to
>>
>> make a bunch of copies.
>
>
> I'm fine with us changing all the exception methods to be changed to
> String instead of char* if Adam prefers that.
>
> https://codereview.chromium.org/19564003/



-- 
erik

[Mike West, 2013-07-18 07:41:23]

Cool. I'll hold on this until https://codereview.chromium.org/19684003/lands.

Thanks!

https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
File
LayoutTests/http/tests/security/cross-frame-access-location-put-expected.txt
(right):

https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
LayoutTests/http/tests/security/cross-frame-access-location-put-expected.txt:2:
SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from
accessing a frame with origin "http://localhost:8000". Protocols, domains, and
ports must match.
On 2013/07/17 18:48:56, arv wrote:
> I think it would read better if the message was "A frame with ..." instead of
> "Blocked a frame with ..."

I'm happy to put together a separate patch where we can chat about the message.
I'd like to get the property name in there somewhere as well, now that you
mention it.

https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
File LayoutTests/http/tests/security/svg-image-leak-expected.txt (right):

https://codereview.chromium.org/19564003/diff/1/LayoutTests/http/tests/securi...
LayoutTests/http/tests/security/svg-image-leak-expected.txt:1: ALERT: FAIL!
Cookies received: img=PASS; import=FAIL
On 2013/07/17 18:48:56, arv wrote:
> Is this change intentional?

Good catch. That was overzealous rebaselining.

[Mike West, 2013-07-19 06:44:05]

Hello again!

I've rebased this patch on top of arv@'s String conversion. Would you mind
taking another look?

-mike

[abarth-chromium, 2013-07-19 08:03:10]

Code change LGTM.  I didn't review the changes to the tests.

[haraken, 2013-07-19 08:50:42]

Given the code change is OK, the test changes look OK. LGTM.

[commit-bot: I haz the power, 2013-07-19 09:21:24]

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

[commit-bot: I haz the power, 2013-07-19 09:59:42]

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

[commit-bot: I haz the power, 2013-07-19 10:15:00]

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

[commit-bot: I haz the power, 2013-07-19 10:50:52]

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

[Mike West, 2013-07-19 11:19:05]

Committed patchset #4 manually as r154564 (presubmit successful).

[arv (Not doing code reviews), 2013-07-19 16:50:13]

LGTM