Skip to content
This repository has been archived by the owner on Mar 30, 2021. It is now read-only.

Fixes issues with plugins not loading in macOS Mojave (issue #668) #670

Merged

Conversation

xrubioj
Copy link
Contributor

@xrubioj xrubioj commented Oct 30, 2018

Workaround for the issue #668.

After some investigation, default user agent is apparently causing Google to serve different HTML. This causes an error (visible in the UI with two "Blocked Plug-In" labels at the bottom of the player). The internal reason is because some problem with Content Security Policy. Quoting the comment from kalkama:

Any updates, since updating to Mojave, I have the blocked plug-in. Further on inspecting the element, all I see is below:

[Error] Refused to load https://play-music.gstatic.com/fe/swf/musicplayer_cp.swf because it does not appear in the object-src directive of the Content Security Policy. [Error] Failed to load resource: Not allowed to authenticate (skyjam, line 0) [Error] Failed to load resource: Not allowed to authenticate (skyjam, line 0)

Given that Safari should use WKWebView (which after some tests fails with the same error) I've concluded that changing the User Agent to simulate Safari could help, and indeed it does.

I've used the User Agent for Safari Version 12.0 (14606.1.36.1.9) running in macOS Mojave 10.14:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Safari/605.1.15

@xrubioj
Copy link
Contributor Author

xrubioj commented Oct 30, 2018

Apparently Travis build if failing for reasons not concerning to this PR.

Also, notice that I've been unable to test the fix in a previous macOS version, as I don't have any available. Maybe an improvement would be the workaround only being applied to Mojave, but more tests will be needed in previous OS versions to see if the forced User Agent causes any issue.

@tjanson
Copy link
Contributor

tjanson commented Oct 30, 2018

Thanks for fixing this!
How did you build the app? I had some trouble:

  • When trying to install with the current Gemfile.lock, I get the same compile error that Travis gets. I then simply installed the current versions (removed the lock file).
  • Xcode complains about some *.pem file being a duplicate, so I removed it in the “Copy Bundle Resources” section.

Radiant Player now builds, opens and plays music fine, but the UI is partly invisible (though still clickable):


screen shot 2018-10-30 at 23 30 37


Did you encounter those problems, and if so, how did you solve them?

@xrubioj
Copy link
Contributor Author

xrubioj commented Oct 31, 2018

Hi Tom. I'm a heavy user of Radiant Player, so I had to try to fix it! :)

About the build, I basically opened the workspace in Xcode (as documentation states: Just be sure to open radiant-player-mac.xcworkspace instead of radiant-player-mac.xcodeproj in order to correctly pull in the dependencies into Xcode.), and the only change I needed to apply was to change the build system to the legacy one (I'm using Xcode 10). This is File -> Workspace Settings... -> set Build System to Legacy System.

I understand that the dependencies in Gemfile are needed to build the website, so I didn't change anything nor didn't try to run it. Now you mention, I may try to fix this as well.

About UI, I want to look at it as it also happened to me. My understanding is that this is due to new Xcode version toolchain changes (looks like Travis is trying to build using Xcode 7.1, while I'm already using Xcode 10). I wanted to create a separated PR for this.

I'll check everything later and see what's the best way to proceed. Any suggestion?

@BarakaAka1Only
Copy link
Contributor

Okay i went ahead and had a look at this matter and with setting the custom useragent via setApplicationNameForUserAgent worked of-course but since this is a Xcode we are talking about adding
Version/12.0
isn't going to work towards the UI as it will error out with
Content Security Policy directive
so since we aren't using a real native browser like Chrome any additional params aren't needed like that because we didn't specify a certain param before. (once that certain param is used than other params i.e Version/12.0 can be used)

You can test this for yourself with

NSString* barakaAgentTest = [webView stringByEvaluatingJavaScriptFromString:@"navigator.userAgent"];
NSLog(@"navigator.userAgent = %@", barakaAgentTest);

my output was

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/605.1.15 (KHTML, like Gecko)

so with just that out we can need en-force Radiant to "use" Safari in our agent as webkit blocks content with the Policy directive blocked-uri as there isn't Safari specified in the user agent so it will assume you never installed Flash

So i put this to the test, i enabled dev tools in the native Safari app and removed Safari in the user agent and this is what i got

image

So yup that's all it needs :)

i would keep the current user agent and push in Safari and leave everything else default

If you want you can test this yourself ;)

(I however didn't get any error with *.pem just done the Legacy thing ;))

Here is a test of the app built with my changes

https://www.dropbox.com/s/ssnyufreplicxl5/Radiant%20Player%20%20Mojave%20Test.zip?dl=0

  • added a check for NSKit
  • radiant player version in user-agent

Happy coding!

BarakaAka1Only added a commit to BarakaAka1Only/radiant-player-mac that referenced this pull request Nov 2, 2018
Thanks to @xrubioj for pointing the his solution

This fix addresses the Blocked Plugin issue and UI properties are
usable than a blanked out UI from @xrubioj original PR
radiant-player#670

Legacy Build System is needed so i enabled it in my workspace

Only if you set `UseModernBuildSystem=NO` in the Workspace setting

```
Xcode 9.3 adds a new IDEWorkspaceChecks.plist file to a workspace’s
shared data, to store the state of necessary workspace checks.
Committing this file to source control will prevent unnecessary
rerunning of those checks for each user opening the workspace.
```
@xrubioj
Copy link
Contributor Author

xrubioj commented Nov 6, 2018

@BarakaAka1Only Works like a charm! Yesterday I used it the whole day. I haven't tested in a version prior to Mojave, but I guess there's no need as you wrapped the User Agent fix in a OS version check.

About the CI failing to compile, I will try to fix it in my branch. Then we can integrate the changes in yours.

Finally, about @tjanson comment on part of the UI not being shown, I did some tests last week, and it could be some kind of race condition: fails in debug build, but debugging and stopping seems to work. Maybe release builds don't expose the same behavior, as @BarakaAka1Only version is working. I'll probably investigate this further and try to do a separated PR if I find some solution.

@jacobwgillespie
Copy link
Member

Nice, thank you for this! I'm going to go ahead and merge this change, and then we can open a followup PR to try to get the CI to build again.

@jacobwgillespie jacobwgillespie merged commit f6730dc into radiant-player:master Nov 6, 2018
@xrubioj
Copy link
Contributor Author

xrubioj commented Nov 6, 2018

Thanks @jacobwgillespie In my opinion, though, I think @BarakaAka1Only PR #671 is better in the sense is more generic, and is safer (as is only targeting Mojave, which is the version we know is failing).

About the CI build, I think I got it. It needs a manual step when compiling in Xcode 10.1 (or maybe because is Mojave), but should work in the CI. I'll commit the change later in a separated PR.

@xrubioj
Copy link
Contributor Author

xrubioj commented Nov 6, 2018

BTW, @jacobwgillespie I've seen PR #672 and #673. Those are already addressing CI build, aren't it? My solution is more conservative, updating less gems.

@jacobwgillespie
Copy link
Member

Yeah, it builds now without any build errors, but the resulting binary crashes immediately for me on Mojave, so something is still likely broken.

@xrubioj
Copy link
Contributor Author

xrubioj commented Nov 6, 2018

Ah, maybe it's because of this:

    eventTap = CGEventTapCreate(kCGSessionEventTap,
                                kCGHeadInsertEventTap,
                                kCGEventTapOptionDefault,
                                mask,
                                event_tap_callback,
                                (__bridge void *)(self));
    [self refreshMikeys];

    if (!eventTap) {
		fprintf(stderr, "failed to create event tap\n");
		exit(1);
    }

I order to test, you have to go to System Preferences -> Privacy -> Accessibility and remove old Radiant Player. First time you run the build, you will get a dialog asking you to open System Preferences to give permission to the app. Then, you will have to run the app again.

That being said, if that's the case for the final, release build, maybe a signature key or something like this has changed, as my understanding is that once you've given permission to the app, as far as the signature is the same, you should need to give it again.

Update: works for me, after removing old Radiant Player in the System Settings, adding it again and running it again. I've tried it by downloading the package from GitHub releases and running it directly from Downloads folder, and also by updating it from the old app. In both cases I had to do the same drill, remove old app from Security Settings, adding it again and finally running the app again. Does this mean there's some kind of signature check? Or is something new from Mojave that need to be addressed? My guess is the later.

Finally, my understanding is that the event tap is used to be able to react to media keys (maybe I'm wrong on this), and thus the app should not require it to work (but this implies changing the way this is initialized, etc.)

@BarakaAka1Only
Copy link
Contributor

@xrubioj Awesome to hear and happy that it is working 👍 @jacobwgillespie thanks for addressing the issue with travis

@xrubioj
Copy link
Contributor Author

xrubioj commented Nov 7, 2018

@jacobwgillespie I've created issue #674 to keep the discussion. I'm already investigating alternatives in order to unregister the app automatically, or to re-register it reusing previous authorization.

@xrubioj xrubioj deleted the mojave-compatibility branch November 18, 2018 11:31
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.

4 participants