Races #18
Merged
Races #18
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
UpdateRender() will destroy the Browser window, which was created due to Navigate causing the addon to see a valid Browser but then destroy it out from under the addon on the Cef thread without it knowing that it is even coming. Instead only UpdateRender() from Navigate() which the addon can overload and observe happening. This also avoids loading the url twice on startup, once from setting the Url and once from the form load afterward.
…-safe
There is only 1 active CefBrowser at any time, created when UpdateRender() calls to
Renderer::BeginRender(). The BeginRender() method will destroy any existing browsers
then make a new one.
The CefBrowser is not known from the addon main thread in BeginRender() as it is created
asynchronously on the Cef UI thread, which then calls to Renderer::OnCreated() once it
exists. We use a semaphore to safely store this CefBrowser in a private field
|Renderer.browser|, and use the same semaphore to access it from the public read-only
field |Renderer.Browser|.
When destroying the browser, we set the |Renderer.browser| private field, using the
semaphore, back to null. This gives us the following invariant:
- If you check that Renderer.Browser is not null on the main thread it will stay
non-null while you are in a task on that thread.
This allows code to do if (Browser != null) { .. use Browser .. } safely. Note that
you can't rely on the variable to stay null, as it is changed to non-null from the
Cef UI thread, but this is not problematic as when it's null there is nothing to use.
Since we're setting the browser back to null on the main thread, we don't need to do
any action in OnBeforeClose().
|
Does this works with multiple window ( |
This returns a List<CefBrowser> instead of storing only a single CefBrowser. However there are some key differences from before to address threading. - The whole list of browsers is not exposed outside of the Renderer class, because browsers after the first in the list can be added *and removed* at any time on the Cef UI thread. - Any use of the list is still guarded by a semaphore, but also any use of a browser that is not the first in the list must also be guarded by the semaphore to prevent it from being destroyed on the Cef thread while it is being used elsewhere. - The list is never null, only empty or non-empty for simplicity to avoid two empty states. - The first browser in the list acts like the single CefBrowser var did. When the first browser is added to the list on the Cef UI thread, it will remain the first browser in the list and not be removed until a navigation which removes it (and all browsers) on the main thread. This means we can still return the first browser in the list outside the class, and expose it as |Browser|.
|
I've put the list back in a way that is thread-safe, please have a look. The commit message explains how it resolves the threading issues. Thanks |
|
Thanks very much! |
Closed
quisquous
pushed a commit
to quisquous/OverlayPlugin
that referenced
this pull request
Nov 6, 2019
Initial Chinese support.
quisquous
added a commit
to quisquous/OverlayPlugin
that referenced
this pull request
Oct 8, 2022
Starting with version 3.0.0, ngrok no longer allows `-config` with a single dash. Co-authored-by: Anssi Mäkinen <anssi.a.makinen@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses race conditions caused on startup and on re-loading a url as described in #17