ScriptState used in DOM objects

54 views
Skip to first unread message

Kentaro Hara

unread,
Oct 17, 2016, 10:04:54 PM10/17/16
to platform-architecture-dev, Yuki Shiino
Hi Shiino-san

I think you're now working on making all scripts run on correct browsing context by correcting ScriptStates used in the code base. I have one question.

Currently we use the following programming pattern in a lot of places:

class DOMObject {
  static DOMObject* create(ScriptState* scriptState) {  // Pass in the ScriptState from the binding layer when the DOM object is constructed.
    return new DOMObject(scriptState);
  }

  DOMObject(ScriptState* scriptState) : m_scriptState(scriptState) { } // Save the ScriptState on the DOMObject.

  void someAsynchronousMethod() {
    if (!m_scriptState.contextIsValid())  // If the ScriptState is already detached, bail out.
      return;
    ScriptState::Scope scope(m_scriptState.get());  // Enter the ScriptState.
    ...;
  }

  RefPtr<ScriptState> m_scriptState;
};

Is the programming pattern still valid? I mean, is it valid to save a ScriptState that has constructed the DOM object and use the ScriptState in an asynchronous method of the DOM object later? If it is not valid, what programming pattern are you planning to introduce?

Thanks!


--
Kentaro Hara, Tokyo, Japan

Yuki Shiino

unread,
Oct 18, 2016, 2:50:01 AM10/18/16
to Kentaro Hara, platform-architecture-dev, Yuki Shiino
TL;DR: Yes, it's correct.


Exactly speaking, it's 99% correct.  If you care details, we need to be aware that the spec requires platform objects are associated with a realm, but we don't do this.

Suppose you're calling a callback function F in someAsynchronousMethod() in your example code, and arguments are platform object A and B.

Blink is (conceptually) doing the following:
    wrapperA = toV8(m_scriptState, domObjA);
    wrapperB = toV8(m_scriptState, domObjB);
    F.call(thisObj, wrapperA, wrapperB);
where wrapperA and wrapperB are always associated with m_scriptState (= thisObj's realm).

However, what the spec requires is that domObjA and domObjB (platform objects) are already associated with their own creation realm.  So, it's possible that domObjA's realm is different from domObjB's realm from a point of view of the spec.  It's unlikely, though.  Usually, domObjA's realm and domObjB's realm are the same as thisObj's realm, and we're doing fine.

Except for such pathological cases, your code looks good.

Cheers,
Yuki Shiino


--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jw2nRTsiwcKTHdaZthdmWw7-hjefC-So%3D99_yNWpzdoZg%40mail.gmail.com.

Kentaro Hara

unread,
Oct 18, 2016, 3:24:29 PM10/18/16
to Yuki Shiino, platform-architecture-dev
On Tue, Oct 18, 2016 at 7:49 AM, Yuki Shiino <yukis...@chromium.org> wrote:
TL;DR: Yes, it's correct.

Thanks, I'm happy to hear that :D

One more question. If it's correct to save m_scriptState on the DOM object, it means that the following CHECK must pass.

class DOMObject {
  void someDOMOperation(ScriptState* scriptState) {  // Imagine that DOMObject.someDOMOperation has [CallWith=ScriptState].
    CHECK(m_scriptState == scriptState);  // This CHECK must pass.
    ...;
  }
  RefPtr<ScriptState> m_scriptState;
};

The CHECK is guaranteed to pass because 1) [CallWith=ScriptState] should pass in the relevant ScriptState of the wrapper and 2) the relevant ScriptState of the wrapper is equal to the ScriptState that has constructed the wrapper.

Am I understanding correctly?

 
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

Yuki Shiino

unread,
Oct 19, 2016, 1:57:02 AM10/19/16
to Kentaro Hara, Yuki Shiino, platform-architecture-dev
2016-10-19 4:23 GMT+09:00 Kentaro Hara <har...@chromium.org>:
On Tue, Oct 18, 2016 at 7:49 AM, Yuki Shiino <yukis...@chromium.org> wrote:
TL;DR: Yes, it's correct.

Thanks, I'm happy to hear that :D

