Bug 203963 - Port ServiceWorkerContainer to the HTML5 event loop
Summary: Port ServiceWorkerContainer to the HTML5 event loop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 202843
  Show dependency treegraph
 
Reported: 2019-11-07 09:33 PST by Chris Dumez
Modified: 2019-11-07 14:37 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.10 KB, patch)
2019-11-07 09:34 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-11-07 09:33:45 PST
Port ServiceWorkerContainer to the HTML5 event loop.
Comment 1 Chris Dumez 2019-11-07 09:34:28 PST
Created attachment 383058 [details]
Patch
Comment 2 Ryosuke Niwa 2019-11-07 13:38:42 PST
Comment on attachment 383058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383058&action=review

> Source/WebCore/workers/service/ServiceWorkerContainer.h:145
> +    bool m_shouldDeferMessageEvents { false };
> +    Vector<Ref<Event>> m_deferredMessageEvents;

Hm... how does GC keep wrappers for objects held by this event alive?
Comment 3 Chris Dumez 2019-11-07 13:52:40 PST
(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 383058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383058&action=review
> 
> > Source/WebCore/workers/service/ServiceWorkerContainer.h:145
> > +    bool m_shouldDeferMessageEvents { false };
> > +    Vector<Ref<Event>> m_deferredMessageEvents;
> 
> Hm... how does GC keep wrappers for objects held by this event alive?

I don't think we have any logic to keep those wrappers alive currently. This is not a regression in this patch though.
Comment 4 Ryosuke Niwa 2019-11-07 13:54:35 PST
(In reply to Chris Dumez from comment #3)
> (In reply to Ryosuke Niwa from comment #2)
> > Comment on attachment 383058 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=383058&action=review
> > 
> > > Source/WebCore/workers/service/ServiceWorkerContainer.h:145
> > > +    bool m_shouldDeferMessageEvents { false };
> > > +    Vector<Ref<Event>> m_deferredMessageEvents;
> > 
> > Hm... how does GC keep wrappers for objects held by this event alive?
> 
> I don't think we have any logic to keep those wrappers alive currently. This
> is not a regression in this patch though.

Okay, sounds like a bug we should track somewhere. How do we track these bugs for service workers?
Comment 5 Chris Dumez 2019-11-07 13:55:53 PST
(In reply to Ryosuke Niwa from comment #4)
> (In reply to Chris Dumez from comment #3)
> > (In reply to Ryosuke Niwa from comment #2)
> > > Comment on attachment 383058 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=383058&action=review
> > > 
> > > > Source/WebCore/workers/service/ServiceWorkerContainer.h:145
> > > > +    bool m_shouldDeferMessageEvents { false };
> > > > +    Vector<Ref<Event>> m_deferredMessageEvents;
> > > 
> > > Hm... how does GC keep wrappers for objects held by this event alive?
> > 
> > I don't think we have any logic to keep those wrappers alive currently. This
> > is not a regression in this patch though.
> 
> Okay, sounds like a bug we should track somewhere. How do we track these
> bugs for service workers?

It is unclear to me at this point that we want to keep those wrappers alive though. You're talking about wrappers to MessagePorts I assume?
Comment 6 Ryosuke Niwa 2019-11-07 14:10:02 PST
(In reply to Chris Dumez from comment #5)
> (In reply to Ryosuke Niwa from comment #4)
> > (In reply to Chris Dumez from comment #3)
> > > (In reply to Ryosuke Niwa from comment #2)
> > > > Comment on attachment 383058 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=383058&action=review
> > > > 
> > > > > Source/WebCore/workers/service/ServiceWorkerContainer.h:145
> > > > > +    bool m_shouldDeferMessageEvents { false };
> > > > > +    Vector<Ref<Event>> m_deferredMessageEvents;
> > > > 
> > > > Hm... how does GC keep wrappers for objects held by this event alive?
> > > 
> > > I don't think we have any logic to keep those wrappers alive currently. This
> > > is not a regression in this patch though.
> > 
> > Okay, sounds like a bug we should track somewhere. How do we track these
> > bugs for service workers?
> 
> It is unclear to me at this point that we want to keep those wrappers alive
> though. You're talking about wrappers to MessagePorts I assume?

Yeah, MessagePorts and other transferred objects. Because you can postMessage to yourself, or some other window, right? In those cases, it's totally observable as to whether the wrapper has changed or not.
Comment 7 WebKit Commit Bot 2019-11-07 14:36:52 PST
Comment on attachment 383058 [details]
Patch

Clearing flags on attachment: 383058

Committed r252206: <https://trac.webkit.org/changeset/252206>
Comment 8 WebKit Commit Bot 2019-11-07 14:36:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-11-07 14:37:19 PST
<rdar://problem/56998897>