diff --git a/CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h b/CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h index 9ece880b2b..1a99952071 100644 --- a/CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h +++ b/CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h @@ -26,21 +26,21 @@ namespace CefSharp private: gcroot _callbackRegistry; gcroot^> _javascriptObjects; - gcroot _browserWrapper; + gcroot _javascriptRootObjectWrapper; public: - BindObjectAsyncHandler(RegisterBoundObjectRegistry^ callbackRegistery, Dictionary^ javascriptObjects, CefBrowserWrapper^ browserWrapper) + BindObjectAsyncHandler(RegisterBoundObjectRegistry^ callbackRegistery, Dictionary^ javascriptObjects, JavascriptRootObjectWrapper^ javascriptRootObjectWrapper) { _callbackRegistry = callbackRegistery; _javascriptObjects = javascriptObjects; - _browserWrapper = browserWrapper; + _javascriptRootObjectWrapper = javascriptRootObjectWrapper; } ~BindObjectAsyncHandler() { _callbackRegistry = nullptr; _javascriptObjects = nullptr; - _browserWrapper = nullptr; + _javascriptRootObjectWrapper = nullptr; } bool Execute(const CefString& name, CefRefPtr object, const CefV8ValueList& arguments, CefRefPtr& retval, CefString& exception) override @@ -139,27 +139,16 @@ namespace CefSharp //https://github.com/cefsharp/CefSharp/issues/3470 if (objectCount > 0 && cachedObjects->Count == objectCount && ignoreCache == false) { - if (Object::ReferenceEquals(_browserWrapper, nullptr)) + if (Object::ReferenceEquals(_javascriptRootObjectWrapper, nullptr)) { - exception = "BindObjectAsyncHandler::Execute - Browser wrapper null, unable to bind objects"; + exception = "BindObjectAsyncHandler::Execute - _javascriptRootObjectWrapper null, unable to bind objects"; return true; } auto browser = context->GetBrowser(); - auto rootObjectWrappers = _browserWrapper->JavascriptRootObjectWrappers; - - JavascriptRootObjectWrapper^ rootObject; - if (!rootObjectWrappers->TryGetValue(StringUtils::ToClr(frame->GetIdentifier()), rootObject)) - { -#ifdef NETCOREAPP - rootObject = gcnew JavascriptRootObjectWrapper(browser->GetIdentifier()); -#else - rootObject = gcnew JavascriptRootObjectWrapper(browser->GetIdentifier(), _browserWrapper->BrowserProcess); -#endif - rootObjectWrappers->TryAdd(StringUtils::ToClr(frame->GetIdentifier()), rootObject); - } + JavascriptRootObjectWrapper^ rootObject = _javascriptRootObjectWrapper; //Cached objects only contains a list of objects not already bound rootObject->Bind(cachedObjects, context->GetGlobal()); diff --git a/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp b/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp index a7b2ceb1f3..6ad271d002 100644 --- a/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp +++ b/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp @@ -156,7 +156,7 @@ namespace CefSharp //TODO: JSB: Split functions into their own classes //Browser wrapper is only used for BindObjectAsync - auto bindObjAsyncFunction = CefV8Value::CreateFunction(kBindObjectAsync, new BindObjectAsyncHandler(_registerBoundObjectRegistry, _javascriptObjects, browserWrapper)); + auto bindObjAsyncFunction = CefV8Value::CreateFunction(kBindObjectAsync, new BindObjectAsyncHandler(_registerBoundObjectRegistry, _javascriptObjects, rootObject)); auto unBindObjFunction = CefV8Value::CreateFunction(kDeleteBoundObject, new RegisterBoundObjectHandler(_javascriptObjects)); auto removeObjectFromCacheFunction = CefV8Value::CreateFunction(kRemoveObjectFromCache, new RegisterBoundObjectHandler(_javascriptObjects)); auto isObjectCachedFunction = CefV8Value::CreateFunction(kIsObjectCached, new RegisterBoundObjectHandler(_javascriptObjects)); @@ -220,16 +220,14 @@ namespace CefSharp frame->SendProcessMessage(CefProcessId::PID_BROWSER, contextReleasedMessage); - auto browserWrapper = FindBrowserWrapper(browser->GetIdentifier()); + auto rootObjectWrappers = _jsRootObjectWrappersByFrameId; - //If we no longer have a browser wrapper reference then there's nothing we can do - if (browserWrapper == nullptr) + //If we no longer have a _jsRootObjectWrappersByFrameId reference then there's nothing we can do + if (Object::ReferenceEquals(rootObjectWrappers, nullptr)) { return; } - auto rootObjectWrappers = browserWrapper->JavascriptRootObjectWrappers; - JavascriptRootObjectWrapper^ wrapper; if (rootObjectWrappers->TryRemove(StringUtils::ToClr(frame->GetIdentifier()), wrapper)) { @@ -305,23 +303,24 @@ namespace CefSharp JavascriptRootObjectWrapper^ CefAppUnmanagedWrapper::GetJsRootObjectWrapper(int browserId, CefString& frameId) { - auto browserWrapper = FindBrowserWrapper(browserId); + auto rootObjectWrappers = _jsRootObjectWrappersByFrameId; - if (browserWrapper == nullptr) + if (Object::ReferenceEquals(rootObjectWrappers, nullptr)) { return nullptr; } - auto rootObjectWrappers = browserWrapper->JavascriptRootObjectWrappers; auto frameIdClr = StringUtils::ToClr(frameId); JavascriptRootObjectWrapper^ rootObject; if (!rootObjectWrappers->TryGetValue(frameIdClr, rootObject)) { #ifdef NETCOREAPP - rootObject = gcnew JavascriptRootObjectWrapper(browserId); + rootObject = gcnew JavascriptRootObjectWrapper(); #else - rootObject = gcnew JavascriptRootObjectWrapper(browserId, browserWrapper->BrowserProcess); + auto browserWrapper = FindBrowserWrapper(browserId); + + rootObject = gcnew JavascriptRootObjectWrapper(browserWrapper == nullptr ? nullptr : browserWrapper->BrowserProcess); #endif rootObjectWrappers->TryAdd(frameIdClr, rootObject); } @@ -350,49 +349,6 @@ namespace CefSharp auto name = message->GetName(); auto argList = message->GetArgumentList(); - auto browserWrapper = FindBrowserWrapper(browser->GetIdentifier()); - //Error handling for missing/closed browser - if (browserWrapper == nullptr) - { - if (name == kJavascriptCallbackDestroyRequest || - name == kJavascriptRootObjectResponse || - name == kJavascriptAsyncMethodCallResponse) - { - //If we can't find the browser wrapper then we'll just - //ignore this as it's likely already been disposed of - return true; - } - - CefString responseName; - if (name == kEvaluateJavascriptRequest) - { - responseName = kEvaluateJavascriptResponse; - } - else if (name == kJavascriptCallbackRequest) - { - responseName = kJavascriptCallbackResponse; - } - else - { - //TODO: Should be throw an exception here? It's likely that only a CefSharp developer would see this - // when they added a new message and haven't yet implemented the render process functionality. - throw gcnew Exception("Unsupported message type"); - } - - auto callbackId = GetInt64(argList, 0); - auto response = CefProcessMessage::Create(responseName); - auto responseArgList = response->GetArgumentList(); - auto errorMessage = String::Format("Request BrowserId : {0} not found it's likely the browser is already closed", browser->GetIdentifier()); - - //success: false - responseArgList->SetBool(0, false); - SetInt64(responseArgList, 1, callbackId); - responseArgList->SetString(2, StringUtils::ToNative(errorMessage)); - frame->SendProcessMessage(sourceProcessId, response); - - return true; - } - //these messages are roughly handled the same way if (name == kEvaluateJavascriptRequest || name == kJavascriptCallbackRequest) { @@ -415,27 +371,13 @@ namespace CefSharp auto frameId = StringUtils::ToClr(frame->GetIdentifier()); int64_t callbackId = GetInt64(argList, 0); + //NOTE: In the rare case when when OnContextCreated hasn't been called we need to manually create the rootObjectWrapper + //It appears that OnContextCreated is only called for pages that have javascript on them, which makes sense + //as without javascript there is no need for a context. + JavascriptRootObjectWrapper^ rootObjectWrapper = GetJsRootObjectWrapper(browser->GetIdentifier(), frame->GetIdentifier()); + if (name == kEvaluateJavascriptRequest) { - JavascriptRootObjectWrapper^ rootObjectWrapper; - browserWrapper->JavascriptRootObjectWrappers->TryGetValue(frameId, rootObjectWrapper); - - //NOTE: In the rare case when when OnContextCreated hasn't been called we need to manually create the rootObjectWrapper - //It appears that OnContextCreated is only called for pages that have javascript on them, which makes sense - //as without javascript there is no need for a context. - if (rootObjectWrapper == nullptr) - { -#ifdef NETCOREAPP - rootObjectWrapper = gcnew JavascriptRootObjectWrapper(browser->GetIdentifier()); -#else - rootObjectWrapper = gcnew JavascriptRootObjectWrapper(browser->GetIdentifier(), browserWrapper->BrowserProcess); -#endif - - browserWrapper->JavascriptRootObjectWrappers->TryAdd(frameId, rootObjectWrapper); - } - - auto callbackRegistry = rootObjectWrapper->CallbackRegistry; - auto script = argList->GetString(1); auto scriptUrl = argList->GetString(2); auto startLine = argList->GetInt(3); @@ -480,8 +422,17 @@ namespace CefSharp } else { - auto responseArgList = response->GetArgumentList(); - SerializeV8Object(result, responseArgList, 2, callbackRegistry); + auto callbackRegistry = rootObjectWrapper == nullptr ? nullptr : rootObjectWrapper->CallbackRegistry; + + if (callbackRegistry == nullptr) + { + errorMessage = StringUtils::ToNative("The callback registry for Frame " + frameId + " is no longer available."); + } + else + { + auto responseArgList = response->GetArgumentList(); + SerializeV8Object(result, responseArgList, 2, callbackRegistry); + } } } else @@ -506,8 +457,6 @@ namespace CefSharp } else { - JavascriptRootObjectWrapper^ rootObjectWrapper; - browserWrapper->JavascriptRootObjectWrappers->TryGetValue(frameId, rootObjectWrapper); auto callbackRegistry = rootObjectWrapper == nullptr ? nullptr : rootObjectWrapper->CallbackRegistry; if (callbackRegistry == nullptr) { @@ -616,7 +565,7 @@ namespace CefSharp { auto jsCallbackId = GetInt64(argList, 0); JavascriptRootObjectWrapper^ rootObjectWrapper; - browserWrapper->JavascriptRootObjectWrappers->TryGetValue(StringUtils::ToClr(frame->GetIdentifier()), rootObjectWrapper); + _jsRootObjectWrappersByFrameId->TryGetValue(StringUtils::ToClr(frame->GetIdentifier()), rootObjectWrapper); if (rootObjectWrapper != nullptr && rootObjectWrapper->CallbackRegistry != nullptr) { rootObjectWrapper->CallbackRegistry->Deregister(jsCallbackId); @@ -728,7 +677,7 @@ namespace CefSharp auto callbackId = GetInt64(argList, 0); JavascriptRootObjectWrapper^ rootObjectWrapper; - browserWrapper->JavascriptRootObjectWrappers->TryGetValue(frameId, rootObjectWrapper); + _jsRootObjectWrappersByFrameId->TryGetValue(frameId, rootObjectWrapper); if (rootObjectWrapper != nullptr) { diff --git a/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h b/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h index 38f8da2881..b989c3467a 100644 --- a/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h +++ b/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h @@ -26,6 +26,7 @@ namespace CefSharp gcroot^> _onBrowserCreated; gcroot^> _onBrowserDestroyed; gcroot^> _browserWrappers; + gcroot^> _jsRootObjectWrappersByFrameId; bool _focusedNodeChangedEnabled; bool _legacyBindingEnabled; bool _jsBindingApiEnabled = true; @@ -48,6 +49,7 @@ namespace CefSharp _onBrowserCreated = onBrowserCreated; _onBrowserDestroyed = onBrowserDestroyed; _browserWrappers = gcnew ConcurrentDictionary(); + _jsRootObjectWrappersByFrameId = gcnew ConcurrentDictionary(); _focusedNodeChangedEnabled = enableFocusedNodeChanged; _javascriptObjects = gcnew Dictionary(); _registerBoundObjectRegistry = gcnew RegisterBoundObjectRegistry(); @@ -67,6 +69,17 @@ namespace CefSharp _browserWrappers = nullptr; } + + if (!Object::ReferenceEquals(_jsRootObjectWrappersByFrameId, nullptr)) + { + for each (JavascriptRootObjectWrapper^ rootObject in Enumerable::OfType(_jsRootObjectWrappersByFrameId)) + { + delete rootObject; + } + + _jsRootObjectWrappersByFrameId = nullptr; + } + delete _onBrowserCreated; delete _onBrowserDestroyed; } diff --git a/CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h b/CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h index ef9ef6dfb5..12ce17ba47 100644 --- a/CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h +++ b/CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h @@ -29,18 +29,12 @@ namespace CefSharp private: MCefRefPtr _cefBrowser; - internal: - //Frame Identifier is used as Key - property ConcurrentDictionary^ JavascriptRootObjectWrappers; - public: CefBrowserWrapper(CefRefPtr cefBrowser) { _cefBrowser = cefBrowser.get(); BrowserId = cefBrowser->GetIdentifier(); IsPopup = cefBrowser->IsPopup(); - - JavascriptRootObjectWrappers = gcnew ConcurrentDictionary(); } !CefBrowserWrapper() @@ -51,16 +45,6 @@ namespace CefSharp ~CefBrowserWrapper() { this->!CefBrowserWrapper(); - - if (JavascriptRootObjectWrappers != nullptr) - { - for each (KeyValuePair entry in JavascriptRootObjectWrappers) - { - delete entry.Value; - } - - JavascriptRootObjectWrappers = nullptr; - } } property int BrowserId; diff --git a/CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.cpp b/CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.cpp index 01f6da2326..a9389a9ca9 100644 --- a/CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.cpp +++ b/CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.cpp @@ -18,9 +18,10 @@ namespace CefSharp JavascriptCallbackWrapper^ wrapper = gcnew JavascriptCallbackWrapper(value, context); _callbacks->TryAdd(newId, wrapper); + auto result = gcnew JavascriptCallback(); result->Id = newId; - result->BrowserId = _browserId; + result->BrowserId = context->GetBrowser()->GetIdentifier(); result->FrameId = StringUtils::ToClr(context->GetFrame()->GetIdentifier()); return result; } diff --git a/CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.h b/CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.h index dd0af41f40..b2872055a9 100644 --- a/CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.h +++ b/CefSharp.BrowserSubprocess.Core/JavascriptCallbackRegistry.h @@ -20,14 +20,13 @@ namespace CefSharp //Is static so ids are unique to this process, which is required until #1984 is implemented //and callbacks are disposed of properly between contexts static Int64 _lastId; - int _browserId; ConcurrentDictionary^ _callbacks; internal: JavascriptCallbackWrapper^ FindWrapper(int64_t id); public: - JavascriptCallbackRegistry(int browserId) : _browserId(browserId) + JavascriptCallbackRegistry() { _callbacks = gcnew ConcurrentDictionary(); } diff --git a/CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h b/CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h index 45b94b8ed4..ae2add0112 100644 --- a/CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h +++ b/CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h @@ -57,9 +57,9 @@ namespace CefSharp public: #ifdef NETCOREAPP - JavascriptRootObjectWrapper(int browserId) + JavascriptRootObjectWrapper() #else - JavascriptRootObjectWrapper(int browserId, IBrowserProcess^ browserProcess) + JavascriptRootObjectWrapper(IBrowserProcess^ browserProcess) #endif { #ifndef NETCOREAPP @@ -67,7 +67,7 @@ namespace CefSharp _wrappedObjects = gcnew List(); #endif _wrappedAsyncObjects = gcnew List(); - _callbackRegistry = gcnew JavascriptCallbackRegistry(browserId); + _callbackRegistry = gcnew JavascriptCallbackRegistry(); _methodCallbacks = gcnew Dictionary(); } diff --git a/CefSharp.Test/Javascript/JavascriptCallbackTests.cs b/CefSharp.Test/Javascript/JavascriptCallbackTests.cs index c548fcdb8b..e87e0a9ff4 100644 --- a/CefSharp.Test/Javascript/JavascriptCallbackTests.cs +++ b/CefSharp.Test/Javascript/JavascriptCallbackTests.cs @@ -57,6 +57,7 @@ public async Task ShouldCancelAfterV8ContextChange() Assert.True(callbackExecuteCancelAfterV8ContextResponse.Success); var callbackExecuteCancelAfterV8ContextCallback = (IJavascriptCallback)callbackExecuteCancelAfterV8ContextResponse.Result; var callbackExecuteCancelAfterV8ContextTask = callbackExecuteCancelAfterV8ContextCallback.ExecuteAsync(); + var frameId = browser.GetMainFrame().Identifier; // change V8 context await browser.LoadUrlAsync(CefExample.HelloWorldUrl); @@ -64,7 +65,7 @@ public async Task ShouldCancelAfterV8ContextChange() await Assert.ThrowsAsync(() => callbackExecuteCancelAfterV8ContextTask); var callbackExecuteCancelAfterV8ContextResult = await callbackExecuteCancelAfterV8ContextCallback.ExecuteAsync(); Assert.False(callbackExecuteCancelAfterV8ContextResult.Success); - Assert.StartsWith("Unable to find JavascriptCallback with Id " + callbackExecuteCancelAfterV8ContextCallback.Id, callbackExecuteCancelAfterV8ContextResult.Message); + Assert.StartsWith($"Frame with Id:{frameId}", callbackExecuteCancelAfterV8ContextResult.Message); var callbackExecuteCancelAfterDisposeResponse = await browser.EvaluateScriptAsync("(function() { return new Promise(resolve => setTimeout(resolve, 1000)); })"); Assert.True(callbackExecuteCancelAfterDisposeResponse.Success); diff --git a/CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs b/CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs index c0eb9fea80..3bfbdd13da 100644 --- a/CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs +++ b/CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs @@ -94,8 +94,10 @@ public async Task ShouldWorkWhenUsingCustomGlobalObjectName() [InlineData("cefSharp.isObjectCached('doesntexist')")] public async Task ShouldFailWhenIsObjectCachedCalledWithInvalidObjectName(string script) { - AssertInitialLoadComplete(); + var loadResponse = await Browser.LoadUrlAsync(CefExample.BindingApiCustomObjectNameTestUrl); + Assert.True(loadResponse.Success); + var response = await Browser.EvaluateScriptAsync(script); Assert.True(response.Success);