XMLHttpRequests should not prevent a page from entering PageCache. Radar: <rdar://problem/19923085>
Created attachment 248495 [details] WIP Patch
Created attachment 248541 [details] Patch
Created attachment 248544 [details] Patch
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?
Created attachment 248557 [details] Patch
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 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.
Created attachment 248588 [details] Patch
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 on attachment 248588 [details] Patch Clearing flags on attachment: 248588 Committed r181480: <http://trac.webkit.org/changeset/181480>
All reviewed patches have been landed. Closing bug.