One more question. If it's correct to save m_scriptState on the DOM object, it means that the following CHECK must pass.

class DOMObject {
  void someDOMOperation(ScriptState* scriptState) {  // Imagine that DOMObject.someDOMOperation has [CallWith=ScriptState].
    CHECK(m_scriptState == scriptState);  // This CHECK must pass.
    ...;
  }
  RefPtr<ScriptState> m_scriptState;
};

The CHECK is guaranteed to pass because 1) [CallWith=ScriptState] should pass in the relevant ScriptState of the wrapper and 2) the relevant ScriptState of the wrapper is equal to the ScriptState that has constructed the wrapper.

Am I understanding correctly?

If you don't think about Chrome extensions (i.e. if it's the main world), it's correct.

There is a tricky case if we consider an isolated world.

In main world (contextA):
    document.myDomObj;  // a DOM object created in contextA
    // myDomObj's m_scriptState == contextA
In an isolated world, e.g. content script (contextB):
    document.myDomObj;  // a DOM object created in context A, but the wrapper object is created in contextB.
    document.myDomObj.someDomOperation();  // [CallWith=ScriptState] passes the wrapper object's relevant realm, i.e. contextB, which is different from the saved m_scriptState.

Key points here are:
- We have only one m_scriptState per DOM object (C++ object in Blink), but
- We have many wrapper objects (as many as isolated worlds), and [CallWith=ScriptState] passes the wrapper's context's ScriptState.
So there must be a case that they don't match.

This is my understanding.

Cheers,
Yuki Shiino

 

 



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

Kentaro Hara

unread,
Oct 19, 2016, 12:18:56 PM10/19/16
to Yuki Shiino, platform-architecture-dev
Hmm, your understanding sounds correct, but this is really problematic, right? It means that there's a way for an extension to execute an arbitrary script on the context of another extension...

Given that one DOM object can be accessed by wrappers from different isolated worlds, it's not safe to save ScriptState on the DOM object and reuse it later.

Maybe should we try to remove 'RefPtr<ScriptState> m_scriptState' from DOM objects by using more [CallWith=ScriptState]'s and associating Blink's posted tasks with ScriptStates?


 
Cheers,
Yuki Shiino

 

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

Yuki Shiino

unread,
Oct 19, 2016, 11:23:37 PM10/19/16
to Kentaro Hara, Yuki Shiino, platform-architecture-dev
Good catch.  That's true.

Off the top of my head, another option could be to convert a ScriptState to a corresponding ScriptState in that world.  Or to always save a ScriptState of the main world if we do, because extensions are already allowed to access to the main world.

Anyway, I didn't think about extension-to-extension leak.  It's a good point.

Cheers,
Yuki Shiino




 

Kentaro Hara

unread,
Oct 20, 2016, 4:37:49 AM10/20/16
to Yuki Shiino, platform-architecture-dev
Off the top of my head, another option could be to convert a ScriptState to a corresponding ScriptState in that world.  Or to always save a ScriptState of the main world if we do, because extensions are already allowed to access to the main world. 
 
Anyway, I didn't think about extension-to-extension leak.  It's a good point.

Remember that a main-world-to-extension leak is already problematic. Imagine the following case:

class DOMObject {
  DOMObject(ScriptState* scriptState) : m_scriptState(scriptState) { }

  ScriptValue domOperation() {
    return toV8(m_scriptState, someObject, ...)
  }

  RefPtr<ScriptState> m_scriptState;
};

If DOMObject's constructor is called on the main world and domOperation() is called on an isolated world, the isolated world will end up with getting a JS wrapper of the main world. It will provide a way for the isolated world to access the window object of the main world.

The key is that we must guarantee the following facts:

- All main/isolated worlds should share the underlying C++ object.

- However, all JS objects must be completely isolated among main/isolated worlds.

I have no immediate idea for how to fix the ScriptState problem. However, I believe that's something we must fix.

Yuki Shiino

unread,
Oct 20, 2016, 4:40:15 AM10/20/16
to Kentaro Hara, Yuki Shiino, platform-architecture-dev
Totally agreed.


To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jyR6TBL-qoTF5ymcQ7eC_r%3DWX2W0vHT2K2bRumVR-3-pA%40mail.gmail.com.

