Summary: | REGRESSION(r212345): [GTK] ASSERT(cookieChangeCallbackMap().contains(jar)) failed in WebCore::stopObservingCookieChanges:54 | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, berto, bugs-noreply, cgarcia, commit-queue, danw, gustavo, mcatanzaro, mrobinson | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | PC | ||||||
OS: | Linux | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=168230 | ||||||
Attachments: |
|
Description
Michael Catanzaro
2017-02-15 09:50:05 PST
I don't think it's r212283, there's absolutely no change in behavior in that revision. If any, it would be r212345. hmm, I was thinking in the UI process, in the web process I updated the cookies observer, there shouldn't be any change in behavior, but it could be... I still think it's more likely r212345, though. Ah, this is a debug assertion: void startObservingCookieChanges(const NetworkStorageSession& storageSession, std::function<void ()>&& callback) { auto* jar = storageSession.cookieStorage(); ASSERT(!cookieChangeCallbackMap().contains(jar)); cookieChangeCallbackMap().add(jar, WTFMove(callback)); g_signal_connect(jar, "changed", G_CALLBACK(soupCookiesChanged), nullptr); } Er sorry, it's stop not start. Crash is on shutdown: void stopObservingCookieChanges(const NetworkStorageSession& storageSession) { auto* jar = storageSession.cookieStorage(); ASSERT(cookieChangeCallbackMap().contains(jar)); cookieChangeCallbackMap().remove(jar); g_signal_handlers_disconnect_by_func(jar, reinterpret_cast<void*>(soupCookiesChanged), nullptr); } So I guess it's not properly paired with a call to startObservingCookieChanges(). I can't reproduce the issue, but I think there are several things with the session management that could leave to problems. This crash could be because because cookie manager sends the stop observing cookie changes relaunching the network process, so if we call this after the network process finishes, we launch a new network process for which start observing changes was not called. But there are more situations this can fail in one way or another. The main problem is that the website data store creates the session ID, but it's web page on creation the one ensuring the session in the web and network processes. So, if you setup your cookie manager before the first web page is created, the start message will not work for ephemeral sessions, because it doesn't exist yet in the network process. But it will exist when we call stop. Were you using a private session when it crashed? In the GTK+ port we always want to observe cookie changes, so we could probably get rid of the start/stop messages and all the inconsistencies. (In reply to comment #5) > I can't reproduce the issue, but I think there are several things with the > session management that could leave to problems. This crash could be because > because cookie manager sends the stop observing cookie changes relaunching > the network process, so if we call this after the network process finishes, > we launch a new network process for which start observing changes was not > called. That could definitely be it. I likely closed Epiphany with Ctrl+C, which would have caused network process termination to race with this shutdown message. > But there are more situations this can fail in one way or another. > The main problem is that the website data store creates the session ID, but > it's web page on creation the one ensuring the session in the web and > network processes. So, if you setup your cookie manager before the first web > page is created, the start message will not work for ephemeral sessions, > because it doesn't exist yet in the network process. But it will exist when > we call stop. Were you using a private session when it crashed? No, I was not using a private session, but it sounds like we need to fix that too. Created attachment 301898 [details]
Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Attachment 301898 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:128: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:146: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/NetworkStorageSession.h:78: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/NetworkStorageSession.h:99: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:80: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:26: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 6 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
r=me on the soup and GTK changes. Also needs an owner. Alex? Committed r212626: <http://trac.webkit.org/changeset/212626> |