Make CreationContext() not crash when there is no creation context.

Make CreationContext() not crash when there is no creation context.

Remote objects don't have a creation context.

BUG=527190
Review-Url: https://codereview.chromium.org/2693203003
Cr-Commit-Position: refs/heads/master@{#43306}
Committed: https://chromium.googlesource.com/v8/v8/+/40b74d04daabe599a6c2f1ea5b2d1eb9cce4ae8c

[dcheng, 2017-02-15 10:34:50]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 10:34:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-02-15 10:39:17]

Description was changed from

==========
Make CreationContext() not crash when there is no creation context.

Remote objects don't have a creation context.

BUG=527190
==========

to

==========
Make CreationContext() not crash when there is no creation context.

Remote objects don't have a creation context.

BUG=527190
==========

[dcheng, 2017-02-15 10:39:17]

dcheng@chromium.org changed reviewers:
+ jochen@chromium.org

[dcheng, 2017-02-15 10:41:55]

Disclaimer: I don't really know what I'm doing, so this patch could be horribly
wrong =)

This is an attempt to fix things like
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2...:
things like the <webview> tag implicitly use out-of-process iframes, and
ContextCanAccessObject() uses creation context as one of these heuristics.

As I understand, this change should be safe for the extensions check iff no
creation context implies a remote object (as remote objects must be safe for
cross-origin access by definition).

[jochen (gone - plz use gerrit), 2017-02-15 10:44:44]

I think it's preferable to add JSFunction::has_context() instead of making
context() return a nullptr

also, can you add a test please?

test/unittests/api/* is the new hotness...

[commit-bot: I haz the power, 2017-02-15 11:03:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 11:03:09]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-15 11:23:13]

(I'll finish the test up tomorrow morning, I'm sleepy =)

https://codereview.chromium.org/2693203003/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2693203003/diff/20001/src/objects.cc#newcode3181
src/objects.cc:3181: return function->has_context()
My understanding is it's preferred for this to be a MaybeHandle. However, that
leads to some weirdness in api.cc, because I think I'll need to do another
branch (on is_null()) there as well? So I opted to do the simpler thing, but I'm
happy to switch to MaybeHandle if there's a variant that doesn't require doing a
ternary here and in api.cc.

[dcheng, 2017-02-15 11:23:20]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 11:23:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 11:50:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 11:50:24]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-15 21:26:56]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 21:26:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 21:30:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 21:30:29]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/34863)

[dcheng, 2017-02-15 21:34:37]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 21:34:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 22:35:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 22:35:09]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-16 07:33:01]

PTAL, test added.

[dcheng, 2017-02-16 11:03:06]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 11:03:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 11:28:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 11:28:49]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-02-16 12:37:02]

jochen@chromium.org changed reviewers:
+ verwaest@chromium.org

[jochen (gone - plz use gerrit), 2017-02-16 12:37:02]

Adding Toon who mentioned an alternative idea (returning a dummy creation
context of sorts)

[Toon Verwaest, 2017-02-17 09:30:27]

What about just exposing a "IsRemoteGlobal()" and using that explicitly? That's
basically just taking jochen's idea further and using the same outside of V8.
Otherwise you're just checking that implicitly anyway through the null context.

[dcheng, 2017-02-17 09:55:43]

On 2017/02/17 09:30:27, Toon Verwaest wrote:
> What about just exposing a "IsRemoteGlobal()" and using that explicitly?
That's
> basically just taking jochen's idea further and using the same outside of V8.
> Otherwise you're just checking that implicitly anyway through the null
context.

So, I would have to expose this on Object with a name like HasCreationContext().
I can't have it associated with Context, since we don't have a Context object at
all. I could try to fake out a Context object, but that's not without
disadvantages as well.

To me, it feels a bit odd, because I guess we wouldn't change CreationContext()
at all if we implement HasCreationContext()... which means CreationContext()
would explode if you called it without checking HasCreationContext(). At that
point, it's basically the same thing as an empty handle anyway...

[Toon Verwaest, 2017-02-20 08:34:26]

ok sounds reasonable. lgtm then

[dcheng, 2017-02-20 09:20:33]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-02-20 09:20:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 09:47:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487582433874590,
"parent_rev": "a0e6e9af660c155dc37541b433ebd30aadd2f8d3", "commit_rev":
"40b74d04daabe599a6c2f1ea5b2d1eb9cce4ae8c"}

[commit-bot: I haz the power, 2017-02-20 09:47:20]

Description was changed from

==========
Make CreationContext() not crash when there is no creation context.

Remote objects don't have a creation context.

BUG=527190
==========

to

==========
Make CreationContext() not crash when there is no creation context.

Remote objects don't have a creation context.

BUG=527190

Review-Url: https://codereview.chromium.org/2693203003
Cr-Commit-Position: refs/heads/master@{#43306}
Committed:
https://chromium.googlesource.com/v8/v8/+/40b74d04daabe599a6c2f1ea5b2d1eb9cce...
==========

[commit-bot: I haz the power, 2017-02-20 09:47:21]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/40b74d04daabe599a6c2f1ea5b2d1eb9cce...