Pipe a script's CORS status through V8 during compilation.

Pipe a script's CORS status through V8 during compilation.

In order to properly sanitize exception data during a 'window.onerror'
handler, we need to know whether a script was served with proper CORS
headers at the time it was loaded into V8. This patch adds a single bool
to ScriptOrigin, and pipes that through the compiler to land on the
Script object. We can then retrieve the parameter when calling the
embedder's exception callback.

BUG=crbug.com/159566
R=mstarzinger@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=15963

[Mike West, 2013-07-26 14:45:07]

Er. Let's try the right codereview tool, shall we?

Sorry about that.

-mike

[Mike West, 2013-07-26 14:45:41]

Hi Yang and Jakob. I'm trying (and failing) to add a single boolean parameter to
the Script object. :)

The code in this patch works (kinda) in Release, but fails to compile in Debug:

# Fatal error in ../src/objects-inl.h, line 2555
# CHECK(object->IsSmi()) failed

I'm absolutely sure I've cargo-culted the wrong bits of code for the definition,
but I'm hitting a wall. If either of you could take a few minutes to help me
figure out what I'm doing wrong, I'd appreciate it.

Thanks!

-mike

[Mike West, 2013-07-29 12:19:12]

Thanks for your help on Friday. I've rebaselined this patch on top of the
cleanup you suggested (https://codereview.chromium.org/20758002/). When you have
a moment, I'd appreciate you taking a look at these two patches.

Thanks!

-mike

[Michael Starzinger, 2013-07-30 09:52:55]

In general I am fine with this change. The main comment is just about naming.
AFAICT the semantics of the new field are completely up to the embedder, V8 is
agnostic towards it's value, and the name should express that.

https://codereview.chromium.org/20646006/diff/16001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/20646006/diff/16001/include/v8.h#newcode946
include/v8.h:946: Handle<Boolean> resource_passed_access_check =
Handle<Boolean>()))
The term "access check" is already used in the API to describe something else.
The semantics of this boolean field are completely up to the embedder (i.e.
Chromium). Could I convince you to use a different term here? One suggestion
would be "embedder check", but I am open towards alternatives.

https://codereview.chromium.org/20646006/diff/16001/include/v8.h#newcode1139
include/v8.h:1139: * cross-origin access control checks when originally loaded.
nit: Can we rephrase the documentation for this method so that it clarifies that
this just threads through the value passed into the ScriptOrigin at compile
time. V8 itself doesn't really perform any access check and in fact is
completely agnostic to the value of this boolean field.

https://codereview.chromium.org/20646006/diff/16001/include/v8.h#newcode1141
include/v8.h:1141: bool DidPassAccessCheck() const;
nit: In light of the above two comments, could I convince you to rename the
method to OriginPassedEmbedderCheck() here?

https://codereview.chromium.org/20646006/diff/16001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/20646006/diff/16001/src/api.cc#newcode2417
src/api.cc:2417: 
nit: Two empty newlines between methods.

https://codereview.chromium.org/20646006/diff/16001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/20646006/diff/16001/test/cctest/test-api.cc#n...
test/cctest/test-api.cc:3578: THREADED_TEST(MessageHandler3) {
This test is not thread-safe as it uses global state (i.e. it writes to the
static "message_received" variable). So s/THREADED_TEST/TEST/ here.

https://codereview.chromium.org/20646006/diff/16001/test/cctest/test-api.cc#n...
test/cctest/test-api.cc:3598: TEST(MessageHandler2) {
nit: Can we move the new test to below this one so that "check_message_2" and
"MessageHandler2" are not torn apart?

[Mike West, 2013-07-30 10:01:58]

Thanks! Comments inline.

https://codereview.chromium.org/20646006/diff/16001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/20646006/diff/16001/include/v8.h#newcode946
include/v8.h:946: Handle<Boolean> resource_passed_access_check =
Handle<Boolean>()))
On 2013/07/30 09:52:55, Michael Starzinger wrote:
> The term "access check" is already used in the API to describe something else.
> The semantics of this boolean field are completely up to the embedder (i.e.
> Chromium). Could I convince you to use a different term here? One suggestion
> would be "embedder check", but I am open towards alternatives.

The goal is to allow the embedder to determine whether or not an exception
(Message) has been thrown from a Script that was loaded in such a way as to
allow exception details to be visible. In practice, this means that the script
was loaded with an 'Access-Control-Allow-Origin' header.

You're correct to say that this is embedder-specific. I'm not sure that node
would care, for instance.

