Bug 142612

Summary: XMLHttpRequests should not prevent a page from entering PageCache
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: XMLAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, barraclough, commit-queue, japhet, kling
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=143505
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2015-03-11 23:08:26 PDT
XMLHttpRequests should not prevent a page from entering PageCache.

Radar: <rdar://problem/19923085>
Comment 1 Chris Dumez 2015-03-11 23:22:05 PDT
Created attachment 248495 [details]
WIP Patch
Comment 2 Chris Dumez 2015-03-12 13:56:34 PDT
Created attachment 248541 [details]
Patch
Comment 3 Chris Dumez 2015-03-12 14:11:21 PDT
Created attachment 248544 [details]
Patch
Comment 4 Alexey Proskuryakov 2015-03-12 14:57:52 PDT
Comment on attachment 248544 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:111
> +        // Only image and XHR loads do prevent the page from entering the PageCache.
> +        switch (cachedResource->type()) {
> +        case CachedResource::RawResource:

Is this an accurate check for XHR? I think that there are more raw resources, such as icons, media, and maybe anything loaded by workers.

> Source/WebCore/xml/XMLHttpRequest.cpp:1298
> +        m_resumeTimer.startOneShot(0);

What if we are suspended again while the timer is pending?
Comment 5 Chris Dumez 2015-03-12 17:36:17 PDT
Created attachment 248557 [details]
Patch
Comment 6 Chris Dumez 2015-03-12 17:40:33 PDT
Comment on attachment 248544 [details]
Patch

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

>> Source/WebCore/loader/DocumentLoader.cpp:111
>> +        case CachedResource::RawResource:
> 
> Is this an accurate check for XHR? I think that there are more raw resources, such as icons, media, and maybe anything loaded by workers.

You are right, this is not strictly correct. I made it correct by making sure all the clients of the resource's clients are XHR. So there is now an isXHR() API on CachedResourceClient. This is a bit annoying but I did not find a better way to do it.

>> Source/WebCore/xml/XMLHttpRequest.cpp:1298
>> +        m_resumeTimer.startOneShot(0);
> 
> What if we are suspended again while the timer is pending?

Right, good catch. It is now handled in suspend().
Comment 7 Alexey Proskuryakov 2015-03-12 22:55:16 PDT
Comment on attachment 248557 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.h:99
> +        bool isXHR() const override final;

I don't think that we use this abbreviation in code often, I'd prefer "isXMLHttpRequest".

> Source/WebCore/loader/cache/CachedResource.h:161
> +    bool isLoadedViaXHROnly() const;

Maybe "allClientsAreXMLHttpRequests"?

> Source/WebCore/xml/XMLHttpRequest.cpp:1255
> +    // Dot not attempt to suspend loading XMLHttpRequests in workers threads for now.
> +    if (!is<Document>(*scriptExecutionContext()))
> +        return !m_loader;

Not sure if I prefer a runtime check or an assertion that this function isn't called in workers (it cannot be called there now).

> Source/WebCore/xml/XMLHttpRequest.cpp:1309
> +    dispatchErrorEvents(eventNames().errorEvent);
> +    m_dispatchErrorOnResuming = false;

Is this the right order? Error event handlers can do many things, and it's not clear to me that anything even guarantees that the object is still alive after returning from one. I think that an event handler can probably also start a new load using this XHR object, and synchronously navigate away from the page (with a javascript: URL navigation), in which case we'd incorrectly overwrite the value of m_dispatchErrorOnResuming.

> Source/WebCore/xml/XMLHttpRequest.cpp:1314
> +    NoEventDispatchAssertion assertNoEventDispatch;

I presume that you ran tests in a debug build locally.
Comment 8 Chris Dumez 2015-03-13 10:11:41 PDT
Created attachment 248588 [details]
Patch
Comment 9 Chris Dumez 2015-03-13 10:13:05 PDT
Comment on attachment 248557 [details]
Patch

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

>> Source/WebCore/xml/XMLHttpRequest.cpp:1314
>> +    NoEventDispatchAssertion assertNoEventDispatch;
> 
> I presume that you ran tests in a debug build locally.

I did and it does not hit assertions, surprisingly.
Comment 10 WebKit Commit Bot 2015-03-13 10:59:22 PDT
Comment on attachment 248588 [details]
Patch

Clearing flags on attachment: 248588

Committed r181480: <http://trac.webkit.org/changeset/181480>
Comment 11 WebKit Commit Bot 2015-03-13 10:59:27 PDT
All reviewed patches have been landed.  Closing bug.