Kentaro Hara

unread,
Oct 26, 2016, 2:12:57 PM10/26/16
to Yuki Shiino, platform-architecture-dev
OK. I investigated the code base, found a couple of actual security bugs where JS objects can leak between extensions (ask me in private emails if you're interested in), but came up with a way to fix the problem :D

[TL;DR]

Let's make the following two changes:

- Stop using RefPtr<ScriptState> on ScriptWrappable objects.
- Remove [ConstructorCallWith=ScriptState].

I think the above two changes will solve almost all the ScriptState-leak problems.


[Details]

The problem can happen in the following scenario:

class DOMObject {
  DOMObject(ScriptState* scriptState) : m_scriptState(scriptState) { }  // Imagine that the object is constructed in a world 1.

  ScriptValue someMethod() { return toV8(m_scriptState, ...); }  // If this method is called in a world 2, the world 2 can get a JS object of the world 1. Wao!!!

  RefPtr<ScriptState> m_scriptState;
};

The key point is that this becomes a problem only when the DOMObject is constructed and used by different worlds. In other words:

- This becomes a problem for DOMObjects that can be constructed and used by different worlds. The only objects that hit this problem would be ScriptWrappables (i.e., objects that have WebIDL interfaces). ScriptWrappables can be touched by multiple worlds.

- This does not become a problem for DOMObjects that are guaranteed to be constructed and used by the same world. For example, it's fine to use the programming pattern for ScriptPromise, ScriptPromiseResolver, ScheduledAction, WindowProxy, WorkerScriptController, callback functions etc. (If those DOMObjects are constructed and used by different worlds, it means that we're already leaking objects between worlds.)

The above facts mean that we just need to forbid the programming pattern only on ScriptWrappables. Specifically, we just need to:

- Stop using RefPtr<ScriptState> on ScriptWrappables.
- Remove [ConstructorCallWith=ScriptState] (because [ConstructorCallWith=ScriptState] is used to pass in the ScriptState to ScriptWrappable's constructor and cache it on the ScriptWrappable).

There are only <20 ScriptWrappables that are caching RefPtr<ScriptState>, so I think this is fixable.

This will also provide a clear answer to the following frequently asked question:

class DOMObject : public ScriptWrappable {
  DOMObject(ScriptState* scriptState) : m_scriptState(scriptState) { }

  ScriptValue someMethod() {
    /* I want to use ScriptState here, but should I use m_scriptState? Or should we pass in ScriptState by using [CallWith=ScriptState]?
        In the first place, what's a difference between m_scriptState and the ScriptState passed by [CallWith=ScriptState]? */
  }

  RefPtr<ScriptState> m_scriptState;
};

Now the answer is clear: You should not cache RefPtr<ScriptState>. You should pass in ScriptState by using [CallWith=ScriptState].

Does it make sense?




Totally agreed.


Kenichi Ishibashi

unread,
Oct 26, 2016, 7:28:09 PM10/26/16
to Kentaro Hara, Yuki Shiino, platform-architecture-dev
How can we get a correct ScriptState when someMethod() isn't called by bindings layer? For example, how we can fix PerformanceObserver::deliver() ?
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.

Elliott Sprehn

unread,
Oct 26, 2016, 7:40:00 PM10/26/16
to Kenichi Ishibashi, Kentaro Hara, Yuki Shiino, platform-architecture-dev
Can the m_callback store the ScriptState internally? I'm not sure I understand why you ever would want to pass a ScriptState into a callback today, doesn't that mean you can mess up and pass a ScriptState from one world or frame into a callback function from another? Having callbacks be tightly bound to a ScriptState make sense to me.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAPLXX-9UzSZRfnwfhL83KBrK56p0bfBg6x0xHkA6GonSB-W2HQ%40mail.gmail.com.

Kenichi Ishibashi

unread,
Oct 26, 2016, 7:46:54 PM10/26/16
to Elliott Sprehn, Kentaro Hara, Yuki Shiino, platform-architecture-dev
Yes, we can store ScriptState in callbacks. While implementing callback bindings, there was a discussion when to pass ScriptState to callbacks and at that point we chose to pass it when we invoke callbacks. Storing ScriptState in callbacks seems the right fix. I'll prepare a CL. Thanks for your suggestion!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAPLXX-9UzSZRfnwfhL83KBrK56p0bfBg6x0xHkA6GonSB-W2HQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAO9Q3iJ6wjfY2mFGnmhSC_jQ8XLaXGa2duUPgWfeyU3939WbkQ%40mail.gmail.com.

Domenic Denicola

unread,
Oct 26, 2016, 7:57:43 PM10/26/16
to Kenichi Ishibashi, Elliott Sprehn, Kentaro Hara, Yuki Shiino, platform-architecture-dev
On Wed, Oct 26, 2016 at 7:46 PM Kenichi Ishibashi <ba...@chromium.org> wrote:
Yes, we can store ScriptState in callbacks. While implementing callback bindings, there was a discussion when to pass ScriptState to callbacks and at that point we chose to pass it when we invoke callbacks. Storing ScriptState in callbacks seems the right fix. I'll prepare a CL. Thanks for your suggestion!

I might be talking about something else entirely, in which case please excuse me, but we recently fixed up the Web IDL spec in a related area. Per spec, callbacks store one realm (~ ScriptState) at conversion time, called their "callback context". They then use this at invocation time to "prepare to run a callback", but they also use the realm of the JavaScript function object itself, which can be different, to "prepare to run script".

The input to the invoking algorithm indeed does not take a realm in the spec, so it does seem more correct to not pass a ScriptState at invoke time in Blink's code too.

I hope this is helpful and at least somewhat related :)


