Bug 187153 – Early return when handling fetch event in case service worker origin does not match origin of a subresource load

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Early return when handling fetch event in case service worker origin does not match origin of a subresource load

[youenn fablet]

<rdar://problem/41329832>

[youenn fablet]

Created attachment 343841 [details]
Patch

[Chris Dumez]

Comment on attachment 343841 [details]
Patch

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

> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:190
> +    RELEASE_LOG_ERROR(ServiceWorker, "%{public}s: service worker is %{public}s://...:%d, request URL is %{public}s://...:%d", message, serviceWorkerURL.protocol().utf8().data(), serviceWorkerPort, requestOriginURL.protocol().utf8().data(), requestOriginURLPort);

I do not believe logging URLs is OK.

[youenn fablet]

Created attachment 343850 [details]
Patch

[Chris Dumez]

Comment on attachment 343850 [details]
Patch

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

> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:195
> +        RELEASE_ASSERT_WITH_MESSAGE(request.url().host() == serviceWorkerURL.host(), "Hosts do not match");

I think we may want this one first as it is the most important, no?

> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:205
> +        ASSERT(url.host() == serviceWorkerURL.host());

ditto.

[youenn fablet]

Created attachment 343860 [details]
Patch for landing

[youenn fablet]

> > Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:195
> > +        RELEASE_ASSERT_WITH_MESSAGE(request.url().host() == serviceWorkerURL.host(), "Hosts do not match");
> 
> I think we may want this one first as it is the most important, no?

Done

[WebKit Commit Bot]

Comment on attachment 343860 [details]
Patch for landing

Clearing flags on attachment: 343860

Committed r233335: <https://trac.webkit.org/changeset/233335>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It looks like after r233335 this test became a flakey timeout:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fclaim-with-redirect.https.html

I built r233335 locally and ran:
run-webkit-tests imported/w3c/web-platform-tests/service-workers/service-worker/claim-with-redirect.https.html --iterations 500 -f

and out of 500 runs got 18 timeouts. I reproduced this multiple times.

I built r233334 locally and ran the same command and received 0 timeout through multiple runs.

[youenn fablet]

(In reply to Truitt Savell from comment #10)
> It looks like after r233335 this test became a flakey timeout:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-
> workers%2Fservice-worker%2Fclaim-with-redirect.https.html

That is great news!!

We are crashing in debug builds:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000207754020 WTFCrash + 16 (Assertions.cpp:267)
1   com.apple.WebKit              	0x0000000106b766d3 WebKit::isValidFetch(WebCore::ResourceRequest const&, WebCore::FetchOptions const&, WebCore::URL const&, WTF::String const&) + 1219 (WebSWContextManagerConnection.cpp:204)
2   com.apple.WebKit              	0x0000000106b75fad WebKit::WebSWContextManagerConnection::startFetch(WTF::ObjectIdentifier<WebCore::SWServerConnectionIdentifierType>, WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, WTF::ObjectIdentifier<WebCore::FetchIdentifierType>, WebCore::ResourceRequest&&, WebCore::FetchOptions&&, IPC::FormDataReference&&, WTF::String&&) + 285 (WebSWContextManagerConnection.cpp:226)

[Chris Dumez]

(In reply to youenn fablet from comment #11)
> (In reply to Truitt Savell from comment #10)
> > It looks like after r233335 this test became a flakey timeout:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-
> > workers%2Fservice-worker%2Fclaim-with-redirect.https.html
> 
> That is great news!!
> 
> We are crashing in debug builds:
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.JavaScriptCore      	0x0000000207754020 WTFCrash + 16
> (Assertions.cpp:267)
> 1   com.apple.WebKit              	0x0000000106b766d3
> WebKit::isValidFetch(WebCore::ResourceRequest const&, WebCore::FetchOptions
> const&, WebCore::URL const&, WTF::String const&) + 1219
> (WebSWContextManagerConnection.cpp:204)
> 2   com.apple.WebKit              	0x0000000106b75fad
> WebKit::WebSWContextManagerConnection::startFetch(WTF::
> ObjectIdentifier<WebCore::SWServerConnectionIdentifierType>,
> WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>,
> WTF::ObjectIdentifier<WebCore::FetchIdentifierType>,
> WebCore::ResourceRequest&&, WebCore::FetchOptions&&,
> IPC::FormDataReference&&, WTF::String&&) + 285
> (WebSWContextManagerConnection.cpp:226)

So the host is not even matching :/ Seems bad :)

[youenn fablet]

OK, this is just a mistake made in this patch, fix is at https://bugs.webkit.org/show_bug.cgi?id=187282