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
youenn fablet
2017-11-03 11:54:56 PDT
Created attachment 325932 [details]
Patch
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 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 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 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
Created attachment 325952 [details]
Patch
Created attachment 325972 [details]
Moving the map
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? Created attachment 325980 [details]
Patch
Created attachment 325981 [details]
Patch
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 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 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<> > > 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.
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. Created attachment 325993 [details]
Patch
Created attachment 325997 [details]
Patch
Comment on attachment 325997 [details]
Patch
r=me if the bots are happy.
Comment on attachment 325997 [details] Patch Clearing flags on attachment: 325997 Committed r224453: <https://trac.webkit.org/changeset/224453> All reviewed patches have been landed. Closing bug. |