Does it make sense?




Totally agreed.






 
Cheers,
Yuki Shiino

 

 
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAPLXX-8VarUW7_Uyss%3DhcZL9%2Beh%2BZ_ytC29e%2BonccHc0j8GnRg%40mail.gmail.com.

Kenichi Ishibashi

unread,
Oct 26, 2016, 8:05:26 PM10/26/16
to Domenic Denicola, Elliott Sprehn, Kentaro Hara, Yuki Shiino, platform-architecture-dev
On Thu, Oct 27, 2016 at 8:57 AM, Domenic Denicola <dom...@google.com> wrote:


On Wed, Oct 26, 2016 at 7:46 PM Kenichi Ishibashi <ba...@chromium.org> wrote:
Yes, we can store ScriptState in callbacks. While implementing callback bindings, there was a discussion when to pass ScriptState to callbacks and at that point we chose to pass it when we invoke callbacks. Storing ScriptState in callbacks seems the right fix. I'll prepare a CL. Thanks for your suggestion!

I might be talking about something else entirely, in which case please excuse me, but we recently fixed up the Web IDL spec in a related area. Per spec, callbacks store one realm (~ ScriptState) at conversion time, called their "callback context". They then use this at invocation time to "prepare to run a callback", but they also use the realm of the JavaScript function object itself, which can be different, to "prepare to run script".

The input to the invoking algorithm indeed does not take a realm in the spec, so it does seem more correct to not pass a ScriptState at invoke time in Blink's code too.

I hope this is helpful and at least somewhat related :)
Thank you for your input! I'm not sure I understand "realm" correctly, but it seems that the fix (not passing ScriptState at invoke time) follows the spec as well :)  


Does it make sense?




Totally agreed.






 
Cheers,
Yuki Shiino

 

 
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

Yuki Shiino

unread,
Oct 27, 2016, 5:14:24 AM10/27/16
to Kenichi Ishibashi, Domenic Denicola, Elliott Sprehn, Kentaro Hara, Yuki Shiino, platform-architecture-dev
I'm afraid that there could be some confusion.  Let me describe my understanding.

I think there are four realms related when we're about to invoke a callback function.
a. What realm is the incumbent realm when invoking a callback function?
b. What realm is the current realm when we convert the arguments?  This is important because the conversion may throw an exception which is created in this realm.
c. What realm is the current realm when running a callback function?
d. What realm is each argument associated with?  This is a tough problem for Blink because we're using a model different from the spec's model.

Please correct me if I'm wrong, but I think what Domenic pointed out are:

