postMessage: v8::Context used at deserialization differs from the one used in script |
||||||
Issue descriptionTracking for issue discovered when implementing limitations on structured cloning. When posting to an iframe, the context used at deserialization differs from the context the object ends up being used in.
,
Mar 22 2017
Script can certainly choose to do this. At present I believe deserialization of MessageEvent data happens in the current context (i.e. the context in which the data getter function is defined). In ordinary use that should be the context in which the MessageEvent was created and dispatched. You can, however, pass either the MessageEvent or the deserialized data to some other same-origin context, because you have direct access to such script contexts.
,
Mar 23 2017
If I understand this correctly, the current behavior is equivalent to a handle hand-over - it's the same object, in the same context. Do we want the current behavior, or do we rather want to rehydrate in the destination context? Added Andreas for a JS language perspective.
,
Mar 24 2017
and +Jochen, maybe he has an insight into this, too (specifically, my last question)
,
Mar 24 2017
Can you provide an example that demonstrates this issue?
,
Mar 24 2017
See this CL: https://codereview.chromium.org/2749503002/ specifically, third_party/WebKit/LayoutTests/http/tests/wasm/wasm_local_iframe_test.html
,
Mar 24 2017
Ok, and what is the context you expect to be used?
,
Mar 24 2017
The context of the recipient of the postMessage.
Here's more data: currently, we install a symbol in the native context. We use this symbol for tagging ("branding") wasm module objects. It appears there's a different native context object in the destination frame, compared to the sending one. (see v8/src/wasm/wasm-js.cc, look for wasm_module_sym and also HasBrand).
When we make a new wasm module object as result of deserialization, we do so (as it appears) in the sender's context, meaning that the value of wasm_module_sym is used from that context. When we call instantiate, we check if the wasm_module_sym is present - by comparing with the one we have in the current context (which, now, is the receiver's context). So instantiation fails, because the wasm_module_sym values are different.
I don't know what the right thing is here: should deserialization happen in the destination context, or should wasm_module_sym be rather associated with the isolate - or something else?
,
Mar 24 2017
Are you sure this is what causes the failure? Note that in your test, the message handler you install on the iframe is a function defined in the main frame. Maybe the module is correctly deserialized in the iframe's context, however, you fail to instantiate it in the context of the main frame.
,
Mar 24 2017
I'm quite sure this (the difference in contexts) is what causes the failure, and also on the identity of the contexts: I set a breakpoint in the serializer and deserializer and compared the value of the context pointer (and got lucky that there was no gc). Did same at instantiation time. The first 2 were the same values, the latter wasn't (they also have a hash, come to think of it, which made me trust what I saw even more so - i.e. gc or not, I assume the hash is constant for the lifetime of an object). Now, maybe what you pointed out, that the message handler is defined in the main frame, may be the problem. What's the right way to set this up - any pointers would be great! Thanks!
,
Mar 24 2017
In a test like this, of expect the message handler to be defined in a separate file, or via the srcdoc attribute of the iframe. Another thing that could be happening here is that the data attribute is defined as a lazy accessor in which case there's currently a bug in v8 where we don't change the context correctly before invoking the getter
,
Mar 24 2017
Interesting - do you know the bug by any chance, if it turns out it's the same issue (and also to see its symptoms and see if they match here). I'll also do what you suggested with the message handler. Taking a step back, it sounds like we do believe that the deserialized object should be contextualized in the recipient's context (seemed intuitively desirable to me, otherwise why not just pass the reference when in same isolate - but worth doublechecking); and, in that case, we're doing the right thing in wasm by having per-context definitions of the wasm symbol we use to determine if an object is a wasm module or not.
,
Mar 24 2017
I'm not sure I entirely understand was this symbol is used for and what you're trying to achieve here? Yuki, is there a bug on file for the lazy accessors not entering the context?
,
Mar 24 2017
It seems I didn't file a bug. I'll file it next week (sorry, I'm running to the last train now).
,
Mar 24 2017
To Jochen's question at comment#13: Andreas may provide more authoritative answer. My understanding is that we want to type-check an object ("is it a WebAssembly.Module object"), and do so by tagging it with a value.
,
Mar 24 2017
You should use an separate instance type for this
,
Mar 27 2017
Just FYI, I filed a bug at https://crbug.com/v8/6156 . re: #11, if the property is a *data* property, then it shouldn't be a lazy accessor because it's not an accessor property, I think? If the property is a data property, then the current context is expected to *not* change because there is no JS function running, i.e. the current context (in C++ callback function) is the caller's context.
,
Apr 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/16aa23bd2b41c31151e42d612f10d683defaa5f1 commit 16aa23bd2b41c31151e42d612f10d683defaa5f1 Author: mtrofin <mtrofin@chromium.org> Date: Wed Apr 19 15:17:21 2017 [wasm] Enable local iframe test. It appears the original reason this bug was disabled does not reproduce anymore; also fixed unrelated test bug (it's unrelated to the original reason the test was disabled, because, originally, instantiation wasn't working full stop because of differences in contexts. Test bug just wasn't receiving data correctly.) BUG= chromium:700788 Review-Url: https://codereview.chromium.org/2822193003 Cr-Commit-Position: refs/heads/master@{#465610} [modify] https://crrev.com/16aa23bd2b41c31151e42d612f10d683defaa5f1/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/16aa23bd2b41c31151e42d612f10d683defaa5f1/third_party/WebKit/LayoutTests/http/tests/wasm/resources/frame.html [modify] https://crrev.com/16aa23bd2b41c31151e42d612f10d683defaa5f1/third_party/WebKit/LayoutTests/http/tests/wasm/wasm_local_iframe_test.html
,
May 3 2017
Issue doesn't repro anymore, and the blocked test could be re-enabled. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by mtrofin@chromium.org
, Mar 22 2017Status: Available (was: Untriaged)