Transitively keep track of an isolated world's children scripts and worlds.

Transitively keep track of an isolated world's children scripts and worlds.

In blink, scripts from extensions run in an isolated world. When new scripts or script fragments are created by the extension, transitively keep track of the fact that an extension is responsible through the originWorld.


BUG=579710

[proberge, 2016-01-20 22:40:21]

Description was changed from

==========
Transitively keep track of an isolated world's children scripts and worlds.

In blink, scripts from extensions run in an isolated world. When new scripts or
script fragments are created by the extension, transitively keep track of the
fact that an extension is responsible through the originWorld.


BUG=579710
==========

to

==========
Transitively keep track of an isolated world's children scripts and worlds.

In blink, scripts from extensions run in an isolated world. When new scripts or
script fragments are created by the extension, transitively keep track of the
fact that an extension is responsible through the originWorld.


BUG=579710
==========

[proberge, 2016-01-20 22:40:21]

proberge@chromium.org changed reviewers:
+ haraken@chromium.org, joenotcharles@chromium.org, pmarch@chromium.org

[haraken, 2016-01-21 10:16:15]

Thanks for splitting the CL!

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:83:
DOMWrapperWorld& m_currentWorld;

I'm wondering if we could simplify the logic around here.

As far as I understand, it's not possible that more than 1 DOMWrapperWorlds are
pushed on the world stack. If that is the case, we won't need to create a link
from the current world to the origin world.

In other words, we can do something like:

- The caller site just calls 'DOMWrapperWorld::OriginWorldScope(m_originWorld)'.

- It records the origin world onto a static variable.

- The DOMActivityLogger code can check if the current world is an isolated world
or not *and* if the recorded origin world is an isolated world or not.

What do you think?

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp
(right):

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:49: ,
m_originWorld(PassRefPtr<DOMWrapperWorld>(world.originWorld()))

PassRefPtr<DOMWrapperWorld>() wouldn't be needed.

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:245: if
(v8::Isolate::GetCurrent()->InContext()) {

v8::Isolate::GetCurrent() is deprecated. Use toIsolate(elementDocument) instead.

BTW, do we really need the InContext check?

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:246: m_originWorld =
PassRefPtr<DOMWrapperWorld>(DOMWrapperWorld::current(v8::Isolate::GetCurrent()).originWorld());

PassRefPtr<DOMWrapperWorld> wouldn't be needed.

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:246: m_originWorld =
PassRefPtr<DOMWrapperWorld>(DOMWrapperWorld::current(v8::Isolate::GetCurrent()).originWorld());

v8::Isolate::GetCurrent() is deprecated.

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right):

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMTimer.cpp:77: m_originWorld =
PassRefPtr<DOMWrapperWorld>(DOMWrapperWorld::current(v8::Isolate::GetCurrent()).originWorld());

Ditto.

[Joe Mason, 2016-01-21 16:13:50]

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:83:
DOMWrapperWorld& m_currentWorld;
On 2016/01/21 10:16:15, haraken wrote:
> 
> I'm wondering if we could simplify the logic around here.
> 
> As far as I understand, it's not possible that more than 1 DOMWrapperWorlds
are
> pushed on the world stack. If that is the case, we won't need to create a link
> from the current world to the origin world.
> 
> In other words, we can do something like:
> 
> - The caller site just calls
'DOMWrapperWorld::OriginWorldScope(m_originWorld)'.
> 
> - It records the origin world onto a static variable.
> 
> - The DOMActivityLogger code can check if the current world is an isolated
world
> or not *and* if the recorded origin world is an isolated world or not.
> 
> What do you think?

That seems more complicated to me rather than simpler. A scoped class that
updates state when it's created and resets it when its destroyed is a common
idiom that's easy to understand and generalize. Recording the world into a
static variable relies on assumptions about the number of DOMWrapperWorlds that
can exist at once and about thread safety. (I assume they're correct
assumptions, but having to keep track of them and reason about them adds
complexity.)

And if there is only one DOMWrapperWorld created at a time, we're storing a
reference and a RefPtr on the stack instead of one RefPtr in a static variable.
That's not a huge difference in efficiency.

[haraken, 2016-01-21 16:41:33]

On 2016/01/21 16:13:50, joenotcharles wrote:
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):
> 
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:83:
> DOMWrapperWorld& m_currentWorld;
> On 2016/01/21 10:16:15, haraken wrote:
> > 
> > I'm wondering if we could simplify the logic around here.
> > 
> > As far as I understand, it's not possible that more than 1 DOMWrapperWorlds
> are
> > pushed on the world stack. If that is the case, we won't need to create a
link
> > from the current world to the origin world.
> > 
> > In other words, we can do something like:
> > 
> > - The caller site just calls
> 'DOMWrapperWorld::OriginWorldScope(m_originWorld)'.
> > 
> > - It records the origin world onto a static variable.
> > 
> > - The DOMActivityLogger code can check if the current world is an isolated
> world
> > or not *and* if the recorded origin world is an isolated world or not.
> > 
> > What do you think?
> 
> That seems more complicated to me rather than simpler. A scoped class that
> updates state when it's created and resets it when its destroyed is a common
> idiom that's easy to understand and generalize.

Just to clarify: What I'm suggesting is essentially just to remove the first
parameter from the current OriginWorldPusher's constructor. My point is that it
looks redundant (and error-prone) to retrieve the current world and use it as a
place to record the origin world.

> Recording the world into a
> static variable relies on assumptions about the number of DOMWrapperWorlds
that
> can exist at once and about thread safety. (I assume they're correct
> assumptions, but having to keep track of them and reason about them adds
> complexity.)
> 
> And if there is only one DOMWrapperWorld created at a time, we're storing a
> reference and a RefPtr on the stack instead of one RefPtr in a static
variable.
> That's not a huge difference in efficiency.

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right):

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMTimer.cpp:112:
DOMWrapperWorld::OriginWorldPusher
worldPusher(ScriptState::forMainWorld(toDocument(context)->frame())->world(),
m_originWorld);

For example, the world in this first parameter is wrong.
ScriptState::forMainWorld(...)->world() just returns the main world but there is
no guarantee that we are in the main world at this point.

That's why I want to remove the first parameter from the OriginWorldPusher
(rather than trying to make the first parameter correct).

[proberge, 2016-01-21 16:45:26]