a. The incumbent realm is determined at step "prepare to run a callback".
Let me skip this one because I don't yet understand the exact step to determine the realm.

b. The current realm used for an exception during the conversions is determined at step "prepare to run script".
This is the relevant realm for the callback function object, i.e. v8::Function::CreationContext() should be used.

c. The current realm used when running a callback function is determined by ECMAScript's [[Call]].
This is the relevant realm for the callback function object, as same as b.

d. The realms associated with each argument are the realm of the creation context of each platform object.  If an argument is not a platform object, no realm is associated (e.g. String and Number are not associated with a realm).

With the spec's model, each platform object is already associated with a realm, and there is no concept of "world".  So, that's all.

With Blink's implementation, we're not directly associating a realm with a platform object, and we added a new concept of "world", so we cannot simply do the same thing that the spec says.  So we won't be 100% correct for this point unless we refactor our codebase drastically.  But we can be 95% correct and safe with the current model.

Our model is to associate all the arguments with a single realm when we're about to call a function.  What realm should we use (as an approximation)?

Options I see now are:
i) The relevant realm for the callback function object.  i.e. we use this single realm for case b, c and d.  In this case, we don't need to save or take a ScriptState at all because we can retrieve the realm from the function object.
ii) The realm used when the callback function object was registered.  In this case, we need to save a ScriptState.  This realm must already have access to the realm i), so no worry about the leak.

Optimistically saying, i) and ii) are the same realm for typical use cases.

bashi@ seems going with ii), but note that you shouldn't use the realm of ii) for case b.

Cheers,
Yuki Shiino


Does it make sense?




Totally agreed.






 
Cheers,
Yuki Shiino

 

 
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAPLXX--%2BWN-wbR%3DCyfnDf1jEbdzFFZ5EBVPf7NYO%2BAw1RVSiHA%40mail.gmail.com.

Kentaro Hara

unread,
Oct 27, 2016, 5:23:34 AM10/27/16
to Yuki Shiino, Kenichi Ishibashi, Domenic Denicola, Elliott Sprehn, platform-architecture-dev
With the spec's model, each platform object is already associated with a realm, and there is no concept of "world".  So, that's all. 
 
With Blink's implementation, we're not directly associating a realm with a platform object, and we added a new concept of "world", so we cannot simply do the same thing that the spec says.  So we won't be 100% correct for this point unless we refactor our codebase drastically.  But we can be 95% correct and safe with the current model.

If you think that a "platform object" in the spec corresponds to a V8 wrapper in our implementation (including a callback function object), we don't need to worry about worlds, right? A V8 wrapper (including a callback function) should not be accessed by multiple worlds. It should be associated with a realm.



Yuki Shiino

unread,
Oct 27, 2016, 5:37:40 AM10/27/16
to Kentaro Hara, Yuki Shiino, Kenichi Ishibashi, Domenic Denicola, Elliott Sprehn, platform-architecture-dev
I'm thinking about a case of a callback function, and in this case, we're running the following code.
    V8CallbackFunction::call(ScriptWrappable* arg1, ScriptWrappable* arg2) {
      arg1_wrapper = toV8(scriptStateX, arg1);
      arg2_wrapper = toV8(scriptStateX, arg2);
      m_callback->Call(arg1_wrapper, arg2_wrapper);
    }
I meant |arg1| and |arg2| as "platform objects", and they're not associated with any realm, and we're associating them with scriptStateX at the step of "converting IDL value to ECMAScript value".

In other words, when converting ECMAScript value to IDL value, we're loosing the associated realm.

Cheers,
Yuki Shiino


To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jzs-ZQGCckm%3DsSs8LFBNKogz8pbhHuO5E0T%3Dd4v-Spnqg%40mail.gmail.com.

Kentaro Hara

unread,
Oct 27, 2016, 5:45:50 AM10/27/16
to Yuki Shiino, Kenichi Ishibashi, Domenic Denicola, Elliott Sprehn, platform-architecture-dev
On Thu, Oct 27, 2016 at 11:36 AM, Yuki Shiino <yukis...@chromium.org> wrote:
I'm thinking about a case of a callback function, and in this case, we're running the following code.
    V8CallbackFunction::call(ScriptWrappable* arg1, ScriptWrappable* arg2) {
      arg1_wrapper = toV8(scriptStateX, arg1);
      arg2_wrapper = toV8(scriptStateX, arg2);
      m_callback->Call(arg1_wrapper, arg2_wrapper);
    }
