Bug 123757

Summary: Subresource loads should not prevent page throttling
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: WebCore Misc.Assignee: Gavin Barraclough <barraclough>
Severity: Normal CC: commit-queue, eflews.bot, gtk-ews, gyuyoung.kim, japhet, rego+ews, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
darin: review+, eflews.bot: commit-queue-
New fix ap: review+

Description Gavin Barraclough 2013-11-04 14:56:25 PST
Some pages leave long running XHRs open, thee should not inhibit processes suspension.
Comment 1 Gavin Barraclough 2013-11-04 15:01:02 PST
Created attachment 215956 [details]
Comment 2 EFL EWS Bot 2013-11-04 15:33:01 PST
Comment on attachment 215956 [details]

Attachment 215956 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/20328760
Comment 3 EFL EWS Bot 2013-11-04 15:51:08 PST
Comment on attachment 215956 [details]

Attachment 215956 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/19598874
Comment 4 kov's GTK+ EWS bot 2013-11-04 15:52:44 PST
Comment on attachment 215956 [details]

Attachment 215956 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/19648854
Comment 5 Darin Adler 2013-11-04 17:19:32 PST
Comment on attachment 215956 [details]

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

Patch looks fine, but seems you missed call sites in the GTK and EFL ports.

> Source/WebCore/loader/ResourceLoaderOptions.h:82
> +    ResourceLoaderOptions(SendCallbackPolicy sendLoadCallbacks, ContentSniffingPolicy sniffContent, DataBufferingPolicy dataBufferingPolicy, StoredCredentials allowCredentials, ClientCredentialPolicy credentialPolicy, SecurityCheckPolicy securityCheck, RequestOriginPolicy requestOriginPolicy, RequestIsXMLHTTP requestIsXMLHTTP)

Good that this argument is an enum.

> Source/WebCore/loader/ResourceLoaderOptions.h:100
> +    RequestIsXMLHTTP requestIsXMLHTTP;

But seems to me this should be a bool.
Comment 6 Gavin Barraclough 2013-11-05 13:23:07 PST
Maciej doesn't like treating XHRs differently from other requests, and has suggested simplifying the page-is-loading check to simply be frame loading + hysteresis. New patch forthcoming.
Comment 7 Gavin Barraclough 2013-11-05 13:30:33 PST
Created attachment 216069 [details]
New fix
Comment 8 Alexey Proskuryakov 2013-11-05 15:15:26 PST
Comment on attachment 216069 [details]
New fix

Looks like we should question whether we should take an activity assertion during frame loads either. If we don't need them for subresources, maybe we don't need it for any loads?

Comment 9 Gavin Barraclough 2013-11-05 17:25:03 PST
Fixed in r158702