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

WIP: Cookie Impl #10

Merged
merged 15 commits into from
Oct 10, 2023
Merged

WIP: Cookie Impl #10

merged 15 commits into from
Oct 10, 2023

Conversation

Shabinder
Copy link
Contributor

PR building Cookie Management in client using WebView.

@Shabinder
Copy link
Contributor Author

@KevinnZou
I have setup a blueprint for the cookie manager feature, you have been invited to the fork and given access to push changes from your side as well.

I will be having somewhat super packed up coming weeks, so you are more than welcome to take this to end stage in case I might take extended time. Appreciate your efforts in here.

@Shabinder Shabinder marked this pull request as draft September 24, 2023 12:28
@KevinnZou
Copy link
Owner

@KevinnZou I have setup a blueprint for the cookie manager feature, you have been invited to the fork and given access to push changes from your side as well.

I will be having somewhat super packed up coming weeks, so you are more than welcome to take this to end stage in case I might take extended time. Appreciate your efforts in here.

Thank you for providing the blueprint setup. It will be very helpful to me. Last week, we focused on transitioning from JavaFX to the CEF browser. We were able to solve the issue today and we plan to release the new version tomorrow. After that, we will have time to address this issue and aim to implement it by the end of next week. We will keep you updated on our progress. Thank you once again for your contribution!

}
}

expect fun getCookieExpirationDate(expiresDate: Long): String
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be better to just use the kotlinx-datetime library?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can if it increases convenience, if its just this usage and no usage in future then adding additional library doesnt make much sense.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense.

@KevinnZou
Copy link
Owner

@Shabinder I have implemented CookieManager for iOS and Desktop. It works well on iOS, but there is a major issue on Desktop.

The problem is that I am using the CefCookieManager obtained through CefCookieManager.getGlobalManager() to manage cookies. However, it doesn't seem to work. Whether it's setCookie or visitUrlCookies, the method calls have no effect.

After some research, I found an existing issue in the JCEF repository that is related to this problem. @DatL4g, could you please take a look at it? If the issue with the CefCookieManager cannot be resolved, we may need to explore alternative methods to support cookie management on the Desktop platform.

@KevinnZou KevinnZou linked an issue Sep 28, 2023 that may be closed by this pull request
interface CookieManager {
suspend fun setCookie(url: String, cookie: Cookie)
suspend fun getCookies(url: String): List<Cookie>
suspend fun removeAllCookies()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also have a delete cookie function for a URL.

suspend fun removeCookies(url: String)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also WebSettings should have following options.


