Bug 143505 – Open WebSockets should not prevent a page from entering PageCache

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Open WebSockets should not prevent a page from entering PageCache. This is currently causing mobile.nytimes.com to not be page-cacheable.

We could close the WebSocket when entering the page cache and fire the "close" event after resuming, similarly to what we did for XMLHttpRequest in Bug 142612.

Radar: <rdar://problem/19923085>

[Chris Dumez]

Created attachment 250319 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 250319 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:481
> +            m_channel->suspend();

Is DocumentWillBecomeInactive the only reason for which canSuspend is respected, or are you changing the behavior for other suspension reasons too? If the latter, how did you confirm that it's right?

[Chris Dumez]

Comment on attachment 250319 [details]
Patch

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

>> Source/WebCore/Modules/websockets/WebSocket.cpp:481
>> +            m_channel->suspend();
> 
> Is DocumentWillBecomeInactive the only reason for which canSuspend is respected, or are you changing the behavior for other suspension reasons too? If the latter, how did you confirm that it's right?

I am not changing the behavior for other suspensions than page cache as far as I know. canSuspend() is only called from ScriptExecutionContext::canSuspendActiveDOMObjects(), which is only called from PageCache::canCachePageContainingThisFrame(). When PageCache::canCache() returns true, we call PageCache::add() which constructs a CachedPage and CachedFrame. In the CachedFrame constructor, we call Document::suspendActiveDOMObjects(ActiveDOMObject::DocumentWillBecomeInactive); This is the only place this ReasonForSuspension is used.

Also note the following comment in ActiveDOMObject.h:
virtual bool canSuspend() const = 0; // Returning false in canSuspend() will prevent the page from entering the PageCache.

This time the comment seems accurate :)

[Alexey Proskuryakov]

Makes sense! We should upgrade the comment to a function name (something like canSuspendForPageCache). And ActiveDOMObject::DocumentWillBecomeInactive is a suspicious name too.

[Chris Dumez]

(In reply to comment #4)
> Makes sense! We should upgrade the comment to a function name (something
> like canSuspendForPageCache). And
> ActiveDOMObject::DocumentWillBecomeInactive is a suspicious name too.

Good idea, I'll do the renaming in a follow-up patch.

[WebKit Commit Bot]

Comment on attachment 250319 [details]
Patch

Clearing flags on attachment: 250319

Committed r182517: <http://trac.webkit.org/changeset/182517>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.