I meant |arg1| and |arg2| as "platform objects", and they're not associated with any realm, and we're associating them with scriptStateX at the step of "converting IDL value to ECMAScript value".

In other words, when converting ECMAScript value to IDL value, we're loosing the associated realm.

What I'm suggesting is not to think about |arg1| and |arg2| as "platform objects". Alternately, think |arg1_wrapper| and |arg2_wrapper| as "platform objects".

Sorry if I'm still missing your point.



Cheers,
Yuki Shiino


Yuki Shiino

unread,
Oct 27, 2016, 5:53:51 AM10/27/16
to Kentaro Hara, Yuki Shiino, Kenichi Ishibashi, Domenic Denicola, Elliott Sprehn, platform-architecture-dev
Well, anyway, callback functions can be invoked with pointers to ScriptWrappable as arguments, and we have to convert them to the wrapper objects.  Regardless of whatever you called it, we're running the code in previous reply.

Again, our code is like this.
    class DOMObj {
      CallbackFunction* m_callback;
      Member<ScriptWrappable> m_obj1;
      Member<ScriptWrappable> m_obj2;

      void foo() {
        m_callback->Call(m_obj1, m_obj2);
      }
    }
DOMObj is holding pointers to ScriptWrappable, not v8::Object.  Thus, we have to do toV8().

Cheers,
Yuki Shiino

Kentaro Hara

unread,
Oct 27, 2016, 6:14:26 AM10/27/16
to Yuki Shiino, Kenichi Ishibashi, Domenic Denicola, Elliott Sprehn, platform-architecture-dev
On Thu, Oct 27, 2016 at 11:53 AM, Yuki Shiino <yukis...@chromium.org> wrote:
Well, anyway, callback functions can be invoked with pointers to ScriptWrappable as arguments, and we have to convert them to the wrapper objects.  Regardless of whatever you called it, we're running the code in previous reply.

Again, our code is like this.
    class DOMObj {
      CallbackFunction* m_callback;
      Member<ScriptWrappable> m_obj1;
      Member<ScriptWrappable> m_obj2;

      void foo() {
        m_callback->Call(m_obj1, m_obj2);
      }
    }
DOMObj is holding pointers to ScriptWrappable, not v8::Object.  Thus, we have to do toV8().

Yes. The CallbackFunction (or DOMObj) needs to store the two ScriptStates: one is for the "callback context" and the other is for the "realm's setting object" (which is a realm of the JavaScript function). Then we need to set the two ScriptStates as described in the spec and invoke the callback. toV8(m_obj1) should be called after entering the "realm's setting object". (In my understanding, "callback context" is just used as a backup incumbent object.)

That way we can realize what the spec requires, right?

Yuki Shiino

unread,
Oct 27, 2016, 8:21:12 AM10/27/16
to Kentaro Hara, Yuki Shiino, Kenichi Ishibashi, Domenic Denicola, Elliott Sprehn, platform-architecture-dev
Discussed offline.  In summay, Blink's design is different from the spec's one, so no single trick makes that they match.

By the way, "callback context" should be tied with the callback function, not with a DOM object.

Yuki Shiino

unread,
Oct 27, 2016, 8:57:45 AM10/27/16
to Domenic Denicola, Kentaro Hara, Kenichi Ishibashi, Elliott Sprehn, platform-architecture-dev, Yuki Shiino
domenic@, let me confirm the following two things.

1) Platform object's relevant realm is the realm where the platform object was created.
This is same as JS function object's relevant realm.

2) Platform object's relevant realm never changes during the object's lifetime.
Is this correct?  Some part of the spec seems saying it's possible to change, though.

It seems we're in the middle of updating the spec on this part, but the above is expected, right?

I'm currently reading the followings.

