Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

App lifecycle separation #1005

Merged
merged 19 commits into from
Sep 16, 2023
Merged

Conversation

SteffenL
Copy link
Collaborator

@SteffenL SteffenL commented Aug 24, 2023

This work more clearly separates application lifecycle from windowing by taking responsibility of managing the app lifecycle only when an existing window isn't passed to the library, and leaves it up to the user when an existing window is passed.

Note that this isn't to say that using an existing window with the library is now fully usable across all platforms, but was it ever?

This is also not about making multiple windows created by the library a key feature but we can remove hinders, align some behaviors across platforms, and perhaps facilitate future work on multi-window support.

The key differences in this work are described below.

Linux:

  • Only call gtk_init_check() before the library creates the window.
  • Exit the event loop when the last window is closed instead of when the first window is closed.

macOS:

  • Reuse registered classes instead of crashing when attempting to register them again. Only the first instance of the webview
    class that creates a window will register classes.
  • Don't overwrite any existing delegate set on the shared NSApplication instance.

Windows:

  • Decouple internal window message handling from run() by using a message-only window. Allows internal messaging when users use their own message loop instead of calling run().
  • Exit from message loop when the last window is closed instead of when the first window is closed.
  • Only perform COM initialization when the library creates the window.
  • Only enable DPI awareness when the library creates the window.

Don't create an NSApplicationDelegate and set it on the shared
NSApplication instance unless the library takes responsibility
of the application lifecycle.
Only perform COM initialization and enable DPI awareness when an
existing window isn't provided.
@ttytm
Copy link
Contributor

ttytm commented Aug 26, 2023

Looks interesting. I'll take a look over the weekend. Thank you for the work!

@SteffenL
Copy link
Collaborator Author

SteffenL commented Aug 26, 2023

On Windows the event loop ends when a single window is closed.

A problem causes the run loop to exit.
@SteffenL
Copy link
Collaborator Author

I've managed to avoid the crashes on macOS but a remaining problem is how the first webview instance's run() call returns during construction of the second instance which then exits the app.

This is turning into a bit of a mess in my opinion.

@SteffenL
Copy link
Collaborator Author

With some more adjustments for macOS...

Screen Shot 2023-08-26 at 14 15 32

@ttytm
Copy link
Contributor

ttytm commented Aug 27, 2023

This looks awesome already. I gave it a shot with the V binding without stopovers. On mac I'm still running into a seg fault as soon as opening the second window. Works on Linux and Windows.

I will try better tomorrow and in the other officially supported langs!

import webview { EventId, JSArgs }

const doc = '<button id="new-window">New window</button>
<script>
	const newWindowButton = document.querySelector("#new-window");
	document.addEventListener("DOMContentLoaded", () => {
		newWindowButton.addEventListener("click", () => {
			window.new_window();
		});
	});
</script>'

