Skip to content
This repository was archived by the owner on Jun 11, 2025. It is now read-only.

Races#18

Merged
hibiyasleep merged 3 commits into
hibiyasleep:masterfrom
danakj:races
Aug 18, 2017
Merged

Races#18
hibiyasleep merged 3 commits into
hibiyasleep:masterfrom
danakj:races

Conversation

@danakj

@danakj danakj commented Aug 16, 2017

Copy link
Copy Markdown

This addresses race conditions caused on startup and on re-loading a url as described in #17

danakj added 2 commits August 16, 2017 14:30
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().
@hibiyasleep

Copy link
Copy Markdown
Owner

Does this works with multiple window (window.open)? when secondary window opens, this.Browser overrided over overlay Browser and user loses control of overlay window. that's why multiple browsers managed in array, which seems dumb but actually works.

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|.
@danakj

danakj commented Aug 17, 2017

Copy link
Copy Markdown
Author

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

@hibiyasleep hibiyasleep merged commit 4c4f120 into hibiyasleep:master Aug 18, 2017
@hibiyasleep

Copy link
Copy Markdown
Owner

Thanks very much!

@hibiyasleep hibiyasleep mentioned this pull request Nov 27, 2017
quisquous pushed a commit to quisquous/OverlayPlugin that referenced this pull request Nov 6, 2019
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants