Bug 150515 – Safari background tabs should be fully suspended where possible.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kat Graff]

Suspending all activity on background tabs where possible to improve performance for users who use multiple tabs.

[Geoffrey Garen]

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]

Created attachment 265649 [details]
Patch

[WebKit Commit Bot]

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]

Just looking for some feedback, especially re: terrible names.

[Ryosuke Niwa]

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 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]

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 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]

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 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]

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 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]

Created attachment 266016 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 266016 [details]
Patch

r=me assuming it builds.

[Kat Graff]

Created attachment 267092 [details]
Patch

[Kat Graff]

Created attachment 267120 [details]
Patch

[Kat Graff]

Created attachment 267122 [details]
Patch

[Ryosuke Niwa]

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]

Created attachment 267153 [details]
Patch

[Ryosuke Niwa]

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]

Created attachment 267167 [details]
Patch

[Ryosuke Niwa]

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]

Created attachment 267181 [details]
Patch

[Chris Dumez]

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]

Created attachment 267222 [details]
Patch

[Ryosuke Niwa]

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]

Created attachment 267224 [details]
Patch

[Ryosuke Niwa]

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]

Created attachment 267225 [details]
Patch

[Kat Graff]

Created attachment 267227 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 267227 [details]
Patch

Clearing flags on attachment: 267227

Committed r194006: <http://trac.webkit.org/changeset/194006>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.