A service worker process should app nap when all its clients app nap, for performance / power reasons. Currently, App Nap is disabled for service worker processes.
<rdar://problem/46785908>
Created attachment 369227 [details] Patch
Created attachment 369298 [details] Patch
Attachment 369298 [details] did not pass style-queue: ERROR: LayoutTests/platform/ios-wk2/TestExpectations:368: Path does not exist. [test/expectations] [5] ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/ios-wk2/TestExpectations:368: Path does not exist. [test/expectations] [5] Total errors found: 2 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 369298 [details] Patch Attachment 369298 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12125261 New failing tests: http/tests/css/filters-on-iframes.html
Created attachment 369306 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 369330 [details] Patch
Attachment 369330 [details] did not pass style-queue: ERROR: LayoutTests/platform/ios-wk2/TestExpectations:368: Path does not exist. [test/expectations] [5] ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/ios-wk2/TestExpectations:368: Path does not exist. [test/expectations] [5] Total errors found: 2 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 369330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369330&action=review This adds a lot of complexity for what it does. Could we try and find a simpler model with less code complexity and less changes of introducing bugs? For example, could we consider not worrying about the client's state and take a user activity for a while every time we ask something from the service worker? (e.g. intercept a load, start the user activity until the load is done, on a hysteresis. postMessage() take a user activity for a while). Basically let the service worker process manage its own process suppression state based on what it is asked to do, rather than worry about the clients. A client could be playing audio and thus not be suitable for app nap and yet not using its service worker process at all. The fact is that a lot of service workers are mostly useful during initial page load. > Source/WebCore/page/Page.h:721 > + bool processSuppressionEnabled() const { return m_processSuppressionEnabled; } Seems like a layer violation for Page to know about process pression, the WebPage already does and it should be enough. I also renamed process suppression to app nap recently for clarity.
(In reply to Chris Dumez from comment #9) > Comment on attachment 369330 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369330&action=review > > This adds a lot of complexity for what it does. Could we try and find a > simpler model with less code complexity and less changes of introducing > bugs? A part of the patch is very similar to what we will need to do to support client visibility. A part of the patch could probably be simplified so that we take a per-service-worker process decision instead of storing a per service worker instance decision in the service worker process. > For example, could we consider not worrying about the client's state > and take a user activity for a while every time we ask something from the > service worker? (e.g. intercept a load, start the user activity until the > load is done, on a hysteresis. postMessage() take a user activity for a > while). Basically let the service worker process manage its own process > suppression state based on what it is asked to do, rather than worry about > the clients. I am unsure whether that this will be any simpler. That could mean that any appnapped page that wants to do processing without app nap could do that in its service worker, provided it sends a postMessage every 30 or 40 seconds. To fix that, we would need to pipe the app nap state of the page in the fetch/postMessage events. We would also need to handle the case of a service worker that postMessage to itself to not be app-napped. There is also the case of a fetch event that takes more than 30 or 40 seconds to load. To handle it, we could keep the user activity until the fetch event is settled but then if the response is big or takes time to load, we would actually need to wait for the whole response to be loaded to not penalize the website. Another potential issue is service workers doing long lasting processing, say server side events where they do a fetch and each time they receive a new message, they post it to their web page. We would potentially penalize web pages using service workers to do that processing compared to a page doing that processing itself. The proposed model (compute service worker state based on its clients) seems simpler to understand and the implications more easy to understand. > > Source/WebCore/page/Page.h:721 > > + bool processSuppressionEnabled() const { return m_processSuppressionEnabled; } > > Seems like a layer violation for Page to know about process pression, the > WebPage already does and it should be enough. I also renamed process > suppression to app nap recently for clarity. This is done for having a simple implementation to enable app-nap from Internals. This can be implemented by adding a testRunner API with IPC from UIProcess to WebProcess to enable/disable app-nap.
Discussed this with Youenn offline and I think we have a plan to maintain the suggested behavior while simplifying the implementation a bit.
Created attachment 369578 [details] Patch
Created attachment 369599 [details] Patch
Comment on attachment 369599 [details] Patch Attachment 369599 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12157069 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star-with-AllowContentSecurityPolicySourceStarToMatchAnyProtocol-enabled.html
Created attachment 369613 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
ping review
Comment on attachment 369599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369599&action=review > Source/WebCore/workers/service/SWClientConnection.h:89 > + virtual void updateThrottleState() = 0; I don't think this needs to be public. Can probably be protected. > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:283 > + if (!m_isThrottleable) You're registering a new client, how come this cannot get us out of throttleable state?
Comment on attachment 369599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369599&action=review > Source/WebKit/WebProcess/WebProcess.h:272 > + bool arePagesThrottleable() const; I would have called this: "areAllPagesThrottleable"
Comment on attachment 369599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369599&action=review >> Source/WebCore/workers/service/SWClientConnection.h:89 >> + virtual void updateThrottleState() = 0; > > I don't think this needs to be public. Can probably be protected. This is used by WebPage to tells its connection to update its throttle state when the page state is changing. >> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:283 >> + if (!m_isThrottleable) > > You're registering a new client, how come this cannot get us out of throttleable state? I am not sure to understand your comment. A new client might be created by a page that is app napped (say a worker or an iframe), the WebSWServerConnection throttle state is updated elsewhere. As of the 'if check', the new client may disable the throttleability of its related context connection only if the new client is not throttleable.
Created attachment 369872 [details] Patch for landing
Comment on attachment 369872 [details] Patch for landing Clearing flags on attachment: 369872 Committed r245299: <https://trac.webkit.org/changeset/245299>
All reviewed patches have been landed. Closing bug.
This caused a nasty bug: https://bugs.webkit.org/show_bug.cgi?id=197902