On 2016/01/21 16:41:33, haraken wrote:
> On 2016/01/21 16:13:50, joenotcharles wrote:
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):
> > 
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:83:
> > DOMWrapperWorld& m_currentWorld;
> > On 2016/01/21 10:16:15, haraken wrote:
> > > 
> > > I'm wondering if we could simplify the logic around here.
> > > 
> > > As far as I understand, it's not possible that more than 1
DOMWrapperWorlds
> > are
> > > pushed on the world stack. If that is the case, we won't need to create a
> link
> > > from the current world to the origin world.
> > > 
> > > In other words, we can do something like:
> > > 
> > > - The caller site just calls
> > 'DOMWrapperWorld::OriginWorldScope(m_originWorld)'.
> > > 
> > > - It records the origin world onto a static variable.
> > > 
> > > - The DOMActivityLogger code can check if the current world is an isolated
> > world
> > > or not *and* if the recorded origin world is an isolated world or not.
> > > 
> > > What do you think?
> > 
> > That seems more complicated to me rather than simpler. A scoped class that
> > updates state when it's created and resets it when its destroyed is a common
> > idiom that's easy to understand and generalize.
> 
> Just to clarify: What I'm suggesting is essentially just to remove the first
> parameter from the current OriginWorldPusher's constructor. My point is that
it
> looks redundant (and error-prone) to retrieve the current world and use it as
a
> place to record the origin world.
> 
> > Recording the world into a
> > static variable relies on assumptions about the number of DOMWrapperWorlds
> that
> > can exist at once and about thread safety. (I assume they're correct
> > assumptions, but having to keep track of them and reason about them adds
> > complexity.)
> > 
> > And if there is only one DOMWrapperWorld created at a time, we're storing a
> > reference and a RefPtr on the stack instead of one RefPtr in a static
> variable.
> > That's not a huge difference in efficiency.
> 
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right):
> 
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/DOMTimer.cpp:112:
> DOMWrapperWorld::OriginWorldPusher
> worldPusher(ScriptState::forMainWorld(toDocument(context)->frame())->world(),
> m_originWorld);
> 
> For example, the world in this first parameter is wrong.
> ScriptState::forMainWorld(...)->world() just returns the main world but there
is
> no guarantee that we are in the main world at this point.
> 
> That's why I want to remove the first parameter from the OriginWorldPusher
> (rather than trying to make the first parameter correct).

I think the "single static variable" case breaks down for the iFrame case
(WebLocalFrameImpl), which doesn't use a OriginWorldPusher but rather taints the
frame permanently.
I'll see if it works with one static field and one private field.

[haraken, 2016-01-21 16:51:42]

On 2016/01/21 16:45:26, proberge wrote:
> On 2016/01/21 16:41:33, haraken wrote:
> > On 2016/01/21 16:13:50, joenotcharles wrote:
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:83:
> > > DOMWrapperWorld& m_currentWorld;
> > > On 2016/01/21 10:16:15, haraken wrote:
> > > > 
> > > > I'm wondering if we could simplify the logic around here.
> > > > 
> > > > As far as I understand, it's not possible that more than 1
> DOMWrapperWorlds
> > > are
> > > > pushed on the world stack. If that is the case, we won't need to create
a
> > link
> > > > from the current world to the origin world.
> > > > 
> > > > In other words, we can do something like:
> > > > 
> > > > - The caller site just calls
> > > 'DOMWrapperWorld::OriginWorldScope(m_originWorld)'.
> > > > 
> > > > - It records the origin world onto a static variable.
> > > > 
> > > > - The DOMActivityLogger code can check if the current world is an
isolated
> > > world
> > > > or not *and* if the recorded origin world is an isolated world or not.
> > > > 
> > > > What do you think?
> > > 
> > > That seems more complicated to me rather than simpler. A scoped class that
> > > updates state when it's created and resets it when its destroyed is a
common
> > > idiom that's easy to understand and generalize.
> > 
> > Just to clarify: What I'm suggesting is essentially just to remove the first
> > parameter from the current OriginWorldPusher's constructor. My point is that
> it
> > looks redundant (and error-prone) to retrieve the current world and use it
as
> a
> > place to record the origin world.
> > 
> > > Recording the world into a
> > > static variable relies on assumptions about the number of DOMWrapperWorlds
> > that
> > > can exist at once and about thread safety. (I assume they're correct
> > > assumptions, but having to keep track of them and reason about them adds
> > > complexity.)
> > > 
> > > And if there is only one DOMWrapperWorld created at a time, we're storing
a
> > > reference and a RefPtr on the stack instead of one RefPtr in a static
> > variable.
> > > That's not a huge difference in efficiency.
> > 
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/frame/DOMTimer.cpp:112:
> > DOMWrapperWorld::OriginWorldPusher
> >
worldPusher(ScriptState::forMainWorld(toDocument(context)->frame())->world(),
> > m_originWorld);
> > 
> > For example, the world in this first parameter is wrong.
> > ScriptState::forMainWorld(...)->world() just returns the main world but
there
> is
> > no guarantee that we are in the main world at this point.
> > 
> > That's why I want to remove the first parameter from the OriginWorldPusher
> > (rather than trying to make the first parameter correct).
> 
> I think the "single static variable" case breaks down for the iFrame case
> (WebLocalFrameImpl), which doesn't use a OriginWorldPusher but rather taints
the
> frame permanently.
> I'll see if it works with one static field and one private field.

(Maybe a single static variable is not a good idea; let me think about it a bit
more.)

[haraken, 2016-01-21 17:38:46]

