Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781443002:
[Bindings] Specialize WindowDocument symbol in V8PrivateProperty (Closed)
DescriptionSpecialize WindowDocument symbol in V8PrivateProperty to use ScopedPersistent
instead of v8::Eternal.
This specialization is needed to resolve an issue in snapshot feature and
should be a short time fix. So other symbols are kept as-is.
BUG=617892
Review-Url: https://codereview.chromium.org/2781443002
Cr-Commit-Position: refs/heads/master@{#461069}
Committed: https://chromium.googlesource.com/chromium/src/+/74b864441c2ce011435c511f23ea5fab6e23a7d1
Patch Set 1 : . #
Total comments: 11
Patch Set 2 : Specialize createV8Private to use ForApi #
Total comments: 7
Patch Set 3 : Rebase #
Depends on Patchset: Messages
Total messages: 35 (22 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Specialize WindowDocument in V8PrivateProperty BUG= ========== to ========== Specialize WindowDocument in V8PrivateProperty to use ScopedPersistent instead of v8::Eternal. This specialization is needed to resolve an issue in snapshot feature and should be a short time fix. So other symbols are kept as-is. BUG=617892 ==========
Description was changed from ========== Specialize WindowDocument in V8PrivateProperty to use ScopedPersistent instead of v8::Eternal. This specialization is needed to resolve an issue in snapshot feature and should be a short time fix. So other symbols are kept as-is. BUG=617892 ========== to ========== Specialize WindowDocument symbol in V8PrivateProperty to use ScopedPersistent instead of v8::Eternal. This specialization is needed to resolve an issue in snapshot feature and should be a short time fix. So other symbols are kept as-is. BUG=617892 ==========
peria@chromium.org changed reviewers: + haraken@chromium.org, jbroman@chromium.org, yukishiino@chromium.org
PTL.
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM. https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:168: static constexpr char kSymbol[] = "Window#Document"; nit: Window#document? https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:191: // TODO(peria): Do not use specialized hack for Window#Document. nit: Window#document or [Ww]indow.document?
LGTM
https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:191: // TODO(peria): Do not use specialized hack for Window#Document. a special hack https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:192: // This is required to put v8::Private key in snapshot, and it cannot in a snapshot
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:194: ScopedPersistent<v8::Private> m_symbolWindowDocument; Can you explain to me why this is correct? If I understand correctly, the problem is that the context snapshot serializer doesn't walk the eternal handles, right? What causes |m_symbolWindowDocument| to actually match the v8::Private that comes from the snapshot? At first glance, it looks to me like |m_symbolWindowDocument| won't be set, and so after loading the snapshot will point to a different private symbol than the one referred to by the window DOM template. (If these refer to different symbols, the invalidation mechanism won't be correct.)
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I also specialized createV8Private for Window#DocumentCachedAccessor to use v8::Private::ForApi in PS2. PTAL. https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:168: static constexpr char kSymbol[] = "Window#Document"; On 2017/03/27 09:06:18, Yuki wrote: > nit: Window#document? Done. This string was created with V8_PRIVATE_PROPERTY_SYMBOL_STRING(Window, DocumentCachedAccessor), so it should be "Window#DocumentCachedAccessor" to be consistent. https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:191: // TODO(peria): Do not use specialized hack for Window#Document. On 2017/03/27 09:06:18, Yuki wrote: > nit: Window#document or [Ww]indow.document? Done. https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:191: // TODO(peria): Do not use specialized hack for Window#Document. On 2017/03/27 09:49:56, haraken wrote: > > a special hack Done. https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:192: // This is required to put v8::Private key in snapshot, and it cannot On 2017/03/27 09:49:56, haraken wrote: > > in a snapshot Done. https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:194: ScopedPersistent<v8::Private> m_symbolWindowDocument; On 2017/03/27 18:06:56, jbroman wrote: > Can you explain to me why this is correct? If I understand correctly, the > problem is that the context snapshot serializer doesn't walk the eternal > handles, right? Right. The difference between v8::Eternal<> and ScopedPersistent is that ScopedPersistent can be removed, while v8::Eternal<> can't. > What causes |m_symbolWindowDocument| to actually match the v8::Private that > comes from the snapshot? At first glance, it looks to me like > |m_symbolWindowDocument| won't be set, and so after loading the snapshot will > point to a different private symbol than the one referred to by the window DOM > template. (If these refer to different symbols, the invalidation mechanism won't > be correct.) This was my mistake in PS1. We have to use Private::ForApi to get the same v8::Private symbol. V8 caches created Private symbols and ForApi() returns cached one with the key string and isolate.
https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:194: ScopedPersistent<v8::Private> m_symbolWindowDocument; On 2017/03/28 08:10:03, peria wrote: > On 2017/03/27 18:06:56, jbroman wrote: > > Can you explain to me why this is correct? If I understand correctly, the > > problem is that the context snapshot serializer doesn't walk the eternal > > handles, right? > > Right. The difference between v8::Eternal<> and ScopedPersistent is > that ScopedPersistent can be removed, while v8::Eternal<> can't. > > > What causes |m_symbolWindowDocument| to actually match the v8::Private that > > comes from the snapshot? At first glance, it looks to me like > > |m_symbolWindowDocument| won't be set, and so after loading the snapshot will > > point to a different private symbol than the one referred to by the window DOM > > template. (If these refer to different symbols, the invalidation mechanism > won't > > be correct.) > > This was my mistake in PS1. We have to use Private::ForApi to get the same > v8::Private symbol. V8 caches created Private symbols and ForApi() returns > cached one with the key string and isolate. I'm okay to use ForApi as a short-term solution, but we'd like to avoid using this API for future, I think. We need to come up with another idea. https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp (right): https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp:14: static inline v8::Local<v8::String> createV8String(v8::Isolate* isolate, nit: anonymous namespace is preferred. nit: inline may not have much effect because compilers may optimize the code without 'inline' keyword. https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp:38: v8::Local<v8::Private> V8PrivateProperty::createCachedV8Private( nit: You may want a TODO comment here, too, to remove this function.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/28 08:17:59, Yuki wrote: > https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): > > https://codereview.chromium.org/2781443002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:194: > ScopedPersistent<v8::Private> m_symbolWindowDocument; > On 2017/03/28 08:10:03, peria wrote: > > On 2017/03/27 18:06:56, jbroman wrote: > > > Can you explain to me why this is correct? If I understand correctly, the > > > problem is that the context snapshot serializer doesn't walk the eternal > > > handles, right? > > > > Right. The difference between v8::Eternal<> and ScopedPersistent is > > that ScopedPersistent can be removed, while v8::Eternal<> can't. > > > > > What causes |m_symbolWindowDocument| to actually match the v8::Private that > > > comes from the snapshot? At first glance, it looks to me like > > > |m_symbolWindowDocument| won't be set, and so after loading the snapshot > will > > > point to a different private symbol than the one referred to by the window > DOM > > > template. (If these refer to different symbols, the invalidation mechanism > > won't > > > be correct.) > > > > This was my mistake in PS1. We have to use Private::ForApi to get the same > > v8::Private symbol. V8 caches created Private symbols and ForApi() returns > > cached one with the key string and isolate. Is it guaranteed that ForApi() returns the same symbol as the one cached in the serializer?
On 2017/03/28 11:18:10, haraken wrote: > Is it guaranteed that ForApi() returns the same symbol as the one cached in the > serializer? Yes, it seems to be tested in V8's cctest SnapshotCreatorIncludeGlobalProxy. https://cs.chromium.org/chromium/src/v8/test/cctest/test-serialize.cc
lgtm as a short-term fix. https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp (right): https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp:14: static inline v8::Local<v8::String> createV8String(v8::Isolate* isolate, nit: This function is equivalent to v8String; consider using that instead? https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:194: ScopedPersistent<v8::Private> m_symbolWindowDocumentCachedAccessor; Out of curiosity, now that we're using v8::Private::ForApi, the only reason this is not an eternal is that the snapshot creator currently complains in that case, right? (Because it seems to me that we would end up populating it correctly, given the private symbol table is serialized correctly.)
Patchset #3 (id:80001) has been deleted
Rebased on https://codereview.chromium.org/2785943002/ https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp (right): https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp:14: static inline v8::Local<v8::String> createV8String(v8::Isolate* isolate, On 2017/03/30 15:55:55, jbroman wrote: > nit: This function is equivalent to v8String; consider using that instead? Done. https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp:38: v8::Local<v8::Private> V8PrivateProperty::createCachedV8Private( On 2017/03/28 08:17:59, Yuki wrote: > nit: You may want a TODO comment here, too, to remove this function. I'm not confident, but this method can be usable for properties in generated code. https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2781443002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:194: ScopedPersistent<v8::Private> m_symbolWindowDocumentCachedAccessor; On 2017/03/30 15:55:55, jbroman wrote: > Out of curiosity, now that we're using v8::Private::ForApi, the only reason this > is not an eternal is that the snapshot creator currently complains in that case, > right? (Because it seems to me that we would end up populating it correctly, > given the private symbol table is serialized correctly.) Yes, I'm thinking so.
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by peria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yukishiino@chromium.org, jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2781443002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490944833945650,
"parent_rev": "b2131772dfe2cf38aeec5cfb5daefb7ae728452b", "commit_rev":
"74b864441c2ce011435c511f23ea5fab6e23a7d1"}
Message was sent while issue was closed.
Description was changed from ========== Specialize WindowDocument symbol in V8PrivateProperty to use ScopedPersistent instead of v8::Eternal. This specialization is needed to resolve an issue in snapshot feature and should be a short time fix. So other symbols are kept as-is. BUG=617892 ========== to ========== Specialize WindowDocument symbol in V8PrivateProperty to use ScopedPersistent instead of v8::Eternal. This specialization is needed to resolve an issue in snapshot feature and should be a short time fix. So other symbols are kept as-is. BUG=617892 Review-Url: https://codereview.chromium.org/2781443002 Cr-Commit-Position: refs/heads/master@{#461069} Committed: https://chromium.googlesource.com/chromium/src/+/74b864441c2ce011435c511f23ea... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/74b864441c2ce011435c511f23ea... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
