Bug 179186

Summary: SW: Implement "Update Registration State" algorithm (unused for now)
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, buildbot, cdumez, commit-queue, ggaren, rniwa, ryanhaddad, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=179246
Attachments:
Description Flags
WIP for EWS
buildbot: commit-queue-
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
WIP2 for EWS
none
Patch
none
Patch
cdumez: commit-queue-
EWS then maybe CQ
none
Fix the ASSERT
commit-queue: commit-queue-
Fix the ASSERT none

Description Brady Eidson 2017-11-02 11:11:44 PDT
SW: Implement "Update Registration State" algorithm

From the server process back to the client process.
Comment 1 Brady Eidson 2017-11-02 11:26:01 PDT
For our sanity we'll do Update Worker State at the same time.
Comment 2 Brady Eidson 2017-11-02 11:28:55 PDT
NM, patches will get too insane.

I'll land "Update Registration State" for now with no effect
Comment 3 Brady Eidson 2017-11-02 11:45:00 PDT
Created attachment 325738 [details]
WIP for EWS
Comment 4 Build Bot 2017-11-02 12:24:34 PDT
Comment on attachment 325738 [details]
WIP for EWS

Attachment 325738 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5078551

Number of test failures exceeded the failure limit.
Comment 5 Build Bot 2017-11-02 12:24:35 PDT
Created attachment 325745 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2017-11-02 13:00:10 PDT
Comment on attachment 325738 [details]
WIP for EWS

Attachment 325738 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5078827

Number of test failures exceeded the failure limit.
Comment 7 Build Bot 2017-11-02 13:00:11 PDT
Created attachment 325755 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 8 Brady Eidson 2017-11-02 16:12:07 PDT
Created attachment 325790 [details]
WIP2 for EWS
Comment 9 Brady Eidson 2017-11-02 16:42:32 PDT
Created attachment 325797 [details]
Patch
Comment 10 Chris Dumez 2017-11-02 18:40:45 PDT
Comment on attachment 325797 [details]
Patch

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

r=me with suggestions

> Source/WebCore/workers/service/ServiceWorkerTypes.h:33
> +    Installing = 0,

Why the explicit values? default is 0-based.

> Source/WebCore/workers/service/server/SWClientConnection.cpp:69
> +    auto result = m_registrations.add(registration.data().key, nullptr);

Could use ensure()

> Source/WebCore/workers/service/server/SWClientConnection.cpp:83
> +    ASSERT(iterator->value && iterator->value->contains(&registration));

We should probably ASSERT(iterator != m_registrations.end()); before that.

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:93
> +    auto result = m_clientRegistrationsByConnection.add(connectionIdentifier, nullptr);

Could use ensure().

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:104
> +    if (!iterator->value)

