Bug 179253 - Implement ServiceWorkerContainer.getRegistration
Summary: Implement ServiceWorkerContainer.getRegistration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 179270
  Show dependency treegraph
 
Reported: 2017-11-03 11:54 PDT by youenn fablet
Modified: 2017-11-15 12:17 PST (History)
7 users (show)

See Also:


Attachments
Patch (26.74 KB, patch)
2017-11-03 12:20 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.41 MB, application/zip)
2017-11-03 13:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (1.12 MB, application/zip)
2017-11-03 13:32 PDT, Build Bot
no flags Details
Patch (28.63 KB, patch)
2017-11-03 13:42 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Moving the map (36.13 KB, patch)
2017-11-03 15:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (36.22 KB, patch)
2017-11-03 15:56 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (36.21 KB, patch)
2017-11-03 16:08 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (37.73 KB, patch)
2017-11-03 17:19 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (37.64 KB, patch)
2017-11-03 18:25 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>