101725 2012-11-09 01:33:46 -0800 [V8] Inspector does not callback IDB methods in context 2012-12-18 00:50:27 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 101573 0 dcarney webkit-unassigned alecflett pfeldman vsevik webkit.review.bot yurys oldest_to_newest 762702 0 dcarney 2012-11-09 01:33:46 -0800 [V8] Inspector does not callback IDB methods in context 762703 1 173236 dcarney 2012-11-09 01:35:29 -0800 Created attachment 173236 Patch 762706 2 dcarney 2012-11-09 01:40:08 -0800 This is a hack in the style of the previous idb hack. It should be done properly, but I'm not sure how to do that here. All three points I injected the context have stack traces similar to: WebCore::IDBOpenDBRequest::create(WebCore::ScriptExecutionContext*, WTF::PassRefPtr<WebCore::IDBAny>, WTF::PassRefPtr<WebCore::IDBDatabaseCallbacksImpl>, long) at IDBOpenDBRequest.cpp:42 WebCore::IDBFactory::openInternal(WebCore::ScriptExecutionContext*, WTF::String const&, long, int&) at IDBFactory.cpp:130 WebCore::IDBFactory::open(WebCore::ScriptExecutionContext*, WTF::String const&, int&) at IDBFactory.cpp:138 start at InspectorIndexedDBAgent.cpp:210 WebCore::InspectorIndexedDBAgent::requestDatabase(WTF::String*, WTF::String const&, WTF::String const&, WTF::PassRefPtr<WebCore::InspectorBackendDispatcher::IndexedDBCommandHandler::RequestDatabaseCallback>) at InspectorIndexedDBAgent.cpp:669 WebCore::InspectorBackendDispatcherImpl::IndexedDB_requestDatabase(long, WebCore::InspectorObject*) at InspectorBackendDispatcher.cpp:2147 WebCore::InspectorBackendDispatcherImpl::dispatch(WTF::String const&) at InspectorBackendDispatcher.cpp:5660 WebCore::InspectorController::dispatchMessageFromFrontend(WTF::String const&) at InspectorController.cpp:343 WebKit::WebDevToolsAgentImpl::dispatchOnInspectorBackend(WebKit::WebString const&) at WebDevToolsAgentImpl.cpp:565 I see no better place to to the context injection. Finally, I used the main world here but I have no idea if that it the correct thing to do. 763187 3 173236 abarth 2012-11-09 11:23:16 -0800 Comment on attachment 173236 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173236&action=review > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:650 > + v8::Handle<v8::Context> context = document->frame()->script()->mainWorldContext(); Why the main world? 763195 4 dcarney 2012-11-09 11:34:25 -0800 (In reply to comment #3) > (From update of attachment 173236 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173236&action=review > > > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:650 > > + v8::Handle<v8::Context> context = document->frame()->script()->mainWorldContext(); > > Why the main world? I had to choose one. This patch is only meant to fix the crashes and inject the contexts where they more or less need to be. Also, I figured there might be an off chance that the inspector only pays attention to main world IDBs. 770775 5 dcarney 2012-11-19 08:32:27 -0800 I've hit another issue that bug 101573 would have triggered an assertion on. I'd like to move forward with this patch if no one has a better solution. I will, however, change it slightly to only enter a context if it's not currently in one. That will replicate the existing (wrong) behaviour. 770835 6 alecflett 2012-11-19 10:13:32 -0800 dcarney: there's a new abstraction, DOMRequest, that should cover this - we're using it in IDBRequest. Can you use that? (that way it will work with JSC too) 770862 7 dcarney 2012-11-19 10:55:51 -0800 (In reply to comment #6) > dcarney: there's a new abstraction, DOMRequest, that should cover this - we're using it in IDBRequest. Can you use that? (that way it will work with JSC too) Thanks for the update. I'll check it out. 770873 8 dcarney 2012-11-19 11:11:49 -0800 (In reply to comment #6) > dcarney: there's a new abstraction, DOMRequest, that should cover this - we're using it in IDBRequest. Can you use that? (that way it will work with JSC too) That won't work. The problem is not the hackiness of the patch versus having a clean abstraction. No v8 context has been entered higher up the stack. 771009 9 alecflett 2012-11-19 14:03:19 -0800 ah, I see what you mean, nevermind 772661 10 173236 abarth 2012-11-21 02:09:30 -0800 Comment on attachment 173236 Patch Ok. Let's land this so that we can add the ASSERT to world context handle. 772675 11 dcarney 2012-11-21 02:20:17 -0800 (In reply to comment #10) > (From update of attachment 173236 [details]) > Ok. Let's land this so that we can add the ASSERT to world context handle. Ok. This is pretty heinous, though, and could lead to more of the same crashes. We could just disable the failing tests and let it crash in WorldContextHandle 772676 12 173236 webkit.review.bot 2012-11-21 02:20:24 -0800 Comment on attachment 173236 Patch Clearing flags on attachment: 173236 Committed r135367: <http://trac.webkit.org/changeset/135367> 772677 13 webkit.review.bot 2012-11-21 02:20:27 -0800 All reviewed patches have been landed. Closing bug. 774273 14 dcarney 2012-11-23 00:33:30 -0800 added yurys and pfeldman: if one of you two knows who could fix this properly, that would be great. The submitted patch is completely wrong, but removing this code will cause test failures in 4 IDB inspector test. It's possible that this incorrect behaviour is causing this crash: http://code.google.com/p/chromium/issues/detail?id=150737. Ping me directly if you need more information about the problem. 775234 15 173236 yurys 2012-11-26 01:12:59 -0800 Comment on attachment 173236 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173236&action=review > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:675 > RefPtr<DatabaseLoader> databaseLoader = DatabaseLoader::create(document, requestCallback); Why do we have to enter V8 context while calling a native function which by the way accepts ScriptExecutionContext and can enter the context if it need one? It seems completely wrong to me. Looks like we should make DatabaseLoader::create instead not to make assumptions about the v8 context stack, is there any problem with that approach? 775246 16 173236 dcarney 2012-11-26 01:35:05 -0800 Comment on attachment 173236 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173236&action=review >> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:675 >> RefPtr<DatabaseLoader> databaseLoader = DatabaseLoader::create(document, requestCallback); > > Why do we have to enter V8 context while calling a native function which by the way accepts ScriptExecutionContext and can enter the context if it need one? It seems completely wrong to me. Looks like we should make DatabaseLoader::create instead not to make assumptions about the v8 context stack, is there any problem with that approach? if ScriptExecutionContext were enough to determine context, I would be using it. Unfortunately, for isolated worlds, ScriptExecutionContext is simply not enough. It could be that the context needs to be stored somewhere earlier and reused at the points I changed, but since I know nothing about the way inspector works, I can't say what the correct solution here is at all. 793434 17 yurys 2012-12-18 00:50:27 -0800 (In reply to comment #16) > (From update of attachment 173236 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173236&action=review > > >> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:675 > >> RefPtr<DatabaseLoader> databaseLoader = DatabaseLoader::create(document, requestCallback); > > > > Why do we have to enter V8 context while calling a native function which by the way accepts ScriptExecutionContext and can enter the context if it need one? It seems completely wrong to me. Looks like we should make DatabaseLoader::create instead not to make assumptions about the v8 context stack, is there any problem with that approach? > > if ScriptExecutionContext were enough to determine context, I would be using it. Unfortunately, for isolated worlds, ScriptExecutionContext is simply not enough. It could be that the context needs to be stored somewhere earlier and reused at the points I changed, but since I know nothing about the way inspector works, I can't say what the correct solution here is at all. Regardless of inspector code why DatabaseLoader::create depends on the JS context that calls it? Can we implement the check in the JS bindings where control flows from JS to native code? 173236 2012-11-09 01:35:29 -0800 2012-11-26 01:35:05 -0800 Patch bug-101725-20121109103337.patch text/plain 3105 dcarney U3VidmVyc2lvbiBSZXZpc2lvbjogMTM0MDI1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggODQwNGNhNjI0OWJiOWNj OGRmODg5YTRkYTFjYzU2NDIwYjNkOTBkZi4uZGU3NzYyZmFkYTgxOTY4YTVmMmIwYjg0NDYwMDU3 MDhlZmVlY2QzZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEyLTExLTA5ICBEYW4g Q2FybmV5ICA8ZGNhcm5leUBnb29nbGUuY29tPgorCisgICAgICAgIFtWOF0gSW5zcGVjdG9yIGRv ZXMgbm90IGNhbGxiYWNrIElEQiBtZXRob2RzIGluIGNvbnRleHQKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwMTcyNQorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEluc2VydGVkIHY4IGNvbnRleHQgYmVmb3Jl IGNhbGxpbmcgb3V0IHRvIElEQi4KKworICAgICAgICBObyBuZXcgdGVzdHMuCisKKyAgICAgICAg KiBpbnNwZWN0b3IvSW5zcGVjdG9ySW5kZXhlZERCQWdlbnQuY3BwOgorICAgICAgICAoV2ViQ29y ZTo6SW5zcGVjdG9ySW5kZXhlZERCQWdlbnQ6OnJlcXVlc3REYXRhYmFzZU5hbWVzRm9yRnJhbWUp OgorICAgICAgICAoV2ViQ29yZTo6SW5zcGVjdG9ySW5kZXhlZERCQWdlbnQ6OnJlcXVlc3REYXRh YmFzZSk6CisgICAgICAgIChXZWJDb3JlOjpJbnNwZWN0b3JJbmRleGVkREJBZ2VudDo6cmVxdWVz dERhdGEpOgorCiAyMDEyLTExLTA4ICBFdWdlbmUgS2x5dWNobmlrb3YgIDxldXN0YXMuYnVnQGdt YWlsLmNvbT4KIAogICAgICAgICBXZWIgSW5zcGVjdG9yOiBUaW1lbGluZTogIlNlbmQgUmVxdWVz dCIgZXZlbnRzIGFyZSBzaG93biBvdXQgb2Ygb3JkZXIuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi Q29yZS9pbnNwZWN0b3IvSW5zcGVjdG9ySW5kZXhlZERCQWdlbnQuY3BwIGIvU291cmNlL1dlYkNv cmUvaW5zcGVjdG9yL0luc3BlY3RvckluZGV4ZWREQkFnZW50LmNwcAppbmRleCAzMmUxYzdkNGY2 NWVhNzc1MGY1MjAxMzZmZDQ2MWIzYTQzZjJiMmUwLi40YTI4M2ZhMDk1OWVhZjBiMTQxMDQzNmQ2 M2JkZTc5Zjg5N2I4ZjNiIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9pbnNwZWN0b3IvSW5z cGVjdG9ySW5kZXhlZERCQWdlbnQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2luc3BlY3Rvci9J bnNwZWN0b3JJbmRleGVkREJBZ2VudC5jcHAKQEAgLTY0NCw2ICs2NDQsMTQgQEAgdm9pZCBJbnNw ZWN0b3JJbmRleGVkREJBZ2VudDo6cmVxdWVzdERhdGFiYXNlTmFtZXNGb3JGcmFtZShFcnJvclN0 cmluZyogZXJyb3JTdHIKICAgICBJREJGYWN0b3J5KiBpZGJGYWN0b3J5ID0gYXNzZXJ0SURCRmFj dG9yeShlcnJvclN0cmluZywgZG9jdW1lbnQpOwogICAgIGlmICghaWRiRmFjdG9yeSkKICAgICAg ICAgcmV0dXJuOworCisjaWYgVVNFKFY4KQorICAgIHY4OjpIYW5kbGVTY29wZSBoYW5kbGVTY29w ZTsKKyAgICB2ODo6SGFuZGxlPHY4OjpDb250ZXh0PiBjb250ZXh0ID0gZG9jdW1lbnQtPmZyYW1l KCktPnNjcmlwdCgpLT5tYWluV29ybGRDb250ZXh0KCk7CisgICAgQVNTRVJUKCFjb250ZXh0Lklz RW1wdHkoKSk7CisgICAgdjg6OkNvbnRleHQ6OlNjb3BlIGNvbnRleHRTY29wZShjb250ZXh0KTsK KyNlbmRpZgorCiAgICAgUmVmUHRyPElEQlJlcXVlc3Q+IGlkYlJlcXVlc3QgPSBpZGJGYWN0b3J5 LT5nZXREYXRhYmFzZU5hbWVzKGRvY3VtZW50KTsKICAgICBpZGJSZXF1ZXN0LT5hZGRFdmVudExp c3RlbmVyKGV2ZW50TmFtZXMoKS5zdWNjZXNzRXZlbnQsIEdldERhdGFiYXNlTmFtZXNDYWxsYmFj azo6Y3JlYXRlKHJlcXVlc3RDYWxsYmFjaywgZG9jdW1lbnQtPnNlY3VyaXR5T3JpZ2luKCktPnRv U3RyaW5nKCkpLCBmYWxzZSk7CiB9CkBAIC02NTcsNiArNjY1LDEzIEBAIHZvaWQgSW5zcGVjdG9y SW5kZXhlZERCQWdlbnQ6OnJlcXVlc3REYXRhYmFzZShFcnJvclN0cmluZyogZXJyb3JTdHJpbmcs IGNvbnN0IFN0CiAgICAgaWYgKCFpZGJGYWN0b3J5KQogICAgICAgICByZXR1cm47CiAKKyNpZiBV U0UoVjgpCisgICAgdjg6OkhhbmRsZVNjb3BlIGhhbmRsZVNjb3BlOworICAgIHY4OjpIYW5kbGU8 djg6OkNvbnRleHQ+IGNvbnRleHQgPSBkb2N1bWVudC0+ZnJhbWUoKS0+c2NyaXB0KCktPm1haW5X b3JsZENvbnRleHQoKTsKKyAgICBBU1NFUlQoIWNvbnRleHQuSXNFbXB0eSgpKTsKKyAgICB2ODo6 Q29udGV4dDo6U2NvcGUgY29udGV4dFNjb3BlKGNvbnRleHQpOworI2VuZGlmCisKICAgICBSZWZQ dHI8RGF0YWJhc2VMb2FkZXI+IGRhdGFiYXNlTG9hZGVyID0gRGF0YWJhc2VMb2FkZXI6OmNyZWF0 ZShkb2N1bWVudCwgcmVxdWVzdENhbGxiYWNrKTsKICAgICBkYXRhYmFzZUxvYWRlci0+c3RhcnQo aWRiRmFjdG9yeSwgZG9jdW1lbnQtPnNlY3VyaXR5T3JpZ2luKCksIGRhdGFiYXNlTmFtZSk7CiB9 CkBAIC02ODEsNiArNjk2LDEzIEBAIHZvaWQgSW5zcGVjdG9ySW5kZXhlZERCQWdlbnQ6OnJlcXVl c3REYXRhKEVycm9yU3RyaW5nKiBlcnJvclN0cmluZywgY29uc3QgU3RyaW5nCiAgICAgICAgIHJl dHVybjsKICAgICB9CiAKKyNpZiBVU0UoVjgpCisgICAgdjg6OkhhbmRsZVNjb3BlIGhhbmRsZVNj b3BlOworICAgIHY4OjpIYW5kbGU8djg6OkNvbnRleHQ+IGNvbnRleHQgPSBkb2N1bWVudC0+ZnJh bWUoKS0+c2NyaXB0KCktPm1haW5Xb3JsZENvbnRleHQoKTsKKyAgICBBU1NFUlQoIWNvbnRleHQu SXNFbXB0eSgpKTsKKyAgICB2ODo6Q29udGV4dDo6U2NvcGUgY29udGV4dFNjb3BlKGNvbnRleHQp OworI2VuZGlmCisKICAgICBSZWZQdHI8RGF0YUxvYWRlcj4gZGF0YUxvYWRlciA9IERhdGFMb2Fk ZXI6OmNyZWF0ZShkb2N1bWVudCwgcmVxdWVzdENhbGxiYWNrLCBpbmplY3RlZFNjcmlwdCwgb2Jq ZWN0U3RvcmVOYW1lLCBpbmRleE5hbWUsIGlkYktleVJhbmdlLCBza2lwQ291bnQsIHBhZ2VTaXpl KTsKICAgICBkYXRhTG9hZGVyLT5zdGFydChpZGJGYWN0b3J5LCBkb2N1bWVudC0+c2VjdXJpdHlP cmlnaW4oKSwgZGF0YWJhc2VOYW1lKTsKIH0K