Bug 179745 – Dispatching an event on a ServiceWorker may fail or crash due to GC

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 327028 [details]
Patch

[Chris Dumez]

Created attachment 327031 [details]
Patch

[Chris Dumez]

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

[Geoffrey Garen]

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

[Chris Dumez]

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.

[Geoffrey Garen]

> > 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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 327076 [details]
Patch

[Chris Dumez]

(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().

[Geoffrey Garen]

> > 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.

[Chris Dumez]

Comment on attachment 327076 [details]
Patch

Clearing flags on attachment: 327076

Committed r224924: <https://trac.webkit.org/changeset/224924>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35591546>