Bug 180566

Summary: Service Worker should use a correct user agent
Product: WebKit Reporter: youenn fablet <youennf>
Component: Service WorkersAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, commit-queue, dbates, ews-watchlist, ggaren, japhet, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Addendum none

Description youenn fablet 2017-12-07 19:05:45 PST
Currently, it is the empty string
Comment 1 Radar WebKit Bug Importer 2017-12-07 19:06:02 PST
<rdar://problem/35926295>
Comment 2 youenn fablet 2017-12-08 13:26:25 PST
Created attachment 328861 [details]
Patch
Comment 3 EWS Watchlist 2017-12-08 13:28:36 PST
Attachment 328861 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h:91:  The parameter name "userAgent" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:74:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 youenn fablet 2017-12-08 14:31:34 PST
Created attachment 328870 [details]
Patch
Comment 5 EWS Watchlist 2017-12-08 14:34:35 PST
Attachment 328870 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h:90:  The parameter name "userAgent" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:74:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 youenn fablet 2017-12-08 15:20:37 PST
Created attachment 328877 [details]
Patch
Comment 7 EWS Watchlist 2017-12-08 15:21:50 PST
Attachment 328877 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h:90:  The parameter name "userAgent" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:74:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Chris Dumez 2017-12-08 15:24:53 PST
Comment on attachment 328870 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:2355
> +    process().processPool().updateServiceWorkerUserAgent(userAgent());

Seems like it would be more robust if this were in WebPageProxy::setUserAgent(), no?
Also, this looks hackish so I think it deserves a FIXME comment.

> Source/WebKit/UIProcess/WebProcessPool.cpp:1030
> +    updateServiceWorkerUserAgent(pageProxy->userAgent());

Won't this temporarily reset the SW user agent to some default one until WebPageProxy::setUserAgent() gets called with the proper one?

Given that the SW process is shared between all pages, I slightly worry about frequent changes to the SW process user agent (especially with sometimes a wrong user agent). A ServiceWorker may get spawned at any time, and one it is spawned, its user agent persists. Therefore, if it ended up using a bad user agent (bad luck), it is stuck with this bad user agent.
Comment 9 youenn fablet 2017-12-08 16:18:11 PST
Created attachment 328882 [details]
Patch
Comment 10 EWS Watchlist 2017-12-08 16:20:27 PST
Attachment 328882 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h:90:  The parameter name "userAgent" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:74:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 WebKit Commit Bot 2017-12-08 17:06:02 PST
Comment on attachment 328882 [details]
Patch

Clearing flags on attachment: 328882

Committed r225716: <https://trac.webkit.org/changeset/225716>
Comment 12 WebKit Commit Bot 2017-12-08 17:06:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 youenn fablet 2017-12-08 21:04:30 PST
Reopening to attach new patch.
Comment 14 youenn fablet 2017-12-08 21:04:32 PST
Created attachment 328901 [details]
Addendum
Comment 15 youenn fablet 2017-12-08 21:05:28 PST
(In reply to youenn fablet from comment #14)
> Created attachment 328901 [details]
> Addendum

We actually need to set the user agent when starting the SW for Safari.
Comment 16 WebKit Commit Bot 2017-12-08 21:36:40 PST
Comment on attachment 328901 [details]
Addendum

Clearing flags on attachment: 328901

Committed r225720: <https://trac.webkit.org/changeset/225720>
Comment 17 WebKit Commit Bot 2017-12-08 21:36:42 PST
All reviewed patches have been landed.  Closing bug.