fn main() {
	w := webview.create()

	w.set_title('Primary Window')
	w.set_size(480, 320, .@none)
	w.set_html(doc)

	w.bind('new_window', fn [w] (_ EventId, _ JSArgs, _ voidptr) {
		w.dispatch(fn (_ &webview.Webview, _ voidptr) {
			w2 := webview.create()
			w2.set_title('Secondary Window')
			w2.set_size(480, 320, .@none)
			w2.set_html('Secondary Window')
		}, 0)
	}, 0)

	w.run()
}
signal 11: segmentation fault
0   libsystem_platform.dylib            0x000000018ccdaa24 _sigtramp + 56
1   ???                                 0xffff8001006a0eb8 0x0 + 18446603340523114168
2   windows                             0x00000001006a0eb8 _ZN7webview6detail22cocoa_wkwebview_engine19cr
eate_app_delegateEv + 308
3   windows                             0x00000001006a095c _ZN7webview6detail22cocoa_wkwebview_engineC2Eb
Pv + 104
4   windows                             0x00000001006a08d0 _ZN7webview7webviewC2EbPv + 72
5   windows                             0x000000010069f684 _ZN7webview7webviewC1EbPv + 56
6   windows                             0x000000010069f5b4 webview_create + 56
7   windows                             0x00000001006b0e48 webview__create + 64
8   windows                             0x00000001006b0d84 anon_fn_3f8d4b88fc9ce584_webview__webview_void
ptr_547 + 40
9   windows                             0x00000001006ada60 _ZZ16webview_dispatchENK3$_0clEv + 36
10  windows                             0x00000001006ada30 _ZNSt3__18__invokeB6v15006IRZ16webview_dispatc
hE3$_0JEEEDTclclsr3stdE7declvalIT_EEspclsr3stdE7declvalIT0_EEEEOS3_DpOS4_ + 24
11  windows                             0x00000001006ad9e8 _ZNSt3__128__invoke_void_return_wrapperIvLb1EE
6__callIJRZ16webview_dispatchE3$_0EEEvDpOT_ + 24
12  windows                             0x00000001006ad9c4 _ZNSt3__110__function12__alloc_funcIZ16webview
_dispatchE3$_0NS_9allocatorIS2_EEFvvEEclB6v15006Ev + 28
13  windows                             0x00000001006ac9c0 _ZNSt3__110__function6__funcIZ16webview_dispat
chE3$_0NS_9allocatorIS2_EEFvvEEclEv + 28
14  windows                             0x00000001006a7264 _ZNKSt3__110__function12__value_funcIFvvEEclB6
v15006Ev + 68
15  windows                             0x00000001006a7214 _ZNKSt3__18functionIFvvEEclEv + 24
16  windows                             0x00000001006a71c4 _ZZN7webview6detail22cocoa_wkwebview_engine8di
spatchENSt3__18functionIFvvEEEENKUlPvE_clES6_ + 36
17  windows                             0x00000001006a7194 _ZZN7webview6detail22cocoa_wkwebview_engine8di
spatchENSt3__18functionIFvvEEEENUlPvE_8__invokeES6_ + 28
18  libdispatch.dylib                   0x000000018cafc400 _dispatch_client_callout + 20
19  libdispatch.dylib                   0x000000018cb0abf8 _dispatch_main_queue_drain + 928
20  libdispatch.dylib                   0x000000018cb0a848 _dispatch_main_queue_callback_4CF + 44
21  CoreFoundation                      0x000000018cdcbc54 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUE
UE__ + 16
22  CoreFoundation                      0x000000018cd893d4 __CFRunLoopRun + 1992
23  CoreFoundation                      0x000000018cd884b8 CFRunLoopRunSpecific + 612
24  HIToolbox                           0x00000001965dadf0 RunCurrentEventLoopInMode + 292
25  HIToolbox                           0x00000001965dac2c ReceiveNextEventCommon + 648
26  HIToolbox                           0x00000001965da984 _BlockUntilNextEventMatchingListInModeWithFilt
er + 76
27  AppKit                              0x000000018ffaf97c _DPSNextEvent + 636
28  AppKit                              0x000000018ffaeb18 -[NSApplication(NSEvent) _nextEventMatchingEve
ntMask:untilDate:inMode:dequeue:] + 716
29  AppKit                              0x000000018ffa2f7c -[NSApplication run] + 464
30  windows                             0x00000001006a4b20 _ZN7webview6detail4objc6invokeIvPFvvEJP11objc_
objectP13objc_selectorEEET_T0_DpT1_ + 40
31  windows                             0x00000001006a1694 _ZN7webview6detail4objc8msg_sendIvJP11objc_obj
ectP13objc_selectorEEET_DpT0_ + 40
32  windows                             0x000000010069f754 _ZN7webview6detail22cocoa_wkwebview_engine3run
Ev + 60
33  windows                             0x000000010069f70c webview_run + 24
34  windows                             0x00000001006d9654 webview__Webview_run + 24
35  windows                             0x00000001006da8b8 main__main + 252
36  windows                             0x00000001006dac8c main + 88
37  dyld                                0x000000018c953f28 start + 2236

@SteffenL
Copy link
Collaborator Author

Not sure but to me it looks like maybe somewhere in create_app_delegate() a method is called on a null pointer.

@SteffenL
Copy link
Collaborator Author

Make sure that w2 isn't destroyed at the end of the scope of the binding handler. Maybe that's it?

@ttytm
Copy link
Contributor

ttytm commented Aug 27, 2023

Unfortunately no change.