  - isCookiesEnabled: Boolean
  - isThirdPartyCookiesEnabled: Boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress overall, kudos @KevinnZou

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also have a delete cookie function for a URL.

suspend fun removeCookies(url: String)

I have implemented it for iOS and Desktop in this commit. It seems that Android does not provide a method to delete cookies for specific URLs so I leave it as TODO.

@DatL4g
Copy link
Collaborator

DatL4g commented Sep 28, 2023

@Shabinder Could not push to your PR directly, opened a PR Shabinder#1

@KevinnZou
The GlobalCookieManager works but is handled on the IO thread and may take a while, since it opens a connection to the local cookie database and so on
I added a in-memory save of cookies, may need some improvements but worked as expected on my end.
It will load all cookies added by websites and manually ones

@Shabinder
Copy link
Contributor Author

@Shabinder Could not push to your PR directly, opened a PR Shabinder#1

Have merged it, and sent u an invite for access aswell.

@KevinnZou
Copy link
Owner

@DatL4g I tested your PR locally and it turns out that it still does not work. We can only get the cookie we set before. It is because we have a local cache. But for other cookies sent by the website, we still cannot get it.

@DatL4g
Copy link
Collaborator

DatL4g commented Sep 30, 2023

@KevinnZou I think we need to stall this on desktop then.

I can write a kotlin library that handles the native bindings etc, so basically a newer, better and maybe depending on Jetbrains CEF version of JCEF.
But this takes some weeks

I already have experience with Kotlin Native and Java/Kotlin bindings so I have the required know-how

@DatL4g
Copy link
Collaborator

DatL4g commented Sep 30, 2023

@KevinnZou we could also read the cookie sqlite file ourself (already tried that with no success until I noticed that we delete all cookies right after adding one 🤦🏻)

But the value is encrypted in there, and I don't know how to decrypt it
(https://stackoverflow.com/questions/22532870/encrypted-cookies-in-chrome)

@KevinnZou
Copy link
Owner

@DatL4g Thanks for your investigation! If the current version of JCEF does not support it, I think we can release a version that supports Android and iOS first. As for the desktop, I am looking forward to seeing your library that is based on Jetbrains CEF, which will definitely have better performance.

@DatL4g
Copy link
Collaborator

DatL4g commented Oct 1, 2023

Please test https://github.com/DATL4G/KCEF by doing the following:

git clone git@github.com:DATL4G/KCEF.git --recurse-submodules

./gradlew example:run # Or run it from IntelliJ

@Shabinder do you use Windows?
I'm using Linux and @KevinnZou uses MacOS, would be nice if someone could test the Windows target as well.

@Shabinder
Copy link
Contributor Author

Please test https://github.com/DATL4G/KCEF by doing the following:

git clone git@github.com:DATL4G/KCEF.git --recurse-submodules

./gradlew example:run # Or run it from IntelliJ

@Shabinder do you use Windows?
I'm using Linux and @KevinnZou uses MacOS, would be nice if someone could test the Windows target as well.

I am a mac user aswell.

@DatL4g
Copy link
Collaborator

DatL4g commented Oct 2, 2023

Tested https://github.com/DATL4G/KCEF on Windows and added some patches.

Added fixes for MacOS as well, @Shabinder @KevinnZou if you tried it before, please do it again.
If you haven't tested it yet, thats fine, as it probably didn't work before.

@KevinnZou
Copy link
Owner

KevinnZou commented Oct 8, 2023

Apologies for the delayed response. I have recently returned from a lengthy vacation.

Regarding your project, I encountered an issue while testing it on my Mac system. Specifically, it crashed right after the Jcef Bundled was downloaded. Here is the corresponding stack trace:

JCEF(11:48:212): initialized stderr logger, severity=LOGSEVERITY_DEFAULT
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x0000000000000000, pid=14303, tid=65587
#
# JRE version: OpenJDK Runtime Environment (17.0.6) (build 17.0.6+0-17.0.6b829.9-10027231)
# Java VM: OpenJDK 64-Bit Server VM (17.0.6+0-17.0.6b829.9-10027231, mixed mode, tiered, compressed oops, compressed class ptrs, g1 gc, bsd-amd64)
# Problematic frame:
# C  0x0000000000000000
#
# No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
JNI global refs:
JNI global refs: 145, weak refs: 6

JNI global refs memory usage: 2099, weak refs: 833

# An error report file with more information is saved as:
# /Users/KCEF/hs_err_pid14303.log
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#
dlopen /Applications/Android Studio.app/Contents/jbr/Contents/Frameworks/Chromium Embedded Framework.framework/Chromium Embedded Framework: dlopen(/Applications/Android Studio.app/Contents/jbr/Contents/Frameworks/Chromium Embedded Framework.framework/Chromium Embedded Framework, 0x0105): tried: '/Applications/Android Studio.app/Contents/jbr/Contents/Frameworks/Chromium Embedded Framework.framework/Chromium Embedded Framework' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/Applications/Android Studio.app/Contents/jbr/Contents/Frameworks/Chromium Embedded Framework.framework/Chromium Embedded Framework' (no such file), '/Applications/Android Studio.app/Contents/jbr/Contents/Frameworks/Chromium Embedded Framework.framework/Chromium Embedded Framework' (no such file), '/System/Library/Frameworks/Chromium Embedded Framework.framework/Chromium Embedded Framework' (no such file, not in dyld cache)
JCEF_I(11:48:215): CefApp: set state NEW
JCEF_V(11:48:218): Fixed args: [--framework-dir-path=/Applications/Android Studio.app/Contents/jbr/Contents/Frameworks/Chromium Embedded Framework.framework, --browser-subprocess-path=/Applications/Android Studio.app/Contents/jbr/Contents/Frameworks/jcef Helper.app/Contents/MacOS/jcef Helper, --main-bundle-path=/Applications/Android Studio.app/Contents/jbr/Contents/Frameworks/jcef Helper.app]
JCEF_I(11:48:221): CefApp: set state INITIALIZING
JCEF_V(11:48:222): Initialize CefApp on Thread[CefInitialize-thread,6,main]

Process finished with exit code 134 (interrupted by signal 6: SIGABRT)

@DatL4g
Copy link
Collaborator

DatL4g commented Oct 8, 2023

@KevinnZou thanks for reporting, I applied some fixes.
Please delete the jcef-bundle folder and pull the latest changes

@KevinnZou
Copy link
Owner

@DatL4g Thanks for your investigation! If the current version of JCEF does not support it, I think we can release a version that supports Android and iOS first. As for the desktop, I am looking forward to seeing your library that is based on JetBrains CEF, which will definitely have better performance.

@DatL4g As previously discussed, I will clean up this MR today and release it with the latest version. Regarding KCEF, Before switching to it, I believe it is necessary to conduct a comprehensive test and wait for public feedback. It may take some time to become stable and ready for use. Let's use this separate issue to discuss future problems with KCEF.

@KevinnZou KevinnZou added the enhancement New feature or request label Oct 9, 2023
@KevinnZou KevinnZou marked this pull request as ready for review October 9, 2023 02:59
Copy link
Owner

@KevinnZou KevinnZou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to some issues with JCEF, we have decided to initially support Android and iOS platforms. Once the KCEF testing is stable, we will switch to it and also support cookies on the Desktop platform.

val cookieList = mutableListOf<Cookie>()
CefCookieManager.getGlobalManager().visitUrlCookies(
val cookieList = mutableSetOf<Cookie>()
desktopCookieManager?.visitUrlCookies(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If desktopCookieManager?.visitUrlCookies cannot return the value synchronously, should it just not work like before? The cookieList will only contain the values obtained from cached cookies which may not be the latest.

@@ -88,14 +90,17 @@ object DesktopCookieManager : CookieManager, CefCookieVisitor {
)
}.forEach(cookieList::add)

return cookieList.toList()
continuation.resume(cookieList.toList())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If desktopCookieManager?.visitUrlCookies does not provide a callback to inform us that the cookies are obtained from the disk, will it be no use to have a suspendCoroutine here? Since the continuation.resume(cookieList.toList()) will always be called at the end of the method, it works just like the return.

interface CookieManager {
suspend fun setCookie(url: String, cookie: Cookie)
suspend fun getCookies(url: String): List<Cookie>
suspend fun removeAllCookies()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also have a delete cookie function for a URL.

suspend fun removeCookies(url: String)

I have implemented it for iOS and Desktop in this commit. It seems that Android does not provide a method to delete cookies for specific URLs so I leave it as TODO.

@KevinnZou KevinnZou merged commit 385e49e into KevinnZou:main Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API exposed for Cookie Management ?
3 participants