Bug 101725 – [V8] Inspector does not callback IDB methods in context

• Component: New Bugs
• Status: RESOLVED FIXED

[Dan Carney]

[V8] Inspector does not callback IDB methods in context

[Dan Carney]

Created attachment 173236 [details]
Patch

[Dan Carney]

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.

[Adam Barth]

Comment on attachment 173236 [details]
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?

[Dan Carney]

(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.

[Dan Carney]

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.

[Alec Flett]

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)

[Dan Carney]

(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.

[Dan Carney]

(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.

[Alec Flett]

ah, I see what you mean, nevermind

[Adam Barth]

Comment on attachment 173236 [details]
Patch

Ok.  Let's land this so that we can add the ASSERT to world context handle.

[Dan Carney]

(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

[WebKit Review Bot]

Comment on attachment 173236 [details]
Patch

Clearing flags on attachment: 173236

Committed r135367: <http://trac.webkit.org/changeset/135367>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dan Carney]

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.

[Yury Semikhatsky]

Comment on attachment 173236 [details]
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?

[Dan Carney]

Comment on attachment 173236 [details]
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.

[Yury Semikhatsky]

(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?