On 2016/01/21 16:51:42, haraken wrote:
> On 2016/01/21 16:45:26, proberge wrote:
> > On 2016/01/21 16:41:33, haraken wrote:
> > > On 2016/01/21 16:13:50, joenotcharles wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > > > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > > > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:83:
> > > > DOMWrapperWorld& m_currentWorld;
> > > > On 2016/01/21 10:16:15, haraken wrote:
> > > > > 
> > > > > I'm wondering if we could simplify the logic around here.
> > > > > 
> > > > > As far as I understand, it's not possible that more than 1
> > DOMWrapperWorlds
> > > > are
> > > > > pushed on the world stack. If that is the case, we won't need to
create
> a
> > > link
> > > > > from the current world to the origin world.
> > > > > 
> > > > > In other words, we can do something like:
> > > > > 
> > > > > - The caller site just calls
> > > > 'DOMWrapperWorld::OriginWorldScope(m_originWorld)'.
> > > > > 
> > > > > - It records the origin world onto a static variable.
> > > > > 
> > > > > - The DOMActivityLogger code can check if the current world is an
> isolated
> > > > world
> > > > > or not *and* if the recorded origin world is an isolated world or not.
> > > > > 
> > > > > What do you think?
> > > > 
> > > > That seems more complicated to me rather than simpler. A scoped class
that
> > > > updates state when it's created and resets it when its destroyed is a
> common
> > > > idiom that's easy to understand and generalize.
> > > 
> > > Just to clarify: What I'm suggesting is essentially just to remove the
first
> > > parameter from the current OriginWorldPusher's constructor. My point is
that
> > it
> > > looks redundant (and error-prone) to retrieve the current world and use it
> as
> > a
> > > place to record the origin world.
> > > 
> > > > Recording the world into a
> > > > static variable relies on assumptions about the number of
DOMWrapperWorlds
> > > that
> > > > can exist at once and about thread safety. (I assume they're correct
> > > > assumptions, but having to keep track of them and reason about them adds
> > > > complexity.)
> > > > 
> > > > And if there is only one DOMWrapperWorld created at a time, we're
storing
> a
> > > > reference and a RefPtr on the stack instead of one RefPtr in a static
> > > variable.
> > > > That's not a huge difference in efficiency.
> > > 
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/frame/DOMTimer.cpp:112:
> > > DOMWrapperWorld::OriginWorldPusher
> > >
> worldPusher(ScriptState::forMainWorld(toDocument(context)->frame())->world(),
> > > m_originWorld);
> > > 
> > > For example, the world in this first parameter is wrong.
> > > ScriptState::forMainWorld(...)->world() just returns the main world but
> there
> > is
> > > no guarantee that we are in the main world at this point.
> > > 
> > > That's why I want to remove the first parameter from the OriginWorldPusher
> > > (rather than trying to make the first parameter correct).
> > 
> > I think the "single static variable" case breaks down for the iFrame case
> > (WebLocalFrameImpl), which doesn't use a OriginWorldPusher but rather taints
> the
> > frame permanently.
> > I'll see if it works with one static field and one private field.
> 
> (Maybe a single static variable is not a good idea; let me think about it a
bit
> more.)

Hmm, what confuses me is the following scenario:

1) World 1 creates an iframe F.
2) World 2 injects a script to the iframe F.
3) The script is fired.

In this case, when the script is fired, what world do you want to blame on? Both
world 1 and world 2, or only world 1, or only world 2?

[proberge, 2016-01-21 21:58:42]

On 2016/01/21 17:38:46, haraken wrote:
> On 2016/01/21 16:51:42, haraken wrote:
> > On 2016/01/21 16:45:26, proberge wrote:
> > > On 2016/01/21 16:41:33, haraken wrote:
> > > > On 2016/01/21 16:13:50, joenotcharles wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > > > > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h
> (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > > > > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:83:
> > > > > DOMWrapperWorld& m_currentWorld;
> > > > > On 2016/01/21 10:16:15, haraken wrote:
> > > > > > 
> > > > > > I'm wondering if we could simplify the logic around here.
> > > > > > 
> > > > > > As far as I understand, it's not possible that more than 1
> > > DOMWrapperWorlds
> > > > > are
> > > > > > pushed on the world stack. If that is the case, we won't need to
> create
> > a
> > > > link
> > > > > > from the current world to the origin world.
> > > > > > 
> > > > > > In other words, we can do something like:
> > > > > > 
> > > > > > - The caller site just calls
> > > > > 'DOMWrapperWorld::OriginWorldScope(m_originWorld)'.
> > > > > > 
> > > > > > - It records the origin world onto a static variable.
> > > > > > 
> > > > > > - The DOMActivityLogger code can check if the current world is an
> > isolated
> > > > > world
> > > > > > or not *and* if the recorded origin world is an isolated world or
not.
> > > > > > 
> > > > > > What do you think?
> > > > > 
> > > > > That seems more complicated to me rather than simpler. A scoped class
> that
> > > > > updates state when it's created and resets it when its destroyed is a
> > common
> > > > > idiom that's easy to understand and generalize.
> > > > 
> > > > Just to clarify: What I'm suggesting is essentially just to remove the
> first
> > > > parameter from the current OriginWorldPusher's constructor. My point is
> that
> > > it
> > > > looks redundant (and error-prone) to retrieve the current world and use
it
> > as
> > > a
> > > > place to record the origin world.
> > > > 
> > > > > Recording the world into a
> > > > > static variable relies on assumptions about the number of
> DOMWrapperWorlds
> > > > that
> > > > > can exist at once and about thread safety. (I assume they're correct
> > > > > assumptions, but having to keep track of them and reason about them
adds
> > > > > complexity.)
> > > > > 
> > > > > And if there is only one DOMWrapperWorld created at a time, we're
> storing
> > a
> > > > > reference and a RefPtr on the stack instead of one RefPtr in a static
> > > > variable.
> > > > > That's not a huge difference in efficiency.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> > > > File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/frame/DOMTimer.cpp:112:
> > > > DOMWrapperWorld::OriginWorldPusher
> > > >
> >
worldPusher(ScriptState::forMainWorld(toDocument(context)->frame())->world(),
> > > > m_originWorld);
> > > > 
> > > > For example, the world in this first parameter is wrong.
> > > > ScriptState::forMainWorld(...)->world() just returns the main world but
> > there
> > > is
> > > > no guarantee that we are in the main world at this point.
> > > > 
> > > > That's why I want to remove the first parameter from the
OriginWorldPusher
> > > > (rather than trying to make the first parameter correct).
> > > 
> > > I think the "single static variable" case breaks down for the iFrame case
> > > (WebLocalFrameImpl), which doesn't use a OriginWorldPusher but rather
taints
> > the
> > > frame permanently.
> > > I'll see if it works with one static field and one private field.
> > 
> > (Maybe a single static variable is not a good idea; let me think about it a
> bit
> > more.)
> 
> Hmm, what confuses me is the following scenario:
> 
> 1) World 1 creates an iframe F.
> 2) World 2 injects a script to the iframe F.
> 3) The script is fired.
> 
> In this case, when the script is fired, what world do you want to blame on?
Both
> world 1 and world 2, or only world 1, or only world 2?

I would want to blame world 2. But wouldn't the same-origin security policy
prevent most of these cases?

[haraken, 2016-01-21 22:46:10]

On 2016/01/21 21:58:42, proberge wrote:
> On 2016/01/21 17:38:46, haraken wrote:
> > On 2016/01/21 16:51:42, haraken wrote:
> > > On 2016/01/21 16:45:26, proberge wrote:
> > > > On 2016/01/21 16:41:33, haraken wrote:
> > > > > On 2016/01/21 16:13:50, joenotcharles wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > > > > > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h
> > (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > > > > > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:83:
> > > > > > DOMWrapperWorld& m_currentWorld;
> > > > > > On 2016/01/21 10:16:15, haraken wrote:
> > > > > > > 
> > > > > > > I'm wondering if we could simplify the logic around here.
> > > > > > > 
> > > > > > > As far as I understand, it's not possible that more than 1
> > > > DOMWrapperWorlds
> > > > > > are
> > > > > > > pushed on the world stack. If that is the case, we won't need to
> > create
> > > a
> > > > > link
> > > > > > > from the current world to the origin world.
> > > > > > > 
> > > > > > > In other words, we can do something like:
> > > > > > > 
> > > > > > > - The caller site just calls
> > > > > > 'DOMWrapperWorld::OriginWorldScope(m_originWorld)'.
> > > > > > > 
> > > > > > > - It records the origin world onto a static variable.
> > > > > > > 
> > > > > > > - The DOMActivityLogger code can check if the current world is an
> > > isolated
> > > > > > world
> > > > > > > or not *and* if the recorded origin world is an isolated world or
> not.
> > > > > > > 
> > > > > > > What do you think?
> > > > > > 
> > > > > > That seems more complicated to me rather than simpler. A scoped
class
> > that
> > > > > > updates state when it's created and resets it when its destroyed is
a
> > > common
> > > > > > idiom that's easy to understand and generalize.
> > > > > 
> > > > > Just to clarify: What I'm suggesting is essentially just to remove the
> > first
> > > > > parameter from the current OriginWorldPusher's constructor. My point
is
> > that
> > > > it
> > > > > looks redundant (and error-prone) to retrieve the current world and
use
> it
> > > as
> > > > a
> > > > > place to record the origin world.
> > > > > 
> > > > > > Recording the world into a
> > > > > > static variable relies on assumptions about the number of
> > DOMWrapperWorlds
> > > > > that
> > > > > > can exist at once and about thread safety. (I assume they're correct
> > > > > > assumptions, but having to keep track of them and reason about them
> adds
> > > > > > complexity.)
> > > > > > 
> > > > > > And if there is only one DOMWrapperWorld created at a time, we're
> > storing
> > > a
> > > > > > reference and a RefPtr on the stack instead of one RefPtr in a
static
> > > > > variable.
> > > > > > That's not a huge difference in efficiency.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> > > > > File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> > > > > third_party/WebKit/Source/core/frame/DOMTimer.cpp:112:
> > > > > DOMWrapperWorld::OriginWorldPusher
> > > > >
> > >
> worldPusher(ScriptState::forMainWorld(toDocument(context)->frame())->world(),
> > > > > m_originWorld);
> > > > > 
> > > > > For example, the world in this first parameter is wrong.
> > > > > ScriptState::forMainWorld(...)->world() just returns the main world
but
> > > there
> > > > is
> > > > > no guarantee that we are in the main world at this point.
> > > > > 
> > > > > That's why I want to remove the first parameter from the
> OriginWorldPusher
> > > > > (rather than trying to make the first parameter correct).
> > > > 
> > > > I think the "single static variable" case breaks down for the iFrame
case
> > > > (WebLocalFrameImpl), which doesn't use a OriginWorldPusher but rather
> taints
> > > the
> > > > frame permanently.
> > > > I'll see if it works with one static field and one private field.
> > > 
> > > (Maybe a single static variable is not a good idea; let me think about it
a
> > bit
> > > more.)
> > 
> > Hmm, what confuses me is the following scenario:
> > 
> > 1) World 1 creates an iframe F.
> > 2) World 2 injects a script to the iframe F.
> > 3) The script is fired.
> > 
> > In this case, when the script is fired, what world do you want to blame on?
> Both
> > world 1 and world 2, or only world 1, or only world 2?
> 
> I would want to blame world 2. But wouldn't the same-origin security policy
> prevent most of these cases?

If the iframe F is from the same origin of the main document, the script will be
fired.

I'm asking this because the current CL is doing a conceptually strange thing.
For example, when a frame is created, you record the world that created the
frame (say, world 1) onto the main world. This means that when some JavaScript
runs in the frame, you assume that the origin world of the JS is the world 1.
However, the JS may be invoked by some actions caused by another world.

In other words, I don't think it makes sense to associate an origin world with a
frame. It's hard to define a notion of "origin world of a frame".

[proberge, 2016-01-22 18:59:18]

On 2016/01/21 22:46:10, haraken wrote:
> On 2016/01/21 21:58:42, proberge wrote:
> > On 2016/01/21 17:38:46, haraken wrote:
> > > On 2016/01/21 16:51:42, haraken wrote:
> > > > On 2016/01/21 16:45:26, proberge wrote:
> > > > > On 2016/01/21 16:41:33, haraken wrote:
> > > > > > On 2016/01/21 16:13:50, joenotcharles wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > > > > > > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h
> > > (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
> > > > > > > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:83:
> > > > > > > DOMWrapperWorld& m_currentWorld;
> > > > > > > On 2016/01/21 10:16:15, haraken wrote:
> > > > > > > > 
> > > > > > > > I'm wondering if we could simplify the logic around here.
> > > > > > > > 
> > > > > > > > As far as I understand, it's not possible that more than 1
> > > > > DOMWrapperWorlds
> > > > > > > are
> > > > > > > > pushed on the world stack. If that is the case, we won't need to
> > > create
> > > > a
> > > > > > link
> > > > > > > > from the current world to the origin world.
> > > > > > > > 
> > > > > > > > In other words, we can do something like:
> > > > > > > > 
> > > > > > > > - The caller site just calls
> > > > > > > 'DOMWrapperWorld::OriginWorldScope(m_originWorld)'.
> > > > > > > > 
> > > > > > > > - It records the origin world onto a static variable.
> > > > > > > > 
> > > > > > > > - The DOMActivityLogger code can check if the current world is
an
> > > > isolated
> > > > > > > world
> > > > > > > > or not *and* if the recorded origin world is an isolated world
or
> > not.
> > > > > > > > 
> > > > > > > > What do you think?
> > > > > > > 
> > > > > > > That seems more complicated to me rather than simpler. A scoped
> class
> > > that
> > > > > > > updates state when it's created and resets it when its destroyed
is
> a
> > > > common
> > > > > > > idiom that's easy to understand and generalize.
> > > > > > 
> > > > > > Just to clarify: What I'm suggesting is essentially just to remove
the
> > > first
> > > > > > parameter from the current OriginWorldPusher's constructor. My point
> is
> > > that
> > > > > it
> > > > > > looks redundant (and error-prone) to retrieve the current world and
> use
> > it
> > > > as
> > > > > a
> > > > > > place to record the origin world.
> > > > > > 
> > > > > > > Recording the world into a
> > > > > > > static variable relies on assumptions about the number of
> > > DOMWrapperWorlds
> > > > > > that
> > > > > > > can exist at once and about thread safety. (I assume they're
correct
> > > > > > > assumptions, but having to keep track of them and reason about
them
> > adds
> > > > > > > complexity.)
> > > > > > > 
> > > > > > > And if there is only one DOMWrapperWorld created at a time, we're
> > > storing
> > > > a
> > > > > > > reference and a RefPtr on the stack instead of one RefPtr in a
> static
> > > > > > variable.
> > > > > > > That's not a huge difference in efficiency.
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> > > > > > File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
> > > > > > third_party/WebKit/Source/core/frame/DOMTimer.cpp:112:
> > > > > > DOMWrapperWorld::OriginWorldPusher
> > > > > >
> > > >
> >
worldPusher(ScriptState::forMainWorld(toDocument(context)->frame())->world(),
> > > > > > m_originWorld);
> > > > > > 
> > > > > > For example, the world in this first parameter is wrong.
> > > > > > ScriptState::forMainWorld(...)->world() just returns the main world
> but
> > > > there
> > > > > is
> > > > > > no guarantee that we are in the main world at this point.
> > > > > > 
> > > > > > That's why I want to remove the first parameter from the
> > OriginWorldPusher
> > > > > > (rather than trying to make the first parameter correct).
> > > > > 
> > > > > I think the "single static variable" case breaks down for the iFrame
> case
> > > > > (WebLocalFrameImpl), which doesn't use a OriginWorldPusher but rather
> > taints
> > > > the
> > > > > frame permanently.
> > > > > I'll see if it works with one static field and one private field.
> > > > 
> > > > (Maybe a single static variable is not a good idea; let me think about
it
> a
> > > bit
> > > > more.)
> > > 
> > > Hmm, what confuses me is the following scenario:
> > > 
> > > 1) World 1 creates an iframe F.
> > > 2) World 2 injects a script to the iframe F.
> > > 3) The script is fired.
> > > 
> > > In this case, when the script is fired, what world do you want to blame
on?
> > Both
> > > world 1 and world 2, or only world 1, or only world 2?
> > 
> > I would want to blame world 2. But wouldn't the same-origin security policy
> > prevent most of these cases?
> 
> If the iframe F is from the same origin of the main document, the script will
be
> fired.
> 
> I'm asking this because the current CL is doing a conceptually strange thing.
> For example, when a frame is created, you record the world that created the
> frame (say, world 1) onto the main world. This means that when some JavaScript
> runs in the frame, you assume that the origin world of the JS is the world 1.
> However, the JS may be invoked by some actions caused by another world.
> 
> In other words, I don't think it makes sense to associate an origin world with
a
> frame. It's hard to define a notion of "origin world of a frame".