The last paragraph:
----
Although at the time of this writing the ECMAScript specification does not reflect this, every ECMAScript object must have an associated Realm. The mechanisms for associating objects with Realms are, for now, underspecified. However, we note that in the case of platform objects, the associated Realm is equal to the object’s relevant Realm, and for non-exotic function objects (i.e. not callable proxies, and not bound functions) the associated Realm is equal to the value of the function object’s [[Realm]] internal slot.


----
The relevant settings object for a non-global platform object o is the environment settings object whose global object is the global object of the global environment associated with o.

Note: The "global environment associated with" concept is from the olden days, before the modern JavaScript specification and its concept of realms. We expect that as the Web IDL specification gets updated, every platform object will have a Realm associated with it, and this definition can be re-cast in those terms. [JAVASCRIPT] [WEBIDL]

Then, the relevant Realm for a platform object is the Realm of its relevant settings object.


1st paragraph:
----
Every platform object is associated with a global environment, just as the initial objects are. It is the responsibility of specifications using Web IDL to state which global environment (or, by proxy, which global object) each platform object is associated with.


3rd paragraph:
----
The global environment that a given platform object is associated with can change after it has been created. 

Cheers,
Yuki Shiino

Domenic Denicola

unread,
Oct 27, 2016, 11:37:30 AM10/27/16
to Yuki Shiino, Domenic Denicola, Kentaro Hara, Kenichi Ishibashi, Elliott Sprehn, platform-architecture-dev
Sorry you ran into this mess. We are indeed trying to update Web IDL to use realms and be clearer about the associated Realm/relevant Realm connection. When you lay out all the definitions like that and how they indirect to each other it looks pretty embarassing :P.

Your 1) is correct.

Regarding 2), I think it is correct that it never changes.You did find a paragraph that says it can, but I think that is actually a Mozilla-specific thing. In Gecko, when you adopt an element from one document in one realm to another document in another realm, the element's realm changes to its new document's realm. In other browsers this is not true. This has been a point of contention for a long time and Mozilla hasn't really shown any signs of changing their mind. Meanwhile, Mozillans have been the editors of Web IDL for a long time, so they coded in some assumptions about that into Web IDL.

So yeah, in practice, in non-Mozilla browsers, an object's associated realm/a platform object's relevant realm never changes.

Yuki Shiino

unread,
Oct 27, 2016, 11:43:40 AM10/27/16
to Domenic Denicola, Yuki Shiino, Domenic Denicola, Kentaro Hara, Kenichi Ishibashi, Elliott Sprehn, platform-architecture-dev
Thanks for the reply.  It's now clear for me, and "the relevant realm never changes" sounds very simple and great.

Cheers,
Yuki Shiino

Kentaro Hara

unread,
Oct 28, 2016, 10:10:08 AM10/28/16
to Domenic Denicola, Yuki Shiino, Domenic Denicola, Kenichi Ishibashi, Elliott Sprehn, platform-architecture-dev
Thanks Domenic!

Let me confirm if I'm understanding things correctly.

Consider the following example:

  iframe1.window.someElement.innerHTML = "<div id=foo></div>";
  iframe2.window.document.body.appendChild(iframe1.window.someElement);
  var div = iframe2.window.document.getElemebtById("foo");

The spec requires that div's realm should be iframe1 because the HTMLDivElement was created in iframe1. However, Blink currently sets div's realm to iframe2 because div's wrapper is created in iframe2.

In other words, the spec requires to set a realm of a platform object when the platform object is created. However, Blink currently sets a realm of a wrapper when the wrapper is created. This is wrong.

Given the current architecture of V8 bindings, it's really hard to fix the behavior. However, in practice it wouldn't be that problematic to set a wrong realm on a wrapper as long as it doesn't leak JS objects between origins and worlds.

Am I understanding correctly?


P.S. Getting back to the original topic of this thread (how to use ScriptStates on DOM objects), the solution I mentioned in #9 is still valid. We should stop caching ScriptStates on ScriptWrappables and remove [ConstructorCallWith=ScriptState].



Domenic Denicola

unread,
Oct 28, 2016, 11:46:05 AM10/28/16
to Kentaro Hara, Yuki Shiino, Domenic Denicola, Kenichi Ishibashi, Elliott Sprehn, platform-architecture-dev
On Fri, Oct 28, 2016 at 10:10 AM Kentaro Hara <har...@chromium.org> wrote:
Thanks Domenic!