@ttytm
Copy link
Contributor

ttytm commented Aug 27, 2023

Okay works when disabling garbage collection, should have tried that earlier... Thanks for the assistance!

@SteffenL
Copy link
Collaborator Author

It seems like I was incorrect regarding the current behavior on macOS when closing windows. When testing it again, it appears that the run loop does stop, and the app does exit. It just takes a few seconds sometimes.

@SteffenL
Copy link
Collaborator Author

SteffenL commented Aug 30, 2023

The window closing behavior is now aligned on all platforms: run() returns when closing all windows.

It relies on a static variable to track the number of windows.

@ttytm
Copy link
Contributor

ttytm commented Aug 31, 2023

Regarding creating multiple windows on macOS:

Go works well with the PR 🙂:+1:.
In V, building with -gc none will also not cause a segfault anymore. But funnily, this is also the case without the PR. It's possible to create multiple windows without GC on mac. This is not the case for Go.

I tried several approaches so that V users would only require [manualfree] for the affected sections to make an app that uses multiple windows mac compatible. Unfortunately, the only solution I found was a that a entire program needs to be set to manual memory management.

@SteffenL
Copy link
Collaborator Author

This is not the case for Go.

Sorry, can you please clarify this bit? I read this as Go working well except when you disable GC on macOS.

Is there any chance the webview library is being compiled with ARC enabled on macOS? If so then you currently need to disable it just for the webview core library.

@ttytm
Copy link
Contributor

ttytm commented Aug 31, 2023

Yes sure, what I mean is that Go is working well with the PR, just regularly compiled. Using the git head it crashes on macOS. I never fiddled with the garbage collector in go so this is all with the defaults.

I'm working on the V wrapper, so I also added the experience made with it. With V it is already possible to create multiple windows on macOS the git head, as long as not using garbage collection. Of course there are still the life cycle issues you fix here, but it won't crash. With the PR it's still required to use manual memory management in V to not segfault.

IDK if and if yes how it's possible to help the GC better understand.

@SteffenL
Copy link
Collaborator Author

By "git head", I guess you mean the master branch that doesn't currently have the fix?

Frankly I'm not really a Go developer and have never worked with V before, but I would just assume that there's a sensible way to avoid your objects from being destroyed by the GC.

For example this snippet that you shared before:

	w.bind('new_window', fn [w] (_ EventId, _ JSArgs, _ voidptr) {
		w.dispatch(fn (_ &webview.Webview, _ voidptr) {
			w2 := webview.create()
			w2.set_title('Secondary Window')
			w2.set_size(480, 320, .@none)
			w2.set_html('Secondary Window')
		}, 0)
	}, 0)

To me this looks like it'll for sure be GC at some point because w2 goes out of scope and nothing else references it.

I assume you made changes to this example so what does it look like now?

@ttytm
Copy link
Contributor

ttytm commented Aug 31, 2023

By "git head", I guess you mean the master branch that doesn't currently have the fix?

Yes, exactly - latest master, fix not yet merged.

I assume you made changes to this example so what does it look like now?

Yes it changed. I'm at my phone currently on the way to do some light work. When I get the chance I add one or two of the approaches here during the day. Else this night. Thanks already 🙏

@ttytm
Copy link
Contributor

ttytm commented Aug 31, 2023

To share a very short insight at first: The segfault happens as soon as calling webview_create a second time, no matter the scope.

E.g.

// windows.v - (just the file name, it's run on mac 😅)
import webview

fn main() {
	w := webview.create()
	w2 := webview.create()
}
signal 11: segmentation fault
0   libsystem_platform.dylib            0x000000018ccdaa24 _sigtramp + 56
1   windows                             0x0000000100928f2c _ZN7webview6detail22cocoa_wkwebview_engine19create_app_d
elegateEv + 308
2   windows                             0x0000000100928f2c _ZN7webview6detail22cocoa_wkwebview_engine19create_app_d
elegateEv + 308
3   windows                             0x00000001009289d0 _ZN7webview6detail22cocoa_wkwebview_engineC2EbPv + 104
4   windows                             0x0000000100928944 _ZN7webview7webviewC2EbPv + 72
5   windows                             0x00000001009276f8 _ZN7webview7webviewC1EbPv + 56
6   windows                             0x0000000100927628 webview_create + 56
7   windows                             0x00000001009613a4 webview__create + 24
8   windows                             0x00000001009629c8 main__main + 88
9   windows                             0x0000000100962ce4 main + 88
10  dyld                                0x000000018c953f28 start + 2236