Even ignoring the frame case, using a single static member doesn't seem to be
working. It's possible for two OriginWorldPushers to exist at once, which causes
issues when one of them is destroyed while the other hasn't finished executing
its script.

Example seen:
DOMTimer OriginWorldPusher 
  ScriptLoader1 OriginWorldPusher 
  ScriptLoader1 ~OriginWorldPusher (clearing the static field)
  ScriptLoader2 OriginWorldPusher (lost the isolated origin)
  ScriptLoader2 ~OriginWorldPusher 
DOMTimer ~OriginWorldPusher 

Regarding the frame: it's possible that I added the WebLocalFrameImpl code to
get attribution working for a case that wasn't working because of an error like
getting the wrong world (as per your DOMTimer comment). I'll double-check.

[haraken, 2016-01-22 20:30:59]

> Even ignoring the frame case, using a single static member doesn't seem to be
> working. It's possible for two OriginWorldPushers to exist at once, which
causes
> issues when one of them is destroyed while the other hasn't finished executing
> its script.
> 
> Example seen:
> DOMTimer OriginWorldPusher <=== (a)
>   ScriptLoader1 OriginWorldPusher  <=== (b)
>   ScriptLoader1 ~OriginWorldPusher (clearing the static field)  <=== (c)
>   ScriptLoader2 OriginWorldPusher (lost the isolated origin)  <=== (d)
>   ScriptLoader2 ~OriginWorldPusher   <=== (e)
> DOMTimer ~OriginWorldPusher 

Is it possible that the world pushed at (b) is different from the world pushed
at (a)? If the answer is no, we can just skip setting the static variable if
some value is already set; i.e., make (b), (c), (d), (e) no-op.

> Regarding the frame: it's possible that I added the WebLocalFrameImpl code to
> get attribution working for a case that wasn't working because of an error
like
> getting the wrong world (as per your DOMTimer comment). I'll double-check.

Overall, I'm not sure if a static variable is a good idea (actually I'm not
sure). However, my point is that creating a "stack of worlds" is doing a
conceptually strange thing (so I want to avoid it). Is it possible that two
different worlds are pushed on the stack? If the answer is yes, it's not clear
which world the DOMActivityLogger should log an activity against. If the answer
is no, there is no reason to create a stack.

So, my point is that we want to avoid creating a stack of worlds. It's doing a
strange thing.

[proberge, 2016-01-27 18:33:15]

On 2016/01/22 20:30:59, haraken wrote:
> > Even ignoring the frame case, using a single static member doesn't seem to
be
> > working. It's possible for two OriginWorldPushers to exist at once, which
> causes
> > issues when one of them is destroyed while the other hasn't finished
executing
> > its script.
> > 
> > Example seen:
> > DOMTimer OriginWorldPusher <=== (a)
> >   ScriptLoader1 OriginWorldPusher  <=== (b)
> >   ScriptLoader1 ~OriginWorldPusher (clearing the static field)  <=== (c)
> >   ScriptLoader2 OriginWorldPusher (lost the isolated origin)  <=== (d)
> >   ScriptLoader2 ~OriginWorldPusher   <=== (e)
> > DOMTimer ~OriginWorldPusher 
> 
> Is it possible that the world pushed at (b) is different from the world pushed
> at (a)? If the answer is no, we can just skip setting the static variable if
> some value is already set; i.e., make (b), (c), (d), (e) no-op.
> 
> > Regarding the frame: it's possible that I added the WebLocalFrameImpl code
to
> > get attribution working for a case that wasn't working because of an error
> like
> > getting the wrong world (as per your DOMTimer comment). I'll double-check.
> 
> Overall, I'm not sure if a static variable is a good idea (actually I'm not
> sure). However, my point is that creating a "stack of worlds" is doing a
> conceptually strange thing (so I want to avoid it). Is it possible that two
> different worlds are pushed on the stack? If the answer is yes, it's not clear
> which world the DOMActivityLogger should log an activity against. If the
answer
> is no, there is no reason to create a stack.
> 
> So, my point is that we want to avoid creating a stack of worlds. It's doing a
> strange thing.

Yes. If we have two extensions/isolated worlds, it's possible for the world
pushed at (a) to be different than the world pushed at (b). Ex:

