[webkit-reviews] review granted: [Bug 179745] Dispatching an event on a ServiceWorker may fail or crash due to GC : [Attachment 327031] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 179745: Dispatching an event on a ServiceWorker may fail or crash due to GC
https://bugs.webkit.org/show_bug.cgi?id=179745

Attachment 327031: Patch

https://bugs.webkit.org/attachment.cgi?id=327031&action=review




--- Comment #4 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 327031
  --> https://bugs.webkit.org/attachment.cgi?id=327031
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