Let me confirm if I'm understanding things correctly.

Consider the following example:

  iframe1.window.someElement.innerHTML = "<div id=foo></div>";
  iframe2.window.document.body.appendChild(iframe1.window.someElement);
  var div = iframe2.window.document.getElemebtById("foo");

The spec requires that div's realm should be iframe1 because the HTMLDivElement was created in iframe1. However, Blink currently sets div's realm to iframe2 because div's wrapper is created in iframe2.

In other words, the spec requires to set a realm of a platform object when the platform object is created. However, Blink currently sets a realm of a wrapper when the wrapper is created. This is wrong.

The spec does require that. But as far as I can tell, Blink follows the spec: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4625. Maybe I am not testing the right thing though, and Blink sets the realm to that of frames[1], even though its .__proto__ and .constructor are still that of frames[0]? If so that's really confusing :(.


Given the current architecture of V8 bindings, it's really hard to fix the behavior. However, in practice it wouldn't be that problematic to set a wrong realm on a wrapper as long as it doesn't leak JS objects between origins and worlds.

I'm not sure how problematic it is. The relevant realm is used for a variety of things. For example we are trying to spec that when you fire an event, you use the Event constructor from the relevant Realm of the target. Here are some tests we are working on: https://github.com/w3c/web-platform-tests/pull/4088/files. But as far as I can tell from these tests, Chrome uses the per-spec relevant Realm to determine which Event constructor to use, so now I'm just confused if Chrome follows the spec for relevant Realm, or if it doesn't.

It's probably true that it's not problematic from a security perspective unless it leaks between origins and worlds.

Kentaro Hara

unread,
Oct 28, 2016, 12:05:53 PM10/28/16
to Domenic Denicola, Yuki Shiino, Domenic Denicola, Kenichi Ishibashi, Elliott Sprehn, platform-architecture-dev
On Fri, Oct 28, 2016 at 5:45 PM, 'Domenic Denicola' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:


On Fri, Oct 28, 2016 at 10:10 AM Kentaro Hara <har...@chromium.org> wrote:
Thanks Domenic!

Let me confirm if I'm understanding things correctly.

Consider the following example:

  iframe1.window.someElement.innerHTML = "<div id=foo></div>";
  iframe2.window.document.body.appendChild(iframe1.window.someElement);
  var div = iframe2.window.document.getElemebtById("foo");

The spec requires that div's realm should be iframe1 because the HTMLDivElement was created in iframe1. However, Blink currently sets div's realm to iframe2 because div's wrapper is created in iframe2.

In other words, the spec requires to set a realm of a platform object when the platform object is created. However, Blink currently sets a realm of a wrapper when the wrapper is created. This is wrong.

The spec does require that. But as far as I can tell, Blink follows the spec: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4625. Maybe I am not testing the right thing though, and Blink sets the realm to that of frames[1], even though its .__proto__ and .constructor are still that of frames[0]? If so that's really confusing :(.

Your test works as you expected because you're touching <div> while <div> is in frames[0]. If you comment out that code, the realm of <div> becomes frames[1].


 

Given the current architecture of V8 bindings, it's really hard to fix the behavior. However, in practice it wouldn't be that problematic to set a wrong realm on a wrapper as long as it doesn't leak JS objects between origins and worlds.

I'm not sure how problematic it is. The relevant realm is used for a variety of things. For example we are trying to spec that when you fire an event, you use the Event constructor from the relevant Realm of the target. Here are some tests we are working on: https://github.com/w3c/web-platform-tests/pull/4088/files. But as far as I can tell from these tests, Chrome uses the per-spec relevant Realm to determine which Event constructor to use, so now I'm just confused if Chrome follows the spec for relevant Realm, or if it doesn't.

So, I don't think Chrome follows the spec unfortunately :/


It's probably true that it's not problematic from a security perspective unless it leaks between origins and worlds.

Yeah, in short term, fixing all security issues is our primary goal.

 
 

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
Reply all
Reply to author
Forward
0 new messages