DOMTimer OriginWorldPusher  <=== world_a
  V8AbstractEventHandler OriginWorldPusher  <=== world_b
  V8AbstractEvenHandler ~OriginWorldPusher  (should pop to world_a)
DOMTimer ~OriginWorldPusher (should pop to null)

[haraken, 2016-01-28 08:14:09]

On 2016/01/27 18:33:15, proberge wrote:
> On 2016/01/22 20:30:59, haraken wrote:
> > > Even ignoring the frame case, using a single static member doesn't seem to
> be
> > > working. It's possible for two OriginWorldPushers to exist at once, which
> > causes
> > > issues when one of them is destroyed while the other hasn't finished
> executing
> > > its script.
> > > 
> > > Example seen:
> > > DOMTimer OriginWorldPusher <=== (a)
> > >   ScriptLoader1 OriginWorldPusher  <=== (b)
> > >   ScriptLoader1 ~OriginWorldPusher (clearing the static field)  <=== (c)
> > >   ScriptLoader2 OriginWorldPusher (lost the isolated origin)  <=== (d)
> > >   ScriptLoader2 ~OriginWorldPusher   <=== (e)
> > > DOMTimer ~OriginWorldPusher 
> > 
> > Is it possible that the world pushed at (b) is different from the world
pushed
> > at (a)? If the answer is no, we can just skip setting the static variable if
> > some value is already set; i.e., make (b), (c), (d), (e) no-op.
> > 
> > > Regarding the frame: it's possible that I added the WebLocalFrameImpl code
> to
> > > get attribution working for a case that wasn't working because of an error
> > like
> > > getting the wrong world (as per your DOMTimer comment). I'll double-check.
> > 
> > Overall, I'm not sure if a static variable is a good idea (actually I'm not
> > sure). However, my point is that creating a "stack of worlds" is doing a
> > conceptually strange thing (so I want to avoid it). Is it possible that two
> > different worlds are pushed on the stack? If the answer is yes, it's not
clear
> > which world the DOMActivityLogger should log an activity against. If the
> answer
> > is no, there is no reason to create a stack.
> > 
> > So, my point is that we want to avoid creating a stack of worlds. It's doing
a
> > strange thing.
> 
> Yes. If we have two extensions/isolated worlds, it's possible for the world
> pushed at (a) to be different than the world pushed at (b). Ex:
> 
> DOMTimer OriginWorldPusher  <=== world_a
>   V8AbstractEventHandler OriginWorldPusher  <=== world_b
>   V8AbstractEvenHandler ~OriginWorldPusher  (should pop to world_a)
> DOMTimer ~OriginWorldPusher (should pop to null)

OK... got it. I'm not really happy about the world's stack (because it's doing a
conceptually strange thing) but I cannot come up with a clearer way to realize
what you want to realize.

One possible clean-up would be:

1) Create a stack as a static variable. You can create something like:

static Vector<RefPtr<DOMWrapperWorld>>& DOMWrapperWorld::originWorldStack() {
  DEFINE_STATIC_LOCAL(OwnPtr<Vector<RefPtr<DOMWrapperWorld>>>, worldStack, (new
Vector<RefPtr<DOMWrapperWorld>>));
  return *worldStack;
}

2) Use the stack to push/pop the worlds. The caller site would look like:

  DOMWrapperWorld::OriginWorldScope scope(m_originWorld);

Would it work?

[proberge, 2016-01-28 22:41:30]

On 2016/01/28 08:14:09, haraken wrote:
> On 2016/01/27 18:33:15, proberge wrote:
> > On 2016/01/22 20:30:59, haraken wrote:
> > > > Even ignoring the frame case, using a single static member doesn't seem
to
> > be
> > > > working. It's possible for two OriginWorldPushers to exist at once,
which
> > > causes
> > > > issues when one of them is destroyed while the other hasn't finished
> > executing
> > > > its script.
> > > > 
> > > > Example seen:
> > > > DOMTimer OriginWorldPusher <=== (a)
> > > >   ScriptLoader1 OriginWorldPusher  <=== (b)
> > > >   ScriptLoader1 ~OriginWorldPusher (clearing the static field)  <=== (c)
> > > >   ScriptLoader2 OriginWorldPusher (lost the isolated origin)  <=== (d)
> > > >   ScriptLoader2 ~OriginWorldPusher   <=== (e)
> > > > DOMTimer ~OriginWorldPusher 
> > > 
> > > Is it possible that the world pushed at (b) is different from the world
> pushed
> > > at (a)? If the answer is no, we can just skip setting the static variable
if
> > > some value is already set; i.e., make (b), (c), (d), (e) no-op.
> > > 
> > > > Regarding the frame: it's possible that I added the WebLocalFrameImpl
code
> > to
> > > > get attribution working for a case that wasn't working because of an
error
> > > like
> > > > getting the wrong world (as per your DOMTimer comment). I'll
double-check.
> > > 
> > > Overall, I'm not sure if a static variable is a good idea (actually I'm
not
> > > sure). However, my point is that creating a "stack of worlds" is doing a
> > > conceptually strange thing (so I want to avoid it). Is it possible that
two
> > > different worlds are pushed on the stack? If the answer is yes, it's not
> clear
> > > which world the DOMActivityLogger should log an activity against. If the
> > answer
> > > is no, there is no reason to create a stack.
> > > 
> > > So, my point is that we want to avoid creating a stack of worlds. It's
doing
> a
> > > strange thing.
> > 
> > Yes. If we have two extensions/isolated worlds, it's possible for the world
> > pushed at (a) to be different than the world pushed at (b). Ex:
> > 
> > DOMTimer OriginWorldPusher  <=== world_a
> >   V8AbstractEventHandler OriginWorldPusher  <=== world_b
> >   V8AbstractEvenHandler ~OriginWorldPusher  (should pop to world_a)
> > DOMTimer ~OriginWorldPusher (should pop to null)
> 
> OK... got it. I'm not really happy about the world's stack (because it's doing
a
> conceptually strange thing) but I cannot come up with a clearer way to realize
> what you want to realize.
> 
> One possible clean-up would be:
> 
> 1) Create a stack as a static variable. You can create something like:
> 
> static Vector<RefPtr<DOMWrapperWorld>>& DOMWrapperWorld::originWorldStack() {
>   DEFINE_STATIC_LOCAL(OwnPtr<Vector<RefPtr<DOMWrapperWorld>>>, worldStack,
(new
> Vector<RefPtr<DOMWrapperWorld>>));
>   return *worldStack;
> }
> 
> 2) Use the stack to push/pop the worlds. The caller site would look like:
> 
>   DOMWrapperWorld::OriginWorldScope scope(m_originWorld);
> 
> Would it work?

