webview crashes when used from two different bundles
httnn opened this issue · 30 comments
i'm building two different "formats" of my app (they're audio plug-ins) as separate plugin bundles. when loading both bundles together in a host application, both of them crash simultaneously. the following unwrap() seems to be causing the crash: https://github.com/ryanmcgrath/cacao/blob/trunk/src/webview/class.rs#L194
if i've understood correctly, this is just where the class in question gets declared (not even initialised), so might the crash happen when trying to declare the class when it's already declared? any other ideas?
Huh, looks like that was never updated to use 'load_or_register_class'. You can see view/appkit.rs for an example of how to swap it.
That newer method does a check to see if the class already exists. Across two bundles it might still fail, in which case we can add a check in there to accommodate - but if you don't mind swapping and trying that first it'd be helpful.
(The issue is that the class registration once-token check happens on the Rust side, but the bundles are using the same ObjC runtime, and so the subclass attempts to get created twice since the Rust code is isolated and doesn't know about one another - my best guess at least, on mobile right now)
still crashes, but now the error is: thread 'unnamed' panicked at 'Subclass of type RSTWebViewDelegate_NSObject could not be allocated.':
this is what the code looks like now:
pub fn register_webview_delegate_class<T: WebViewDelegate>() -> *const Class {
load_or_register_class("NSObject", "RSTWebViewDelegate", |decl| unsafe {
decl.add_ivar::<usize>(WEBVIEW_DELEGATE_PTR);hope i swapped it correctly!
Hmmm, yeah, so this use-case more or less means that the way subclass handling is done needs to be rewritten. I'm stuck on a train at the moment and can look into this for a bit.
Would you give the feat/change-objc-classdec branch a whirl? I refactored the class loading/register logic to check the runtime if there's no record held for it, and updated the Webview component to route through that method as well. I think this should solve your issue, but lemme know~
(The browser and todos_list examples both run without issue, but I don't have a multiple-bundle setup such as yours to test with at the moment)
that was quick! it's still crashing unfortunately, the error being the same as above:
thread 'unnamed' panicked at 'Subclass of type RSTWebViewDelegate_NSObject could not be allocated.': /Users/max/code/cacao/src/foundation/class.rs:134
btw, my code is public at https://github.com/maxjvh/nih_plug_webview in the example folder but setting up an environment to load both bundles is a bit involved (requires downloading a DAW and setting it up). can guide you through it tho if you're interested!
Huh, so this is an odd one. As far as I understand this all, attempting to get a class in Objective-C will return nil if the class can't be created - i.e, if the name exists already. (in objc_allocateClassPair)
It appears that the attempts to check with the runtime (via getClass) don't return the existing class, but the ClassDecl fails presumably due to the name existing as a class (unless there's some nuance of bundles that I'm missing here...).
As an aside, I think it might be prudent for cacao to also add on a semver to the class definitions that get injected - if plugins share the same visibility of everything in the runtime, then it might be worth doing it to sandbox changes.
Well, while I debug this, one workaround would be to just set a custom NAME on your webview delegate depending on how the bundle is being built (a feature flag or something). It's definitely not ideal, but I'd probably need to sit down and debug this further to fix it the "right" way - I'll keep this open to track it in the meantime.
(I pushed a change to the feat/change-objc-classdec branch that'd allow this, so hope it helps for now - it's probably worth doing on that trait anyway, so the change is worth it)
(I figure it's also worth tagging @madsmtm for perspective given they probably deal with the intricacies of the ObjC runtime than most people these days, lol)
Hmm, haven't really dealt much with bundles (from what I understand they're basically Apple's fancy dynamic libraries), but from what I understand you're trying to load two bundles which contain a class with the same name? Is this even supported in Objective-C proper?
It appears that the attempts to check with the runtime (via
getClass) don't return the existing class, but theClassDeclfails presumably due to the name existing as a class (unless there's some nuance of bundles that I'm missing here...).
As a fix, you could probably do something like:
let n: usize = 0;
loop {
let name = format!("{}_{}_{}", subclass_name, superclass_name, n);
if let Some(decl) = ClassDecl::new(name, superclass) {
// Initialize class
return decl;
}
n += 1; // Or a random number, if you want to avoid pathological cases
}(Though of course the class could also fail to allocate in OOM situations, so this is a bit brittle).
Yeah, I contemplated doing this... but it feels like ignoring the root cause (which I still don't understand, frankly). :(
Hmm, haven't really dealt much with bundles (from what I understand they're basically Apple's fancy dynamic libraries), but from what I understand you're trying to load two bundles which contain a class with the same name? Is this even supported in Objective-C proper?
the details on how bundles work in correlation with Objective-C is also a mystery to me unfortunately. i haven't heard of a crash like this ever before though, so can't be too common. i wonder if this might even be because of some kind of missing bundle build option..? like "wrap declared classes in bundle namespace" or something lol. could dig around a little.
Well, while I debug this, one workaround would be to just set a custom NAME on your webview delegate depending on how the bundle is being built
this seems very reasonable, thanks!
specifying different class names for different bundles seemed to work for the delegate class, wohoo! but now it's failing when trying to declare the webview class: let decl = ClassDecl::new("RSTWebView", superclass).unwrap(); 😞
Heh, I guess that's not surprising... I wonder if there's some logic towards add a bundled feature flag, and then it just appends a _{bundle} to the subclass name? That's still duplication, but ultimately it shouldn't be that bad I don't think... since this is somewhat of a corner case, and it's at best just doing duplication per bundle.
(I can update the branch for this in a bit if you'd wanna try it out)
I also think there has to be something with associating bundles and classes, since bundleForClass: exists.
@maxjvh Feel free to pull that branch and give it a whirl; the attempt is very basic so feel free to tweak if you find something I missed, but the examples run with it - if there is a bundle ID for the current running process, it will take that and use it in the injected subclass name.
I would want to clean this up and look more into it before merging this, but your setup is the most ideal for testing it as a solution I think.
(A solution to a problem that I still think I'm missing entirely...)
btw, this is how a very popular and widely used framework for building these plug-ins in various formats seems to be doing it: https://github.com/juce-framework/JUCE/blob/965d0ca4be178c4a0000b116d460e15c30311992/modules/juce_core/native/juce_mac_ObjCHelpers.h#L369 so it essentially just creates random names 😅
Ha! Well that makes me feel slightly less like a complete moron... I think we can do better than randomized (I really prefer to keep them readable for debugging purposes), but that's actually super helpful context so thanks for flagging it!
(Oh, I forgot to also update the WebView class registration fn to use load_or_register, so that wouldn't have hit... updated/tested/pushed~)
getting closer but: thread 'unnamed' panicked at 'Subclass of type CacaoWebView_WKWebView could not be allocated.': <omitted>/cacao/src/foundation/class.rs:161
just to make sure, the bundle identifier is read from the Info.plist file, right?
Huh, well that's interesting - it looks like that class name isn't getting the bundle ID applied, which means [NSBundle mainBundle].bundleIdentifier is returning nil... but I've offhand go no clue why that would return nil if it's in a valid bundle with an Info.plist.
At this point I am growing inclined to just copy what JUCE does, or take something akin to mads approach further up in this thread. What a weird issue, not something I ever expected to be debugging.
I'm a bit busy tonight but poke me if I forget about this tomorrow?
it was actually still only logging the superclass and subclass names but i fixed that and turns out that the mainBundle ID is the one of the host application 🙃 so might be that it's not even possible to get the plug-in bundle ID when running inside a host app. seems like JUCE might've had a reason for randomising the bundle name!
anyway, thanks a lot for your help on this! it's already more than i could ask for.
with a random class name it indeed works. this is what i tried it with in foundation/class.rs:
let objc_subclass_name = format!("{}_{}_{:x}", subclass_name, superclass_name, rand::random::<i64>());
using the rand crate.
Alright, I updated this branch to do this - we'll now generate and append a random u64 to all subclass names. I think it might still be worth writing some logic here to repeatedly try to declare a new subclass name rather than panic if one can't be declared... i.e, this should remove something like 99% of the issues, but it still feels like there might be a dangling thing to consider here.
I think my final question comes down to: is the only reason class declaration/allocation would return nil that the name is already registered/can't be registered? If so then I think the repeatedly try logic is fine, otherwise I guess I'd be wary of masking another issue.
can confirm that the branch is now working, but don't know in which other cases allocating a class might fail. looks like Apple's documentation isn't super helpful either 😕
can confirm that the branch is now working
Cool! I'll merge these changes next chance I get, but there's some other PRs that I've been horribly neglectful of that I'd like to get to first - so if you're cool to use that branch for now...
Glad we got to the bottom of this, and thanks for the odd use-case to test against!
definitely cool with that! thanks for your help 💜
Merged via #67.