client.postMessage delivers too early |
||||
Issue descriptionhttps://static-misc.glitch.me/resulting-client-message/ 1. Load the above page. 2. Refresh once service worker has installed. 3. See log. Log: Navigated to https://static-misc.glitch.me/resulting-client-message/ Registering service worker Got message Sent one second later https://static-misc.glitch.me/resulting-client-message/sw.js The "sent immediately" message is missing. According to the spec, messages should be buffered until the end (https://html.spec.whatwg.org/multipage/parsing.html#the-end step 4.2). This works as expected in Firefox Nightly.
,
Dec 18
Thanks, let's try to fix this in 73 as sending messages to the client is one of the primary use cases of resultingClientId.
,
Dec 18
If we could include startMessages(), that would be great.
,
Dec 18
Can you help me understand how startMessages relates to this? Delivering too early seems more severe because the message is lost right? startMessages() allows the document to declare it wants messages before DOMContentLoaded fires?
,
Dec 18
It's related, but yes they could be handled separately. I just thought if we were adding queueing it might not be hard to expose startMessages() at the same time.
,
Dec 28
,
Jan 21
(2 days ago)
I think I can pick this up.
,
Yesterday
(46 hours ago)
I thought this might be a quick fix but it looks like there are a several things broken here. The path from SW to page is: ServiceWorkerGlobalScopeClient::PostMessageToClient (renderer/sw) -> ServiceWorkerVersion::PostMessageToClient (browser) -> ServiceWorkerProviderHost::PostMessageToClient (browser) -> ServiceWorkerProviderContext::PostMessageToClient (renderer/page) -> WebServiceWorkerProviderImpl::PostMessageToClient (renderer/page) -> ServiceWorkerContainer::DispatchMessageEvent (renderer/page). 1. Clients.get() seems to resolve on "prerendering". If you use Jake's test link enough, eventually pasting the URL into the omnibox but before pressing enter will cause the SW to post message to a client that doesn't exist: ServiceWorkerVersion::PostMessageToClient bails at if (!provider_host). Clients.get() probably shouldn't resolve for prerendering since evidently we don't have a SWProviderHost for the client, so it's useless. Also this currently fails an incorrect DCHECK at GetWindowClientInfoOnUI: // Service workers do no prerender, this would be an invalid visibility state. // DCHECK_NE(visibility, PageVisibilityState::kPrerender); 2. ServiceWorkerProviderContext::PostMessageToClient drops the message if the |web_service_worker_provider| hasn't been set yet, which happens if the message comes too early. I.e., it doesn't call WebServiceWorkerProviderImpl::PostMessageToClient. 3. ServiceWorkerContainer::DispatchMessageEvent can dispatch the message event before DOMContentLoaded (as per the bug report). Setting aside the prerendering thing, looks like SWPC needs to queue up messages until WebSWProviderImpl/SWContainer is created, and SWContainer needs to queue up messages until DOMContentLoaded.
,
Yesterday
(25 hours ago)
Even with the queuing things don't work. SWContainer seems to be created before the Document even started parsing, so Document::HasFinishedParsing() returns true, which usually signifies that DOMContentLoaded was fired. SWContainer gets created at: #1 0x7f6a1a755fc5 blink::ServiceWorkerContainer::ServiceWorkerContainer() #2 0x7f6a1a751751 blink::MakeGarbageCollected<>() #3 0x7f6a1a751597 blink::ServiceWorkerContainer::From() #4 0x7f6a1a74bb1e blink::NavigatorServiceWorker::serviceWorker() #5 0x7f6a1a74b6f7 blink::NavigatorServiceWorker::serviceWorker() #6 0x7f6a1a74b48b blink::NavigatorServiceWorker::From() #7 0x7f6a1a74b3c4 blink::NavigatorServiceWorker::From() #8 0x7f6a1a085195 blink::ModulesInitializer::OnClearWindowObjectInMainWorld() #9 0x7f6a1e0052ef blink::LocalFrameClientImpl::DispatchDidClearWindowObjectInMainWorld() #10 0x7f6a1e752b26 blink::FrameLoader::DispatchDidClearWindowObjectInMainWorld() #11 0x7f6a1d8fb728 blink::LocalWindowProxy::Initialize() #12 0x7f6a1d8fd9e5 blink::LocalWindowProxy::UpdateDocument() #13 0x7f6a1d905d79 blink::ScriptController::UpdateDocument() #14 0x7f6a1e0fd8fd blink::LocalDOMWindow::InstallNewDocument() #15 0x7f6a1e73d5a1 blink::DocumentLoader::InstallNewDocument() #16 0x7f6a1e73bcb3 blink::DocumentLoader::CommitNavigation() #17 0x7f6a1e73b3e6 blink::DocumentLoader::CommitData() #18 0x7f6a1e73df5c blink::DocumentLoader::DataReceived() #19 0x7f6a1c738912 blink::Resource::NotifyDataReceived() #20 0x7f6a1c7385d1 blink::Resource::AppendData() #21 0x7f6a1c7313de blink::RawResource::AppendData() #22 0x7f6a1c7683a7 blink::ResourceLoader::DidReceiveData() #23 0x7f6a2364368a content::WebURLLoaderImpl::Context::OnReceivedData() #24 0x7f6a236446e7 content::WebURLLoaderImpl::RequestPeerImpl::OnReceivedData() #25 0x7f6a2363c44d content::URLResponseBodyConsumer::OnReadable() A little while later the Document starts parsing (SetParsingState is called to transition from kFinishedParsing(2) to kParsing(0)): #1 0x7f6a1dd8084a blink::Document::SetParsingState() #2 0x7f6a1dd80585 blink::Document::ImplicitOpen() #3 0x7f6a1dd808fc blink::Document::OpenForNavigation() #4 0x7f6a1e73d8ff blink::DocumentLoader::InstallNewDocument() #5 0x7f6a1e73bcb3 blink::DocumentLoader::CommitNavigation() #6 0x7f6a1e73b3e6 blink::DocumentLoader::CommitData() #7 0x7f6a1e73df5c blink::DocumentLoader::DataReceived() #8 0x7f6a1c738912 blink::Resource::NotifyDataReceived() #9 0x7f6a1c7385d1 blink::Resource::AppendData() #10 0x7f6a1c7313de blink::RawResource::AppendData() #11 0x7f6a1c7683a7 blink::ResourceLoader::DidReceiveData() #12 0x7f6a2364368a content::WebURLLoaderImpl::Context::OnReceivedData() #13 0x7f6a236446e7 content::WebURLLoaderImpl::RequestPeerImpl::OnReceivedData() #14 0x7f6a2363c44d content::URLResponseBodyConsumer::OnReadable() In the meantime once SWContainer is created, we dispatch the queued messages from step (2) above. We might just be resolving Clients.get() too early. Clients.get() should resolve when the client is execution ready, it seems weird to say it's execution ready when it's still doing things like "CommitNavigation" and "InstallNewDocument". Resolving Clients.get() is triggered by ServiceWorkerNavigationLoader construction, which happens on RenderFrameImpl::CommitNavigation which happens before DocumentLoader::CommitNavigation: #1 0x7f1974f551bd content::ServiceWorkerNetworkProvider::ServiceWorkerNetworkProvider() #2 0x7f1974f54995 content::ServiceWorkerNetworkProvider::CreateForNavigation() #3 0x7f1974ee0205 content::RenderFrameImpl::BuildServiceWorkerNetworkProviderForNavigation() #4 0x7f1974ee2a43 content::RenderFrameImpl::CommitNavigation() Execution ready is supposed to be set here: https://html.spec.whatwg.org/multipage/browsers.html#windows:concept-environment-execution-ready-flag Could there be a better place to do that?
,
Yesterday
(25 hours ago)
s/ServiceWorkerNavigationLoader/ServiceWorkerNetworkProvider
,
Today
(34 minutes ago)
Another discovery is that when the SWContainer doesn't get created because it's a sandboxed iframe, we still have SWNetworkProvider/SWProviderContext created. We're currently resolving Clients.get() but shouldn't do so since it's a different origin. If we delay Clients.get() to until SWContainer is created, Clients.get() will just never resolve. We should really reject it. This is related to issue 771815: my patch that delays resolving will make sandboxed-iframe-fetch-event.https.html pass because Clients.matchAll() doesn't include the client, but the bug described probably still exists. For sandboxed iframes, we should revoke the controller on the renderer side and tell the browser that this client does not have the origin it thinks it does. |
||||
►
Sign in to add a comment |
||||
Comment 1 by jakearchibald@chromium.org
, Dec 17