Bug 179253

Summary: Implement ServiceWorkerContainer.getRegistration
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, esprehn+autocc, kondapallykalyan, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 179270    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Moving the map
none
Patch
none
Patch
none
Patch
none
Patch none

Description youenn fablet 2017-11-03 11:54:56 PDT
Implement ServiceWorkerContainer.getRegistration
Comment 1 youenn fablet 2017-11-03 12:20:23 PDT
Created attachment 325932 [details]
Patch
Comment 2 Build Bot 2017-11-03 13:10:44 PDT
Comment on attachment 325932 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/getregistration.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register.https.html
Comment 3 Build Bot 2017-11-03 13:10:45 PDT
Created attachment 325940 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 4 Build Bot 2017-11-03 13:32:28 PDT
Comment on attachment 325932 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/getregistration.https.html
Comment 5 Build Bot 2017-11-03 13:32:30 PDT
Created attachment 325948 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 6 youenn fablet 2017-11-03 13:42:16 PDT
Created attachment 325952 [details]
Patch
Comment 7 youenn fablet 2017-11-03 15:22:55 PDT
Created attachment 325972 [details]
Moving the map
Comment 8 Chris Dumez 2017-11-03 15:44:01 PDT
Comment on attachment 325972 [details]
Moving the map

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

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:179
> +void ServiceWorkerContainer::getRegistration(ScriptExecutionContext& context, const String& clientURL, Ref<DeferredPromise>&& promise)

I believe this is the caller's scriptExecutionContext, which is likely not what we want. You likely want ServiceWorkerContainer::scriptExecutionContext(), i.e. the context object's script execution context.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:190
> +    setPendingActivity(this);

You made me add makePendingActivity() for lambda like these and you're not even using it :P

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:192
> +        auto protectedThis = makeRef(*this);

Why don't we just do this in the lambda capture?

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:203
> +            auto& context = *scriptExecutionContext();

This looks like the right script execution context.

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/skip-waiting-without-using-registration.https-expected.txt:2
> +Harness Error (TIMEOUT), message = null

New timeout but no new TestExpection?
Comment 9 youenn fablet 2017-11-03 15:56:57 PDT
Created attachment 325980 [details]
Patch
Comment 10 youenn fablet 2017-11-03 16:08:45 PDT
Created attachment 325981 [details]
Patch
Comment 11 Chris Dumez 2017-11-03 16:35:38 PDT
Comment on attachment 325981 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:181
> +    ScriptExecutionContext& context = *scriptExecutionContext();

This is unsafe. scriptExecutionContext() can return null.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:192
> +    return m_swConnection->matchRegistration(context.topOrigin(), parsedURL, [promise = WTFMove(promise), protectedThis = makeRef(*this), this] (auto&& result) mutable {

I think we should take a PendingActivity here.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:201
> +            auto& context = *scriptExecutionContext();

scriptExecutionContext() can return null.
Comment 12 youenn fablet 2017-11-03 16:41:16 PDT
Comment on attachment 325981 [details]
Patch

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

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:192
>> +    return m_swConnection->matchRegistration(context.topOrigin(), parsedURL, [promise = WTFMove(promise), protectedThis = makeRef(*this), this] (auto&& result) mutable {
> 
> I think we should take a PendingActivity here.

OK, will do and change canSuspend implementation.

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:201
>> +            auto& context = *scriptExecutionContext();
> 
> scriptExecutionContext() can return null.

We actually checked whether we were stopped above, so we are fine.
Comment 13 Chris Dumez 2017-11-03 16:44:14 PDT
Comment on attachment 325981 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerContainer.h:106
> +    HashMap<ServiceWorkerRegistrationKey, RefPtr<ServiceWorkerRegistration>> m_registrations;

Oh, and I think the value should be a Ref<>
Comment 14 youenn fablet 2017-11-03 17:03:21 PDT
> > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:181
> > +    ScriptExecutionContext& context = *scriptExecutionContext();
> 
> This is unsafe. scriptExecutionContext() can return null.

I don't think that this case can happen when being called from JS binding since there we just created a promise.
I guess this might happen when being called from other C++ code but this is clearly not the intent.
Adding an extra if might not be that bad.
Comment 15 youenn fablet 2017-11-03 17:07:27 PDT
I will keep the patch with the current registration lifetime model.
I think we will need to change it so that ServiceWorkerContainer owns registrations objects.
Management will then be based on orders from the StorageProcess.
Not keeping refs is observable from pages when keeping JS workers references but not JS registrations.
Comment 16 youenn fablet 2017-11-03 17:19:54 PDT
Created attachment 325993 [details]
Patch
Comment 17 youenn fablet 2017-11-03 18:25:00 PDT
Created attachment 325997 [details]
Patch
Comment 18 Chris Dumez 2017-11-03 18:45:42 PDT
Comment on attachment 325997 [details]
Patch

r=me if the bots are happy.
Comment 19 Chris Dumez 2017-11-03 19:12:11 PDT
Comment on attachment 325997 [details]
Patch

Clearing flags on attachment: 325997

Committed r224453: <https://trac.webkit.org/changeset/224453>
Comment 20 Chris Dumez 2017-11-03 19:12:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2017-11-15 12:17:17 PST
<rdar://problem/35567260>