Broken Local/Persistent abstraction |
||
Issue description
Our API-exposed concepts provide an indirection as follows.
- Persistent 'p' -> [internal global storage] -> JSObject
- Local 'l' -> [internal handle scope storage] -> JSObject
V8 embedders rely on the fact that {Local|Persistent}::IsEmpty() return true for empty handles.
Now, if we get into the state where 'p' or 'l' point to non-null internal storage that then points to nullptr, we will
(a) pollute the handle scopes and global handles further as any early IsEmpty() bailout will fail
(b) not be able to reliable tell whether a Local/Persistent points to an actual object.
I've observed this when patching the following CL [1] when creating a new ScriptValue through the the IDL-generated code in [2].
After the line marked in [2] we have a Local<Value> detailValue pointing to a slot in the internal handle scope storage that points to nullptr.
Aside from pollution of the backing stores this also caused a bug in wrapper tracing where we relied on Persistents returning false for IsEmpty() to be backed by a non-empty heap object.
[1]: https://chromium-review.googlesource.com/c/484162/
[2]: https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8CustomEventInit.cpp?type=cs&q=V8CustomEventInit::toImpl&l=47
,
Apr 26 2017
I assume that this situation always requires a Persistent handle, as a Local doesn't have this kind of reset method With that assumption, shouldn't it be enough to destroy the persistent after Reset()? Probably something the embedder has to do?
,
Apr 26 2017
> I assume that this situation always requires a Persistent handle, as a Local doesn't have this kind of reset method Yes, however in the example mentioned the problem is that we populate a Local<Value> with this weird state in V8 itself. The state the propagates further, but as far as I can see right now, the problem is somewhere buried in !v8Object->Get(context, keys[0].Get(isolate)).ToLocal(&detailValue) Does this make any sense?
,
Apr 26 2017
It was my understanding that such storage should never be updated to point to 0x0, but should always point to a valid heap object (or SMI), even if that object is undefined (or for internal handles, the_hole). The only exception I'm aware of is weak persistent handles, but those should reset the Persistent<T> as part of the weak callback process. How is this state getting created in the first place?
,
Apr 26 2017
Like I described in #0. Basically in [2] the v8::Local<v8::Value> detailValue for some reason is modified into this state. I didn't have time to investigate further. Applying the CL in [1], breaking at that location and then watching the Local should yield in more insights.
,
Apr 27 2017
After some more debugging I figured that this is WAI :) We are talking about Value, which can either be a heap object or a Smi. So we can very well have v8::Local<v8::Value> -> [internal handle scope slot] -> 0 which means that we have a v8::Value of 0. The fix I landed mitigates the issue for Smi 0 but nothing else. I will open a bug about tracing and prepare a fix. |
||
►
Sign in to add a comment |
||
Comment 1 by mlippautz@chromium.org
, Apr 26 2017