Use correct creation context during Iterable.forEach iteration

Use correct creation context during Iterable.forEach iteration

Use |thisValue| instead of |scriptState->context()->Global()|, since this
is simpler and since Global() actually returns a WindowProxy object that
may change and become incorrect to use as creation context depending on
what the callback function does.

BUG=605910
Committed: https://crrev.com/0cd7a9f853e3cb7c035b4ab9e07a503552267f9d
Cr-Commit-Position: refs/heads/master@{#389785}

[Jens Widell, 2016-04-25 09:20:17]

jl@opera.com changed reviewers:
+ haraken@chromium.org, yhirano@chromium.org

[Jens Widell, 2016-04-25 09:20:17]

PTAL

Note that I don't really understand why this fix works, and am thus not at all
sure this is a correct fix.

Essentially I tried to understand why NodeIterator doesn't have the same bug in
its calling of NodeFilter.acceptNode(), and it appears the key difference was
that it didn't "cache" scriptState->context()->Global() between calls like I did
in forEach().

[haraken, 2016-04-25 09:48:14]

Would you cc me on the bug?

[Jens Widell, 2016-04-25 09:49:29]

On 2016/04/25 09:48:14, haraken wrote:
> Would you cc me on the bug?

Done.

[Yuki, 2016-04-25 09:57:14]

On 2016/04/25 09:49:29, Jens Widell wrote:
> On 2016/04/25 09:48:14, haraken wrote:
> > Would you cc me on the bug?
> 
> Done.

I've just started getting understanding of the issue, but let me ask a question:
what context should we use?

http://heycam.github.io/webidl/#es-iterator-prototype-object
I'm not yet sure if this part of the spec applies or not (I've just started
reading), but if it should apply, then it seems that we should use "the
function's creation context" instead of the current context.
(See "the ECMAScript global environment associated with this Function")

If you're not yet sure, I think we need to understand this point.

[Jens Widell, 2016-04-25 10:29:08]

On 2016/04/25 09:57:14, Yuki wrote:
> On 2016/04/25 09:49:29, Jens Widell wrote:
> > On 2016/04/25 09:48:14, haraken wrote:
> > > Would you cc me on the bug?
> > 
> > Done.
> 
> I've just started getting understanding of the issue, but let me ask a
question:
> what context should we use?
> 
> http://heycam.github.io/webidl/#es-iterator-prototype-object
> I'm not yet sure if this part of the spec applies or not (I've just started
> reading), but if it should apply, then it seems that we should use "the
> function's creation context" instead of the current context.
> (See "the ECMAScript global environment associated with this Function")
> 
> If you're not yet sure, I think we need to understand this point.

[Jens Widell, 2016-04-25 11:10:11]

On 2016/04/25 09:57:14, Yuki wrote:
> On 2016/04/25 09:49:29, Jens Widell wrote:
> > On 2016/04/25 09:48:14, haraken wrote:
> > > Would you cc me on the bug?
> > 
> > Done.
> 
> I've just started getting understanding of the issue, but let me ask a
question:
> what context should we use?
> 
> http://heycam.github.io/webidl/#es-iterator-prototype-object
> I'm not yet sure if this part of the spec applies or not (I've just started
> reading), but if it should apply, then it seems that we should use "the
> function's creation context" instead of the current context.
> (See "the ECMAScript global environment associated with this Function")
> 
> If you're not yet sure, I think we need to understand this point.

I should think the correct context is
|thisValue.v8Value().As<v8::Object>()->CreationContext()|. It doesn't make a
difference here though, in the TC that's the same as |scriptState->context()|.
But conceptually, I think using the latter is wrong.

The key is apparently to not cache the return value of v8::Context::Global()
here. But I can't say I understand why.

[haraken, 2016-04-25 11:52:04]

On 2016/04/25 11:10:11, Jens Widell wrote:
> On 2016/04/25 09:57:14, Yuki wrote:
> > On 2016/04/25 09:49:29, Jens Widell wrote:
> > > On 2016/04/25 09:48:14, haraken wrote:
> > > > Would you cc me on the bug?
> > > 
> > > Done.
> > 
> > I've just started getting understanding of the issue, but let me ask a
> question:
> > what context should we use?
> > 
> > http://heycam.github.io/webidl/#es-iterator-prototype-object
> > I'm not yet sure if this part of the spec applies or not (I've just started
> > reading), but if it should apply, then it seems that we should use "the
> > function's creation context" instead of the current context.
> > (See "the ECMAScript global environment associated with this Function")
> > 
> > If you're not yet sure, I think we need to understand this point.
> 
> I should think the correct context is
> |thisValue.v8Value().As<v8::Object>()->CreationContext()|. It doesn't make a
> difference here though, in the TC that's the same as |scriptState->context()|.
> But conceptually, I think using the latter is wrong.
> 
> The key is apparently to not cache the return value of v8::Context::Global()
> here. But I can't say I understand why.

Yeah, I don't understand why scriptState->context()->Global() changes during the
iteration.

