Bug 179253 – Implement ServiceWorkerContainer.getRegistration

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Implement ServiceWorkerContainer.getRegistration

[youenn fablet]

Created attachment 325932 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 325952 [details]
Patch

[youenn fablet]

Created attachment 325972 [details]
Moving the map

[Chris Dumez]

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?

[youenn fablet]

Created attachment 325980 [details]
Patch

[youenn fablet]

Created attachment 325981 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

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.

[Chris Dumez]

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

[youenn fablet]

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

[youenn fablet]

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.

[youenn fablet]

Created attachment 325993 [details]
Patch

[youenn fablet]

Created attachment 325997 [details]
Patch

[Chris Dumez]

Comment on attachment 325997 [details]
Patch

r=me if the bots are happy.

[Chris Dumez]

Comment on attachment 325997 [details]
Patch

Clearing flags on attachment: 325997

Committed r224453: <https://trac.webkit.org/changeset/224453>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35567260>