Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2413583002:
[inspector] added check that context always survives inspected context (Closed)
Description[inspector] added check that context always survives inspected context
Inspected context is created in V8InspectorImpl::contextCreated method and destroyed in V8InspectorImpl::contextDestroyed.
Both methods takes valid v8::Local<v8::Context> handle to the same context, it means that context is created before InspectedContext constructor and is always destroyed after InspectedContext destructor therefore context weak callback in inspected context should be never called.
It's possible only if inspector client doesn't call contextDestroyed which is considered an error.
Therefore CHECK(false) is added into context weak callback to be sure that v8::Context always survives inspected context.
BUG=chromium:652548
R=dgozman@chromium.org
Committed: https://crrev.com/2082afcf3c8afe5002689ff3208d546f5775fb9a
Cr-Commit-Position: refs/heads/master@{#40290}
Patch Set 1 #Patch Set 2 : added check #Patch Set 3 : added check(false) #Patch Set 4 : a #Patch Set 5 : improve comment #Messages
Total messages: 29 (20 generated)
Dmitry, please take a look!
The CQ bit was checked by kozyatinskiy@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 kozyatinskiy@chromium.org
Let's instead do two separate callbacks, and perform a lookup by id in both.
On 2016/10/12 19:13:52, dgozman wrote: > Let's instead do two separate callbacks, and perform a lookup by id in both. We need to store id somewhere and it looks like we've only one place - weak callback parameter but SetWeakCallback method contains a note: * Install a finalization callback on this object. * NOTE: There is no guarantee as to *when* or even *if* the callback is * invoked. The invocation is performed solely on a best effort basis. * As always, GC-based finalization should *not* be relied upon for any * critical form of resource management! And we can leak this small wrapper structure with two integers and two pointers. I think if note say that there is no guarantee as to *when* or even *if* the callback is invoked then we probably wouldn't like to discard in injected script source in second pass callback and should use more reliable contextDestroyed call. WDYT?
As was discussed offline, I removed second pass callback and added additional check into V8InspectorImpl::getContext. Please take a look!
The CQ bit was checked by kozyatinskiy@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.
Description was changed from ========== [inspector] removed SetSecondPassCallback usage from InspectedContext Let's consider InspectedContext lifetime: - create in V8InspectorImpl::contextCreated; - destroy in V8InspectorImpl::contextDestroyed or in second pass of weak callback on context handler. Second destroy place looks redundant. If inspector user doesn't call contextDestroyed then leaked InspectorContext looks like the lesser evil because every arguments of every console.log call will be leaked too. BUG=chromium:652548 R=dgozman@chromium.org ========== to ========== [inspector] added check that context always survives inspected context Inspected context is created in V8InspectorImpl::contextCreated method and destroyed in V8InspectorImpl::contextDestroyed. Both methods takes valid v8::Local<v8::Context> handle to the same context, it means that context is created before InspectedContext constructor and is always destroyed after InspectedContext destructor therefore this callback should be never called. It's possible only if inspector client doesn't call contextDestroyed but than at least all arguments of console.log calls will be retained forever. In this CL CHECK(false) is added into context weak callback to be sure that v8::Context always survives inspected context. BUG=chromium:652548 R=dgozman@chromium.org ==========
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dmitry, please take another look!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [inspector] added check that context always survives inspected context Inspected context is created in V8InspectorImpl::contextCreated method and destroyed in V8InspectorImpl::contextDestroyed. Both methods takes valid v8::Local<v8::Context> handle to the same context, it means that context is created before InspectedContext constructor and is always destroyed after InspectedContext destructor therefore this callback should be never called. It's possible only if inspector client doesn't call contextDestroyed but than at least all arguments of console.log calls will be retained forever. In this CL CHECK(false) is added into context weak callback to be sure that v8::Context always survives inspected context. BUG=chromium:652548 R=dgozman@chromium.org ========== to ========== [inspector] added check that context always survives inspected context Inspected context is created in V8InspectorImpl::contextCreated method and destroyed in V8InspectorImpl::contextDestroyed. Both methods takes valid v8::Local<v8::Context> handle to the same context, it means that context is created before InspectedContext constructor and is always destroyed after InspectedContext destructor therefore context weak callback in inspected context should be never called. It's possible only if inspector client doesn't call contextDestroyed but than at least all arguments of console.log calls will be retained forever. In this CL CHECK(false) is added into context weak callback to be sure that v8::Context always survives inspected context. BUG=chromium:652548 R=dgozman@chromium.org ==========
The CQ bit was checked by kozyatinskiy@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...
Description was changed from ========== [inspector] added check that context always survives inspected context Inspected context is created in V8InspectorImpl::contextCreated method and destroyed in V8InspectorImpl::contextDestroyed. Both methods takes valid v8::Local<v8::Context> handle to the same context, it means that context is created before InspectedContext constructor and is always destroyed after InspectedContext destructor therefore context weak callback in inspected context should be never called. It's possible only if inspector client doesn't call contextDestroyed but than at least all arguments of console.log calls will be retained forever. In this CL CHECK(false) is added into context weak callback to be sure that v8::Context always survives inspected context. BUG=chromium:652548 R=dgozman@chromium.org ========== to ========== [inspector] added check that context always survives inspected context Inspected context is created in V8InspectorImpl::contextCreated method and destroyed in V8InspectorImpl::contextDestroyed. Both methods takes valid v8::Local<v8::Context> handle to the same context, it means that context is created before InspectedContext constructor and is always destroyed after InspectedContext destructor therefore context weak callback in inspected context should be never called. It's possible only if inspector client doesn't call contextDestroyed which is considered an error. Therefore CHECK(false) is added into context weak callback to be sure that v8::Context always survives inspected context. BUG=chromium:652548 R=dgozman@chromium.org ==========
Let's try this and collect all crashes :-) lgtm
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 kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2413583002/#ps80001 (title: "improve comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [inspector] added check that context always survives inspected context Inspected context is created in V8InspectorImpl::contextCreated method and destroyed in V8InspectorImpl::contextDestroyed. Both methods takes valid v8::Local<v8::Context> handle to the same context, it means that context is created before InspectedContext constructor and is always destroyed after InspectedContext destructor therefore context weak callback in inspected context should be never called. It's possible only if inspector client doesn't call contextDestroyed which is considered an error. Therefore CHECK(false) is added into context weak callback to be sure that v8::Context always survives inspected context. BUG=chromium:652548 R=dgozman@chromium.org ========== to ========== [inspector] added check that context always survives inspected context Inspected context is created in V8InspectorImpl::contextCreated method and destroyed in V8InspectorImpl::contextDestroyed. Both methods takes valid v8::Local<v8::Context> handle to the same context, it means that context is created before InspectedContext constructor and is always destroyed after InspectedContext destructor therefore context weak callback in inspected context should be never called. It's possible only if inspector client doesn't call contextDestroyed which is considered an error. Therefore CHECK(false) is added into context weak callback to be sure that v8::Context always survives inspected context. BUG=chromium:652548 R=dgozman@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] added check that context always survives inspected context Inspected context is created in V8InspectorImpl::contextCreated method and destroyed in V8InspectorImpl::contextDestroyed. Both methods takes valid v8::Local<v8::Context> handle to the same context, it means that context is created before InspectedContext constructor and is always destroyed after InspectedContext destructor therefore context weak callback in inspected context should be never called. It's possible only if inspector client doesn't call contextDestroyed which is considered an error. Therefore CHECK(false) is added into context weak callback to be sure that v8::Context always survives inspected context. BUG=chromium:652548 R=dgozman@chromium.org ========== to ========== [inspector] added check that context always survives inspected context Inspected context is created in V8InspectorImpl::contextCreated method and destroyed in V8InspectorImpl::contextDestroyed. Both methods takes valid v8::Local<v8::Context> handle to the same context, it means that context is created before InspectedContext constructor and is always destroyed after InspectedContext destructor therefore context weak callback in inspected context should be never called. It's possible only if inspector client doesn't call contextDestroyed which is considered an error. Therefore CHECK(false) is added into context weak callback to be sure that v8::Context always survives inspected context. BUG=chromium:652548 R=dgozman@chromium.org Committed: https://crrev.com/2082afcf3c8afe5002689ff3208d546f5775fb9a Cr-Commit-Position: refs/heads/master@{#40290} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2082afcf3c8afe5002689ff3208d546f5775fb9a Cr-Commit-Position: refs/heads/master@{#40290} |
