Dispatching an event on a ServiceWorker may fail or crash due to GC. We need to make sure that a ServiceWorker's wrapper stays alive as long as we may dispatch events on it.
Created attachment 327028 [details] Patch
Created attachment 327031 [details] Patch
Crash trace for reference: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000040bcfc0b4 WTFCrash + 36 (Assertions.cpp:270) 1 com.apple.WebCore 0x00000003fe22b3be WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext&) const + 398 (JSEventListener.h:128) 2 com.apple.WebCore 0x00000003fe22a84d WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) + 205 (JSEventListener.cpp:97) 3 com.apple.WebCore 0x00000003fe773df2 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener>, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>) + 962 (EventTarget.cpp:287) 4 com.apple.WebCore 0x00000003fe76b8ca WebCore::EventTarget::fireEventListeners(WebCore::Event&) + 314 (EventTarget.cpp:228) 5 com.apple.WebCore 0x00000003fe7739a9 WebCore::EventTarget::dispatchEvent(WebCore::Event&) + 233 (EventTarget.cpp:188) 6 com.apple.WebCore 0x00000003ffb77c84 WebCore::ServiceWorker::scheduleTaskToUpdateState(WebCore::ServiceWorkerState)::$_3::operator()(WebCore::ScriptExecutionContext&) const + 84 (ServiceWorker.cpp:100) 7 com.apple.WebCore 0x00000003ffb77ae4 WTF::Function<void (WebCore::ScriptExecutionContext&)>::CallableWrapper<WebCore::ServiceWorker::scheduleTaskToUpdateState(WebCore::ServiceWorkerState)::$_3>::call(WebCore::ScriptExecutionContext&) + 52 (Function.h:101) 8 com.apple.WebCore 0x00000003fe1113be WTF::Function<void (WebCore::ScriptExecutionContext&)>::operator()(WebCore::ScriptExecutionContext&) const + 158 (Function.h:56) 9 com.apple.WebCore 0x00000003fe0fed8d WebCore::ScriptExecutionContext::Task::performTask(WebCore::ScriptExecutionContext&) + 29 (ScriptExecutionContext.h:183) 10 com.apple.WebCore 0x00000003fe7269c6 WebCore::Document::postTask(WebCore::ScriptExecutionContext::Task&&)::$_2::operator()() + 278 (Document.cpp:5749) 11 com.apple.WebCore 0x00000003fe7267a9 WTF::Function<void ()>::CallableWrapper<WebCore::Document::postTask(WebCore::ScriptExecutionContext::Task&&)::$_2>::call() + 25 (Function.h:101) 12 com.apple.JavaScriptCore 0x000000040bd316fb WTF::Function<void ()>::operator()() const + 139 (Function.h:56) 13 com.apple.JavaScriptCore 0x000000040bd3130b WTF::dispatchFunctionsFromMainThread() + 331 (MainThread.cpp:129) 14 com.apple.JavaScriptCore 0x000000040bd33fc1 WTF::timerFired(__CFRunLoopTimer*, void*) + 49 (MainThreadMac.mm:111) 15 com.apple.CoreFoundation 0x00007fff4abe4ce4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
Comment on attachment 327031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327031&action=review r=me > Source/WebCore/ChangeLog:12 > + implementation objects keeps a PendingActivity alive while it may dispatch JS events. object > Source/WebCore/ChangeLog:13 > + The only even dispatched on ServiceWorker objects is the "statechange" one. We may event > Source/WebCore/workers/service/ServiceWorker.cpp:78 > + serviceWorker->updatePendingActivityForEventDispatch(); Since this is required for correctness upon allocation, I think it would be clearer to do this in the ServiceWorker constructor. > Source/WebCore/workers/service/ServiceWorker.cpp:175 > + // FIXME: We should do better. Would be nice to explain here that this prevents us from entering the page cache when we have a service worker. > Source/WebCore/workers/service/ServiceWorker.cpp:181 > + removeFromAllWorkers(*this); Are we required to remove ourselves from the map upon stop, or can we wait for our destructor? Since we added ourselves in the constructor, I think it's a little clearer to remove ourselves in the destructor. > Source/WebCore/workers/service/ServiceWorker.cpp:188 > + // ServiceWorker can dispatch events until they become redundant or they are stopped. ServiceWorkers
Comment on attachment 327031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327031&action=review >> Source/WebCore/workers/service/ServiceWorker.cpp:78 >> + serviceWorker->updatePendingActivityForEventDispatch(); > > Since this is required for correctness upon allocation, I think it would be clearer to do this in the ServiceWorker constructor. The reason I moved it here is because I hit an assertion when doing this in the constructor. The reason is that this refs |this| when taking a PendingActivity and doing so in the constructor would mean we ref *before* the object has been adopted. (Should I use relax adoptionRequirements() instead?) >> Source/WebCore/workers/service/ServiceWorker.cpp:181 >> + removeFromAllWorkers(*this); > > Are we required to remove ourselves from the map upon stop, or can we wait for our destructor? Since we added ourselves in the constructor, I think it's a little clearer to remove ourselves in the destructor. I'll drop this and replace with an assertion to make sure we do not recycle/reuse workers that have been stopped.
> > Since this is required for correctness upon allocation, I think it would be clearer to do this in the ServiceWorker constructor. > > The reason I moved it here is because I hit an assertion when doing this in > the constructor. The reason is that this refs |this| when taking a > PendingActivity and doing so in the constructor would mean we ref *before* > the object has been adopted. (Should I use relax adoptionRequirements() > instead?) I think I would either relaxAdoptionRequirement() or add a create method to encapsulate the updatePendingActivityForEventDispatch() call and make the constructor private. It's kind of annoying that the m_adoptionIsRequired mechanism prevents you from calling a function that uses RefPtr from your constructor. It is definitely not a programming error to do that. > >> Source/WebCore/workers/service/ServiceWorker.cpp:181 > >> + removeFromAllWorkers(*this); > > > > Are we required to remove ourselves from the map upon stop, or can we wait for our destructor? Since we added ourselves in the constructor, I think it's a little clearer to remove ourselves in the destructor. > > I'll drop this and replace with an assertion to make sure we do not > recycle/reuse workers that have been stopped. Sounds good.
(In reply to Geoffrey Garen from comment #6) > > > Since this is required for correctness upon allocation, I think it would be clearer to do this in the ServiceWorker constructor. > > > > The reason I moved it here is because I hit an assertion when doing this in > > the constructor. The reason is that this refs |this| when taking a > > PendingActivity and doing so in the constructor would mean we ref *before* > > the object has been adopted. (Should I use relax adoptionRequirements() > > instead?) > > I think I would either relaxAdoptionRequirement() or add a create method to > encapsulate the updatePendingActivityForEventDispatch() call and make the > constructor private. I do not understand this comment. The constructor already is private and the call to updatePendingActivityForEventDispatch() is already in the create function of the class.
Created attachment 327076 [details] Patch
(In reply to Chris Dumez from comment #7) > (In reply to Geoffrey Garen from comment #6) > > > > Since this is required for correctness upon allocation, I think it would be clearer to do this in the ServiceWorker constructor. > > > > > > The reason I moved it here is because I hit an assertion when doing this in > > > the constructor. The reason is that this refs |this| when taking a > > > PendingActivity and doing so in the constructor would mean we ref *before* > > > the object has been adopted. (Should I use relax adoptionRequirements() > > > instead?) > > > > I think I would either relaxAdoptionRequirement() or add a create method to > > encapsulate the updatePendingActivityForEventDispatch() call and make the > > constructor private. > > I do not understand this comment. The constructor already is private and the > call to updatePendingActivityForEventDispatch() is already in the create > function of the class. Anyway, latest iteration now uses relaxAdoptionRequirements().
> > I think I would either relaxAdoptionRequirement() or add a create method to > > encapsulate the updatePendingActivityForEventDispatch() call and make the > > constructor private. > > I do not understand this comment. The constructor already is private and the > call to updatePendingActivityForEventDispatch() is already in the create > function of the class. Sorry, I didn't notice that this was the getOrCreate function :(. I think relaxAdoptionRequirement() is fine too, so I'll cq+ this new patch.
Comment on attachment 327076 [details] Patch Clearing flags on attachment: 327076 Committed r224924: <https://trac.webkit.org/changeset/224924>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35591546>