An example using dispatch and adding the window into the applications scope:

import webview

const doc = '<button id="new-window">New window</button>
<script>
	const newWindowButton = document.querySelector("#new-window");
	document.addEventListener("DOMContentLoaded", () => {
		newWindowButton.addEventListener("click", () => {
			window.new_window();
		});
	});
</script>'

[heap]
struct App {
mut:
	w  &webview.Webview
	w2 &webview.Webview
}

fn main() {
	mut app := App{
		w: webview.create()
		w2: unsafe { nil }
	}
	app.w.set_title('Primary Window')
	app.w.set_size(480, 320, .@none)
	app.w.set_html(doc)
	app.w.bind_ctx('new_window', fn (e &webview.Event, mut ctx App) {
		e.dispatch_ctx(fn (mut ctx App) {
			ctx.w2 = webview.create()
			ctx.w2.set_title('Secondary Window')
			ctx.w2.set_size(480, 320, .@none)
			ctx.w2.set_html('Secondary Window')
		}, ctx)
	}, app)

	app.w.run()
}

The example above run with the current version of the V library on Linux and Windows. Some other attempts included modifying the wrapper functions in the library to force it into a working version. They didn't work either on mac so I won't go further into them. In the end the only way was to disable garbage collection entirely.

@SteffenL
Copy link
Collaborator Author

I hate to ask something that may seem so obvious but did you see proof that webview.h is the same version as in this PR when compiled with V?

I guess you could just put an #error hello directive in webview.h and if it errors with "hello" then that would prove it.

If that doesn't help then it would be interesting to figure out what's causing the segfault inside create_app_delegate(). Not sure if you're able to place breakpoints here and step through the code, but if not, maybe it could help to put log lines such as std::cout << "something" << std::endl; before each call to the Objective-C runtime inside create_app_delegate() (and you'll need to add #include <iostream> near the other #include directives).

@ttytm
Copy link
Contributor

ttytm commented Aug 31, 2023

That's good steps to come closer to the problem, thank you! @SteffenL

I'll walk the dog and come back to this.

@ttytm
Copy link
Contributor

ttytm commented Aug 31, 2023

I hate to ask something that may seem so obvious but did you see proof that webview.h is the same version as in this PR when compiled with V?

I made sure of this. In fact I just manually copied the webview.h and webview.o files that were created in the PR build that made Go work.

I guess you could just put an #error hello directive in webview.h and if it errors with "hello" then that would prove it.

Adding #error hello to the header file confirmed that.

But! Adding the std::couts, recompiling, adding them to the V lib and not seeing them, did reveal that something is not right.
Deleting the cache in .vmodules did the trick.

Screenshot 2023-09-01 at 01 11 53

So @SteffenL it works. You're the man 💪 👍 !

@SteffenL
Copy link
Collaborator Author

SteffenL commented Sep 1, 2023

The multiwindow.cc example has been removed to narrow down the scope of this PR.

If we're going to fully support this then we also need examples that cover passing an existing window, and that isn't something that worked well across all platforms even before this PR. The library is missing features that are important for managing windows so that shouldn't be the main focus of this PR.

The initial post has been rewritten and describes the changes in more detail.

@SteffenL SteffenL marked this pull request as ready for review September 1, 2023 04:43
@SteffenL SteffenL added the enhancement New feature, enhancement of an existing feature or a request label Sep 1, 2023
@SteffenL SteffenL added this to the v0.11.0 milestone Sep 6, 2023
@SteffenL SteffenL merged commit 8c149b9 into webview:master Sep 16, 2023
27 checks passed
@SteffenL SteffenL deleted the app-lifecycle-separation branch September 16, 2023 19:44
@SteffenL SteffenL linked an issue Sep 17, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, enhancement of an existing feature or a request
Development

Successfully merging this pull request may close these issues.

Spawning multiple windows on macOS doesn't work
2 participants