I seem to keep on tripping up
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
If there's multiple tabs open, each with their own renderer thread, I'm guessing
they shouldn't be using the same static world stack.

There's a few uses of DEFINE_STATIC_LOCAL in DOMWrapperWorld.cc, but they're
protected by an ASSERT(inMainThread()). Is the main thread shared between
tabs/windows?

[haraken, 2016-01-29 05:00:47]

On 2016/01/28 22:41:30, proberge wrote:
> On 2016/01/28 08:14:09, haraken wrote:
> > On 2016/01/27 18:33:15, proberge wrote:
> > > On 2016/01/22 20:30:59, haraken wrote:
> > > > > Even ignoring the frame case, using a single static member doesn't
seem
> to
> > > be
> > > > > working. It's possible for two OriginWorldPushers to exist at once,
> which
> > > > causes
> > > > > issues when one of them is destroyed while the other hasn't finished
> > > executing
> > > > > its script.
> > > > > 
> > > > > Example seen:
> > > > > DOMTimer OriginWorldPusher <=== (a)
> > > > >   ScriptLoader1 OriginWorldPusher  <=== (b)
> > > > >   ScriptLoader1 ~OriginWorldPusher (clearing the static field)  <===
(c)
> > > > >   ScriptLoader2 OriginWorldPusher (lost the isolated origin)  <=== (d)
> > > > >   ScriptLoader2 ~OriginWorldPusher   <=== (e)
> > > > > DOMTimer ~OriginWorldPusher 
> > > > 
> > > > Is it possible that the world pushed at (b) is different from the world
> > pushed
> > > > at (a)? If the answer is no, we can just skip setting the static
variable
> if
> > > > some value is already set; i.e., make (b), (c), (d), (e) no-op.
> > > > 
> > > > > Regarding the frame: it's possible that I added the WebLocalFrameImpl
> code
> > > to
> > > > > get attribution working for a case that wasn't working because of an
> error
> > > > like
> > > > > getting the wrong world (as per your DOMTimer comment). I'll
> double-check.
> > > > 
> > > > Overall, I'm not sure if a static variable is a good idea (actually I'm
> not
> > > > sure). However, my point is that creating a "stack of worlds" is doing a
> > > > conceptually strange thing (so I want to avoid it). Is it possible that
> two
> > > > different worlds are pushed on the stack? If the answer is yes, it's not
> > clear
> > > > which world the DOMActivityLogger should log an activity against. If the
> > > answer
> > > > is no, there is no reason to create a stack.
> > > > 
> > > > So, my point is that we want to avoid creating a stack of worlds. It's
> doing
> > a
> > > > strange thing.
> > > 
> > > Yes. If we have two extensions/isolated worlds, it's possible for the
world
> > > pushed at (a) to be different than the world pushed at (b). Ex:
> > > 
> > > DOMTimer OriginWorldPusher  <=== world_a
> > >   V8AbstractEventHandler OriginWorldPusher  <=== world_b
> > >   V8AbstractEvenHandler ~OriginWorldPusher  (should pop to world_a)
> > > DOMTimer ~OriginWorldPusher (should pop to null)
> > 
> > OK... got it. I'm not really happy about the world's stack (because it's
doing
> a
> > conceptually strange thing) but I cannot come up with a clearer way to
realize
> > what you want to realize.
> > 
> > One possible clean-up would be:
> > 
> > 1) Create a stack as a static variable. You can create something like:
> > 
> > static Vector<RefPtr<DOMWrapperWorld>>& DOMWrapperWorld::originWorldStack()
{
> >   DEFINE_STATIC_LOCAL(OwnPtr<Vector<RefPtr<DOMWrapperWorld>>>, worldStack,
> (new
> > Vector<RefPtr<DOMWrapperWorld>>));
> >   return *worldStack;
> > }
> > 
> > 2) Use the stack to push/pop the worlds. The caller site would look like:
> > 
> >   DOMWrapperWorld::OriginWorldScope scope(m_originWorld);
> > 
> > Would it work?
> 
> I seem to keep on tripping up
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
> If there's multiple tabs open, each with their own renderer thread, I'm
guessing
> they shouldn't be using the same static world stack.
> 
> There's a few uses of DEFINE_STATIC_LOCAL in DOMWrapperWorld.cc, but they're
> protected by an ASSERT(inMainThread()). Is the main thread shared between
> tabs/windows?

Yeah, that's a good question. I think the main thread is shared by all tabs but
I'll double-check.

Note that if you don't think that DEFINE_STATIC_LOCAL(Vector<DOMWrapperWorld>)
will work, your approach won't work either. Your approach is creating a world
stack on the main world but the main world is created by DEFINE_STATIC_LOCAL(),
which means that it has the same problem.

[proberge, 2016-01-29 17:59:51]