I'd prefer something more specific than "embedder check", as it's entirely
possible that more such checks might come up in the future (via either Blink or
another embedder's use-case). 

I'll think about it over lunch. :)

https://codereview.chromium.org/20646006/diff/16001/include/v8.h#newcode1139
include/v8.h:1139: * cross-origin access control checks when originally loaded.
On 2013/07/30 09:52:55, Michael Starzinger wrote:
> nit: Can we rephrase the documentation for this method so that it clarifies
that
> this just threads through the value passed into the ScriptOrigin at compile
> time. V8 itself doesn't really perform any access check and in fact is
> completely agnostic to the value of this boolean field.

Will do.

https://codereview.chromium.org/20646006/diff/16001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/20646006/diff/16001/test/cctest/test-api.cc#n...
test/cctest/test-api.cc:3598: TEST(MessageHandler2) {
On 2013/07/30 09:52:55, Michael Starzinger wrote:
> nit: Can we move the new test to below this one so that "check_message_2" and
> "MessageHandler2" are not torn apart?

That looks like poor copy/paste on my part. I'll move the new methods.

[Mike West, 2013-07-30 10:22:43]

https://codereview.chromium.org/20646006/diff/16001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/20646006/diff/16001/include/v8.h#newcode946
include/v8.h:946: Handle<Boolean> resource_passed_access_check =
Handle<Boolean>()))
On 2013/07/30 10:01:58, Mike West wrote:
> I'll think about it over lunch. :)

How about "resource_is_shared_cross_origin" and "Script::IsSharedCrossOrigin"?
That fits with the notion of ScriptOrigin describing the location of the script
in a resource, and avoids the term "access_check".

WDYT?

[Michael Starzinger, 2013-07-30 10:27:10]

https://codereview.chromium.org/20646006/diff/16001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/20646006/diff/16001/include/v8.h#newcode946
include/v8.h:946: Handle<Boolean> resource_passed_access_check =
Handle<Boolean>()))
On 2013/07/30 10:22:43, Mike West wrote:
> On 2013/07/30 10:01:58, Mike West wrote:
> > I'll think about it over lunch. :)
> 
> How about "resource_is_shared_cross_origin" and "Script::IsSharedCrossOrigin"?
> That fits with the notion of ScriptOrigin describing the location of the
script
> in a resource, and avoids the term "access_check".
> 
> WDYT?

Yep, SGTM, I am fine with that.

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

I've renamed the various bits and pieces. Mind taking another look, Michael?

-mike

[Michael Starzinger, 2013-07-30 11:19:07]

LGTM. I'll land this for you after we did today's roll. Our tree is about to be
closed.

[Mike West, 2013-07-30 11:21:46]

On 2013/07/30 11:19:07, Michael Starzinger wrote:
> LGTM. I'll land this for you after we did today's roll. Our tree is about to
be
> closed.

Thank you. :)

[Mike West, 2013-07-30 13:02:13]

Would you mind holding off on landing this for a few minutes? I'd like to make
sure I haven't screwed something up. :)

[Mike West, 2013-07-30 13:40:20]

On 2013/07/30 13:02:13, Mike West wrote:
> Would you mind holding off on landing this for a few minutes? I'd like to make
> sure I haven't screwed something up. :)

I added another test. Things look fine on this end. :)

[Mike West, 2013-07-30 14:15:12]

On 2013/07/30 13:40:20, Mike West wrote:
> On 2013/07/30 13:02:13, Mike West wrote:
> > Would you mind holding off on landing this for a few minutes? I'd like to
make
> > sure I haven't screwed something up. :)
> 
> I added another test. Things look fine on this end. :)

Ha! I lied. As just discussed, the compilation cache was causing some flake in
the embedder when the same file was loaded twice in subsequent layout tests. The
latest patchset addresses that issue.

[Michael Starzinger, 2013-07-30 14:22:00]

Still LGTM, just one minor comment about one of the test cases.

https://codereview.chromium.org/20646006/diff/46001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/20646006/diff/46001/test/cctest/test-api.cc#n...
test/cctest/test-api.cc:3696: 
nit: Two empty newlines between methods.

https://codereview.chromium.org/20646006/diff/46001/test/cctest/test-api.cc#n...
test/cctest/test-api.cc:3730: v8::V8::AddMessageListener(check_message_5b);
Better also reset the "message_received" variable to false here before the
second message listener is installed.

[Mike West, 2013-07-30 14:29:27]

On 2013/07/30 14:22:00, Michael Starzinger wrote:
> Still LGTM, just one minor comment about one of the test cases.
> 
> https://codereview.chromium.org/20646006/diff/46001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (right):
> 
>
https://codereview.chromium.org/20646006/diff/46001/test/cctest/test-api.cc#n...
> test/cctest/test-api.cc:3696: 
> nit: Two empty newlines between methods.
> 
>
https://codereview.chromium.org/20646006/diff/46001/test/cctest/test-api.cc#n...
> test/cctest/test-api.cc:3730: v8::V8::AddMessageListener(check_message_5b);
> Better also reset the "message_received" variable to false here before the
> second message listener is installed.

Done and done. Thank you.

[Michael Starzinger, 2013-07-30 17:06:00]

Committed patchset #7 manually as r15963 (presubmit successful).