Summary: | [iOS][WK1] ASSERTION FAILED: m_creationThread.ptr() == &Thread::current() in ServiceWorkerContainer::~ServiceWorkerContainer() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | NEW --- | ||||||||||||
Severity: | Normal | CC: | ap, benjamin, cdumez, cmarcelo, esprehn+autocc, ews-watchlist, kangil.han, kondapallykalyan, webkit-bug-importer, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Local Build | ||||||||||||
Hardware: | iPhone / iPad | ||||||||||||
OS: | iOS 12 | ||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=183682 https://bugs.webkit.org/show_bug.cgi?id=196692 |
||||||||||||
Attachments: |
|
Description
Daniel Bates
2018-09-27 15:46:32 PDT
The assert probably just needs to be updated to take into account main thread/web thread handling. This is WK1, it should not have service workers enabled so we should not be constructing a ServiceWorkerContainer at all I believe. Created attachment 351020 [details]
Patch
Comment on attachment 351020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351020&action=review > Source/WebCore/page/NavigatorBase.cpp:155 > + return m_serviceWorkerContainer.get(); I am not sure this is worth the effort. If we go with it, I would keep returning a ServiceWorkerContainer&. We could assert RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() and create m_serviceWorkerContainer in this method. Comment on attachment 351020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351020&action=review > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:34 > + visitor.addOpaqueRoot(wrapped().serviceWorker()); Note that we currently do not need a RuntimeEnabledCheck here... > Source/WebCore/bindings/js/JSWorkerNavigatorCustom.cpp:34 > + visitor.addOpaqueRoot(wrapped().serviceWorker()); ... or here. > Source/WebCore/dom/ScriptExecutionContext.cpp:569 > + return navigator ? navigator->serviceWorker() : nullptr; or here. >> Source/WebCore/page/NavigatorBase.cpp:155 >> + return m_serviceWorkerContainer.get(); > > I am not sure this is worth the effort. > If we go with it, I would keep returning a ServiceWorkerContainer&. > We could assert RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() and create m_serviceWorkerContainer in this method. If we keep returning a ServiceWorkerContainer& here then we'd have to check RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() at call sites (e.g. in GC visitors). Comment on attachment 351020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351020&action=review >>> Source/WebCore/page/NavigatorBase.cpp:155 >>> + return m_serviceWorkerContainer.get(); >> >> I am not sure this is worth the effort. >> If we go with it, I would keep returning a ServiceWorkerContainer&. >> We could assert RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() and create m_serviceWorkerContainer in this method. > > If we keep returning a ServiceWorkerContainer& here then we'd have to check RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() at call sites (e.g. in GC visitors). Also, if we initialized m_serviceWorkerContainer in this method, then we'd need an extra member to store the navigator's scriptExecutionContext since the ServiceWorkerContainer constructor requires it. > If we keep returning a ServiceWorkerContainer& here then we'd have to check
> RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() at call
> sites (e.g. in GC visitors).
Maybe a "ServiceWorkerContainer* serviceWorkerContainerIfAny()" would do the trick for these?
As of the ScriptExecutionContext, can it be passed to the may-create getter?
Comment on attachment 351020 [details]
Patch
I do not really understand the problem. Returning a raw pointer adds basically no complexity at call sites, as you can see in my patch.
Created attachment 366902 [details]
Patch
Created attachment 366937 [details]
Patch
Created attachment 366942 [details]
Patch
Comment on attachment 366942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366942&action=review > Source/WTF/wtf/ThreadObserver.h:33 > +class ThreadObserver { The name is very generic considering what it does. I am also not convinced we need a new class just for this? I am even more confused this is used as a data member and not a base class. (In reply to Chris Dumez from comment #13) > Comment on attachment 366942 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=366942&action=review > > > Source/WTF/wtf/ThreadObserver.h:33 > > +class ThreadObserver { > > The name is very generic considering what it does. I am also not convinced > we need a new class just for this? I am even more confused this is used as a > data member and not a base class. I thought of ThreadChecker initially and can change to that. This can be used as a base class although I do not see how much it improves things there. As of its use, we could try to use something like that for all our WKWebView delegates taking completion handler. We could check that they are called and destroyed on the thread they are created on. Comment on attachment 366942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366942&action=review >>> Source/WTF/wtf/ThreadObserver.h:33 >>> +class ThreadObserver { >> >> The name is very generic considering what it does. I am also not convinced we need a new class just for this? I am even more confused this is used as a data member and not a base class. > > I thought of ThreadChecker initially and can change to that. > > This can be used as a base class although I do not see how much it improves things there. > > As of its use, we could try to use something like that for all our WKWebView delegates taking completion handler. > We could check that they are called and destroyed on the thread they are created on. This class does not check anything though. All it does is remember the creation thread. You could have "checked" in a destructor that the object gets destroyed on the creation thread but you did not. Also note that ServiceWorkerContainer is already an ActiveDOMObject and that ActiveDOMObject already saves m_creationThread. (In reply to youenn fablet from comment #14) > (In reply to Chris Dumez from comment #13) > > Comment on attachment 366942 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=366942&action=review > > > > > Source/WTF/wtf/ThreadObserver.h:33 > > > +class ThreadObserver { > > > > The name is very generic considering what it does. I am also not convinced > > we need a new class just for this? I am even more confused this is used as a > > data member and not a base class. > > I thought of ThreadChecker initially and can change to that. > This can be used as a base class although I do not see how much it improves > things there. It would improve things before: 1. You would not need a data member in the subclass 2. You could just call isCreationThread() instead of m_dataMember.isCreationThread() 3. You could add a destructor to ThreadObserver (or whatever it should be named) to make sure it gets destroyed on the creation thread, so that subclasses do not have to. In any case, my preference would be to not add a class in WTF for this at all. (In reply to Chris Dumez from comment #16) > (In reply to youenn fablet from comment #14) > > (In reply to Chris Dumez from comment #13) > > > Comment on attachment 366942 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=366942&action=review > > > > > > > Source/WTF/wtf/ThreadObserver.h:33 > > > > +class ThreadObserver { > > > > > > The name is very generic considering what it does. I am also not convinced > > > we need a new class just for this? I am even more confused this is used as a > > > data member and not a base class. > > > > I thought of ThreadChecker initially and can change to that. > > > This can be used as a base class although I do not see how much it improves > > things there. > > It would improve things before: > 1. You would not need a data member in the subclass A data member in the subclass is not needed in either cases. Availability to subclass in both cases can be public, private or protected. > 3. You could add a destructor to ThreadObserver (or whatever it should be > named) to make sure it gets destroyed on the creation thread, so that > subclasses do not have to. Sure, I had this in mind, for instance to issues we faced with callback destructors. I do not see how it makes a difference regarding the subclass or data member issue though. > In any case, my preference would be to not add a class in WTF for this at > all. I do not feel strongly about this class. When looking at the code before and after the patch, it seems cleaner after without all these #ifdef NDEBUG. If we start doing more of these checks in the future, it seems like a nice addition to me. |