If that really happens, this wouldn't be the only place we need to worry about.

[Yuki, 2016-04-25 15:18:28]

On 2016/04/25 11:52:04, haraken wrote:
> On 2016/04/25 11:10:11, Jens Widell wrote:
> > On 2016/04/25 09:57:14, Yuki wrote:
> > > On 2016/04/25 09:49:29, Jens Widell wrote:
> > > > On 2016/04/25 09:48:14, haraken wrote:
> > > > > Would you cc me on the bug?
> > > > 
> > > > Done.
> > > 
> > > I've just started getting understanding of the issue, but let me ask a
> > question:
> > > what context should we use?
> > > 
> > > http://heycam.github.io/webidl/#es-iterator-prototype-object
> > > I'm not yet sure if this part of the spec applies or not (I've just
started
> > > reading), but if it should apply, then it seems that we should use "the
> > > function's creation context" instead of the current context.
> > > (See "the ECMAScript global environment associated with this Function")
> > > 
> > > If you're not yet sure, I think we need to understand this point.
> > 
> > I should think the correct context is
> > |thisValue.v8Value().As<v8::Object>()->CreationContext()|. It doesn't make a
> > difference here though, in the TC that's the same as
|scriptState->context()|.
> > But conceptually, I think using the latter is wrong.
> > 
> > The key is apparently to not cache the return value of v8::Context::Global()
> > here. But I can't say I understand why.
> 
> Yeah, I don't understand why scriptState->context()->Global() changes during
the
> iteration.
> 
> If that really happens, this wouldn't be the only place we need to worry
about.

https://codereview.chromium.org/1919873002/
I think this way is the right way to go.  We should use the context
associated with the given function.  If you guys agree, jl@, would you mind
to follow this way adding a layout test?

