RESOLVED FIXED 150515
Safari background tabs should be fully suspended where possible.
https://bugs.webkit.org/show_bug.cgi?id=150515
Summary Safari background tabs should be fully suspended where possible.
Kat Graff
Reported 2015-10-23 15:10:27 PDT
Suspending all activity on background tabs where possible to improve performance for users who use multiple tabs.
Attachments
Patch (35.14 KB, patch)
2015-11-16 18:30 PST, Kat Graff
no flags
Archive of layout-test-results from ews103 for mac-mavericks (243.97 KB, application/zip)
2015-11-16 19:10 PST, Build Bot
no flags
Archive of layout-test-results from ews115 for mac-yosemite (322.07 KB, application/zip)
2015-11-16 19:16 PST, Build Bot
no flags
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (195.61 KB, application/zip)
2015-11-16 19:19 PST, Build Bot
no flags
Patch (24.81 KB, patch)
2015-11-20 16:29 PST, Kat Graff
no flags
Patch (17.21 KB, patch)
2015-12-10 04:27 PST, Kat Graff
no flags
Patch (17.12 KB, patch)
2015-12-10 12:58 PST, Kat Graff
no flags
Patch (16.89 KB, patch)
2015-12-10 13:02 PST, Kat Graff
no flags
Patch (16.73 KB, patch)
2015-12-10 19:44 PST, Kat Graff
no flags
Patch (16.26 KB, patch)
2015-12-11 02:11 PST, Kat Graff
no flags
Patch (16.37 KB, patch)
2015-12-11 11:59 PST, Kat Graff
no flags
Patch (19.22 KB, patch)
2015-12-11 23:16 PST, Kat Graff
no flags
Patch (19.21 KB, patch)
2015-12-11 23:40 PST, Kat Graff
no flags
Patch (19.24 KB, patch)
2015-12-11 23:48 PST, Kat Graff
no flags
Patch (19.24 KB, patch)
2015-12-12 00:05 PST, Kat Graff
no flags
Geoffrey Garen
Comment 1 2015-10-26 09:51:01 PDT
Let's collect some data on web apps that have user-facing features while in background tabs, what the features are, and how they're implemented. Hopefully, we can design a way to pause tabs without breaking these features. I'll start with Facebook. Feature: Chat. Behavior: Play a sound and flash the titlebar when you get a new message. Implementation: ???
Kat Graff
Comment 2 2015-11-16 18:30:06 PST
WebKit Commit Bot
Comment 3 2015-11-16 18:32:16 PST
Attachment 265649 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kat Graff
Comment 4 2015-11-16 18:43:06 PST
Just looking for some feedback, especially re: terrible names.
Ryosuke Niwa
Comment 5 2015-11-16 19:03:42 PST
Comment on attachment 265649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265649&action=review We should add a WebKit API test for this (see Tools/TestWebKitAPI). > Source/WebCore/dom/Document.cpp:4559 > + for (auto* element : m_documentSuspensionCallbackElements) > + element->prepareElementForDocumentSuspension(); You shouldn't duplicate all these code in documentWillSuspendForPageCache. > Source/WebCore/dom/Document.cpp:4568 > + m_frame->animation().suspendAnimationsForDocument(this); It seems like suspending animation is also a good thing for page cache. > Source/WebCore/dom/Document.cpp:4575 > + m_frame->clearTimers(); This function also calls suspendAnimationsForDocument. Why do we need to call it twice? > Source/WebCore/dom/Document.cpp:4589 > + m_frame->view()->forceLayout(); Do we really need to always force layout each time we resume a document? It seems like we just need to remember whether there was a pending style recalc when we suspended the document, and schedule a recalc here if there was one. > Source/WebCore/dom/Document.cpp:4596 > + Vector<Element*> elements; Ditto about the code duplication. > Source/WebCore/dom/Document.cpp:6632 > - if (!frame() || !frame()->page()) > + if (!frame() || !frame()->page() || m_isTabSuspended) > return; We can't just ignore this timer. We need to call this function when we're resuming the document. We should do this by stopping m_didAssociateFormControlsTimer, and remembering that we stopped it. > Source/WebCore/dom/Document.h:1763 > + bool m_isTabSuspended; > + You should use { false } (new syntax) to initialize this boolean here instead of doing it manually in the constructor. > Source/WebCore/dom/Element.h:350 > - virtual void documentWillSuspendForPageCache() { } > - virtual void documentDidResumeFromPageCache() { } > + virtual void prepareElementForDocumentSuspension() { } > + virtual void resumeElementFromDocumentSuspension() { } Why don't we do this rename in a separate patch? The old name did follow WebKit naming convention of "will" and "did" nicely though. > Source/WebCore/page/DOMWindow.cpp:576 > +void DOMWindow::suspendForTabSuspension() Why do we need a separate version of these functions? What's the point of resuming & suspending between two suspension modes?
Build Bot
Comment 6 2015-11-16 19:10:28 PST
Comment on attachment 265649 [details] Patch Attachment 265649 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/439338 Number of test failures exceeded the failure limit.
Build Bot
Comment 7 2015-11-16 19:10:32 PST
Created attachment 265651 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 8 2015-11-16 19:16:12 PST
Comment on attachment 265649 [details] Patch Attachment 265649 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/439331 Number of test failures exceeded the failure limit.
Build Bot
Comment 9 2015-11-16 19:16:16 PST
Created attachment 265652 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 10 2015-11-16 19:19:39 PST
Comment on attachment 265649 [details] Patch Attachment 265649 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/439363 Number of test failures exceeded the failure limit.
Build Bot
Comment 11 2015-11-16 19:19:43 PST
Created attachment 265653 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Antti Koivisto
Comment 12 2015-11-16 20:11:29 PST
Comment on attachment 265649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265649&action=review I think the problems with page cache are partly because there are no clear state transitions between the three distinct suspension states (None, Tab, PageCache). If these we tracked more explicitly as mutually exclusive states and transitions between them implemented then I think many of them would disappear or at least become more understandable. Or maybe Tab and PageCache suspension could be exactly the same? > Source/WebCore/dom/Document.cpp:4515 > + if (!renderView()) > + return; > + > + renderView()->setIsInWindow(false); > + if (renderView()->usesCompositing()) > + renderView()->compositor().cancelCompositingLayerUpdate(); > } Could put renderView() to a local. > Source/WebCore/dom/Document.h:1762 > + bool m_isTabSuspended; Is this really needed? Suspension state should be per page. Can't we just keep the suspension state in a single place, in Page? Could use modern initialization: bool m_isTabSuspended { false }; > Source/WebCore/dom/Element.h:350 > + virtual void prepareElementForDocumentSuspension() { } > + virtual void resumeElementFromDocumentSuspension() { } Word 'Element' does not add information as these functions are scoped to class Element already. 'Document' doesn't add much either. Maybe prepareForSuspension/resumeFromSuspension? > Source/WebCore/loader/DocumentLoader.cpp:1355 > +// #if ENABLE(TAB_SUSPENSION) > + if (!document() || !document()->inPageCache()) > + return; > +/** #else > // A page in the PageCache should not be able to start loads. > ASSERT_WITH_SECURITY_IMPLICATION(!document() || !document()->inPageCache()); > +#endif **/ Tab suspension shouldn't affect page cached documents in any way. It should be mutually exclusive with page cache suspension and these assert shouldn't still be valid. > Source/WebCore/page/DOMWindow.cpp:397 > , m_suspendedForPageCache(false) > + , m_suspendedForTabSuspension(false) Can we just track suspension state in a single place, Page? Trying to keep all these booleans in sync seems bug prone and unnecessary. We shouldn't ever be both suspended for page and tab suspension. If these are needed they could be combined to something like enum class SuspensionState { None, Tab, PageCache } > Source/WebCore/page/DOMWindow.cpp:485 > - willDestroyDocumentInFrame(); > + if (!m_suspendedForPageCache) > + willDestroyDocumentInFrame(); Why change m_suspendedForPageCache case? > Source/WebCore/page/DOMWindow.cpp:583 > +void DOMWindow::suspendForTabSuspension() > +{ > + ASSERT(!m_suspendedForTabSuspension); > + if (m_suspendedForPageCache) > + resumeFromPageCache(); > + disconnectDOMWindowProperties(); > + m_suspendedForTabSuspension = true; > +} I don't understand this function. Why are we suspending a tab that is in page cache? Shouldn't that just be ASSERT(!m_suspendedForPageCache)? Is disconnectDOMWindowProperties is actually something we want to do to suspend tabs? > Source/WebCore/page/DOMWindow.h:376 > + bool m_suspendedForTabSuspension; Could use modern initialization: bool m_suspendedForTabSuspension { false }; > Source/WebCore/page/Page.cpp:1270 > void Page::setViewState(ViewState::Flags viewState) > { > + if (m_isTabSuspended && !viewState) > + return; Why this? > Source/WebCore/page/Page.cpp:1304 > + if (!isVisible()) { > + if (canSuspendPageForTabSuspension()) > + suspendPageForTabSuspension(); > + } else > + resumePageFromTabSuspension(); Why isn't isVisible() test just part of canSuspendPageForTabSuspension()? Instead of calling suspendPageForTabSuspension()/resumePageFromTabSuspension() from multiple places it might be safer and cleaner to have single updateTabSuspensionState() or similar that implements this logic. All places that may cause change in the suspension state would call it. > Source/WebCore/page/Page.cpp:1315 > + resumePageFromTabSuspension(); > + } else { > + setViewState((m_pageActivityState & ~(ViewState::IsVisible | ViewState::IsVisibleOrOccluded)) | ViewState::IsVisuallyIdle); > + if (canSuspendPageForTabSuspension()) > + suspendPageForTabSuspension(); This would call it. > Source/WebCore/page/Page.cpp:1357 > + if (canSuspendPageForTabSuspension()) > + suspendPageForTabSuspension(); And this function. > Source/WebCore/page/Page.cpp:1836 > +bool Page::canSuspendPageForTabSuspension() > +{ > + return !(m_pageActivityState & (PageActivityState::AudiblePlugin | PageActivityState::PageLoadActivity)) && PageCache::singleton().canCache(this); > +} This would be more readable and expandable as multiline function if (m_pageActivityState & (PageActivityState::AudiblePlugin | PageActivityState::PageLoadActivity)) return false; if (!PageCache::singleton().canCache(this)) return false; return true; > Source/WebCore/page/Page.cpp:1848 > + setIsTabSuspended(true); You probably don't need a setter at all since you are only changing the variable internally in Page. > Source/WebCore/page/Page.cpp:1861 > + setIsTabSuspended(false); Same here. > Source/WebCore/page/Page.h:654 > + bool m_isTabSuspended; Could use modern initialization: bool m_isTabSuspended { false }; > Source/WebCore/page/Page.h:656 > + void setIsTabSuspended(bool isTabSuspended) { m_isTabSuspended = isTabSuspended; } Shouldn't be needed.
Kat Graff
Comment 13 2015-11-20 16:29:29 PST
Ryosuke Niwa
Comment 14 2015-11-20 16:41:39 PST
Comment on attachment 266016 [details] Patch r=me assuming it builds.
Kat Graff
Comment 15 2015-12-10 04:27:24 PST
Kat Graff
Comment 16 2015-12-10 12:58:45 PST
Kat Graff
Comment 17 2015-12-10 13:02:23 PST
Ryosuke Niwa
Comment 18 2015-12-10 14:36:52 PST
Comment on attachment 267122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267122&action=review I don't think the approach of suspending a tab synchronously when the viability state changes is sound. We probably want to schedule a one shot timer when the visibility state changes, and suspend the page only when that timer fires. > Source/WebCore/dom/Document.cpp:4525 > + suspendActiveDOMObjects(ActiveDOMObject::PageCache); > + suspendScriptedAnimationControllerCallbacks(); Why did you change the order in which these functions are called? You should explain that in per-function change log comment if it's intentional. Also, it's weird that documentWillSuspend suspend active dom objects but documentDidResume does not resume. I think it's better to keep calling these two functions in the caller side instead. > Source/WebCore/dom/Document.cpp:4548 > + page()->lockAllOverlayScrollbarsToHidden(true); Why is this only needed in the tab suspension case? It seems that Document::setInPageCache already calls this. Why can't we just call this in documentWillSuspend instead? > Source/WebCore/dom/Document.cpp:4554 > + m_visualUpdatesAllowed = false; > + m_visualUpdatesSuppressionTimer.stop(); > + Why can't we do this in the page cache case? > Source/WebCore/dom/Document.cpp:4556 > + ASSERT(m_frame); > + m_frame->clearTimers(); It seems like the page cache code also clears timers. Why can't we move this into documentWillSuspend? > Source/WebCore/dom/Document.cpp:4569 > + documentDidResume(); > + > + resumeActiveDOMObjects(ActiveDOMObject::PageWillBeSuspended); > + resumeScriptedAnimationControllerCallbacks(); > + > + ASSERT(m_frame); > + m_frame->animation().resumeAnimationsForDocument(this); > + > + m_visualUpdatesAllowed = true; Ditto. All of these changes should be sharable with page cache code. > Source/WebCore/history/CachedFrame.cpp:139 > + if (frame.page()->isTabSuspended()) > + frame.page()->setIsTabSuspended(false); How can a page be in a page cache and be tab-suspended given we're already clearing this flag in CachedFrame::CachedFrame? And why is it okay for it to be resumed as if it was in the page cache? > Source/WebCore/history/CachedFrame.cpp:172 > + if (frame.page()->isTabSuspended()) > + frame.page()->setIsTabSuspended(false); Why is it okay to pretend as if the page had never been tab-suspended here? > Source/WebCore/loader/FrameLoader.cpp:1825 > + if (m_frame.page()->isTabSuspended()) > + m_frame.page()->setIsTabSuspended(false); When can we get into this state? Either CachedFrameBase::restore or this function is getting called first, and only one or the other should be needed. > Source/WebCore/page/Page.cpp:146 > +bool Page::s_tabSuspension = false; I would call this s_tabSuspensionIsEnabled. > Source/WebCore/page/Page.cpp:1301 > + m_pageActivityState = activityState; > + setIsTabSuspended(!isVisible()); I don't think it's safe to synchronously suspend a page here in the case a media RefCounter token issued by PageThrottler got deleted because that could happen during a page destruction and other undesirable timing. > Source/WebCore/page/Page.cpp:1311 > + > + setIsTabSuspended(!isVisible); setViewState calls setIsVisibleInternal so why is this even needed? Also, this function appears to be only called in WebKit1. > Source/WebCore/page/Page.cpp:1350 > + > + setIsTabSuspended(!isVisible); Again, I don't think we can synchronously suspend a document here. For example, a page created by pre-render will be an invisible page but I don't think we want to suspend that here. But we can't check whether we're in the pre-render mode or not until setIsPrersnder is called in WebPage::WebPage (WebKit2/WebProcess/WebPage/WebPage.cpp). > Source/WebCore/page/Page.cpp:1836 > + return s_tabSuspension && !(m_pageActivityState & PageActivityState::UnsuspendableFlags) && PageCache::singleton().canCache(this); We should also exclude pre-render pages. > Source/WebCore/page/Page.cpp:1859 > +void Page::setTabSuspension(bool enable) should be called setTabSuspensionEnabled. > Source/WebCore/page/PageThrottler.h:54 > static const Flags AllFlags = UserInputActivity | AudiblePlugin | MediaActivity | PageLoadActivity; > + static const Flags UnsuspendableFlags = MediaActivity | PageLoadActivity; Why is it okay to suspend when we have an audible plugin and/or user input activity?
Kat Graff
Comment 19 2015-12-10 19:44:37 PST
Ryosuke Niwa
Comment 20 2015-12-10 20:39:29 PST
Comment on attachment 267153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267153&action=review > Source/WebCore/dom/Document.cpp:4507 > + if (m_isSuspended) > + return; > + You should explain why you're adding this check in the change log as a per-function comment. > Source/WebCore/dom/Document.cpp:4520 > + page()->lockAllOverlayScrollbarsToHidden(true); It's a bit weird that we lock overlay scrollbars whenever a page is in the background but I suppose we can fix it later if someone complains. > Source/WebCore/loader/FrameLoader.cpp:1823 > + You should remove this superfluous change. > Source/WebCore/page/Page.cpp:1835 > + return s_tabSuspensionIsEnabled && !m_isPrerender && !m_pageActivityState && PageCache::singleton().canCache(this); You should compare m_pageActivityState against NoFlags. > Source/WebCore/page/Page.cpp:1868 > +void Page::timerFired() This function name is too generic for Page. How about tabSuspensionTimerFired? > Source/WebCore/page/Page.cpp:1870 > + setIsTabSuspended(m_isTabSuspended); We should check canTabSuspend() again here because it may have changed since the last time we checked. Also, we should probably not call setIsTabSuspended if m_isTabSuspended was false. > Source/WebCore/page/Page.h:662 > + Timer m_suspensionTimer; For consistency, this timer should probably be called m_tabSuspensionTimer.
Kat Graff
Comment 21 2015-12-11 02:11:43 PST
Ryosuke Niwa
Comment 22 2015-12-11 03:16:42 PST
Comment on attachment 267167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267167&action=review > Source/WebKit2/ChangeLog:7 > + You should mention that you're adding a runtime flag to enable this feature via user defaults. > Source/WebCore/page/Page.cpp:1855 > +void Page::scheduleTabSuspension(bool shouldSuspend) It's weird for a function that can synchronously resume a document to be called scheduleTabSuspension but I can't think of a better name now. > Source/WebCore/page/Page.cpp:1870 > +void Page::timerFired() This should be renamed to tabSuspensionTimerFired.
Kat Graff
Comment 23 2015-12-11 11:59:16 PST
Chris Dumez
Comment 24 2015-12-11 12:18:44 PST
Comment on attachment 267181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267181&action=review > Source/WebCore/page/Page.cpp:146 > +bool Page::s_tabSuspensionIsEnabled = false; Shouldn't this be a setting instead? > Source/WebCore/page/Page.cpp:227 > + , m_tabSuspensionTimer(*this, &Page::tabSuspensionTimerFired) nit: Could be a lambda to avoid adding a new method, considering the implementation is trivial. > Source/WebCore/page/Page.cpp:1297 > + if (m_pageActivityState == activityState) How can this happen? I see one call site in PageThrottler::setActivityFlag() and it already does this check. > Source/WebCore/page/Page.cpp:1301 > + m_pageActivityState = activityState; Seems to me we could ask m_pageThrottler for the current pageActivityState when needed and avoid adding a data member to Page for this. > Source/WebCore/page/Page.cpp:1850 > +void Page::setTabSuspensionEnabled(bool enable) setting? > Source/WebCore/page/Page.cpp:1867 > + m_isTabSuspensionScheduled = shouldSuspend; This seems confusing. If canTabSuspend() returns false, this may be true even though we did not schedule anything. I think the problem is the naming, how about m_shouldSuspendTab? > Source/WebCore/page/Page.h:663 > + bool canTabSuspend(); We normally try to avoid mixing methods and members. The methods should probably be declared higher in the class.
Kat Graff
Comment 25 2015-12-11 23:16:32 PST
Ryosuke Niwa
Comment 26 2015-12-11 23:22:58 PST
Comment on attachment 267222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267222&action=review > Source/WebCore/history/PageCache.cpp:152 > + if (frame.document()&&!frame.document()->canSuspendActiveDOMObjectsForDocumentSuspension(&unsuspendableObjects)) { Nit: Need spaces around &&. > Source/WebCore/page/Page.cpp:1840 > + for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) { We can use auto* for frame as well. > Source/WebCore/page/Page.cpp:1874 > + setIsTabSuspended(m_shouldTabSuspend); We don't need to check m_shouldTabSuspend since m_shouldTabSuspend should always be true at this point. Why don't we ASSERT(m_shouldTabSuspend) instead?
Kat Graff
Comment 27 2015-12-11 23:40:39 PST
Ryosuke Niwa
Comment 28 2015-12-11 23:44:35 PST
Comment on attachment 267224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267224&action=review > Source/WebCore/page/Page.cpp:1299 > + if (!activityState && !isVisible()) We should check against PageActivityState::NoFlags as well.
Kat Graff
Comment 29 2015-12-11 23:48:04 PST
Kat Graff
Comment 30 2015-12-12 00:05:56 PST
WebKit Commit Bot
Comment 31 2015-12-12 01:27:06 PST
Comment on attachment 267227 [details] Patch Clearing flags on attachment: 267227 Committed r194006: <http://trac.webkit.org/changeset/194006>
WebKit Commit Bot
Comment 32 2015-12-12 01:27:14 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.