Should we assert and iterator != m_clientRegistrationsByConnection.end() ?
Comment 11 Brady Eidson 2017-11-02 19:26:15 PDT
(In reply to Chris Dumez from comment #10)
> Comment on attachment 325797 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325797&action=review
> 
> r=me with suggestions
> 
> > Source/WebCore/workers/service/ServiceWorkerTypes.h:33
> > +    Installing = 0,
> 
> Why the explicit values? default is 0-based.

In an earlier version of this patch (and in a follow-up) I was going to use them as array indices.

I agree it's not needed, but I get paranoid about "enums as indices" anyways. *sigh*

> 
> > Source/WebCore/workers/service/server/SWClientConnection.cpp:69
> > +    auto result = m_registrations.add(registration.data().key, nullptr);
> 
> Could use ensure()

Yah (I'd never seen this before)

> > Source/WebCore/workers/service/server/SWServerRegistration.cpp:104
> > +    if (!iterator->value)
> 
> Should we assert and iterator != m_clientRegistrationsByConnection.end() ?

Nah, but similar.
Comment 12 Brady Eidson 2017-11-02 19:28:39 PDT
Created attachment 325821 [details]
Patch
Comment 13 Brady Eidson 2017-11-02 19:29:04 PDT
Comment on attachment 325821 [details]
Patch

EWS (haven't finished building locally...) then I'll cq+
Comment 14 Chris Dumez 2017-11-02 19:39:09 PDT
Comment on attachment 325821 [details]
Patch

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

> Source/WebCore/workers/service/server/SWClientConnection.cpp:67
> +//    static NeverDestroyed<HashMap<String, String>> mimeTypeToLabelTitleMap;

Pretty sure you did not mean to have this here :D
Comment 15 Chris Dumez 2017-11-02 19:42:20 PDT
Comment on attachment 325821 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:95
> +    }

});
Comment 16 Brady Eidson 2017-11-02 20:03:17 PDT
(In reply to Chris Dumez from comment #14)
> Comment on attachment 325821 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325821&action=review
> 
> > Source/WebCore/workers/service/server/SWClientConnection.cpp:67
> > +//    static NeverDestroyed<HashMap<String, String>> mimeTypeToLabelTitleMap;
> 
> Pretty sure you did not mean to have this here :D

I don't even know how in the heck it got there!
Comment 17 Brady Eidson 2017-11-02 20:03:47 PDT
(In reply to Brady Eidson from comment #16)
> (In reply to Chris Dumez from comment #14)
> > Comment on attachment 325821 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=325821&action=review
> > 
> > > Source/WebCore/workers/service/server/SWClientConnection.cpp:67
> > > +//    static NeverDestroyed<HashMap<String, String>> mimeTypeToLabelTitleMap;
> > 
> > Pretty sure you did not mean to have this here :D
> 
> I don't even know how in the heck it got there!

NM I do.
Comment 18 Brady Eidson 2017-11-02 20:04:13 PDT
(In reply to Chris Dumez from comment #15)
> Comment on attachment 325821 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325821&action=review
> 
> > Source/WebCore/workers/service/server/SWServerRegistration.cpp:95
> > +    }
> 
> });

And if UnifiedSource hadn't broken Xcode's look-ahead compiling... *sigh*
Comment 19 Brady Eidson 2017-11-02 20:05:25 PDT
Created attachment 325827 [details]
EWS then maybe CQ
Comment 20 WebKit Commit Bot 2017-11-02 21:36:23 PDT
Comment on attachment 325827 [details]
EWS then maybe CQ

Clearing flags on attachment: 325827

Committed r224379: <https://trac.webkit.org/changeset/224379>
Comment 21 WebKit Commit Bot 2017-11-02 21:36:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Chris Dumez 2017-11-02 22:44:56 PDT
FYI, I saw this crash when running the SW tests locally in debug:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010e080f04 WTFCrash + 36 (Assertions.cpp:270)
1   com.apple.WebCore             	0x0000000116de1459 WebCore::SWServerRegistration::removeClientServiceWorkerRegistration(unsigned long long, unsigned long long) + 393 (SWServerRegistration.cpp:107)
2   com.apple.WebCore             	0x0000000116de05b9 WebCore::SWServer::removeClientServiceWorkerRegistration(WebCore::SWServer::Connection&, WebCore::ServiceWorkerRegistrationKey const&, unsigned long long) + 137 (SWServer.cpp:239)
3   com.apple.WebCore             	0x0000000116de0524 WebCore::SWServer::Connection::removeServiceWorkerRegistrationInServer(WebCore::ServiceWorkerRegistrationKey const&, unsigned long long) + 52 (SWServer.cpp:115)
4   com.apple.WebKit              	0x0000000107a2e260 void IPC::callMemberFunctionImpl<WebKit::WebSWServerConnection, void (WebCore::SWServer::Connection::*)(WebCore::ServiceWorkerRegistrationKey const&, unsigned long long), std::__1::tuple<WebCore::ServiceWorkerRegistrationKey, unsigned long long>, 0ul, 1ul>(WebKit::WebSWServerConnection*, void (WebCore::SWServer::Connection::*)(WebCore::ServiceWorkerRegistrationKey const&, unsigned long long), std::__1::tuple<WebCore::ServiceWorkerRegistrationKey, unsigned long long>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>) + 192 (HandleMessage.h:41)
5   com.apple.WebKit              	0x0000000107a2dfd0 void IPC::callMemberFunction<WebKit::WebSWServerConnection, void (WebCore::SWServer::Connection::*)(WebCore::ServiceWorkerRegistrationKey const&, unsigned long long), std::__1::tuple<WebCore::ServiceWorkerRegistrationKey, unsigned long long>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<WebCore::ServiceWorkerRegistrationKey, unsigned long long>&&, WebKit::WebSWServerConnection*, void (WebCore::SWServer::Connection::*)(WebCore::ServiceWorkerRegistrationKey const&, unsigned long long)) + 96 (HandleMessage.h:47)
6   com.apple.WebKit              	0x0000000107a2bfe3 void IPC::handleMessage<Messages::WebSWServerConnection::RemoveServiceWorkerRegistrationInServer, WebKit::WebSWServerConnection, void (WebCore::SWServer::Connection::*)(WebCore::ServiceWorkerRegistrationKey const&, unsigned long long)>(IPC::Decoder&, WebKit::WebSWServerConnection*, void (WebCore::SWServer::Connection::*)(WebCore::ServiceWorkerRegistrationKey const&, unsigned long long)) + 339 (HandleMessage.h:127)
7   com.apple.WebKit              	0x0000000107a2b8d6 WebKit::WebSWServerConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 502 (WebSWServerConnectionMessageReceiver.cpp:64)
8   com.apple.WebKit              	0x000000010734bd0e WebKit::StorageToWebProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 734
9   com.apple.WebKit              	0x0000000106dddc63 IPC::Connection::dispatchMessage(IPC::Decoder&) + 51 (Connection.cpp:902)
10  com.apple.WebKit              	0x0000000106dd3248 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 712
11  com.apple.WebKit              	0x0000000106dde26a IPC::Connection::dispatchOneMessage() + 1530 (Connection.cpp:959)
12  com.apple.WebKit              	0x0000000106df654d IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()() + 29 (Connection.cpp:896)
13  com.apple.WebKit              	0x0000000106df64a9 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call() + 25 (Function.h:101)
14  com.apple.JavaScriptCore      	0x000000010e0b654b WTF::Function<void ()>::operator()() const + 139 (Function.h:56)
15  com.apple.JavaScriptCore      	0x000000010e0d6b83 WTF::RunLoop::performWork() + 211 (RunLoop.cpp:107)
16  com.apple.JavaScriptCore      	0x000000010e0d7404 WTF::RunLoop::performWork(void*) + 36 (RunLoopCF.cpp:38)
17  com.apple.CoreFoundation      	0x00007fff3cff46e1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
18  com.apple.CoreFoundation      	0x00007fff3d0ae3ac __CFRunLoopDoSource0 + 108
19  com.apple.CoreFoundation      	0x00007fff3cfd7180 __CFRunLoopDoSources0 + 208
20  com.apple.CoreFoundation      	0x00007fff3cfd65fd __CFRunLoopRun + 1293
21  com.apple.CoreFoundation      	0x00007fff3cfd5e63 CFRunLoopRunSpecific + 483
22  com.apple.Foundation          	0x00007fff3f14eba6 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 277
23  com.apple.Foundation          	0x00007fff3f14ea7e -[NSRunLoop(NSRunLoop) run] + 76
24  libxpc.dylib                  	0x00007fff650fa3ff _xpc_objc_main + 536
25  libxpc.dylib                  	0x00007fff650f907e xpc_main + 417
26  com.apple.WebKit.Storage      	0x0000000106c9b13b main + 1195 (XPCServiceMain.mm:148)
27  libdyld.dylib                 	0x00007fff64e2d115 start + 1
Comment 23 Brady Eidson 2017-11-03 09:37:42 PDT
Created attachment 325905 [details]
Fix the ASSERT
Comment 24 Brady Eidson 2017-11-03 09:39:02 PDT
Reopening for CQ followup
Comment 25 WebKit Commit Bot 2017-11-03 09:40:18 PDT
Comment on attachment 325905 [details]
Fix the ASSERT

Rejecting attachment 325905 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 325905, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/5091493
Comment 26 Ryan Haddad 2017-11-03 10:41:37 PDT
*** Bug 179246 has been marked as a duplicate of this bug. ***
Comment 27 Brady Eidson 2017-11-03 12:40:50 PDT
Created attachment 325935 [details]
Fix the ASSERT
Comment 28 WebKit Commit Bot 2017-11-03 13:12:13 PDT
Comment on attachment 325935 [details]
Fix the ASSERT

Clearing flags on attachment: 325935

Committed r224418: <https://trac.webkit.org/changeset/224418>
Comment 29 WebKit Commit Bot 2017-11-03 13:12:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Radar WebKit Bug Importer 2017-11-15 12:17:29 PST
<rdar://problem/35567268>