On my understanding, the key point of the issue is to detach the frame.  The
exploit code is detaching the frame.  It makes context->Global() point to a
strange place (I don't understand exactly what it will point to).

context->Global() is a global proxy object (= outer global object), whose
hidden prototype object is a global object (= inner global object = window
object).
    [global proxy object] --(hidden prototype)--> [global obejct]
When the frame is detached, IIUC, this hidden prototype chain gets cut.
    [global proxy object] --> ??? (somewhere else)
    (new global proxy object???) --(hidden prototype)--> [global object]
At this point, the saved context->Gloabl()'s context is NOT the same of
the [global object].  They have nothing to do.

My understanding about frame-detachment and navigation is imperfect, maybe
wrong.
But, at least I believe that the (old) global proxy object no longer points to
the global object once the frame gets detached.  And this is the cause.

Back to the solution and correct concept, the given function should run in
the context associated with that function, that guarantee that the context
never leaks.  So, I'd suggest to use the function's context.

BTW, V8ScriptRunner::callFunction uses isolate->GetCurrentContext() to run
the given function.  I feel it uncomfortable.  I'm not yet 100% sure, but this
also should be the function's context, I think.

[haraken, 2016-04-25 15:27:53]

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

[haraken, 2016-04-25 15:27:54]

+jochen +verwaest

It sounds dangerous that context()->Global() keeps pointing to an unknown object
after the frame is detached.

I guess we need to land jochen's CL
(https://codereview.chromium.org/1421113006/) somehow? Jochen's CL makes changes
to unconditionally detach global objects of all frames.

Either way, for this specific bug, the fix yukishiino@ proposed in #9 makes
sense.

[jochen (gone - plz use gerrit), 2016-04-25 15:54:18]

On 2016/04/25 at 15:27:54, haraken wrote:
> +jochen +verwaest
> 
> It sounds dangerous that context()->Global() keeps pointing to an unknown
object after the frame is detached.
> 
> I guess we need to land jochen's CL
(https://codereview.chromium.org/1421113006/) somehow? Jochen's CL makes changes
to unconditionally detach global objects of all frames.
> 
> Either way, for this specific bug, the fix yukishiino@ proposed in #9 makes
sense.

I just never got around to manually comparing the failed layout tests to Firefox
/ spec and rebaseline accordingly

[yhirano, 2016-04-26 01:36:23]

> #9

I still don't understand why jl@'s change fixes the behavior. Could you explain
the fact with your theory?

[Yuki, 2016-04-26 05:01:35]

On 2016/04/26 01:36:23, yhirano wrote:
> > #9
> 
> I still don't understand why jl@'s change fixes the behavior. Could you
explain
> the fact with your theory?

ScriptState's m_context is correct during the loop.  Therefore,
if you put scriptState->context()->Global() in the loop, it retrieves the NEW
global proxy object, which is different from the original global proxy object.
This is my hypothesis.  I've not tested it on my machine, though.

I'll defer the rest of work to jl@ as long as jl@ is okay with it.

[Jens Widell, 2016-04-26 09:55:40]

On 2016/04/26 05:01:35, Yuki wrote:
> On 2016/04/26 01:36:23, yhirano wrote:
> > > #9
> > 
> > I still don't understand why jl@'s change fixes the behavior. Could you
> explain
> > the fact with your theory?
> 
> ScriptState's m_context is correct during the loop.  Therefore,
> if you put scriptState->context()->Global() in the loop, it retrieves the NEW
> global proxy object, which is different from the original global proxy object.
> This is my hypothesis.  I've not tested it on my machine, though.
> 
> I'll defer the rest of work to jl@ as long as jl@ is okay with it.

So, I have updated the patch now, to use |thisValue.v8Value().As<v8::Object>()|
as the creation context.

My understanding here is that using a global object (or rather global proxy) is
unnecessary and dangerous since its characteristics can change. So we should
just just some other, simpler, object that references the correct v8::Context.

Using the callback function as creation context, as suggested in #9, is wrong, I
think. I assume the right thing is always to create FontFace objects in the same
context as the FontFaceSet object that references them. The callback function
can very easily reference a different context, for instance if you (in a
same-origin situation) do

  frames[0].document.fonts.forEach(function (f) { ... });

If the callback function is used as creation context, the fonts would be created
in the parent document's context here, meaning |f instanceof FontFace| would be
true but |f instanceof frames[0].FontFace| false. The opposite is true for the
FontFaceSet object in question.

As for layout tests, I haven't really been able to write a test that shows
anything wrong, other than the original TC from the bug. Given its nature, is it
a good thing to add as a layout test? I don't know what the procedure is for
this sort of thing...

[Jens Widell, 2016-04-26 11:51:47]

On 2016/04/26 09:55:40, Jens Widell wrote:
> As for layout tests, I haven't really been able to write a test that shows
> anything wrong, 

Alright, I figured out how to detect the error with a rather simple test, now
added.

[haraken, 2016-04-26 12:39:37]

LGTM

(I'm curious if this is the only place we need this fix though.)

[Jens Widell, 2016-04-26 14:04:20]

On 2016/04/26 12:39:37, haraken wrote:
> LGTM
> 
> (I'm curious if this is the only place we need this fix though.)

I intend to go through the code and check for that, but more eyes are better, so
don't let that stop you (or anyone else) from also investigating this. :-)

[Jens Widell, 2016-04-26 14:05:21]

Description was changed from

==========
Use correct creation context during Iterable.forEach iteration

Evaluate 'scriptState->context()->Global()' once per iterated item instead
of once per call to forEach().

BUG=605910
==========

to

==========
Use correct creation context during Iterable.forEach iteration

Use |thisValue| instead of |scriptState->context()->Global()|, since this
is simpler and since Global() actually returns a WindowProxy object that
may change and become incorrect to use as creation context depending on
what the callback function does.

BUG=605910
==========

[Jens Widell, 2016-04-26 14:07:15]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2016-04-26 14:07:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1918763002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1918763002/40001

[commit-bot: I haz the power, 2016-04-26 15:23:09]

Description was changed from

==========
Use correct creation context during Iterable.forEach iteration

Use |thisValue| instead of |scriptState->context()->Global()|, since this
is simpler and since Global() actually returns a WindowProxy object that
may change and become incorrect to use as creation context depending on
what the callback function does.

BUG=605910
==========

to

==========
Use correct creation context during Iterable.forEach iteration

Use |thisValue| instead of |scriptState->context()->Global()|, since this
is simpler and since Global() actually returns a WindowProxy object that
may change and become incorrect to use as creation context depending on
what the callback function does.

BUG=605910
==========

[commit-bot: I haz the power, 2016-04-26 15:23:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-26 15:25:26]

Description was changed from

==========
Use correct creation context during Iterable.forEach iteration

Use |thisValue| instead of |scriptState->context()->Global()|, since this
is simpler and since Global() actually returns a WindowProxy object that
may change and become incorrect to use as creation context depending on
what the callback function does.

BUG=605910
==========

to

==========
Use correct creation context during Iterable.forEach iteration

Use |thisValue| instead of |scriptState->context()->Global()|, since this
is simpler and since Global() actually returns a WindowProxy object that
may change and become incorrect to use as creation context depending on
what the callback function does.

BUG=605910

Committed: https://crrev.com/0cd7a9f853e3cb7c035b4ab9e07a503552267f9d
Cr-Commit-Position: refs/heads/master@{#389785}
==========

[commit-bot: I haz the power, 2016-04-26 15:25:27]

Patchset 3 (id:??) landed as
https://crrev.com/0cd7a9f853e3cb7c035b4ab9e07a503552267f9d
Cr-Commit-Position: refs/heads/master@{#389785}

[Yuki, 2016-04-27 06:25:09]

yukishiino@chromium.org changed reviewers:
+ yukishiino@chromium.org

[Yuki, 2016-04-27 06:25:10]

LGTM.

FYI, Firefox behaves different from Blink.
In the callback function f(arg) { ... },
    arg instanceof childWindow.FontFace
        => true
    arg instanceof parentWindow.FontFace
        => true (Blink returns false)
    childWindow.FontFace === parentWindow.FontFace
        => false
interesting...