On 2016/01/29 05:00:47, haraken wrote:
> On 2016/01/28 22:41:30, proberge wrote:
> > On 2016/01/28 08:14:09, haraken wrote:
> > > On 2016/01/27 18:33:15, proberge wrote:
> > > > On 2016/01/22 20:30:59, haraken wrote:
> > > > > > Even ignoring the frame case, using a single static member doesn't
> seem
> > to
> > > > be
> > > > > > working. It's possible for two OriginWorldPushers to exist at once,
> > which
> > > > > causes
> > > > > > issues when one of them is destroyed while the other hasn't finished
> > > > executing
> > > > > > its script.
> > > > > > 
> > > > > > Example seen:
> > > > > > DOMTimer OriginWorldPusher <=== (a)
> > > > > >   ScriptLoader1 OriginWorldPusher  <=== (b)
> > > > > >   ScriptLoader1 ~OriginWorldPusher (clearing the static field)  <===
> (c)
> > > > > >   ScriptLoader2 OriginWorldPusher (lost the isolated origin)  <===
(d)
> > > > > >   ScriptLoader2 ~OriginWorldPusher   <=== (e)
> > > > > > DOMTimer ~OriginWorldPusher 
> > > > > 
> > > > > Is it possible that the world pushed at (b) is different from the
world
> > > pushed
> > > > > at (a)? If the answer is no, we can just skip setting the static
> variable
> > if
> > > > > some value is already set; i.e., make (b), (c), (d), (e) no-op.
> > > > > 
> > > > > > Regarding the frame: it's possible that I added the
WebLocalFrameImpl
> > code
> > > > to
> > > > > > get attribution working for a case that wasn't working because of an
> > error
> > > > > like
> > > > > > getting the wrong world (as per your DOMTimer comment). I'll
> > double-check.
> > > > > 
> > > > > Overall, I'm not sure if a static variable is a good idea (actually
I'm
> > not
> > > > > sure). However, my point is that creating a "stack of worlds" is doing
a
> > > > > conceptually strange thing (so I want to avoid it). Is it possible
that
> > two
> > > > > different worlds are pushed on the stack? If the answer is yes, it's
not
> > > clear
> > > > > which world the DOMActivityLogger should log an activity against. If
the
> > > > answer
> > > > > is no, there is no reason to create a stack.
> > > > > 
> > > > > So, my point is that we want to avoid creating a stack of worlds. It's
> > doing
> > > a
> > > > > strange thing.
> > > > 
> > > > Yes. If we have two extensions/isolated worlds, it's possible for the
> world
> > > > pushed at (a) to be different than the world pushed at (b). Ex:
> > > > 
> > > > DOMTimer OriginWorldPusher  <=== world_a
> > > >   V8AbstractEventHandler OriginWorldPusher  <=== world_b
> > > >   V8AbstractEvenHandler ~OriginWorldPusher  (should pop to world_a)
> > > > DOMTimer ~OriginWorldPusher (should pop to null)
> > > 
> > > OK... got it. I'm not really happy about the world's stack (because it's
> doing
> > a
> > > conceptually strange thing) but I cannot come up with a clearer way to
> realize
> > > what you want to realize.
> > > 
> > > One possible clean-up would be:
> > > 
> > > 1) Create a stack as a static variable. You can create something like:
> > > 
> > > static Vector<RefPtr<DOMWrapperWorld>>&
DOMWrapperWorld::originWorldStack()
> {
> > >   DEFINE_STATIC_LOCAL(OwnPtr<Vector<RefPtr<DOMWrapperWorld>>>, worldStack,
> > (new
> > > Vector<RefPtr<DOMWrapperWorld>>));
> > >   return *worldStack;
> > > }
> > > 
> > > 2) Use the stack to push/pop the worlds. The caller site would look like:
> > > 
> > >   DOMWrapperWorld::OriginWorldScope scope(m_originWorld);
> > > 
> > > Would it work?
> > 
> > I seem to keep on tripping up
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
> > If there's multiple tabs open, each with their own renderer thread, I'm
> guessing
> > they shouldn't be using the same static world stack.
> > 
> > There's a few uses of DEFINE_STATIC_LOCAL in DOMWrapperWorld.cc, but they're
> > protected by an ASSERT(inMainThread()). Is the main thread shared between
> > tabs/windows?
> 
> Yeah, that's a good question. I think the main thread is shared by all tabs
but
> I'll double-check.
> 
> Note that if you don't think that DEFINE_STATIC_LOCAL(Vector<DOMWrapperWorld>)
> will work, your approach won't work either. Your approach is creating a world
> stack on the main world but the main world is created by
DEFINE_STATIC_LOCAL(),
> which means that it has the same problem.

Added a few checks to protect the static field from non-main-world access. It
seems to be working and the code is definitely cleaner than before.
PTAL

[proberge, 2016-01-29 18:00:48]

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp
(right):

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:49: ,
m_originWorld(PassRefPtr<DOMWrapperWorld>(world.originWorld()))
On 2016/01/21 10:16:15, haraken wrote:
> 
> PassRefPtr<DOMWrapperWorld>() wouldn't be needed.

V8AbstractEventListener shouldn't have a RefPtr<DOMWrapperWorld> m_originWorld?
Don't we want to keep the reference so it doesn't get deleted?

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1615523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:245: if
(v8::Isolate::GetCurrent()->InContext()) {
On 2016/01/21 10:16:15, haraken wrote:
> 
> v8::Isolate::GetCurrent() is deprecated. Use toIsolate(elementDocument)
instead.
> 
> BTW, do we really need the InContext check?

Where is the deprecation notice? Neither api.cc nor v8.h mention that it is
deprecated.

We do need the "in context" check. I ran into some crashes without it.

[esprehn, 2016-01-30 11:01:59]

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

[esprehn, 2016-01-30 11:01:59]

I don't think this works, if you append an iframe it'll run script in the main
world, and async so it'll not be under your Scope object.

ex. <iframe srcdoc="<script>...</script>">

or html imports, or create some DOM using XMLHttpRequest's responseXML and then
insert it into the page.

This doesn't catch any of those. I'm really not happy about this feature, it
gives a false sense of security that we somehow know an extension is the creator
of an Element, but it's actually fuzzy and easy to get around.

[haraken, 2016-01-30 14:25:39]

On 2016/01/30 11:01:59, esprehn wrote:
> I don't think this works, if you append an iframe it'll run script in the main
> world, and async so it'll not be under your Scope object.
> 
> ex. <iframe srcdoc="<script>...</script>">
> 
> or html imports, or create some DOM using XMLHttpRequest's responseXML and
then
> insert it into the page.
> 
> This doesn't catch any of those. I'm really not happy about this feature, it
> gives a false sense of security that we somehow know an extension is the
creator
> of an Element, but it's actually fuzzy and easy to get around.

Yeah, agreed. The "stack of worlds" is doing some strange things and
conceptually it doesn't make sense.

[esprehn, 2016-02-10 01:55:35]

ex.

content script does:

var s = document.createElement('script');
s.textContent = 
  "Promise.resolve().then(() => {
    "var xhr = new XMLHttpRequest(); " +
    "xhr.open('GET', 'data:text/html,<div>'); " +
    "xhr.responseType = 'document'; " +
    "xhr.onload = () => { document.body.appendChild(xhr.response); }"
  "}";
document.body.appendChild(s);

you could also use a MutationObserver, or many other ways to run async scripts
outside your scope. You could also probably use srcdoc which gets parsed async.

[proberge, 2016-02-10 17:47:41]

On 2016/02/10 01:55:35, esprehn wrote:
> ex.
> 
> content script does:
> 
> var s = document.createElement('script');
> s.textContent = 
>   "Promise.resolve().then(() => {
>     "var xhr = new XMLHttpRequest(); " +
>     "xhr.open('GET', 'data:text/html,<div>'); " +
>     "xhr.responseType = 'document'; " +
>     "xhr.onload = () => { document.body.appendChild(xhr.response); }"
>   "}";
> document.body.appendChild(s);
> 
> you could also use a MutationObserver, or many other ways to run async scripts
> outside your scope. You could also probably use srcdoc which gets parsed
async.

The promise + xhr + onload example seems to be covered by the
V8AbstractEventListener scope. (the xhr.onload event listener created during the
MicroTask does get the isolated origin world).
However, MutationObserver indeed appears to not be covered by the code and would
require adding the private field and OriginWorldScope code to
V8MutationCallback.

I understand that we want to avoid adding this complexity to every single way to
run async scripts. Do you know of a place in the script execution chain that
"narrows" enough to cover most cases? As a bad example, we could have each
v8::Function keep track of which world created it.